bug in 'rm' completion

bug in 'rm' completion

[Greg Klanderman]

Running current cvs trunk:

| [~] greg@lwm| zsh -f
| lwm% echo $ZSH_VERSION
| 4.3.10-dev-1
| lwm% echo $ZSH_PATCHLEVEL
| 1.4806
| lwm% rm .zcompdump
| lwm% autoload -U compinit
| lwm% compinit -u
| lwm% mkdir temp2
| lwm% cd temp2
| lwm% touch foo foo-bar

At this point, if you type "rm f" and hit <tab> it correctly completes
to "rm foo".  If you then hit <tab> again, it incorrectly completes to
"rm foo-bar".

| lwm% rm foo-bar 
| lwm% rm foo 
| lwm% 
| lwm% touch foo{1,2,3}

Now, type "rm foo2" and hit <tab> and it incorrectly thinks there are
no completions, whereas it should insert a space.

Will look into fixing these at some point but figured I'd report them
first in case the fix is obvious to anyone out there.. I assume both
examples are manifestations of the same bug.

cheers,
Greg

Re: bug in 'rm' completion

[Greg Klanderman]

>>>>> On November 9, 2009 Greg Klanderman <gak@klanderman.net> wrote:

> | lwm% mkdir temp2
> | lwm% cd temp2
> | lwm% touch foo foo-bar

> At this point, if you type "rm f" and hit <tab> it correctly completes
> to "rm foo".  If you then hit <tab> again, it incorrectly completes to
> "rm foo-bar".

It looks like the code in _rm is trying to ignore completing other
files already listed to be removed on the command line, but brokenly
including the current word being completed in the ignore pattern.

The patch below seems to fix the problem, provided you are completing
the last word on the line.  So for example,

% mkdir temp
% cd temp
% touch bar foo foo-bar
% rm f<tab> bar

if you hit <tab> with the cursor in the location indicated, the
problem still exists with my patch, so I don't really think this is
the right fix.. I guess I should use $CURRENT to exclude the current
word?

Can someone explain where $line is coming from in _rm?  I don't see it
listed in the 'Completion Special Parameters' or 'Parameters Set By
The Shell' sections of the manual, and the name is too generic to
effectively search for.  Is it the same as $words, which I do see
documented in 'Completion Special Parameters'?  Hmm, not exactly, as
$line does not contain the command.. can I assume line=$words[2,-1]
and adjust $CURRENT appropriately?

BTW, what's the deal with _files vs _path_files?  I see that revision
1.2 of _rm switched from using _path_files to _files:

| revision 1.2
| date: 2008/11/23 18:23:29;  author: barts;  state: Exp;  lines: +1 -1
| users/13477: call _files instead of _path_files to correctly handle
| cycling through choices.

but there are plenty of completion functions using each.

thanks,
Greg

Re: bug in 'rm' completion

[Bart Schaefer]

On Nov 9, 12:29am, Greg Klanderman wrote:
}
} Will look into fixing these at some point but figured I'd report them
} first in case the fix is obvious to anyone out there.. I assume both
} examples are manifestations of the same bug.

At first I thought this was a(nother) problem with _path_files but it
appears instead to be pretty plainly this in _rm:

case $state in
  (file)
    declare -a ignored
    ignored=(${line//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})
    _files -F ignored && ret=0
    ;;
esac

A _complete_debug trace shows:

+_rm:33> case file (file)
+_rm:35> declare -a ignored
+_rm:36> ignored=( foo ) 
+_rm:37> _files -F ignored

The most recent change was:

2009-08-17  Peter Stephenson  <p.w.stephenson@ntlworld.com>

        * 27219: Completion/Unix/Type/_files: "_files -F <array>" wasn't
        correctly handled, which broke duplicate filtering in _rm.

(BTW "wasn't" has a UTF-8 apostrophe in the actual ChangeLog file.)

So _rm is attempting to filter duplicates out of the completion list,
but ends up filtering out unambiguous prefixes as well.  Fixing one
bug exposed a different one.

The easy fix of swapping in

    { _files -F ignored || _files }

solves the foo{1,2,3} variant but not the foo{,-bar} variant because
the presence of foo-bar allows _files -F to succeed even though it
skips foo.  There may not be a way to satisfy all constraints here.

Re: bug in 'rm' completion

[Bart Schaefer]

On Nov 9, 11:17am, Greg Klanderman wrote:
}
} The patch below seems to fix the problem, provided you are completing
} the last word on the line.

You forgot the patch.

} Can someone explain where $line is coming from in _rm?

It's declared in both _main_complete and _arguments and appears to be
set by "comparguments -W" at _arguments:377.  Unfortunately there's no
good documentation for "comparguments" because it was treated as a
helper builtin that no end user would ever need to call directly.

} BTW, what's the deal with _files vs _path_files?

The main thing is that _files handles grouping the files according
to the files, directories, etc. according to the file-patterns zstyle.
_path_files is a lower-level function that manages the path traversal
to find appropriate files to complete.  Most cases probably ought to
be calling _files unless the intent is to impose a different kind of
grouping or to avoid having file-patterns applied, but it's a bit of a
judgment call in each instance.

Re: bug in 'rm' completion

[Greg Klanderman]

>>>>> On November 9, 2009 Bart Schaefer <schaefer@brasslantern.com> wrote:

> You forgot the patch.

Hi Bart,

Duh.. see below.  It's just not considering the last word from the
line when generating the ignore pattern.  What I think is needed is to
generally remove the current word from incorporation into the ignore
pattern, but I don't know how to do that.  If I knew how to relate
$CURRENT to an element of $line that would do it.

greg


Index: Completion/Unix/Command/_rm
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Unix/Command/_rm,v
retrieving revision 1.2
diff -u -r1.2 _rm
--- Completion/Unix/Command/_rm	23 Nov 2008 18:23:29 -0000	1.2
+++ Completion/Unix/Command/_rm	9 Nov 2009 15:49:39 -0000
@@ -33,7 +33,7 @@
 case $state in
   (file)
     declare -a ignored
-    ignored=(${line//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})
+    ignored=(${line[1,-2]//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})
     _files -F ignored && ret=0
     ;;
 esac

Re: bug in 'rm' completion

[Greg Klanderman]

>>>>> On November 9, 2009 Greg Klanderman <gak@klanderman.net> wrote:

> What I think is needed is to
> generally remove the current word from incorporation into the ignore
> pattern, but I don't know how to do that.  If I knew how to relate
> $CURRENT to an element of $line that would do it.

I think the patch below is the correct fix.  I'm still not sure
whether it should be using $line or $words and I'm not certain on the
difference between two and three colons in the _arguments definition
('*::files:->file' vs '*:::files:->file') but in both cases I suspect
it doesn't actually matter.

Let me know what you think..
Greg


Index: Completion/Unix/Command/_rm
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Unix/Command/_rm,v
retrieving revision 1.2
diff -u -r1.2 _rm
--- Completion/Unix/Command/_rm	23 Nov 2008 18:23:29 -0000	1.2
+++ Completion/Unix/Command/_rm	9 Nov 2009 21:07:52 -0000
@@ -5,7 +5,7 @@
   '(-f --force)'{-f,--force}'[ignore nonexistent files, never prompt]'
   '(-I --interactive)-i[prompt before every removal]'
   '(-r -R --recursive)'{-r,-R,--recursive}'[remove directories and their contents recursively]'
-  '*:files:->file'
+  '*::files:->file'
 )
 if _pick_variant gnu=gnu unix --help; then
   opts+=(-S)
@@ -33,7 +33,8 @@
 case $state in
   (file)
     declare -a ignored
-    ignored=(${line//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})
+    ignored=(${line[1,CURRENT-1]//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})
+    ignored+=(${line[CURRENT+1,-1]//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})
     _files -F ignored && ret=0
     ;;
 esac

Re: bug in 'rm' completion

[Bart Schaefer]

On Nov 9, 12:52pm, Greg Klanderman wrote:
}
} Duh.. see below.  It's just not considering the last word from the
} line when generating the ignore pattern.  What I think is needed is to
} generally remove the current word from incorporation into the ignore
} pattern, but I don't know how to do that.  If I knew how to relate
} $CURRENT to an element of $line that would do it.

Amazingly there is a descriptive comment in Src/Zle/computil.c for the
behavior of comparguments.  It says:

case 'W':
    /* This gets two parameter names as arguments.  The first is set to
     * the current word sans any option prefixes handled by comparguments.
     * The second parameter is set to an array containing the options on
     * the line and their arguments.  I.e. the stuff _arguments returns
     * to its caller in the `line' and `opt_args' parameters. */

If that's accurate, $line should be a single word and $opt_args should
be an array.  However, the C code appears to be setting the first arg
as an array and the second as a hash, which agrees with the yodl doc
for the _arguments function:

     During the performance of the action the array `line' will be set
     to the command name and normal arguments from the command line,
     i.e. the words from the command line excluding all options and
     their arguments.  Options are stored in the associative array
     `opt_args' with option names as keys and their arguments as the
     values.  For options that have more than one argument these are
     given as one string, separated by colons.  All colons in the
     original arguments are preceded with backslashes.

Only Sven W. knows why the name "line" was chosen for this; forensics
might determine at what point the comment in computil.c corresponded
to reality, but it's probably not woth the effort.


On Nov 9,  4:20pm, Greg Klanderman wrote:
} Subject: Re: bug in 'rm' completion
}
} I think the patch below is the correct fix.  I'm still not sure
} whether it should be using $line or $words and I'm not certain on the
} difference between two and three colons in the _arguments definition
} ('*::files:->file' vs '*:::files:->file') but in both cases I suspect
} it doesn't actually matter.

Doc says:

      With two colons before the MESSAGE, the words special array
      and the CURRENT special parameter are modified to refer only
      to the normal arguments when the ACTION is executed or
      evaluated.  With three colons before the MESSAGE they are
      modified to refer only to the normal arguments covered by
      this description.

I believe this means that *:: and *::: are equivalent in this case
because there are no other descriptions to "cover" other arguments.

You're correct that using one of these forms will update CURRENT to
point into $line, which is what's needed for your solution.
 
} +    ignored=(${line[1,CURRENT-1]//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})
} +    ignored+=(${line[CURRENT+1,-1]//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})

I'd be inclined to wrap that in a check that ((CURRENT > 1)) just so
it's obvious that we don't ignore the only word on the line, rather
than rely on $line[1,0] to do the right thing, but otherwise this
seems right to me.

Re: bug in 'rm' completion

[Greg Klanderman]

>>>>> On November 10, 2009 Bart Schaefer <schaefer@brasslantern.com> wrote:

> } +    ignored=(${line[1,CURRENT-1]//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})
> } +    ignored+=(${line[CURRENT+1,-1]//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})

> I'd be inclined to wrap that in a check that ((CURRENT > 1))

As long as you mean to wrap just the first line with that check..
otherwise it won't work correctly, for example:

% rm -rf <tab> foo bar baz

> just so
> it's obvious that we don't ignore the only word on the line, rather
> than rely on $line[1,0] to do the right thing, but otherwise this
> seems right to me.

Do you want the analogous check wrapped around the second line?
Can we rely on $line[$#line+1,-1] to do the right thing?  If not
then presumably you want something like the new patch below..

thanks,
Greg


Index: Completion/Unix/Command/_rm
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Unix/Command/_rm,v
retrieving revision 1.2
diff -u -r1.2 _rm
--- Completion/Unix/Command/_rm	23 Nov 2008 18:23:29 -0000	1.2
+++ Completion/Unix/Command/_rm	12 Nov 2009 02:47:20 -0000
@@ -5,7 +5,7 @@
   '(-f --force)'{-f,--force}'[ignore nonexistent files, never prompt]'
   '(-I --interactive)-i[prompt before every removal]'
   '(-r -R --recursive)'{-r,-R,--recursive}'[remove directories and their contents recursively]'
-  '*:files:->file'
+  '*::files:->file'
 )
 if _pick_variant gnu=gnu unix --help; then
   opts+=(-S)
@@ -33,7 +33,11 @@
 case $state in
   (file)
     declare -a ignored
-    ignored=(${line//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})
+    ignored=()
+    (( CURRENT > 1 )) &&
+      ignored+=(${line[1,CURRENT-1]//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})
+    (( CURRENT < $#line )) &&
+      ignored+=(${line[CURRENT+1,-1]//(#m)[\[\]()\\*?#<>~\^]/\\$MATCH})
     _files -F ignored && ret=0
     ;;
 esac

Re: bug in 'rm' completion

[Bart Schaefer]

On Nov 11,  9:48pm, Greg Klanderman wrote:
} Subject: Re: bug in 'rm' completion
}
} otherwise it won't work correctly, for example:
} 
} % rm -rf <tab> foo bar baz

I guess I can't say that's an obscure case, but it sure does take a lot
of work to arrange it. :-)
 
} Do you want the analogous check wrapped around the second line?
} Can we rely on $line[$#line+1,-1] to do the right thing?

I think the latter is reliable, but the test doesn't hurt anything.

} then presumably you want something like the new patch below..

Committed.

Re: bug in 'rm' completion

[Greg Klanderman]

>>>>> On November 11, 2009 Bart Schaefer <schaefer@brasslantern.com> wrote:

> Committed.

Thanks Bart!

greg