PATCH: Re: seg fault from _arguments

[Sven Wischnowsky <wischnow@xxxxxxxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxxxx