One last _values / compvalues nit, and a nasty crash bug

One last _values / compvalues nit, and a nasty crash bug

[Bart Schaefer]

First the nit:

I don't expect anyone to leap to do anything about this, but now that we
have both -S and -s (and it's too bad their meanings aren't reversed as
the string given to -s is the one that has to be passed to compadd -S, but
it's too late for that now) the issue arises of what happens when you pass
the same string to both.

E.g.:

zsh% _tv() { _values -S : -s : test 'a: :(foo)' b }
zsh% compedef _tv :
zsh% : a<TAB>
zsh% : a:<TAB>
zsh% : a:b 

At this point, the thing that comes after `a:' should have been considered
an argument, not a new value.  If the argument to `a' is required rather
than optional, there's no reason the same separator can't be used both
between the value and its argument, and between the argument and the next
value.

Now the bug -- press TAB three times after `: a' and get a crash:

schaefer<501> function _tv { _values -S '' -s : 'a: :(foo)' b }
schaefer<502> compdef _tv :
schaefer<503> function _tv { _values -S '' -s : test 'a: :(foo)' b }
schaefer<504> : afoo:afoo:BUG: attempt to free more than allocated.
_values:149: command not found: 0R^O^H\M-^?\M-^?\M-^?\M-^?@T^O^H\M-^?\M-^?\M-^?\M-^?pR^O^H\M-^?\M-^?\M-^?\M-^?\M-@S^O^H\M-^?\M-^?\M-^?\M-^?\M-pP^O^H\M-^?\M-^?\M-^?\M-^?ws
zsh: segmentation fault (core dumped)  zsh-4.0.1-pre-4

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: One last _values / compvalues nit, and a nasty crash bug

[Sven Wischnowsky]

Bart Schaefer wrote:

> Now the bug -- press TAB three times after `: a' and get a crash:
> 
> schaefer<501> function _tv { _values -S '' -s : 'a: :(foo)' b }
> schaefer<502> compdef _tv :
> schaefer<503> function _tv { _values -S '' -s : test 'a: :(foo)' b }
> schaefer<504> : afoo:afoo:BUG: attempt to free more than allocated.
> _values:149: command not found: 0R^O^H\M-^?\M-^?\M-^?\M-^?@T^O^H\M-^?\M-^?\M-^?\M-^?pR^O^H\M-^?\M-^?\M-^?\M-^?\M-@S^O^H\M-^?\M-^?\M-^?\M-^?\M-pP^O^H\M-^?\M-^?\M-^?\M-^?ws
> zsh: segmentation fault (core dumped)  zsh-4.0.1-pre-4

That was because compvalues couldn't handle an empty string as a
separator.  It still can't, but the patch makes it test that and report
an error.  More below.

> I don't expect anyone to leap to do anything about this, but now that we
> have both -S and -s (and it's too bad their meanings aren't reversed as
> the string given to -s is the one that has to be passed to compadd -S, but
> it's too late for that now) the issue arises of what happens when you pass
> the same string to both.
> 
> E.g.:
> 
> zsh% _tv() { _values -S : -s : test 'a: :(foo)' b }
> zsh% compedef _tv :
> zsh% : a<TAB>
> zsh% : a:<TAB>
> zsh% : a:b 
> 
> At this point, the thing that comes after `a:' should have been considered
> an argument, not a new value.  If the argument to `a' is required rather
> than optional, there's no reason the same separator can't be used both
> between the value and its argument, and between the argument and the next
> value.

Yes.  Enabling it to handle this correctly would require changing the
way the current word is parsed inside the compvalues builtin (and a
change in the shell code).  And if we do that we could equally well make
it support an empty argument separator as well, although that would be
slightly more complicated because it would have to step through the
string to see if it has reached a point where the prefix matches on of
the possible values.  Then we would have to make it complete both
possible values and their arguments if we have a value which is a prefix
of another value and gets an argument (with `foo:... foobar' specs we
would have to execute the action for the argument and complete `foobar'
after `foo<TAB>').  That would then require some more changes in shell
code.

Hm.  This should still be less complicated than in _arguments and
actually seems quite doable.  I don't want to promise a working solution
any time soon, though.

Bye
  Sven

Index: Src/Zle/computil.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/computil.c,v
retrieving revision 1.54
diff -u -r1.54 computil.c
--- Src/Zle/computil.c	2001/05/08 12:24:22	1.54
+++ Src/Zle/computil.c	2001/05/10 08:42:31
@@ -2096,7 +2096,7 @@
 
     while (args[0][0] == '-' && (args[0][1] == 's' || args[0][1] == 'S') &&
            !args[0][2]) {
-	if (args[1][0] && args[1][1]) {
+	if (!args[1][0] || (args[1][0] && args[1][1])) {
 	    zwarnnam(nam, "invalid separator: %s", args[1], 0);
 	    return NULL;
 	}

-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: One last _values / compvalues nit, and a nasty crash bug

[Oliver Kiddle]

Back on 10th May, Sven Wischnowsky wrote:
> 
> Bart Schaefer wrote:
> 
> > Now the bug -- press TAB three times after `: a' and get a crash:
> >
> > schaefer<501> function _tv { _values -S '' -s : 'a: :(foo)' b }
> > schaefer<502> compdef _tv :
> > schaefer<503> function _tv { _values -S '' -s : test 'a: :(foo)' b }
> > schaefer<504> : afoo:afoo:BUG: attempt to free more than allocated.
> > _values:149: command not found: 0R^O^H\M-^?\M-^?\M-^?\M-^?@T^O^H\M-^?\M-^?\M-^?\M-^?pR^O^H\M-^?\M-^?\M-^?\M-^?\M-@S^O^H\M-^?\M-^?\M-^?\M-^?\M-pP^O^H\M-^?\M-^?\M-^?\M-^?ws
> > zsh: segmentation fault (core dumped)  zsh-4.0.1-pre-4
> 
> That was because compvalues couldn't handle an empty string as a
> separator.  It still can't, but the patch makes it test that and report
> an error.  More below.

But we use _values in a number of places with and empty string as a
separator so this isn't good, for example:
wget -n<tab>
: $fpath[(<tab>
both now produce the invalid separator error where they previously
worked. I don't suppose it is only the -S separator (as opposed to -s)
that compvalues doesn't like an empty string?

Is there any chance that compvalues, comparguments etc could be given
some sort of documentation. I know this was mentioned recently but it
could go in either the zsh-development-guide or completion-style-guide
file.

Oliver

Re: One last _values / compvalues nit, and a nasty crash bug

[Sven Wischnowsky]

Oliver Kiddle wrote:

> ...
> 
> But we use _values in a number of places with and empty string as a
> separator so this isn't good, for example:
> wget -n<tab>
> : $fpath[(<tab>
> both now produce the invalid separator error where they previously
> worked. I don't suppose it is only the -S separator (as opposed to -s)
> that compvalues doesn't like an empty string?

Yes.  Until now.

Below is the patch to change the parsing or the current word for
_values.  I've tried it with empty value and argument separators and it
seems to work.  I'm going to commit this on the development branch. 
When it has got some testing, it should probably be applied to the other
branch as well (please someone remind me...).

> Is there any chance that compvalues, comparguments etc could be given
> some sort of documentation. I know this was mentioned recently but it
> could go in either the zsh-development-guide or completion-style-guide
> file.

Hrm.  I don't like either place for it.  These builtins are so
specialised that they won't be of any use outside _values and
_arguments.  Would it be enough if I add some comments in computil.c in
the bin_* functions?


Bye
  Sven

Index: Completion/Base/Utility/_values
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Base/Utility/_values,v
retrieving revision 1.4
diff -u -r1.4 _values
--- Completion/Base/Utility/_values	2001/06/01 14:29:11	1.4
+++ Completion/Base/Utility/_values	2001/06/11 11:39:06
@@ -27,7 +27,7 @@
 
     compvalues -V noargs args opts
 
-    if [[ "$PREFIX" = *${argsep}${~test} ]]; then
+    if [[ -n "$argsep" && "$PREFIX" = *${argsep}${~test} ]]; then
       local name
 
       name="${PREFIX%%${argsep}*}"
Index: Src/Zle/computil.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/computil.c,v
retrieving revision 1.58
diff -u -r1.58 computil.c
--- Src/Zle/computil.c	2001/06/01 08:53:55	1.58
+++ Src/Zle/computil.c	2001/06/11 11:39:07
@@ -2103,10 +2103,7 @@
 
     while (args[0][0] == '-' && (args[0][1] == 's' || args[0][1] == 'S') &&
            !args[0][2]) {
-	if (!args[1][0] || (args[1][0] && args[1][1])) {
-	    zwarnnam(nam, "invalid separator: %s", args[1], 0);
-	    return NULL;
-	}
+
         if (args[0][1] == 's') {
             hassep = 1;
             sep = args[1][0];
@@ -2314,20 +2311,122 @@
 static struct cvstate cv_laststate;
 static int cv_parsed = 0, cv_alloced = 0;
 
+/* Get the next value in the string.  Return it's definition and update the
+ * sp pointer to point to the end of the value (plus argument, if any).
+ * If there is no next value, the string pointer is set to null.  In any
+ * case ap will point to the beginning of the argument or will be a null
+ * pointer if there is no argument.
+ */
+
+static Cvval
+cv_next(Cvdef d, char **sp, char **ap)
+{
+    Cvval r = NULL;
+    char *s = *sp;
+
+    if (!*s) {
+        *sp = *ap = NULL;
+
+        return NULL;
+    }
+    if ((d->hassep && !d->sep) || !d->argsep) {
+        char sav, ec, *v = s, *os;
+
+        ec = ((d->hassep && d->sep) ? d->sep : d->argsep);
+
+        do {
+            sav = *++s;
+            *s = '\0';
+            if ((r = cv_get_val(d, v))) {
+                *s = sav;
+
+                break;
+            }
+            *s = sav;
+        } while (*s && *s != ec);
+
+        os = s;
+
+        if (d->hassep && d->sep) {
+            if ((s = strchr(s, d->sep)))
+                *sp = s + 1;
+            else
+                *sp = NULL;
+        } else
+            *sp = s;
+        if (d->argsep && *os == d->argsep) {
+            *ap = os + 1;
+            *sp = NULL;
+        } else if (r && r->type != CVV_NOARG)
+            *ap = os;
+        else
+            *ap = NULL;
+
+        return r;
+
+    } else if (d->hassep) {
+        char *ns = strchr(s, d->sep), *as, *sap, sav;
+        int skip = 0;
+
+        if (d->argsep && (as = strchr(s, d->argsep)) && (!ns || as <= ns)) {
+            *ap = as + 1;
+            ns = strchr(as + 1, d->sep);
+            skip = 1;
+            sap = as;
+        } else {
+            *ap = NULL;
+            sap = ns;
+        }
+        if (sap) {
+            sav = *sap;
+            *sap = '\0';
+        }
+        if ((!(r = cv_get_val(d, s)) || r->type == CVV_NOARG) && skip)
+            ns = as;
+
+        if (sap)
+            *sap = sav;
+
+        *sp = ((!ns || (ns == as && r && r->type != CVV_NOARG)) ? NULL : ns + 1);
+
+        return r;
+    } else {
+        char *as = strchr(s, d->argsep), *sap, sav;
+
+        *sp = NULL;
+
+        if (as) {
+            *ap = as + 1;
+            sap = as;
+            sav = *as;
+            *sap = '\0';
+        } else
+            *ap = sap = NULL;
+
+        r = cv_get_val(d, s);
+
+        if (sap)
+            *sap = sav;
+
+        return r;
+    }
+}
+
 /* Parse the current word. */
 
 static void
 cv_parse_word(Cvdef d)
 {
-    Cvval ptr;
+    Cvval val;
     struct cvstate state;
-    char *str, *eq;
+    char *str, *arg = NULL, *pign = compprefix;
+    int nosfx = 0;
 
     if (cv_alloced)
 	freelinklist(cv_laststate.vals, freestr);
 
-    for (ptr = d->vals; ptr; ptr = ptr->next)
-	ptr->active = 1;
+    for (val = d->vals; val; val = val->next)
+	val->active = 1;
 
     state.d = d;
     state.def = NULL;
@@ -2335,104 +2434,91 @@
     state.vals = (LinkList) znewlinklist();
 
     cv_alloced = 1;
-
-    if (d->hassep) {
-	if (d->sep) {
-	    char *end;
-	    int heq;
-
-	    for (str = compprefix, end = strchr(str, d->sep); end;) {
-		*end = '\0';
-
-		if ((heq = !!(eq = strchr(str, d->argsep))))
-		    *eq++ = '\0';
-		else
-		    eq = "";
-
-		if ((ptr = cv_get_val(d, str))) {
-		    zaddlinknode(state.vals, ztrdup(str));
-		    zaddlinknode(state.vals, ztrdup(eq));
-
-		    cv_inactive(d, ptr->xor);
-		}
-		if (heq)
-		    eq[-1] = d->argsep;
-
-		*end = d->sep;
-		str = end + 1;
-		end = strchr(str, d->sep);
-	    }
-	    ignore_prefix(str - compprefix);
-
-	    if ((str = strchr(compsuffix, d->sep))) {
-		char *beg = str;
-
-		for (str++; str; str = end) {
-		    if ((end = strchr(str, d->sep)))
-			*end = '\0';
-
-		    if ((heq = !!(eq = strchr(str, d->argsep))))
-			*eq++ = '\0';
-		    else
-			eq = "";
-
-		    if ((ptr = cv_get_val(d, str))) {
-			zaddlinknode(state.vals, ztrdup(str));
-			zaddlinknode(state.vals, ztrdup(eq));
-
-			cv_inactive(d, ptr->xor);
-		    }
-		    if (heq)
-			eq[-1] = d->argsep;
-		    if (end)
-			*end++ = d->sep;
-		}
-		ignore_suffix(strlen(beg));
-	    }
-	} else {
-	    char tmp[2];
-
-	    tmp[1] = '\0';
 
-	    for (str = compprefix; *str; str++) {
-		tmp[0] = *str;
-		if ((ptr = cv_get_val(d, tmp))) {
-		    zaddlinknode(state.vals, ztrdup(tmp));
-		    zaddlinknode(state.vals, ztrdup(""));
-
-		    cv_inactive(d, ptr->xor);
-		}
-	    }
-	    for (str = compsuffix; *str; str++) {
-		tmp[0] = *str;
-		if ((ptr = cv_get_val(d, tmp))) {
-		    zaddlinknode(state.vals, ztrdup(tmp));
-		    zaddlinknode(state.vals, ztrdup(""));
-
-		    cv_inactive(d, ptr->xor);
-		}
-	    }
-	    ignore_prefix(strlen(compprefix));
-	    ignore_suffix(strlen(compsuffix));
-	}
-    }
-    str = tricat(compprefix, compsuffix, "");
-    zsfree(compprefix);
-    zsfree(compsuffix);
-    compprefix = str;
-    compsuffix = ztrdup("");
-
-    if ((eq = strchr(str, d->argsep))) {
-	*eq++ = '\0';
-
-	if ((ptr = cv_get_val(d, str)) && ptr->type != CVV_NOARG) {
-	    eq[-1] = d->argsep;
-	    ignore_prefix(eq - str);
-	    state.def = ptr->arg;
-	    state.val = ptr;
-	} else
-	    eq[-1] = d->argsep;
+    for (str = compprefix; str && *str; ) {
+        if ((val = cv_next(d, &str, &arg))) {
+            zaddlinknode(state.vals, ztrdup(val->name));
+            if (arg) {
+                if (str) {
+                    char sav = str[-1];
+
+                    str[-1] = '\0';
+                    zaddlinknode(state.vals, ztrdup(arg));
+                    str[-1] = sav;
+                } else {
+                    zaddlinknode(state.vals, tricat(arg, compsuffix, ""));
+                    nosfx = 1;
+                }
+            } else
+                zaddlinknode(state.vals, ztrdup(""));
+
+            cv_inactive(d, val->xor);
+
+            if (str)
+                pign = str;
+            else
+                val->active = 1;
+        }
     }
+    state.val = val;
+    if (val && arg && !str)
+        state.def = val->arg;
+
+    if (!nosfx && d->hassep) {
+        int ign = 0;
+        char *more = NULL;
+
+        ignore_prefix(pign - compprefix);
+
+        if (!d->sep && (!val || val->type == CVV_NOARG)) {
+            ign = strlen(compsuffix);
+            more = compsuffix;
+        } else {
+            if (d->sep) {
+                char *ns = strchr(compsuffix, d->sep), *as;
+
+                if (d->argsep && (as = strchr(compsuffix, d->argsep)) &&
+                    (!ns || as <= ns)) {
+                    ign = strlen(as);
+                } else
+                    ign = (ns ? strlen(ns) : 0);
+
+                more = (ns ? ns + 1 : NULL);
+            } else if (d->argsep) {
+                char *as;
+
+                if ((as = strchr(compsuffix, d->argsep)))
+                    ign = strlen(as);
+            }
+        }
+        if (ign)
+            ignore_suffix(ign);
+
+        while (more && *more) {
+            if ((val = cv_next(d, &str, &arg))) {
+                zaddlinknode(state.vals, ztrdup(val->name));
+                if (arg) {
+                    if (str) {
+                        char sav = str[-1];
+
+                        str[-1] = '\0';
+                        zaddlinknode(state.vals, ztrdup(arg));
+                        str[-1] = sav;
+                    } else {
+                        zaddlinknode(state.vals, tricat(arg, compsuffix, ""));
+                        nosfx = 1;
+                    }
+                } else
+                    zaddlinknode(state.vals, ztrdup(""));
+
+                cv_inactive(d, val->xor);
+            }
+        }
+    } else if (arg)
+        ignore_prefix(arg - compprefix);
+    else
+        ignore_prefix(pign - compprefix);
+
     memcpy(&cv_laststate, &state, sizeof(state));
 }
 

-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

seg fault from _arguments

[Oliver Kiddle]

This is using a zsh I compiled yesterday from the development branch:

To reproduce it do:
	mount -ob<tab>

There would need to be a space after the -o for this to complete to
something useful and it works well if a space is there. I just missed it
once.

Sven Wischnowsky wrote:
> 
> > worked. I don't suppose it is only the -S separator (as opposed to -s)
> > that compvalues doesn't like an empty string?
> 
> Yes.  Until now.

Great, thanks.

> Hrm.  I don't like either place for it.  These builtins are so
> specialised that they won't be of any use outside _values and
> _arguments.  Would it be enough if I add some comments in computil.c in
> the bin_* functions?

That would be fine and as good a place as any.

Also, if I replace the top of _values with this:
  local garbage subopts usecc
  zparseopts -D -a garbage C=usecc O:=subopts M: J: V: 1 2 n F: X:
  subopts=( "${(@P)subopts[2]}" )
why does completion after `mount -o block=' not work. I get things like
`%Bblock\ size%b', `-M', `-J', `ws' (i.e compadd options) offered as
completions instead of 512, 1024 and 2048. Note that this is on Linux so
that part of _mount.

Oliver

PATCH: Re: seg fault from _arguments

[Sven Wischnowsky]

Oliver Kiddle wrote:

> This is using a zsh I compiled yesterday from the development branch:
> 
> To reproduce it do:
> 	mount -ob<tab>
> 
> There would need to be a space after the -o for this to complete to
> something useful and it works well if a space is there. I just missed it
> once.

Oops.  When changing the test on line 1442, I should have changed the
test on line 1436, too.

> ...
> 
> Great, thanks.
> 
> > Hrm.  I don't like either place for it.  These builtins are so
> > specialised that they won't be of any use outside _values and
> > _arguments.  Would it be enough if I add some comments in computil.c in
> > the bin_* functions?
> 
> That would be fine and as good a place as any.

Done that.

> Also, if I replace the top of _values with this:
>   local garbage subopts usecc
>   zparseopts -D -a garbage C=usecc O:=subopts M: J: V: 1 2 n F: X:
>   subopts=( "${(@P)subopts[2]}" )
> why does completion after `mount -o block=' not work. I get things like
> `%Bblock\ size%b', `-M', `-J', `ws' (i.e compadd options) offered as
> completions instead of 512, 1024 and 2048. Note that this is on Linux so
> that part of _mount.

Because the first and the last line together make subopts contain one
element: an empty string.  I've fixed and added it to _values, while I
was there anyway.


The other stuff in this patch (_mount and _dir_list) fixes something I
happened to see.  _mount was using `_files -S , -/' to complete lists of
directory names -- which obviously can't work correctly.  We've had a
utility function for that, _dir_list, which just wasn't intelligent
enough, so I've changed it a bit.

I'm planning to commit this to the 4.0 branch, too, together with 14841,
I've just decided.  Also, I think we could apply the BSD-_mount patch
there -- it just adds some more case-branches to _mount.


Bye
  Sven

Index: Completion/Base/Utility/_values
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Base/Utility/_values,v
retrieving revision 1.5
diff -u -r1.5 _values
--- Completion/Base/Utility/_values	2001/06/11 11:46:23	1.5
+++ Completion/Base/Utility/_values	2001/06/13 11:02:10
@@ -1,15 +1,11 @@
 #autoload
 
-local subopts opt usecc
+local subopts opt usecc garbage
 
 subopts=()
-while [[ "$1" = -(O*|C) ]]; do
-  case "$1" in
-  -C) usecc=yes; shift ;;
-  -O) subopts=( "${(@P)2}" ); shift 2 ;;
-  *)  subopts=( "${(@P)${1[3,-1]}}" ); shift ;;
-  esac
-done
+zparseopts -D -E -a garbage C=usecc O:=subopts M: J: V: 1 2 n F: X:
+
+(( $#subopts )) && subopts=( "${(@P)subopts[2]}" )
 
 if compvalues -i "$@"; then
 
Index: Completion/Unix/Command/_mount
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Unix/Command/_mount,v
retrieving revision 1.3
diff -u -r1.3 _mount
--- Completion/Unix/Command/_mount	2001/06/13 08:45:05	1.3
+++ Completion/Unix/Command/_mount	2001/06/13 11:02:10
@@ -444,7 +444,7 @@
   irix*)
     args=( -s
       '-a[mount all filesystems in /etc/fstab]'
-      '-b[mount all filesystems in /etc/fstab except those listed]:list of directories:_files -/ -S,'
+      '-b[mount all filesystems in /etc/fstab except those listed]:list of directories:_dir_list -s,'
       '-c[check any dirty filesystems before mounting]'
       '-f[fake a new /etc/mtab entry, but don'\''t mount any filesystems]'
       '-h[mount all filesystems associated with host]:hostnames:_hosts'
@@ -575,7 +575,7 @@
     irix*)
       args=(
 	'-a[unmount all mounted file systems]'
-	'-b[unmount all filesystems in /etc/fstab except those listed]:list of directories:_files -/ -S,'
+	'-b[unmount all filesystems in /etc/fstab except those listed]:list of directories:_dir_list -s,'
 	'-h[unmount all filesystems associated with host]:hostnames:_hosts'
 	'-k[kill all processes with files open on filesystems before unmounting]'
 	'-t[unmount all filesystems of specified type]:file system type:->fstype'
Index: Completion/Unix/Type/_dir_list
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Unix/Type/_dir_list,v
retrieving revision 1.1
diff -u -r1.1 _dir_list
--- Completion/Unix/Type/_dir_list	2001/04/02 11:37:01	1.1
+++ Completion/Unix/Type/_dir_list	2001/06/13 11:02:10
@@ -1,7 +1,27 @@
 #autoload
 
-local suf
+# options:
+#  -s <sep> to specify the separator (default is a colon)
+#  -S       to say that the separator should be added as a suffix (instead
+#           of the default slash)
 
-compset -P '*:'
-compset -S ':*' || suf=":"
-_files -S "$suf" -r ': \t\t\-' -/
+local sep=: dosuf suf
+
+while [[ "$1" = -(s*|S) ]]; do
+  case "$1" in
+  -s)  sep="$2"; shift 2;;
+  -s*) sep="${1[3,-1]}"; shift;;
+  -S)  dosuf=yes; shift;;
+  esac
+done
+
+compset -P "*${sep}"
+compset -S "${sep}*" || suf="$sep"
+
+if [[ -n "$dosuf" ]]; then
+  suf=(-S "$suf")
+else
+  suf=()
+fi
+
+_files "$suf[@]" -r "${sep}"' /\t\t\-' -/ "$@"
Index: Src/Zle/computil.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/computil.c,v
retrieving revision 1.59
diff -u -r1.59 computil.c
--- Src/Zle/computil.c	2001/06/11 11:46:23	1.59
+++ Src/Zle/computil.c	2001/06/13 11:02:11
@@ -1433,7 +1433,7 @@
 	    }
 	} else if (state.opt == 2 && d->single &&
 		   ((state.curopt = ca_get_sopt(d, line, &pe, &sopts)) ||
-		    (sopts && nonempty(sopts)))) {
+		    (cur != compcurrent && sopts && nonempty(sopts)))) {
 	    /* Or maybe it's a single-letter option? */
 
 	    char *p;
@@ -1782,6 +1782,9 @@
     }
     switch (args[0][1]) {
     case 'i':
+        /* This initialises the internal data structures. Arguments are the
+         * auto-description string, the optional -s, -S, -A and -M options
+         * given to _arguments and the specs. */
 	if (compcurrent > 1 && compwords[0]) {
 	    Cadef def;
 	    int cap = ca_parsed, multi, first = 1, use, ret = 0;
@@ -1843,6 +1846,11 @@
 	return 1;
 
     case 'D':
+        /* This returns the descriptions, actions and sub-contexts for the
+         * things _arguments has to execute at this place on the line (the
+         * sub-contexts are used as tags).
+         * The return value is particularly important here, it says if 
+         * there are arguments to completely at all. */
 	{
 	    LinkList descr, act, subc;
 	    Caarg arg;
@@ -1874,6 +1882,10 @@
 	    return ret;
 	}
     case 'O':
+        /* This returns the descriptions for the options in the arrays whose
+         * names are given as arguments.  The descriptions are strings in a
+         * form usable by _describe.  The return value says if there are any
+         * options to be completed. */
 	{
 	    LinkList next = newlinklist();
 	    LinkList direct = newlinklist();
@@ -1929,6 +1941,12 @@
 	    return (ca_laststate.singles ? 2 : 1);
 	}
     case 'L':
+        /* This tests if the beginning of the current word matches an option.
+         * It is for cases like `./configure --pre=/<TAB>' which should
+         * complete to `--prefix=/...'.  The options name isn't fully typed
+         * and _arguments finds out that there is no option `--pre' and that
+         * it should complete some argument to an option.  It then uses -L
+         * to find the option the argument is for. */
 	{
 	    LinkList descr, act, subc;
 	    Caopt opt;
@@ -1955,6 +1973,11 @@
 	    return ret;
 	}
     case 's':
+        /* This returns zero if we are completing single letter options.
+         * It also uses its argument as the name of a parameter and sets
+         * that to a string describing the argument behaviour of the last
+         * option in the current word so that we can get the auto-suffix
+         * right. */
 	for (; lstate; lstate = lstate->snext)
 	    if (lstate->d->single && lstate->singles &&
 		lstate->actopts
@@ -1974,14 +1997,25 @@
 	    }
 	return 1;
     case 'M':
+        /* This returns the match specs defined for the set of specs we are
+         * using.  Returned, as usual in a parameter whose name is given as
+         * the argument. */
 	setsparam(args[1], ztrdup(ca_laststate.d->match));
 	return 0;
     case 'a':
+        /* This just sets the return value.  To zero if there would be or
+         * were any normal arguments to be completed.  Used to decide if
+         * _arguments should say `no arguments' or `no more arguments'. */
 	for (; lstate; lstate = lstate->snext)
 	    if (lstate->d->args || lstate->d->rest)
 		return 0;
 	return 1;
     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. */
 	{
 	    Castate s;
 	    char **ret, **p;
@@ -2563,6 +2597,8 @@
     }
     switch (args[0][1]) {
     case 'i':
+        /* This initialises the internal data structures.  The arguments are
+         * just the arguments that were given to _values itself. */
 	{
 	    Cvdef def = get_cvdef(nam, args + 1);
 	    int cvp = cv_parsed;
@@ -2581,6 +2617,9 @@
 	return 1;
 
     case 'D':
+        /* This returns the description and action to use if we are at
+         * a place where some action has to be used at all.  In that case
+         * zero is returned and non-zero otherwise. */
 	{
 	    Caarg arg = cv_laststate.def;
 
@@ -2593,6 +2632,8 @@
 	    return 1;
 	}
     case 'C':
+        /* This returns the sub-context (i.e.: the tag) to use when executing
+         * an action. */
 	{
 	    Caarg arg = cv_laststate.def;
 
@@ -2604,6 +2645,10 @@
 	    return 1;
 	}
     case 'V':
+        /* This is what -O is for comparguments: it returns (in three arrays)
+         * the values for values without arguments, with arguments and with
+         * optional arguments (so that we can get the auto-suffixes right).
+         * As for comparguments, the strings returned are usable for _describe. */
 	{
 	    LinkList noarg = newlinklist();
 	    LinkList arg = newlinklist();
@@ -2637,6 +2682,8 @@
 	    return 0;
 	}
     case 's':
+        /* This returns the value separator, if any, and sets the return
+         * value to say if there is such a separator. */
 	if (cv_laststate.d->hassep) {
 	    char tmp[2];
 
@@ -2648,6 +2695,7 @@
 	}
 	return 1;
     case 'S':
+        /* Like -s, but for the separator between values and their arguments. */
 	{
 	    char tmp[2];
 
@@ -2657,9 +2705,15 @@
 	}
 	return 0;
     case 'd':
+        /* This returns the description string (first argument to _values)
+         * which is passed down to _describe. */
 	setsparam(args[1], ztrdup(cv_laststate.d->descr));
 	return 0;
     case 'L':
+        /* Almost the same as for comparguments.  This gets a value name
+         * and returns the description and action of its first argument, if
+         * any.  The rest (prefix matching) is in _values.  Return non-zero
+         * if there is no such option. */
 	{
 	    Cvval val = cv_get_val(cv_laststate.d, args[1]);
 
@@ -2675,6 +2729,8 @@
 	    return 1;
 	}
     case 'v':
+        /* Again, as for comparguments.  This returns the values and their
+         * arguments as an array which will be stored in val_args in _values. */
 	if (cv_laststate.vals) {
 	    char **ret, **p;
 	    LinkNode n;

-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: Re: seg fault from _arguments

[Oliver Kiddle]

Sven Wischnowsky wrote:

> 14841 isn't applied to the 4.0 branch either. Has anyone found any
> problems with this? (It fixes a real -- and somewhat ugly -- bug.)

I recompiled after 14841 and have had no problems since other than the
one you just fixed.

> Because the first and the last line together make subopts contain one
> element: an empty string.  I've fixed and added it to _values, while I
> was there anyway.

Thanks.

> I'm planning to commit this to the 4.0 branch, too, together with 14841,
> I've just decided.  Also, I think we could apply the BSD-_mount patch
> there -- it just adds some more case-branches to _mount.

Seems sensible to me. _mount included, especially as it'll save the
FreeBSD folks the hassle of maintaining their patch.

Oliver

Re: PATCH: Re: seg fault from _arguments

[Jos Backus]

On Wed, Jun 13, 2001 at 01:00:48PM +0200, Sven Wischnowsky wrote:
> Also, I think we could apply the BSD-_mount patch
> there -- it just adds some more case-branches to _mount.

Yes. Thanks Sven.

-- 
Jos Backus                 _/  _/_/_/        "Modularity is not a hack."
                          _/  _/   _/                -- D. J. Bernstein
                         _/  _/_/_/             
                    _/  _/  _/    _/
josb@cncdsl.com     _/_/   _/_/_/            use Std::Disclaimer;