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?

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.

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.

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

akinomyoga mentioned this pull request Apr 5, 2024

fix(scp remote_files): do not filter generated paths with "$cur" #1158

Merged

Rogach and others added 3 commits May 13, 2024 20:37

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

4f1a16c

test(scp): require fixture live_pwd for test_remote_path_with_spaces

c46234a

akinomyoga force-pushed the pr/rsync-fix-space-escapes branch from a60f0b4 to c46234a Compare May 13, 2024 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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

Rogach commented Mar 29, 2023

akinomyoga commented Mar 29, 2023 •

edited

akinomyoga commented Mar 29, 2023

akinomyoga left a comment

akinomyoga left a comment

scop left a comment

scop Apr 2, 2023

akinomyoga Apr 2, 2023 •

edited

Rogach Apr 3, 2023

Rogach Apr 3, 2023

Rogach Apr 3, 2023

akinomyoga Apr 3, 2023

jucor Oct 8, 2023 •

edited

Rogach commented Apr 3, 2023

akinomyoga left a comment

jucor commented Oct 8, 2023

akinomyoga commented Oct 8, 2023

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

Are you sure you want to change the base?

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

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

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

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

akinomyoga commented Mar 29, 2023 •

edited

akinomyoga Apr 2, 2023 •

edited

jucor Oct 8, 2023 •

edited