[PATCH] remote files completion: remove double-escaping

[PATCH] remote files completion: remove double-escaping

[Lyn Fugmann]

Removes the double escaping in the remote files completion. This affects rsync and scp.
For example, instead of a space character in a remote filename turning into `\\\ `, it will now correctly turn into `\ `.
While scp apparently works with either one, rsync requires the latter since version 3.2.4[1] (unless the legacy behavior is explicitly enabled).
This has been a problem for almost two years now.

[1]: https://download.samba.org/pub/rsync/NEWS#3.2.4


diff --git a/Completion/Unix/Type/_remote_files b/Completion/Unix/Type/_remote_files
index 93e1b7f43..4d4a7abbf 100644
--- a/Completion/Unix/Type/_remote_files
+++ b/Completion/Unix/Type/_remote_files
@@ -60,10 +60,7 @@ if zstyle -T ":completion:${curcontext}:files" remote-access; then
     dirprefix=${dir}/
   fi
 
-  if [[ -z $QIPREFIX ]]
-    then rempat="${dirprefix}${PREFIX%%[^./][^/]#}\*"
-    else rempat="${dirprefix}${(q)PREFIX%%[^./][^/]#}\*"
-  fi
+  rempat="${dirprefix}${(q)PREFIX%%[^./][^/]#}\*"
 
   # remote filenames
   remfiles=(${(M)${(f)"$(
@@ -92,9 +89,9 @@ if zstyle -T ":completion:${curcontext}:files" remote-access; then
   while _tags; do
     while _next_label remote-files expl ${suf:-remote directory}; do
       [[ -n $suf ]] &&
-          compadd "$args[@]" "$expl[@]" -d remdispf -- ${(q)remdispf%[*=|]} && ret=0
+          compadd "$args[@]" "$expl[@]" -d remdispf -- ${remdispf%[*=|]} && ret=0
       compadd ${suf:+-S/} $autoremove "$args[@]" "$expl[@]" -d remdispd \
-	-- ${(q)remdispd%/} && ret=0
+	-- ${remdispd%/} && ret=0
     done
     (( ret )) || return 0
   done

Re: [PATCH] remote files completion: remove double-escaping

[Mikael Magnusson]

On Wed, Apr 10, 2024 at 12:47 PM Lyn Fugmann <me@fugi.dev> wrote:
>
> Removes the double escaping in the remote files completion. This affects rsync and scp.
> For example, instead of a space character in a remote filename turning into `\\\ `, it will now correctly turn into `\ `.
> While scp apparently works with either one, rsync requires the latter since version 3.2.4[1] (unless the legacy behavior is explicitly enabled).
> This has been a problem for almost two years now.

This should probably be behind a zstyle (I guess it's fine if it
defaults to the new behavior), as currently this patch will break
completion for older versions of scp and rsync. That way users can
easily configure it per command or globally if they want.

> diff --git a/Completion/Unix/Type/_remote_files b/Completion/Unix/Type/_remote_files
> index 93e1b7f43..4d4a7abbf 100644
> --- a/Completion/Unix/Type/_remote_files
> +++ b/Completion/Unix/Type/_remote_files
> @@ -60,10 +60,7 @@ if zstyle -T ":completion:${curcontext}:files" remote-access; then
>      dirprefix=${dir}/
>    fi
>
> -  if [[ -z $QIPREFIX ]]
> -    then rempat="${dirprefix}${PREFIX%%[^./][^/]#}\*"
> -    else rempat="${dirprefix}${(q)PREFIX%%[^./][^/]#}\*"
> -  fi
> +  rempat="${dirprefix}${(q)PREFIX%%[^./][^/]#}\*"
>
>    # remote filenames
>    remfiles=(${(M)${(f)"$(
> @@ -92,9 +89,9 @@ if zstyle -T ":completion:${curcontext}:files" remote-access; then
>    while _tags; do
>      while _next_label remote-files expl ${suf:-remote directory}; do
>        [[ -n $suf ]] &&
> -          compadd "$args[@]" "$expl[@]" -d remdispf -- ${(q)remdispf%[*=|]} && ret=0
> +          compadd "$args[@]" "$expl[@]" -d remdispf -- ${remdispf%[*=|]} && ret=0
>        compadd ${suf:+-S/} $autoremove "$args[@]" "$expl[@]" -d remdispd \
> -       -- ${(q)remdispd%/} && ret=0
> +       -- ${remdispd%/} && ret=0
>      done
>      (( ret )) || return 0
>    done

-- 
Mikael Magnusson

Re: [PATCH] remote files completion: remove double-escaping

[Bart Schaefer]

On Wed, Apr 10, 2024 at 11:45 AM Mikael Magnusson <mikachu@gmail.com> wrote:
>
> On Wed, Apr 10, 2024 at 12:47 PM Lyn Fugmann <me@fugi.dev> wrote:
> >
> > Removes the double escaping in the remote files completion. This affects rsync and scp.
> > For example, instead of a space character in a remote filename turning into `\\\ `, it will now correctly turn into `\ `.
> > While scp apparently works with either one, rsync requires the latter since version 3.2.4[1] (unless the legacy behavior is explicitly enabled).
> > This has been a problem for almost two years now.
>
> This should probably be behind a zstyle (I guess it's fine if it
> defaults to the new behavior), as currently this patch will break
> completion for older versions of scp and rsync. That way users can
> easily configure it per command or globally if they want.

Doesn't this actually depend on the remote version rather than the
local command?  Might need to be configurable based on the
destination?

See also thread starting with workers/50484 from 2022 Subject: rsync completions

Re: [PATCH] remote files completion: remove double-escaping

[Lyn Fugmann]

On 2024-04-12 20:59, Bart Schaefer wrote:
> On Wed, Apr 10, 2024 at 11:45 AM Mikael Magnusson <mikachu@gmail.com> wrote:
>>
>> On Wed, Apr 10, 2024 at 12:47 PM Lyn Fugmann <me@fugi.dev> wrote:
>>>
>>> Removes the double escaping in the remote files completion. This affects rsync and scp.
>>> For example, instead of a space character in a remote filename turning into `\\\ `, it will now correctly turn into `\ `.
>>> While scp apparently works with either one, rsync requires the latter since version 3.2.4[1] (unless the legacy behavior is explicitly enabled).
>>> This has been a problem for almost two years now.
>>
>> This should probably be behind a zstyle (I guess it's fine if it
>> defaults to the new behavior), as currently this patch will break
>> completion for older versions of scp and rsync. That way users can
>> easily configure it per command or globally if they want.
> 
> Doesn't this actually depend on the remote version rather than the
> local command?  Might need to be configurable based on the
> destination?
> 
> See also thread starting with workers/50484 from 2022 Subject: rsync completions

I finally got around to testing this. The behavior appears to depend 
only on the local version of rsync, not on the remote version. Tested 
using local rsync 3.2.3 and 3.2.7, remote rsync 3.1.3 and 3.2.7. So I 
think the patch should be fine as is. (I would expect very few people to 
run an over two years old version of rsync and a recent version of zsh 
on the same local machine.)

Re: [PATCH] remote files completion: remove double-escaping

[Lawrence Velázquez]

> On May 28, 2024, at 11:40 AM, Lyn Fugmann <me@fugi.dev> wrote:
> 
> I finally got around to testing this. The behavior appears to depend only on the local version of rsync, not on the remote version. Tested using local rsync 3.2.3 and 3.2.7, remote rsync 3.1.3 and 3.2.7. So I think the patch should be fine as is. (I would expect very few people to run an over two years old version of rsync and a recent version of zsh on the same local machine.)

A GitHub user is inquiring about the status of this contribution.

https://github.com/zsh-users/zsh/pull/114#issuecomment-2212591419

-- 
vq

Re: [PATCH] remote files completion: remove double-escaping

[Eric Cook]

On 7/8/24 5:17 PM, Lawrence Velázquez wrote:
>> On May 28, 2024, at 11:40 AM, Lyn Fugmann <me@fugi.dev> wrote:
>>
>> I finally got around to testing this. The behavior appears to depend only on the local version of rsync, not on the remote version. Tested using local rsync 3.2.3 and 3.2.7, remote rsync 3.1.3 and 3.2.7. So I think the patch should be fine as is. (I would expect very few people to run an over two years old version of rsync and a recent version of zsh on the same local machine.)
>
> A GitHub user is inquiring about the status of this contribution.
>
> https://github.com/zsh-users/zsh/pull/114#issuecomment-2212591419
>
a possible fix, open to a better option letter for _remote_files than -Q, since compadd has -Q.

diff --git a/Completion/Unix/Command/_rsync b/Completion/Unix/Command/_rsync
index c65266dbd..81d25a3f4 100644
--- a/Completion/Unix/Command/_rsync
+++ b/Completion/Unix/Command/_rsync
@@ -60,7 +60,14 @@ elif compset -P 1 '*::' || compset -P 1 'rsync://*/'; then
  elif compset -P 'rsync://'; then
    _rsync_user_or_host / "$@"
  elif compset -P 1 '*:'; then
-  _remote_files -- ssh
+  if [[ -v opt_args[(i)client---old-args] || $RSYNC_OLD_ARGS = 1 ]]; then
+    _remote_files -- ssh
+  else
+    # the 3.2.4+ way that rsync handles filenames does not protect *, ? and []
+    # so those characters still need to be escaped to prevent being treated as
+    # a pattern in the remote shell.
+    _remote_files -Q '[][*?]' -- ssh
+  fi
  else
    _rsync_user_or_host : "$@"
  fi
@@ -236,6 +243,7 @@ _rsync() {
      '*--include=[do not exclude files matching pattern]:pattern' \
      '--files-from=[read list of source-file names from specified file]:file:_files' \
      '(-0 --from0)'{-0,--from0}'[all *-from file lists are delimited by nulls]' \
+    '--old-args[disable the modern arg-protection idiom]' \
      '(-s --secluded-args)'{-s,--secluded-args}'[use the protocol to safely send arguments]' \
      "--trust-sender[trust the remote sender's file list]" \
      '--copy-as=[specify user & optional group for the copy]:user:_rsync_users_groups' \
diff --git a/Completion/Unix/Type/_remote_files b/Completion/Unix/Type/_remote_files
index 93e1b7f43..3f5459aad 100644
--- a/Completion/Unix/Type/_remote_files
+++ b/Completion/Unix/Type/_remote_files
@@ -11,6 +11,7 @@
  # - -g: specify a pattern to match against files
  #       p, = and * glob qualifiers supported
  # - -h: specify the remote host, default is ${IPREFIX%:}
+# - -Q: specify a pattern of characters to escape in the returned filenames
  # - -W: specify the parent directory to list files from,
  #       default is the home directory
  #
@@ -31,14 +32,14 @@


  # There should be coloring based on all the different ls -F classifiers.
-local expl rempat remfiles remdispf remdispd args cmd suf ret=1
+local expl rempat remfiles remdispf{,q} remdispd{,q} args cmd suf ret=1
  local -a args cmd_args
-local glob host dir dirprefix
+local glob host dir esc dirprefix

  if zstyle -T ":completion:${curcontext}:files" remote-access; then

    # Parse options to _remote_files. Stops at the first "--".
-  zparseopts -D -E -a args / g:=glob h:=host W:=dir
+  zparseopts -D -E -a args / g:=glob h:=host W:=dir Q:=esc
    (( $#host)) && shift host || host="${IPREFIX%:}"

    args=( ${argv[1,(i)--]} )
@@ -85,6 +86,14 @@ if zstyle -T ":completion:${curcontext}:files" remote-access; then
      remdispf=( ${(M)remdispf:#${~glob[2]}} )
    fi

+  if (( $#esc )); then
+    remdispfq=(${${remdispf%[*=|]}//(#b)(${~esc[2]})/\\$match[1]})
+    remdispdq=(${${remdispd%/}//(#b)(${~esc[2]})/\\$match[1]})
+  else
+    remdispfq=(${(q)remdispf%[*=|]})
+    remdispdq=(${(q)remdispd%/})
+  fi
+
    local -a autoremove
    [[ -o autoremoveslash ]] && autoremove=(-r "/ \t\n\-")

@@ -92,9 +101,9 @@ if zstyle -T ":completion:${curcontext}:files" remote-access; then
    while _tags; do
      while _next_label remote-files expl ${suf:-remote directory}; do
        [[ -n $suf ]] &&
-          compadd "$args[@]" "$expl[@]" -d remdispf -- ${(q)remdispf%[*=|]} && ret=0
+        compadd "$args[@]" "$expl[@]" -d remdispf -- $remdispfq && ret=0
        compadd ${suf:+-S/} $autoremove "$args[@]" "$expl[@]" -d remdispd \
-	-- ${(q)remdispd%/} && ret=0
+        -- $remdispdq && ret=0
      done
      (( ret )) || return 0
    done

Re: [PATCH] remote files completion: remove double-escaping

[Oliver Kiddle]

Eric Cook wrote:
> On 7/8/24 5:17 PM, Lawrence Velázquez wrote:
> >> On May 28, 2024, at 11:40 AM, Lyn Fugmann <me@fugi.dev> wrote:
> >>
> >> I finally got around to testing this. The behavior appears to depend only on the local version of rsync, not on the remote version. Tested using local rsync 3.2.3 and 3.2.7, remote rsync 3.1.3 and 3.2.7. So I think the patch should be fine as is. (I would expect very few people to run an over two years old version of rsync and a recent version of zsh on the same local machine.)
> >
> > A GitHub user is inquiring about the status of this contribution.
> >
> > https://github.com/zsh-users/zsh/pull/114#issuecomment-2212591419
> >
> a possible fix, open to a better option letter for _remote_files than -Q, since compadd has -Q.

-Q with compadd's meaning is unlikely to be useful with _remote_files so
overloading it should be harmless enough.

-O is the scp option to use the old protocol. Given that option, scp
completion would ideally fallback to the old behaviour.

Oliver

Re: [PATCH] remote files completion: remove double-escaping

[Eric Cook]

On 7/14/24 7:22 PM, Oliver Kiddle wrote:
> Eric Cook wrote:
>> On 7/8/24 5:17 PM, Lawrence Velázquez wrote:
>>>> On May 28, 2024, at 11:40 AM, Lyn Fugmann <me@fugi.dev> wrote:
>>>>
>>>> I finally got around to testing this. The behavior appears to depend only on the local version of rsync, not on the remote version. Tested using local rsync 3.2.3 and 3.2.7, remote rsync 3.1.3 and 3.2.7. So I think the patch should be fine as is. (I would expect very few people to run an over two years old version of rsync and a recent version of zsh on the same local machine.)
>>>
>>> A GitHub user is inquiring about the status of this contribution.
>>>
>>> https://github.com/zsh-users/zsh/pull/114#issuecomment-2212591419
>>>
>> a possible fix, open to a better option letter for _remote_files than -Q, since compadd has -Q.
>
> -Q with compadd's meaning is unlikely to be useful with _remote_files so
> overloading it should be harmless enough.
>
> -O is the scp option to use the old protocol. Given that option, scp
> completion would ideally fallback to the old behaviour.
>
> Oliver
>

Committed the patch i submitted. I am unsure what changes, if any, that _scp needs.