fix(rsync,ssh): do not overescape spaces in remote filenames #910

Rogach · 2023-03-29T09:57:03Z

Fixes #848.

If a remote machine contains a file named a b, completing rsync command results in rsync remote:a\\\ b, which results in rsync failing to find the file.

This commit removes the extra slashes, and now completion results in rsync remote:a\ b.

scp somehow accepts both variants, so this change won't break it.

akinomyoga · 2023-03-29T10:22:00Z

Can we detect the rsync version and switch the behavior depending on whether the version is lower than 3.2.4 or not? 3.2.4 (2022-04) seems still new, so we expect there are still two different types of rsync in the market.

Rogach · 2023-03-29T10:33:04Z

Sure, but that'll require an extra call to rsync --version and an ugly extra argument to _comp_xfunc_ssh_scp_remote_files.

akinomyoga · 2023-03-29T10:51:34Z

Do you have a better solution?

Rogach · 2023-03-29T11:18:44Z

I've added a commit with the rsync version check.

However, if I were a maintainer, I'd much prefer the simpler solution of just working with the latest version. By the time bash-completion 2.12 is released and propagates to the distributions, it is likely that rsync will also be updated in those distributions. It would be strange if bash-completion were updated while rsync remained on an older version.

I've done a quick check on the distributions:

Ubuntu 22.04 LTS has rsync 3.2.7 (after security update)
Ubuntu 22.10 has rsync 3.2.7
Debian stable has rsync 3.2.3
Debian testing has rsync 3.2.7
Fedora 36-38 has rsync 3.2.7

Arch Linux, of course, has the latest version. I was actually quite surprised to find out that Arch still uses bash-completion from 2020.

From a maintenance perspective, I believe it would be much better not to check the rsync version, as this significantly complicates the code.

akinomyoga

Thank you for updating the PR.

I have a question. The function _comp_xfunc_ssh_scp_remote_files seems to be also called from the completions for other commands ssh and sshfs. I guess we want to keep the original behavior for these commands. Or did these commands also change its interpretation of the escapes?

The others are cosmetic.

completions/rsync

completions/ssh

Rogach · 2023-03-29T11:26:25Z

I have a question. The function _comp_xfunc_ssh_scp_remote_files seems to be also called from the completions for other commands ssh and sshfs. I guess we want to keep the original behavior for these commands. Or did these commands also change its interpretation of the escapes?

It was my understanding that they accept both versions of these arguments (tested on my versions), but I'm not sure for how long they had this behavior. I'll invert the check so that we only change the behavior for the newer rsyncs.

akinomyoga · 2023-03-29T11:32:24Z

However, if I were a maintainer, I'd much prefer the simpler solution of just working with the latest version. By the time bash-completion 2.12 is released and propagates to the distributions, it is likely that rsync will also be updated in those distributions. It would be strange if bash-completion were updated while rsync remained on an older version.

Users are allowed to anytime download the latest version of bash-completion from GitHub independently of the OS distributions.

From a maintenance perspective, I believe it would be much better not to check the rsync version, as this significantly complicates the code.

Ubuntu LTS 18.04 will soon expire, but LTS 20.04 (with rsync 3.1.3) will still continue to be maintained until 2025-04. That's the idea of LTS. Also, we perform CI tests under Debian 10 (w/ rsync 3.1.3), CentOS 7 (w/ rsync 3.1.2), and Ubuntu 14.04 (w/ rsync ???). We actually support as old Bash as 4.2, (which is the reason that we have tests in Ubuntu 14.04).

akinomyoga · 2023-03-29T11:35:51Z

It was my understanding that they accept both versions of these arguments (tested on my versions), but I'm not sure for how long they had this behavior. I'll invert the check so that we only change the behavior for the newer rsyncs.

Thank you. I think that's safer for now.

akinomyoga

Thank you for addressing all the points. Two points are raised by shfmt of the CI.

completions/rsync

completions/ssh

akinomyoga

These are additional suggestions/comments.

completions/rsync

scop

Left some comments.

Also, I think at least _comp_cmd_rsync__vercomp would deserve some tests.

completions/rsync

scop · 2023-04-02T07:42:14Z

completions/ssh

@@ -434,8 +434,11 @@ _comp_cmd_scp__path_esc='[][(){}<>"'"'"',:;^&!$=?`\\|[:space:]]'
 # Complete remote files with ssh.  If the first arg is -d, complete on dirs
 # only.  Returns paths escaped with three backslashes.
 # shellcheck disable=SC2120
+# @param  $1 `no-overescape` if we should use 1 backslash for escaping instead of 3


Even though I agree that the original escape seems a bit overdoing it, it is the traditional/standard one that's been there for a long time, and it is still the say ssh does it -- rsync is kind of a "guest" here. In that sense I'd prefer a more neutral variable name here.

Also, requiring a positional boolean-like parameter to have a specific value like here feels somewhat unconventional. Making it an option (i.e. dash prefixed, no value required as it's a boolean) would be more in line with rest of the codebase.

Also, I overlooked the fact that $1 is already used for -d.

edit:

I think the added documentation line of @param should be put before the shellcheck line.

I think we should test the new behavior of scp_remote_files too.

In that sense I'd prefer a more neutral variable name here.

What name would you suggest?

I think we should test the new behavior of scp_remote_files too.

How shall I approach that? It calls ssh inside to fetch completions, so simply calling the function as it is now won't work.

I see there's a mechanism with LIVE_HOST in test_scp, however to test remote escaping I need to know list of filenames that are present on the remote machine, and at least one of the files needs to have a space in it.

I think we should test the new behavior of scp_remote_files too.

How shall I approach that? It calls ssh inside to fetch completions, so simply calling the function as it is now won't work.

This is a good question. I confess that I haven't thought about it deeply.

Also, I haven't recognized the LIVE_HOST approach used by test_scp.py until now. Hmm, I'm not sure how LIVE_HOST is set up in the repository. There doesn't seem to be a ssh config file that contains bash_completion as a hostname:

(venv) [murase@chatoyancy 1 bash-completion]$ grep bash_completion $(git ls-files | grep config) .pre-commit-config.yaml: files: ^(bash_completion(\.d/[^/]+\.bash)?|completions/.+|test/(config/bashrc|update-test-cmd-list)|.+\.sh(\.in)?)$ .pre-commit-config.yaml: files: ^(bash_completion(\.d/[^/]+\.bash)?|completions/.+|test/(config/bashrc|update-test-cmd-list)|.+\.sh(\.in)?)$ bash-completion-config.cmake.in:set (BASH_COMPLETION_COMPATDIR "@sysconfdir@/bash_completion.d") doc/configuration.md:Sourced late by `bash_completion`, pretty much after everything else. doc/configuration.md:and override ones installed by bash_completion. Defaults to doc/configuration.md:`~/.bash_completion` if unset or null. doc/configuration.md:### `$XDG_CONFIG_HOME/bash_completion` doc/configuration.md:Sourced by the `bash_completion.sh` `profile.d` script. This file is doc/configuration.md:compatibility completion files that are loaded eagerly from `bash_completion` doc/configuration.md:use are `/etc/bash_completion.d`, and `bash_completion.d` relative to doc/configuration.md:`bash_completion` location. doc/configuration.md:with this pattern will not be sourced by `bash_completion` at its load time. test/config/bashrc:# installed via the same PATH expansion in `bash_completion.have()' test/config/bashrc:# For clean test state, avoid sourcing user's ~/.bash_completion test/config/bashrc:export BASH_COMPLETION_COMPAT_DIR="$SRCDIRABS/../bash_completion.d"

Another way is to prepare a mock command/function named ssh, which mimics the command results (as discussed in #573). This can be an option if we actually do not have the working LIVE_HOST.

@scop Are there any references on what is LIVE_HOST used in test_scp.py and how to set it up?

@scop I realise this is the current comment requested that's still blocking. However, now that the latest rsync has a bigger install base, the problem (that this PR would fix) is spreading. Would you consider merging it even without this test, to pull users out of their misery, please?

@scop To summarize the conversation, the initial test followed the LIVE_HOST approach that one of the scp tests used. The problem was that it was non-trivial how to properly set the host for the continuous integration (CI). This has been blocking this PR for a long time. However, the existing test case turned out to be skipped in the CI. We decided to use an approach that does not require LIVE_HOST: the one using mock functions that reproduces the fixed output of the command ssh or rsync.

completions/rsync

completions/ssh

Rogach · 2023-04-03T10:45:09Z

Applied changes according to comments.

Regarding scp_remote_files test - added tests that expect LIVE_HOST remote files to include file spaces in filename.txt.

akinomyoga

Thank you for updating the PR. Here are the comments on the new part.

completions/ssh

test/t/test_rsync.py

completions/ssh

jucor · 2023-10-08T09:50:50Z

I got bitten by this problem today. Would it be possible to get this merged, please?
This is also referred in https://www.reddit.com/r/pop_os/comments/124jmp1/synchronizing_rsync_so_it_only_singlequotes/
Thanks a lot!

akinomyoga · 2023-10-08T11:23:01Z

rebased. The test using LIVE_HOST is not fixed.

jucor · 2024-08-29T18:51:09Z

The existing test that uses LIVE_HOST appears to be skipped (through the fixture live_pwd) in the CI. This implies that LIVE_HOST is not available on the CI.

Would it be fair to suggest that we split that specific problem with LIVE_HOST into a separate issue, as it requires a different skillset and was not introduced here? Yes it does need sorting, but since (again if I understand correctly) the LIVE_HOST was already used before, maybe it shouldn't block this PR, should it?

We could skip the new test like the old one was.

We could then merge this PR.

akinomyoga · 2024-08-29T18:51:33Z

@akinomyoga If I'm correct with the above, I guess we need a test fixture that mocks a remote server, or something of the kind. How do you normally test the existing tab completion, please? Maybe we could use that.

Existing tab completions are tested using real commands. We currently do not use any mock commands for tests. See #573.

jucor · 2024-08-29T18:54:42Z

Existing tab completions are tested using real commands. We currently do not use any mock commands for tests. See #573.

Nice, thanks for the pointer. So introducing a mock would be a completely new approach here, bit of a scope creep 😅

How do you currently (before this PR) test the tab completion of paths?

akinomyoga · 2024-08-29T19:02:03Z

The existing test that uses LIVE_HOST appears to be skipped (through the fixture live_pwd) in the CI. This implies that LIVE_HOST is not available on the CI.

Would it be fair to suggest that we split that specific problem with LIVE_HOST into a separate issue, as it requires a different skillset and was not introduced here? Yes it does need sorting, but since (again if I understand correctly) the LIVE_HOST was already used before, maybe it shouldn't block this PR, should it?

We could skip the new test like the old one was.

We can skip the new test, but that is not the problem. As I mentioned, the new behavior needs to be tested in the CI. The existing test using LIVE_HOST seems just an extra test case; even if the existing test is skipped, there are other tests independent of LIVE_HOST. On the other hand, this PR adds only one test, which depends on LIVE_HOST, but it adds no other tests that do not require LIVE_HOST. It's not a problem that the added test using LIVE_HOST is skipped, but then we need to add other tests.

jucor · 2024-08-29T19:09:51Z

OK, now I see where you're coming from, thanks for taking the time to explain. Let me check what's the current tests are for the tab complete for paths.

Do we agree that, as long as this PR does the same level of tests for tab complete of paths as there was for the old behaviour, this can be merged?

akinomyoga · 2024-08-29T19:17:02Z

Do we agree that, as long as this PR does the same level of tests for tab complete of paths as there was for the old behaviour, this can be merged?

In general, new features and behaviors should come with new tests. Speficailly for the present case, we haven't tested anything about the path completion of rsync/ssh before (probably because we didn't realize the path specification could be a problem). Then, you are suggesting adding no tests effectively, which is not useful. Why don't we add tests that do not depend on LIVE_HOST?

akinomyoga · 2024-08-29T19:25:00Z

@Rogach Could you convert the test or add a new one so that it doesn't depend on LIVE_HOST?

edit: If you take the mock approach, for example, you can run tests in a local environment to collect pairs of a set of arguments and the corresponding output, and mock a command that reproduces the output using the recorded mapping.

jucor · 2024-08-29T19:25:17Z

In general, new features and behaviors should come with new tests. Speficailly for the present case, we haven't tested anything about the path completion of rsync/ssh before (probably because we didn't realize the path specification could be a problem). Then, you are suggesting adding no tests effectively, which is not useful. Why don't we add tests that do not depend on LIVE_HOST?

I hear you.
If we can code those new tests without having to mock a full ssh, we could.

I'm trying to be pragmatic and avoid users doing the kind of tricks referred there: https://www.reddit.com/r/pop_os/comments/124jmp1/comment/js1mdp8/

I wholeheartedly agree that more code should mean more tests.
I am also aware that adding new mocks and fixtures is possibly not within @Rogach 's wheelhouse, as illustrated by the year since this is open. Given that it was acceptable for the tab completion for paths so far to not have tests, and the particularities of this request (the hoops users are jumping through to get this feature, the broken compatibility of the status quo with rsync, the time this problem has been open, and the precedent for no test on tab completion for paths), I'm hoping that you will considerate merging with the test skipped.

Of course, it's your project, your call, I can only advocate :)

Fixes scop#848. If a remote machine contains a file named `a b`, completing rsync command results in `rsync remote:a\\\ b`, which results in rsync failing to find the file. This commit removes the extra slashes, and now completion results in `rsync remote:a\ b`. scp somehow accepts both variants, so this change won't break it.

Rogach · 2024-08-30T13:46:15Z

Here you go, I've figured out how to make a test with a mock function.

Rogach · 2024-08-30T14:11:56Z

1: T1 Title exceeds max length (99>72): "fix(rsync,ssh): do not depend on LIVE_HOST when testing remote files with spaces, use mocks instead"

I'm open to suggestions for a better commit title.

How you are supposed to fit anything useful into 72 characters, anyway?

akinomyoga · 2024-08-30T14:31:10Z

I'm open to suggestions for a better commit title.

Something like this? I'd use test for the type of the commit.

test(rsync,ssh): test remote path with spaces using mock commands

edit: I changed "filenames" to "path" because it matches the name of the test function test_remote_path_with_spaces.

How you are supposed to fit anything useful into 72 characters, anyway?

By giving up something that is not so important. If you want to include details, you can do it in the second (or a later) paragraph (separated by an empty line).

completions/rsync

akinomyoga

In addition to the fix for CI, I'd suggest changing the quoting of the string literal. The current quoted string is not so special that it requires triple quotations, and we haven't used triple quotations for anything other than docstrings.

test/t/test_rsync.py

test/t/test_scp.py

akinomyoga · 2024-09-05T22:41:27Z

@yedayak Could you also quickly review this PR if there aren't obvious problems you notice or points to improve?

akinomyoga · 2024-09-06T14:35:02Z

Thank you!

jucor · 2024-09-06T15:07:44Z

Woot woot, thanks everyone!

akinomyoga reviewed Mar 29, 2023

View reviewed changes

completions/rsync Outdated Show resolved Hide resolved

completions/rsync Outdated Show resolved Hide resolved

completions/rsync Outdated Show resolved Hide resolved

completions/ssh Outdated Show resolved Hide resolved

Rogach force-pushed the pr/rsync-fix-space-escapes branch from c944277 to 1d72577 Compare March 29, 2023 12:36

akinomyoga requested changes Mar 29, 2023

View reviewed changes

completions/rsync Outdated Show resolved Hide resolved

completions/ssh Outdated Show resolved Hide resolved

akinomyoga reviewed Mar 29, 2023

View reviewed changes

completions/rsync Outdated Show resolved Hide resolved

completions/rsync Outdated Show resolved Hide resolved

Rogach force-pushed the pr/rsync-fix-space-escapes branch from 1d72577 to e02ff4e Compare March 30, 2023 09:04

akinomyoga approved these changes Mar 30, 2023

View reviewed changes

akinomyoga reviewed Mar 30, 2023

View reviewed changes

completions/rsync Outdated Show resolved Hide resolved

Rogach force-pushed the pr/rsync-fix-space-escapes branch from e02ff4e to 03eb3ab Compare March 30, 2023 09:31

akinomyoga approved these changes Mar 30, 2023

View reviewed changes

scop requested changes Apr 2, 2023

View reviewed changes

akinomyoga reviewed Apr 2, 2023

View reviewed changes

completions/ssh Outdated Show resolved Hide resolved

Rogach force-pushed the pr/rsync-fix-space-escapes branch from 03eb3ab to 9022d56 Compare April 3, 2023 10:43

Rogach force-pushed the pr/rsync-fix-space-escapes branch 3 times, most recently from 9708e3a to 87f3e60 Compare April 3, 2023 11:00

akinomyoga reviewed Apr 3, 2023

View reviewed changes

completions/ssh Outdated Show resolved Hide resolved

completions/ssh Outdated Show resolved Hide resolved

test/t/test_rsync.py Outdated Show resolved Hide resolved

completions/ssh Outdated Show resolved Hide resolved

Rogach force-pushed the pr/rsync-fix-space-escapes branch from 87f3e60 to 9fb38ca Compare April 3, 2023 12:18

akinomyoga mentioned this pull request Apr 3, 2023

fix: quote boolean command names such as if "$boolean_variable"; then #914

Closed

Rogach force-pushed the pr/rsync-fix-space-escapes branch from 9fb38ca to f1155c4 Compare June 18, 2023 23:54

akinomyoga force-pushed the pr/rsync-fix-space-escapes branch from f1155c4 to a60f0b4 Compare October 8, 2023 11:22

Rogach and others added 3 commits August 30, 2024 17:09

fix(rsync): overescape remote paths if rsync version is < 3.2.4

c40229c

test(scp): require fixture live_pwd for test_remote_path_with_spaces

78e1675

Rogach force-pushed the pr/rsync-fix-space-escapes branch from c46234a to 0abe57a Compare August 30, 2024 13:43

Rogach force-pushed the pr/rsync-fix-space-escapes branch from 0abe57a to 4da52c2 Compare August 30, 2024 14:35

akinomyoga reviewed Aug 30, 2024

View reviewed changes

completions/rsync Outdated Show resolved Hide resolved

akinomyoga reviewed Aug 31, 2024

View reviewed changes

test/t/test_rsync.py Outdated Show resolved Hide resolved

test/t/test_scp.py Outdated Show resolved Hide resolved

Rogach force-pushed the pr/rsync-fix-space-escapes branch from 4da52c2 to 0dc2c37 Compare August 31, 2024 08:15

test(rsync,ssh): test remote filenames with spaces using mock commands

1da052f

Rogach force-pushed the pr/rsync-fix-space-escapes branch from 0dc2c37 to 1da052f Compare August 31, 2024 09:13

akinomyoga approved these changes Sep 3, 2024

View reviewed changes

yedayak mentioned this pull request Sep 5, 2024

test(scp): Add unit tests for getting remote files #1244

Merged

yedayak approved these changes Sep 6, 2024

View reviewed changes

akinomyoga merged commit e8dc253 into scop:main Sep 6, 2024
7 checks passed

github-actions bot mentioned this pull request Sep 9, 2024

chore: release 2.15.0 #1193

Merged

yedayak mentioned this pull request Oct 25, 2024

Escaping of remote filenames containing spaces is broken for rsync #1255

Open

fix(rsync,ssh): do not overescape spaces in remote filenames #910

fix(rsync,ssh): do not overescape spaces in remote filenames #910

Conversation

Rogach commented Mar 29, 2023

akinomyoga commented Mar 29, 2023

Rogach commented Mar 29, 2023

akinomyoga commented Mar 29, 2023

Rogach commented Mar 29, 2023

akinomyoga left a comment

Choose a reason for hiding this comment

Rogach commented Mar 29, 2023

akinomyoga commented Mar 29, 2023 • edited Loading

akinomyoga commented Mar 29, 2023

akinomyoga left a comment

Choose a reason for hiding this comment

akinomyoga left a comment

Choose a reason for hiding this comment

scop left a comment

Choose a reason for hiding this comment

scop Apr 2, 2023

Choose a reason for hiding this comment

akinomyoga Apr 2, 2023 • edited Loading

Choose a reason for hiding this comment

Rogach Apr 3, 2023

Choose a reason for hiding this comment

Rogach Apr 3, 2023

Choose a reason for hiding this comment

Rogach Apr 3, 2023

Choose a reason for hiding this comment

akinomyoga Apr 3, 2023

Choose a reason for hiding this comment

jucor Oct 8, 2023 • edited Loading

Choose a reason for hiding this comment

akinomyoga Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Rogach commented Apr 3, 2023

akinomyoga left a comment

Choose a reason for hiding this comment

jucor commented Oct 8, 2023

akinomyoga commented Oct 8, 2023

jucor commented Aug 29, 2024

akinomyoga commented Aug 29, 2024

jucor commented Aug 29, 2024 • edited Loading

akinomyoga commented Aug 29, 2024 • edited Loading

jucor commented Aug 29, 2024

akinomyoga commented Aug 29, 2024

akinomyoga commented Aug 29, 2024 • edited Loading

jucor commented Aug 29, 2024

Rogach commented Aug 30, 2024

Rogach commented Aug 30, 2024

akinomyoga commented Aug 30, 2024 • edited Loading

akinomyoga left a comment • edited Loading

Choose a reason for hiding this comment

akinomyoga commented Sep 5, 2024

akinomyoga commented Sep 6, 2024

jucor commented Sep 6, 2024

akinomyoga commented Mar 29, 2023 •

edited

Loading

akinomyoga Apr 2, 2023 •

edited

Loading

jucor Oct 8, 2023 •

edited

Loading

akinomyoga Aug 31, 2024 •

edited

Loading

jucor commented Aug 29, 2024 •

edited

Loading

akinomyoga commented Aug 29, 2024 •

edited

Loading

akinomyoga commented Aug 29, 2024 •

edited

Loading

akinomyoga commented Aug 30, 2024 •

edited

Loading

akinomyoga left a comment •

edited

Loading