PATCH: Fix _file_descriptors

PATCH: Fix _file_descriptors

[Mikael Magnusson]

I noticed file descriptor completion didn't work, with the verbose style set
because when this is run,
  fds=( /dev/fd/<0-9>(N:t) )
the /dev/fd dir is open while the glob is performed, which results in
a spurious entry in the result, which then cannot be dereferenced. The
result is that the list array is not aligned to the fds array (and also
an error message is output), and an fd that doesn't exist is completed.

If the verbose style is not set, the spurious fd is not filtered.

---
 Completion/Zsh/Type/_file_descriptors |   34 ++++++++++++++++++--------------
 1 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/Completion/Zsh/Type/_file_descriptors b/Completion/Zsh/Type/_file_descriptors
index 3e251b7..38e2bf5 100644
--- a/Completion/Zsh/Type/_file_descriptors
+++ b/Completion/Zsh/Type/_file_descriptors
@@ -1,31 +1,35 @@
 #autoload
 
-local i fds expl list link sep
+local i expl link sep cmd
+local -a fds list
 
-fds=( /dev/fd/<0-9>(N:t) )
-
-if zstyle -T ":completion:${curcontext}:" verbose && [[ -h /proc/$$/fd/$fds[1] ]]; then
+if zstyle -T ":completion:${curcontext}:" verbose && [ -h /proc/$$/fd/<->([1]) ]; then
   zstyle -s ":completion:${curcontext}:" list-separator sep || sep=--
 
   if zmodload -F zsh/stat b:zstat; then
-    for i in "${fds[@]}"; do
-      zstat +link -A link /proc/$$/fd/$i
-      list+=( "$i $sep ${link[1]}" )
-    done
+    cmd='zstat +link -A link $REPLY; link=$link[1]'
   elif (( $+commands[readlink] )); then
-    for i in "${fds[@]}"; do
-      list+=( "$i $sep $(readlink /proc/$$/fd/$i)" )
-    done
+    cmd='link=$(readlink $REPLY)'
   else
-    for i in "${fds[@]}"; do
-      list+=( "$i $sep $(ls -l /proc/$$/fd/$i|sed 's/.*-> //' )" )
-    done
+    cmd='link=$(ls -l $REPLY|sed "s/.*-> //" )'
   fi
 
-  if (( $list[(I)* $sep ?*] )); then
+  # Filter out the fd for the dir opened during the glob (/proc/$$/fd)
+  : /proc/$$/fd/<->(e,$cmd'
+                       if [[ $link == /proc/$$/fd ]]; then
+                         false
+                       else
+                         fds+=( $REPLY:t )
+                         list+=( "$REPLY:t $sep $link" )
+                       fi
+                      ',)
+
+  if (( $list[(I)<-> $sep ?*] )); then
     _wanted file-descriptors expl 'file descriptor' compadd "$@" -d list -a - fds
     return
   fi
+else
+  fds=( /dev/fd/<->(N:t) )
 fi
 
 _wanted file-descriptors expl 'file descriptor' compadd -a "$@" - fds
-- 
1.7.5.4

Re: PATCH: Fix _file_descriptors

[Oliver Kiddle]

--- On Sun, 26/2/12, Mikael Magnusson <mikachu@gmail.com> wrote:

> I noticed file descriptor completion
> didn't work, with the verbose style set
> because when this is run,
>   fds=( /dev/fd/<0-9>(N:t) )
> the /dev/fd dir is open while the glob is performed, which
> results in
> a spurious entry in the result, which then cannot be
> dereferenced. The
> result is that the list array is not aligned to the fds
> array (and also
> an error message is output), and an fd that doesn't exist is
> completed.

I'm not sure what you mean by "cannot be dereferenced".

In any case, this whole function can be simplified by using the newish :A modifier. Also, I'm fairly certain that it is intentional that this function only completes file descriptors from 0 - 9.

With :A, assigning fds can just be:

fds=( /dev/fd/<0-9>(e,'[[ $REPLY:A != /proc/$$/fd ]]',) )

It's probably best to build up list then with a for loop after checking the style but again :A can be used instead of trying the three old mechanisms.

Another approach would be to use a subshell to open the directory:
  fds=( $(print /proc/$$/fd/<0-9>(N:t)) ) 
But I think there are some platforms that have /dev/fd but not /proc/$$/fd.

Oliver

Re: PATCH: Fix _file_descriptors

[Mikael Magnusson]

On 27 February 2012 17:08, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> --- On Sun, 26/2/12, Mikael Magnusson <mikachu@gmail.com> wrote:
>
>> I noticed file descriptor completion
>> didn't work, with the verbose style set
>> because when this is run,
>>   fds=( /dev/fd/<0-9>(N:t) )
>> the /dev/fd dir is open while the glob is performed, which
>> results in
>> a spurious entry in the result, which then cannot be
>> dereferenced. The
>> result is that the list array is not aligned to the fds
>> array (and also
>> an error message is output), and an fd that doesn't exist is
>> completed.
>
> I'm not sure what you mean by "cannot be dereferenced".

The link does not exist, so you cannot lstat it.

> In any case, this whole function can be simplified by using the newish :A modifier.

:A doesn't do anything useful for fds that point to things like
pipe:[21325192] or 'file (deleted)'. (nor does (:a)).
% echo /proc/$$/fd/2(:A); zstat +link /proc/$$/fd/2
/proc/3555/fd/2
/tmp/foo/test (deleted)

% echo /proc/$$/fd/3(:A); zstat +link /proc/$$/fd/3
/proc/3555/fd/3
socket:[219517545]

(We could also compare with ztcp -L before checking the symlinks, etc.)

> Also, I'm fairly certain that it is intentional that this function only completes file descriptors from 0 - 9.

I'm not, the code was originally a loop over {0..9} and probably the 9
was just some arbitrary low number. What reason could there possibly
be to do this?

> With :A, assigning fds can just be:
>
> fds=( /dev/fd/<0-9>(e,'[[ $REPLY:A != /proc/$$/fd ]]',) )
>
> It's probably best to build up list then with a for loop after checking the style but again :A can be used instead of trying the three old mechanisms.

As I noted originally, the problem with this is that there will be one
link in $fds that doesn't exist when you try to build up the list
array.

> Another approach would be to use a subshell to open the directory:
>  fds=( $(print /proc/$$/fd/<0-9>(N:t)) )
> But I think there are some platforms that have /dev/fd but not /proc/$$/fd.

These should still work, as the fallback still uses /dev/fd. I guess
we could try both for the verbose style too though.

-- 
Mikael Magnusson