Useless assignment in _rm

Useless assignment in _rm

[zsugabubus]

Hi!

As much as I understand, the assignment is not needed because in the
next line the whole array will be reassigned.

It caused issues with `rm -r<TAB> anything`:
  _rm:72: line: assignment to invalid subscript range
  _rm:72: line: assignment to invalid subscript range

-- 
zsugabubus

diff --git a/Completion/Unix/Command/_rm b/Completion/Unix/Command/_rm
index ea9190d..82b382b 100644
--- a/Completion/Unix/Command/_rm
+++ b/Completion/Unix/Command/_rm
@@ -69,7 +69,6 @@ _arguments -C -s -S $opts \

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

Re: Useless assignment in _rm

[Daniel Shahaf]

zsugabubus wrote on Sun, 21 Jun 2020 14:09 +0200:
> It caused issues with `rm -r<TAB> anything`:
>   _rm:72: line: assignment to invalid subscript range
>   _rm:72: line: assignment to invalid subscript range
> 

CURRENT is 0 at that point which causes the error.  In «rm -r<TAB> -f
foo», CURRENT gets set to -1.

I suppose anything that uses «*::…» or «*:::…» should verify that
CURRENT is >=1 before using it?

> As much as I understand, the assignment is not needed because in the
> next line the whole array will be reassigned.

That's not quite right: the assignment uses $line.  The patch causes
«rm foo<TAB>» when foo and foobar both exist to complete foobar;
without the patch the completion is considered ambiguous.

> diff --git a/Completion/Unix/Command/_rm b/Completion/Unix/Command/_rm
> index ea9190d..82b382b 100644
> --- a/Completion/Unix/Command/_rm
> +++ b/Completion/Unix/Command/_rm
> @@ -69,7 +69,6 @@ _arguments -C -s -S $opts \
> 
>  case $state in
>    (file)
> -    line[CURRENT]=()
>      line=( ${line//(#m)[\[\]()\\*?#<>~\^\|]/\\$MATCH} )
>      _files -F line && ret=0
>      ;;

Possible further improvements here: use ${(b)} in the assignment and
use the «zstyle … ignore-line other» functionality rather than reinvent it.

However, as above, I think the fix to the bug is just to check CURRENT.

Cheers,

Daniel

Re: Useless assignment in _rm

[zsugabubus]

On Sun, Jun 21, 2020 at 01:32:57PM +0000, Daniel Shahaf wrote:
> I suppose anything that uses «*::…» or «*:::…» should verify that
> CURRENT is >=1 before using it?

I searched for similar assignments and queries but I could not find
other place where an extra check would be needed.

(“$words[CURRENT]” is safe no?, because a negative number will just
index backwards so it gives the correct word; and out-of-bounds or zero
just returns nothing.)

> > As much as I understand, the assignment is not needed because in the
> > next line the whole array will be reassigned.
> 
> That's not quite right: the assignment uses $line.  The patch causes
> «rm foo<TAB>» when foo and foobar both exist to complete foobar;
> without the patch the completion is considered ambiguous.

You are absolutely right!!

-- 
zsugabubus

diff --git a/Completion/Unix/Command/_rm b/Completion/Unix/Command/_rm
index ea9190d..e66b77f 100644
--- a/Completion/Unix/Command/_rm
+++ b/Completion/Unix/Command/_rm
@@ -69,7 +69,7 @@ _arguments -C -s -S $opts \

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

Re: Useless assignment in _rm

[Daniel Shahaf]

zsugabubus wrote on Mon, 22 Jun 2020 12:27 +0200:
> diff --git a/Completion/Unix/Command/_rm b/Completion/Unix/Command/_rm
> index ea9190d..e66b77f 100644
> --- a/Completion/Unix/Command/_rm
> +++ b/Completion/Unix/Command/_rm
> @@ -69,7 +69,7 @@ _arguments -C -s -S $opts \
> 
>  case $state in
>    (file)
> -    line[CURRENT]=()
> +    (( CURRENT > 0 )) && line[CURRENT]=()
>      line=( ${line//(#m)[\[\]()\\*?#<>~\^\|]/\\$MATCH} )
>      _files -F line && ret=0
>      ;;

Thanks; I'll commit this unless someone sees a reason not to.

In the future, could you put patches _above_ the dash-dash-space line?
Many MUAs trim by default everything after that line.

Thanks,

Daniel

Re: Useless assignment in _rm

[Daniel Shahaf]

Daniel Shahaf wrote on Tue, 23 Jun 2020 12:40 +0000:
> zsugabubus wrote on Mon, 22 Jun 2020 12:27 +0200:
> > diff --git a/Completion/Unix/Command/_rm b/Completion/Unix/Command/_rm
> > index ea9190d..e66b77f 100644
> > --- a/Completion/Unix/Command/_rm
> > +++ b/Completion/Unix/Command/_rm
> > @@ -69,7 +69,7 @@ _arguments -C -s -S $opts \
> > 
> >  case $state in
> >    (file)
> > -    line[CURRENT]=()
> > +    (( CURRENT > 0 )) && line[CURRENT]=()
> >      line=( ${line//(#m)[\[\]()\\*?#<>~\^\|]/\\$MATCH} )
> >      _files -F line && ret=0
> >      ;;  
> 
> Thanks; I'll commit this unless someone sees a reason not to.

Applied, thanks.

I edited your vanity plate of an email address out.  I'm afraid it
amounts to name-calling and as such has no place in official
documentation, which ChangeLog is.

Also, I forgot to say it before, thanks for auditing other uses of *::
and *:::.