zsh-workers
 help / color / Atom feed
* Useless assignment in _rm
@ 2020-06-21 12:09 zsugabubus
  2020-06-21 13:32 ` Daniel Shahaf
  0 siblings, 1 reply; 5+ messages in thread
From: zsugabubus @ 2020-06-21 12:09 UTC (permalink / raw)
  To: zsh-workers

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
     ;;

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Useless assignment in _rm
  2020-06-21 12:09 Useless assignment in _rm zsugabubus
@ 2020-06-21 13:32 ` Daniel Shahaf
  2020-06-22 10:27   ` zsugabubus
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Shahaf @ 2020-06-21 13:32 UTC (permalink / raw)
  To: zsugabubus; +Cc: zsh-workers

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Useless assignment in _rm
  2020-06-21 13:32 ` Daniel Shahaf
@ 2020-06-22 10:27   ` zsugabubus
  2020-06-23 12:40     ` Daniel Shahaf
  0 siblings, 1 reply; 5+ messages in thread
From: zsugabubus @ 2020-06-22 10:27 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers

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
     ;;

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Useless assignment in _rm
  2020-06-22 10:27   ` zsugabubus
@ 2020-06-23 12:40     ` Daniel Shahaf
  2020-06-28 13:13       ` Daniel Shahaf
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Shahaf @ 2020-06-23 12:40 UTC (permalink / raw)
  To: zsugabubus; +Cc: zsh-workers

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Useless assignment in _rm
  2020-06-23 12:40     ` Daniel Shahaf
@ 2020-06-28 13:13       ` Daniel Shahaf
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Shahaf @ 2020-06-28 13:13 UTC (permalink / raw)
  To: zsugabubus; +Cc: zsh-workers

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 *:::.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-21 12:09 Useless assignment in _rm zsugabubus
2020-06-21 13:32 ` Daniel Shahaf
2020-06-22 10:27   ` zsugabubus
2020-06-23 12:40     ` Daniel Shahaf
2020-06-28 13:13       ` Daniel Shahaf

zsh-workers

Archives are clonable: git clone --mirror http://inbox.vuxu.org/zsh-workers

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.zsh.workers


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git