PATCH: reverse matching (tentative)

PATCH: reverse matching (tentative)

[Peter Stephenson]

We have this paragraph about the (R) subscript flag in the manual:

   Note  that  this  flag  can give odd results on failure.  For an
   ordinary array the item substituted  is  that  corresponding  to
   subscript 0.  If the option KSH_ARRAYS is not in effect, this is
   the same as the element corresponding to subscript  1,  although
   the  form  ${array[(I)pattern]}  will evaluate to 0 for a failed
   match.  If the option KSH_ARRAYS is in effect, the subscript  is
   still  0 for a failed match; this cannot be distinguished from a
   successful match without testing ${array[0]}  against  the  pat-
   tern.

I can't imagine anyone has ever found this other than a complete
nuisance (as I did, yet again).  Does anyone see any problems if
I fix this so that the empty string is returned?  We can do that
by returning an index off the end of an array if the value required
is an entry rather than the index itself.  This keeps the (useful)
values returned by (i) and (I) as they were before.

We still have the problem with 0 being ambiguous for (I) in the case
of KSH_ARRAYS.  I think we've dug that hole so deep we can't climb out.
The only alternative would be to return a value off the end of the
array, but it's now so ingrained that (I) returns 0 for failure that
that's too painful.  We could do it only if KSH_ARRAYS is set,
but the combination of zsh subscripting and ksh/bash arrays isn't
very common anyway.


Index: README
===================================================================
RCS file: /cvsroot/zsh/zsh/README,v
retrieving revision 1.44
diff -u -r1.44 README
--- README	8 May 2007 10:02:59 -0000	1.44
+++ README	15 May 2007 09:56:44 -0000
@@ -72,6 +72,10 @@
 $search starts with "%" considers the "%" to be part of the search
 string as before.
 
+Parameter subscripts of the form ${array[(R)test]} now return the
+empty string if they fail to match.  The previous longstanding behaviour
+was confusing and useless.
+
 The MULTIBYTE option is on by default where it is available; this
 causes many operations to recognise characters as in the current locale.
 Older versions of the shell always assumed a character was one byte.
Index: Doc/Zsh/params.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/params.yo,v
retrieving revision 1.36
diff -u -r1.36 params.yo
--- Doc/Zsh/params.yo	8 Feb 2007 10:43:30 -0000	1.36
+++ Doc/Zsh/params.yo	15 May 2007 09:56:46 -0000
@@ -234,15 +234,7 @@
 Like `tt(r)', but gives the last match.  For associative arrays, gives
 all possible matches. May be used for assigning to ordinary array
 elements, but not for assigning to associative arrays.
-
-Note that this flag can give odd results on failure.  For an ordinary array
-the item substituted is that corresponding to subscript 0.  If the option
-tt(KSH_ARRAYS) is not in effect, this is the same as the element
-corresponding to subscript 1, although the form tt(${array[(I)pattern]})
-will evaluate to 0 for a failed match.  If the option tt(KSH_ARRAYS) is in
-effect, the subscript is still 0 for a failed match; this cannot be
-distinguished from a successful match without testing tt(${array[0]})
-against the pattern.
+On failure the empty string is returned.
 )
 item(tt(i))(
 Like `tt(r)', but gives the index of the match instead; this may not be
@@ -251,13 +243,16 @@
 is compared to the pattern, and the first matching key found is the
 result.
 
-See `tt(r)' for discussion of subscripts of failed matches.
+On failure, a value one past the end of the array or string is returned.
 )
 item(tt(I))(
 Like `tt(i)', but gives the index of the last match, or all possible
 matching keys in an associative array.
 
-See `tt(R)' for discussion of subscripts of failed matches.
+On failure the value 0 is returned.  If the option tt(KSH_ARRAYS) is in
+effect, the subscript is still 0 for a failed match; this cannot be
+distinguished from a successful match without testing tt(${array[0]})
+against the pattern.
 )
 item(tt(k))(
 If used in a subscript on an associative array, this flag causes the keys
Index: Src/params.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/params.c,v
retrieving revision 1.126
diff -u -r1.126 params.c
--- Src/params.c	14 May 2007 09:25:31 -0000	1.126
+++ Src/params.c	15 May 2007 09:56:47 -0000
@@ -1276,6 +1276,19 @@
 			if (pprog && pattry(pprog, *p) && !--num)
 			    return r;
 		    }
+		    /*
+		     * Failed to match.
+		     * If we're returning an index, return 0 to show
+		     * we've gone off the start.  Unfortunately this
+		     * is ambiguous with KSH_ARRAYS set, but we're
+		     * stuck with that now.
+		     *
+		     * If the index is to be turned into an element,
+		     * return an index that does not point to a valid
+		     * element (since 0 is treated the same as 1).
+		     */
+		    if (!ind)
+			r = len + 1;
 		} else
 		    for (r = 1 + beg, p = ta + beg; *p; r++, p++)
 			if (pprog && pattry(pprog, *p) && !--num)
@@ -1495,7 +1508,13 @@
 		    }
 		}
 	    }
-	    return down ? 0 : slen + 1;
+	    /*
+	     * Failed to match.
+	     * If the argument selects an element rather than
+	     * its index, ensure the element is empty.
+	     * See comments on the array case above.
+	     */
+	    return (down && ind) ? 0 : slen + 1;
 	}
     }
     return r;
Index: Test/D06subscript.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/D06subscript.ztst,v
retrieving revision 1.9
diff -u -r1.9 D06subscript.ztst
--- Test/D06subscript.ztst	6 Sep 2005 15:28:18 -0000	1.9
+++ Test/D06subscript.ztst	15 May 2007 09:56:48 -0000
@@ -29,12 +29,11 @@
 >*, [how] I [wonder] what?  You are!
 >] I [
 
-  # $s[(R)x] actually is $s[0], but zsh treats 0 as 1 for subscripting.
   print $s[(i)x] : $s[(I)x]
   print $s[(r)x] : $s[(R)x]
 0:Scalar pattern subscripts that do not match
 >61 : 0
->: T
+>:
 
   print -R $s[$s[(i)\[]] $s[(i)$s[(r)\*]] $s[(i)${(q)s[(r)\]]}]
 0:Scalar subscripting using a pattern subscript to get the index

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


To access the latest news from CSR copy this link into a web browser:  http://www.csr.com/email_sig.php

To get further information regarding CSR, please visit our Investor Relations page at http://ir.csr.com/csr/about/overview

Re: PATCH: reverse matching (tentative)

[Matt Wozniski]

On 5/15/07, Peter Stephenson wrote:
>    match.  If the option KSH_ARRAYS is in effect, the subscript  is
>    still  0 for a failed match; this cannot be distinguished from a
>    successful match without testing ${array[0]}  against  the  pat-
>    tern.

I have a function in my .zshrc that has to work around exactly that
'feature' - I'd be all for just removing that and having it return a
blank string.

~Matt

Re: PATCH: reverse matching (tentative)

[Peter Stephenson]

On Tue, 15 May 2007 17:06:37 -0400
"Matt Wozniski" <godlygeek@gmail.com> wrote:
> On 5/15/07, Peter Stephenson wrote:
> >    match.  If the option KSH_ARRAYS is in effect, the subscript  is
> >    still  0 for a failed match; this cannot be distinguished from a
> >    successful match without testing ${array[0]}  against  the  pat-
> >    tern.
> 
> I have a function in my .zshrc that has to work around exactly that
> 'feature' - I'd be all for just removing that and having it return a
> blank string.

I've committed this.

Only (R) has changed.  Indices returned from subscript searches should
be as before.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


To access the latest news from CSR copy this link into a web browser:  http://www.csr.com/email_sig.php

To get further information regarding CSR, please visit our Investor Relations page at http://ir.csr.com/csr/about/overview

bindkey <whatever> history-beginning-search-backwards; echo $widgts = crash

[Maddi Kopfermann]

Hi Zsh Comrades,

Yesterday i found what it seems to be a very old bug.

bindkey <whatever> history-beginning-search-backwards &&
echo $widgets crashes zsh, it does with zsh -f.
On #zsh Frank Terbeck had it crash even with 3.1.7. All
versions that have history-beginning-search-backwards did
crash, it seems.

gdb zsh core has:

        Program terminated with signal 11, Segmentation
        fault.
        #0  widgetstr (w=0x0) at
        ../../../Src/Zle/zleparameter.c:78
        78      ../../../Src/Zle/zleparameter.c: No such
        file or directory.
                in ../../../Src/Zle/zleparameter.c


                Greetings,
                        Matthias

Re: bindkey <whatever> history-beginning-search-backwards; echo $widgts = crash

[Maddi Kopfermann]

On Mon, May 21, 2007 at 10:55:37PM -0700, Phil Pennock:
> 
> % zsh -fc 'bindkey "^o" history-beginning-search-forward; echo $widgets'

right, this does not crash the shell here, thanks!
Thanks for the solution!
I should have waited 'til being awake before trying it out
and saying: "It crashes the shell".
400 lines of zsh bindings need a little more checkup on my
part :)

> And FWIW the supplied patch does prevent this crashing when given a bad
> name.

Makes sense,

                Matthias

Re: bindkey <whatever> history-beginning-search-backwards; echo $widgts = crash

[Peter Stephenson]

On Mon, 21 May 2007 18:59:23 -0700
Phil Pennock <zsh-workers+phil.pennock@spodhuis.org> wrote:
> On 2007-05-21 at 18:46 -0700, Phil Pennock wrote:
> > This is a bug when there is no widget with the supplied name; the
> > value for the type can't be calculated.  The code which loops over
> > the widgets doesn't notice that the widget doesn't really exist and
> > the code which produces a stringification doesn't handle it.
> 
> Is it better for the $widgets associative array to pretend that such a
> key doesn't exist, or to acknowledge that it does exist with undefined
> value?

I think the latter.  If the user is convinced the widget should exist
and has tried to bind something to it the shell should show as much.
The juice stains the lips, the stain acts as a warning (as it were).
(No, that doesn't really make sense.)

I've committed that patch.  Thanks.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


To access the latest news from CSR copy this link into a web browser:  http://www.csr.com/email_sig.php

To get further information regarding CSR, please visit our Investor Relations page at http://ir.csr.com/csr/about/overview

Re: bindkey <whatever> history-beginning-search-backwards; echo $widgts = crash

[Maddi Kopfermann]

On Mon, May 21, 2007 at 10:37:29PM -0700, Bart Schaefer:
> } Sadly not only in that case!
> 
> Why do you think so?  In every example you gave, you first bound a key
> to a widget that does not exist, and then tried to echo $widgets.
> 
> } zsh -fc "bindkey '^o' history-beginning-<TAB> && echo $widgets"
> } crashes the shell
> } 
> } the attempt to let completion work crashes the shell.
> 
> I don't understand what that example is supposed to be showing.  What
> do you mean by "let completion work"?  Completion only happens in the
> line editor in an interactive shell, you can't pass a literal tab to
> zsh -fc and expect anything to happen.  And even if you mean that you
> did type a tab in ZLE, the completion you show would be ambiguous --
> ordinarily it would take more than one tab at that position to complete
> a valid widget name.

I see my mistake. Whatever I did was with an already
"infected" zsh so that _every_ "echo $widgets" does crash it,
no matter what i write before it.
In a zsh without any configuration  (zsh -f) those described cases
_don't_ crash the shell.

                Matthias

Re: bindkey <whatever> history-beginning-search-backwards; echo $widgts = crash

[Phil Pennock]

On 2007-05-22 at 05:32 +0200, Maddi Kopfermann wrote:
> Am Montag, dem 21. Mai 2007 um 18:46 Uhr, Phil Pennock:
> > 
> > history-beginning-search-backwards is not a standard widget; aside from
> > this bug, you need to find which widget you really want.
> 
> zsh -fc "bindkey '^o' history-beginning-backward && echo $widgets"
> 
> crashes the shell
> 
> zsh -fc "bindkey '^o' history-beginning-backwards && echo $widgets"
> 
> crashes the shell
> 
> zsh -fc "bindkey '^o' history-beginning-forward && echo $widgets"
> 
> crashes the shell
> 
> zsh -fc "bindkey '^o' history-beginning-<TAB> && echo $widgets"
> crashes the shell
> 
> the attempt to let completion work crashes the shell.
> > This is a bug when there is no widget with the supplied name;
> 
> Sadly not only in that case!

But, uhm, those aren't widgets; see zshzle(1) for details.
history-beginning-<TAB> would take you as far as
history-beginning-search- but that's not a complete widget, there are
four different widgets with that prefix.  Well, okay, you might be set
up to tab-complete further.  But not in zsh -fc "cmd..." -- if someone
has completion set up to correctly work recursively there, I'd like to
know the settings.

history-search-forward ?  history-beginning-search-forward ?  And also
"-backward".

Meanwhile,
% zsh -fc "cmdname"
will execute cmdname in a non-interactive shell and without any startup
files you might have; but the variables inside that need escaping so
that they'll be expanded by the non-interactive shell.  I think you
want:

% zsh -fc 'bindkey "^o" history-beginning-search-forward; echo $widgets'

And FWIW the supplied patch does prevent this crashing when given a bad
name.

Re: bindkey <whatever> history-beginning-search-backwards; echo $widgts = crash

[Bart Schaefer]

On May 22,  5:32am, Maddi Kopfermann wrote:
} 
} > This is a bug when there is no widget with the supplied name;
} 
} Sadly not only in that case!

Why do you think so?  In every example you gave, you first bound a key
to a widget that does not exist, and then tried to echo $widgets.

} zsh -fc "bindkey '^o' history-beginning-<TAB> && echo $widgets"
} crashes the shell
} 
} the attempt to let completion work crashes the shell.

I don't understand what that example is supposed to be showing.  What
do you mean by "let completion work"?  Completion only happens in the
line editor in an interactive shell, you can't pass a literal tab to
zsh -fc and expect anything to happen.  And even if you mean that you
did type a tab in ZLE, the completion you show would be ambiguous --
ordinarily it would take more than one tab at that position to complete
a valid widget name.

Re: bindkey <whatever> history-beginning-search-backwards; echo $widgts = crash

[Maddi Kopfermann]

Am Montag, dem 21. Mai 2007 um 18:46 Uhr, Phil Pennock:
> 
> history-beginning-search-backwards is not a standard widget; aside from
> this bug, you need to find which widget you really want.

zsh -fc "bindkey '^o' history-beginning-backward && echo $widgets"

crashes the shell

zsh -fc "bindkey '^o' history-beginning-backwards && echo $widgets"

crashes the shell

zsh -fc "bindkey '^o' history-beginning-forward && echo $widgets"

crashes the shell

zsh -fc "bindkey '^o' history-beginning-<TAB> && echo $widgets"
crashes the shell

the attempt to let completion work crashes the shell.
> This is a bug when there is no widget with the supplied name;

Sadly not only in that case!

                Matthias

Re: bindkey <whatever> history-beginning-search-backwards; echo $widgts = crash

[Phil Pennock]

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

On 2007-05-21 at 18:46 -0700, Phil Pennock wrote:
> This is a bug when there is no widget with the supplied name; the value
> for the type can't be calculated.  The code which loops over the widgets
> doesn't notice that the widget doesn't really exist and the code which
> produces a stringification doesn't handle it.

Is it better for the $widgets associative array to pretend that such a
key doesn't exist, or to acknowledge that it does exist with undefined
value?

Previous patch implemented the latter.

This patch implements the former, keeping the "undefined" fallback
around just in case there's another bug.

Intuitively, the $widgets array shouldn't pretend that nodes don't
exist, but then there's conflicting behaviour with getpmwidgets() which
returns an empty string.

I leave the decision to those with more experience.
-Phil

[-- Attachment #2: widget-crash2.patch --]
[-- Type: text/x-diff, Size: 782 bytes --]

diff -urp zsh-head/Src/Zle/zleparameter.c zsh-bindkey-crash/Src/Zle/zleparameter.c
--- zsh-head/Src/Zle/zleparameter.c	Tue Mar  7 13:31:44 2006
+++ zsh-bindkey-crash/Src/Zle/zleparameter.c	Mon May 21 18:52:20 2007
@@ -75,6 +75,8 @@ createspecialhash(char *name, GetNodeFun
 static char *
 widgetstr(Widget w)
 {
+    if (!w)
+	return dupstring("undefined");
     if (w->flags & WIDGET_INT)
 	return dupstring("builtin");
     if (w->flags & WIDGET_NCOMP) {
@@ -127,6 +129,8 @@ scanpmwidgets(UNUSED(HashTable ht), Scan
 
     for (i = 0; i < thingytab->hsize; i++)
 	for (hn = thingytab->nodes[i]; hn; hn = hn->next) {
+	    if (!((Thingy) hn)->widget)
+		continue;
 	    pm.node.nam = hn->nam;
 	    if (func != scancountparams &&
 		((flags & (SCANPM_WANTVALS|SCANPM_MATCHVAL)) ||

Re: bindkey <whatever> history-beginning-search-backwards; echo $widgts = crash

[Phil Pennock]

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

On 2007-05-21 at 22:21 +0200, Maddi Kopfermann wrote:
> Hi Zsh Comrades,
> 
> Yesterday i found what it seems to be a very old bug.
> 
> bindkey <whatever> history-beginning-search-backwards &&

history-beginning-search-backwards is not a standard widget; aside from
this bug, you need to find which widget you really want.

> echo $widgets crashes zsh, it does with zsh -f.
> On #zsh Frank Terbeck had it crash even with 3.1.7. All
> versions that have history-beginning-search-backwards did
> crash, it seems.

This is a bug when there is no widget with the supplied name; the value
for the type can't be calculated.  The code which loops over the widgets
doesn't notice that the widget doesn't really exist and the code which
produces a stringification doesn't handle it.

% bindkey '^Z' fred
% ^Z
No such widget `fred'
% print ${(v)widgets}
[segfault]

The attached patch ensures that there's an appropriate value for the
key; it'll do as a work-around for now.

#0  0x0000000801918291 in widgetstr (w=0x0) at zleparameter.c:78
78          if (w->flags & WIDGET_INT)
(gdb) bt
#0  0x0000000801918291 in widgetstr (w=0x0) at zleparameter.c:78
#1  0x00000008019184f8 in scanpmwidgets (ht=0x55cd40, func=0x80069a6d0 <scanparamvals>, flags=1) at zleparameter.c:134
#2  0x000000080067229f in scanmatchtable (ht=0x55cd40, pprog=0x0, sorted=0, flags1=0, flags2=33554432, 
    scanfunc=0x80069a6d0 <scanparamvals>, scanflags=1) at hashtable.c:381
#3  0x000000080067258b in scanhashtable (ht=0x55cd40, sorted=0, flags1=0, flags2=33554432, scanfunc=0x80069a6d0 <scanparamvals>, 
    scanflags=1) at hashtable.c:444
#4  0x000000080069a959 in paramvalarr (ht=0x55cd40, flags=1) at params.c:547
#5  0x000000080069aa25 in getvaluearr (v=0x7fffffffdf40) at params.c:565
#6  0x000000080069e409 in getarrvalue (v=0x7fffffffdf40) at params.c:1862
#7  0x00000008006bda52 in paramsubst (l=0x545068, n=0x545098, str=0x7fffffffe088, qt=0, ssub=0) at subst.c:2034
#8  0x00000008006b9afa in stringsubst (list=0x545068, node=0x545098, ssub=0, asssub=0) at subst.c:193
#9  0x00000008006b9400 in prefork (list=0x545068, flags=0) at subst.c:91
#10 0x0000000800664304 in execcmd (state=0x7fffffffe650, input=0, output=0, how=18, last1=2) at exec.c:2039
#11 0x000000080066253e in execpline2 (state=0x7fffffffe650, pcode=387, how=18, input=0, output=0, last1=0) at exec.c:1343
#12 0x000000080066199c in execpline (state=0x7fffffffe650, slcode=4098, how=18, last1=0) at exec.c:1129
#13 0x0000000800661261 in execlist (state=0x7fffffffe650, dont_change_job=0, exiting=0) at exec.c:935
#14 0x0000000800660dda in execode (p=0x544f68, dont_change_job=0, exiting=0) at exec.c:793
#15 0x000000080067bd5c in loop (toplevel=1, justonce=0) at init.c:180
#16 0x000000080067ede0 in zsh_main (argc=1, argv=0x7fffffffe780) at init.c:1347
#17 0x00000000004006cb in main (argc=1, argv=0x7fffffffe780) at ./main.c:93

-Phil

[-- Attachment #2: widget-crash.patch --]
[-- Type: text/x-diff, Size: 457 bytes --]

diff -urp zsh-head/Src/Zle/zleparameter.c zsh-bindkey-crash/Src/Zle/zleparameter.c
--- zsh-head/Src/Zle/zleparameter.c	Tue Mar  7 13:31:44 2006
+++ zsh-bindkey-crash/Src/Zle/zleparameter.c	Mon May 21 18:37:22 2007
@@ -75,6 +75,8 @@ createspecialhash(char *name, GetNodeFun
 static char *
 widgetstr(Widget w)
 {
+    if (!w)
+	return dupstring("undefined");
     if (w->flags & WIDGET_INT)
 	return dupstring("builtin");
     if (w->flags & WIDGET_NCOMP) {

bindkey <whatever> history-beginning-search-backwards; echo $widgts = crash

[Maddi Kopfermann]

Hi Zsh Comrades,

Yesterday i found what it seems to be a very old bug.

bindkey <whatever> history-beginning-search-backwards &&
echo $widgets crashes zsh, it does with zsh -f.
On #zsh Frank Terbeck had it crash even with 3.1.7. All
versions that have history-beginning-search-backwards did
crash, it seems.

gdb zsh core has:

        Program terminated with signal 11, Segmentation
        fault.
        #0  widgetstr (w=0x0) at
        ../../../Src/Zle/zleparameter.c:78
        78      ../../../Src/Zle/zleparameter.c: No such
        file or directory.
                in ../../../Src/Zle/zleparameter.c


                Greetings,
                        Matthias

PS: This is a resent because I sadly posted the email not as a new thread