Unexpected behavior for completion funcion _remote_files()

Unexpected behavior for completion funcion _remote_files()

[Yoshio Hanawa]

[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]

Hello all,

When I tried Zsh completion funcion _remote_files with 'docker exec ...', I
found the command line options for _remote_files (in this case, 'exec ...')
are passed directly to compadd. I think it's unexpected behavior.


--- Completion/Unix/Type/_remote_file:75-78
***
      [[ -n $suf ]] &&
          compadd "$@" "$expl[@]" -d remdispf ${(q)remdispf%[*=|]} && ret=0
      compadd ${suf:+-S/} -r "/ \t\n\-" "$@" "$expl[@]" -d remdispd \
${(q)remdispd%/} && ret=0
***


"$@" in compadd arguments seems to be unnecessary, so should simply be
removed.

The following patch works fine on my environment.


*** zsh-5.2-orig/Completion/Unix/Type/_remote_files 2016-09-06
22:55:25.000000000 +0900
--- zsh-5.2/Completion/Unix/Type/_remote_files 2016-09-06
22:56:12.000000000 +0900
***************
*** 74,81 ****
    while _tags; do
      while _next_label files expl ${suf:-remote directory}; do
        [[ -n $suf ]] &&
!           compadd "$@" "$expl[@]" -d remdispf ${(q)remdispf%[*=|]} &&
ret=0
!       compadd ${suf:+-S/} -r "/ \t\n\-" "$@" "$expl[@]" -d remdispd \
  ${(q)remdispd%/} && ret=0
      done
      (( ret )) || return 0
--- 74,81 ----
    while _tags; do
      while _next_label files expl ${suf:-remote directory}; do
        [[ -n $suf ]] &&
!           compadd "$expl[@]" -d remdispf ${(q)remdispf%[*=|]} && ret=0
!       compadd ${suf:+-S/} -r "/ \t\n\-" "$expl[@]" -d remdispd \
  ${(q)remdispd%/} && ret=0
      done
      (( ret )) || return 0

Re: Unexpected behavior for completion funcion _remote_files()

[Oliver Kiddle]

Yoshio Hanawa wrote:
> When I tried Zsh completion funcion _remote_files with 'docker exec ...', I
> found the command line options for _remote_files (in this case, 'exec ...')
> are passed directly to compadd. I think it's unexpected behavior.

Yes, _remote_files shouldn't pass options that come after -- to compadd.
Thanks for the report.

> "$@" in compadd arguments seems to be unnecessary, so should simply be
> removed.

Ideally, any extra options from before -- should be passed on.
At the very least it should allow for them being there - currently, just using
_wanted with _remote_files fails. Using this would allow docker
completion to override the description given with the files if 'remote
file' is not applicable.

> The following patch works fine on my environment.

Could you perhaps try the following? I don't have docker so have only
tested this with ssh.

Thanks

Oliver

diff --git a/Completion/Unix/Type/_remote_files b/Completion/Unix/Type/_remote_files
index db33164..54bd438 100644
--- a/Completion/Unix/Type/_remote_files
+++ b/Completion/Unix/Type/_remote_files
@@ -28,16 +28,19 @@
 
 
 # There should be coloring based on all the different ls -F classifiers.
-local expl rempat remfiles remdispf remdispd args cmd cmd_args suf ret=1
+local expl rempat remfiles remdispf remdispd args cmd suf ret=1
+local -a args cmd_args
 local glob host
 
 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
-  shift
   (( $#host)) && shift host || host="${IPREFIX%:}"
 
+  args=( ${argv[1,(I)--]} )
+  shift ${#args}
+  args[-1]=()
   # Command to run on the remote system.
   cmd="$1"
   shift
@@ -45,9 +48,9 @@ if zstyle -T ":completion:${curcontext}:files" remote-access; then
   # Handle arguments to ssh.
   if [[ $cmd == ssh ]]; then
     zparseopts -D -E -a cmd_args p: 1 2 4 6 F:
-    cmd_args="-o BatchMode=yes $cmd_args -a -x"
+    cmd_args=( -o BatchMode=yes "$cmd_args[@]" -a -x )
   else
-    cmd_args="$@"
+    cmd_args=( "$@" )
   fi
 
   if [[ -z $QIPREFIX ]]
@@ -74,8 +77,8 @@ if zstyle -T ":completion:${curcontext}:files" remote-access; then
   while _tags; do
     while _next_label files expl ${suf:-remote directory}; do
       [[ -n $suf ]] &&
-          compadd "$@" "$expl[@]" -d remdispf ${(q)remdispf%[*=|]} && ret=0
-      compadd ${suf:+-S/} -r "/ \t\n\-" "$@" "$expl[@]" -d remdispd \
+          compadd "$args[@]" "$expl[@]" -d remdispf ${(q)remdispf%[*=|]} && ret=0
+      compadd ${suf:+-S/} -r "/ \t\n\-" "$args[@]" "$expl[@]" -d remdispd \
 	${(q)remdispd%/} && ret=0
     done
     (( ret )) || return 0

Re: Unexpected behavior for completion funcion _remote_files()

[Yoshio Hanawa]

[-- Attachment #1: Type: text/plain, Size: 3197 bytes --]

Hi.

> Ideally, any extra options from before -- should be passed on.

I understood your intention well.

> Could you perhaps try the following?

Your patch works fine with 'docker exec ...'.

I also check '_remote_files -x foobar -- docker exec ..',
it works as expected.

Thank you so much.


2016-09-07 0:55 GMT+09:00 Oliver Kiddle <okiddle@yahoo.co.uk>:

> Yoshio Hanawa wrote:
> > When I tried Zsh completion funcion _remote_files with 'docker exec
> ...', I
> > found the command line options for _remote_files (in this case, 'exec
> ...')
> > are passed directly to compadd. I think it's unexpected behavior.
>
> Yes, _remote_files shouldn't pass options that come after -- to compadd.
> Thanks for the report.
>
> > "$@" in compadd arguments seems to be unnecessary, so should simply be
> > removed.
>
> Ideally, any extra options from before -- should be passed on.
> At the very least it should allow for them being there - currently, just
> using
> _wanted with _remote_files fails. Using this would allow docker
> completion to override the description given with the files if 'remote
> file' is not applicable.
>
> > The following patch works fine on my environment.
>
> Could you perhaps try the following? I don't have docker so have only
> tested this with ssh.
>
> Thanks
>
> Oliver
>
> diff --git a/Completion/Unix/Type/_remote_files b/Completion/Unix/Type/_
> remote_files
> index db33164..54bd438 100644
> --- a/Completion/Unix/Type/_remote_files
> +++ b/Completion/Unix/Type/_remote_files
> @@ -28,16 +28,19 @@
>
>
>  # There should be coloring based on all the different ls -F classifiers.
> -local expl rempat remfiles remdispf remdispd args cmd cmd_args suf ret=1
> +local expl rempat remfiles remdispf remdispd args cmd suf ret=1
> +local -a args cmd_args
>  local glob host
>
>  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
> -  shift
>    (( $#host)) && shift host || host="${IPREFIX%:}"
>
> +  args=( ${argv[1,(I)--]} )
> +  shift ${#args}
> +  args[-1]=()
>    # Command to run on the remote system.
>    cmd="$1"
>    shift
> @@ -45,9 +48,9 @@ if zstyle -T ":completion:${curcontext}:files"
> remote-access; then
>    # Handle arguments to ssh.
>    if [[ $cmd == ssh ]]; then
>      zparseopts -D -E -a cmd_args p: 1 2 4 6 F:
> -    cmd_args="-o BatchMode=yes $cmd_args -a -x"
> +    cmd_args=( -o BatchMode=yes "$cmd_args[@]" -a -x )
>    else
> -    cmd_args="$@"
> +    cmd_args=( "$@" )
>    fi
>
>    if [[ -z $QIPREFIX ]]
> @@ -74,8 +77,8 @@ if zstyle -T ":completion:${curcontext}:files"
> remote-access; then
>    while _tags; do
>      while _next_label files expl ${suf:-remote directory}; do
>        [[ -n $suf ]] &&
> -          compadd "$@" "$expl[@]" -d remdispf ${(q)remdispf%[*=|]} &&
> ret=0
> -      compadd ${suf:+-S/} -r "/ \t\n\-" "$@" "$expl[@]" -d remdispd \
> +          compadd "$args[@]" "$expl[@]" -d remdispf ${(q)remdispf%[*=|]}
> && ret=0
> +      compadd ${suf:+-S/} -r "/ \t\n\-" "$args[@]" "$expl[@]" -d remdispd
> \
>         ${(q)remdispd%/} && ret=0
>      done
>      (( ret )) || return 0
>

Re: Unexpected behavior for completion funcion _remote_files()

[Daniel Shahaf]

Oliver Kiddle wrote on Tue, Sep 06, 2016 at 17:55:59 +0200:
>  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
> -  shift
>    (( $#host)) && shift host || host="${IPREFIX%:}"
>  
> +  args=( ${argv[1,(I)--]} )

Should be (i), not (I), in case the [<cmd options>] themselves contain
a '--' word.

Re: Unexpected behavior for completion funcion _remote_files()

[Oliver Kiddle]

On 6 Sep, Daniel Shahaf wrote:
> > +  args=( ${argv[1,(I)--]} )
>
> Should be (i), not (I), in case the [<cmd options>] themselves contain
> a '--' word.

I had intentionally used (I) to cope with the case where there is no
'--' on the line at all.

This should handle either case.

Oliver

diff --git a/Completion/Unix/Type/_remote_files b/Completion/Unix/Type/_remote_files
index 54bd438..dbfb561 100644
--- a/Completion/Unix/Type/_remote_files
+++ b/Completion/Unix/Type/_remote_files
@@ -38,9 +38,9 @@ if zstyle -T ":completion:${curcontext}:files" remote-access; then
   zparseopts -D -E -a args / g:=glob h:=host
   (( $#host)) && shift host || host="${IPREFIX%:}"
 
-  args=( ${argv[1,(I)--]} )
+  args=( ${argv[1,(i)--]} )
   shift ${#args}
-  args[-1]=()
+  [[ args[-1] = -- ]] && args[-1]=()
   # Command to run on the remote system.
   cmd="$1"
   shift

Re: Unexpected behavior for completion funcion _remote_files()

[Daniel Shahaf]

Oliver Kiddle wrote on Mon, Sep 12, 2016 at 17:30:15 +0200:
> On 6 Sep, Daniel Shahaf wrote:
> > > +  args=( ${argv[1,(I)--]} )
> >
> > Should be (i), not (I), in case the [<cmd options>] themselves contain
> > a '--' word.
> 
> I had intentionally used (I) to cope with the case where there is no
> '--' on the line at all.
> 
> This should handle either case.
> 
> Oliver
> 
> diff --git a/Completion/Unix/Type/_remote_files b/Completion/Unix/Type/_remote_files
> index 54bd438..dbfb561 100644
> --- a/Completion/Unix/Type/_remote_files
> +++ b/Completion/Unix/Type/_remote_files
> @@ -38,9 +38,9 @@ if zstyle -T ":completion:${curcontext}:files" remote-access; then
>    zparseopts -D -E -a args / g:=glob h:=host
>    (( $#host)) && shift host || host="${IPREFIX%:}"
>  
> -  args=( ${argv[1,(I)--]} )
> +  args=( ${argv[1,(i)--]} )
>    shift ${#args}
> -  args[-1]=()
> +  [[ args[-1] = -- ]] && args[-1]=()

In latest master, scp behaves as follows:

% scp foo:<TAB>
-J         -d         -default-  remdispd   remdispf

It bisects to the patch I'm replying to (98581594b50d == 39295).

This is the behaviour regardless of whether 'foo' is a valid hostname or
not.

Cheers,

Daniel

>    # Command to run on the remote system.
>    cmd="$1"
>    shift
> 

Re: Unexpected behavior for completion funcion _remote_files()

[Bart Schaefer]

On Nov 3,  2:59pm, Daniel Shahaf wrote:
} Subject: Re: Unexpected behavior for completion funcion _remote_files()
}
} > +  [[ args[-1] = -- ]] && args[-1]=()

That's an obvious typo, it should be $args[-1] ...

Hardly seems worth sending a patch.