PATCH: tidy up some of the _arguments set code

[Oliver Kiddle <okiddle@xxxxxxxxxxx>]

This patch cleans up some code associated with exclusion lists in _arguments.

In particular, there was some code for applying explicit exclusions between
sets. Newer code manages this differently and the left over bits of old code
cause some minor problems. This fixes one of the things I had marked as
questionable in the test cases.

I've also got rid of a couple of struct members that were not
used. ca_inactive was changed at some point to only ever return 0 without
changing callers to ignore the return value. A few counters - ret->nodopts
etc were mirrored in local variables which doesn't make the code easier to
understand. Perhaps that was faster if the compiler put them in registers
but there's a better way to optimise that function if really needed
(not repeating the parse of common options).

There's also the odd extra comment and test case here.

Oliver

diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index d2f0c99..cc879c4 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -917,7 +917,6 @@ struct cadef {
     int argsactive;		/* if normal arguments are still allowed */
 				/* used while parsing a command line */
     char *set;			/* set name prefix (<name>-), shared */
-    char *sname;		/* set name */
     int flags;			/* see CDF_* below */
     char *nonarg;		/* pattern for non-args (-A argument) */
 };
@@ -956,7 +955,7 @@ struct caarg {
     char *end;			/* end-pattern for ::<pat>:... */
     char *opt;			/* option name if for an option */
     int num;			/* it's the num'th argument */
-    int min;			/* it's also this argument, using opt. args */
+    int min;			/* earliest possible arg pos, given optional args */
     int direct;			/* true if argument number was given explicitly */
     int active;			/* still allowed on command line */
     char *set;			/* set name, shared */
@@ -1020,7 +1019,6 @@ freecadef(Cadef d)
 	s = d->snext;
 	zsfree(d->match);
 	zsfree(d->set);
-	zsfree(d->sname);
 	if (d->defs)
 	    freearray(d->defs);
 
@@ -1147,8 +1145,11 @@ alloc_cadef(char **args, int single, char *match, char *nonarg, int flags)
 	ret->defs = NULL;
 	ret->ndefs = 0;
     }
+    ret->nopts = 0;
+    ret->ndopts = 0;
+    ret->nodopts = 0;
     ret->lastt = time(0);
-    ret->set = ret->sname = NULL;
+    ret->set = NULL;
     if (single) {
 	ret->single = (Caopt *) zalloc(256 * sizeof(Caopt));
 	memset(ret->single, 0, 256 * sizeof(Caopt));
@@ -1184,11 +1185,9 @@ parse_cadef(char *nam, char **args)
     char **orig_args = args, *p, *q, *match = "r:|[_-]=* r:|=*", **xor, **sargs;
     char *adpre, *adsuf, *axor = NULL, *doset = NULL, **setp = NULL;
     char *nonarg = NULL;
-    int single = 0, anum = 1, xnum, nopts, ndopts, nodopts, flags = 0;
+    int single = 0, anum = 1, xnum, flags = 0;
     int state = 0, not = 0;
 
-    nopts = ndopts = nodopts = 0;
-
     /* First string is the auto-description definition. */
 
     for (p = args[0]; *p && (p[0] != '%' || p[1] != 'd'); p++);
@@ -1255,8 +1254,6 @@ parse_cadef(char *nam, char **args)
 
     all = ret = alloc_cadef(orig_args, single, match, nonarg, flags);
     optp = &(ret->opts);
-    anum = 1;
-
     sargs = args;
 
     /* Get the definitions. */
@@ -1267,8 +1264,9 @@ parse_cadef(char *nam, char **args)
 		char *p;
 		int l;
 
-		if (setp)
+		if (setp) /* got to first set: skip to ours */
 		    args = setp;
+		/* else we were the first set so don't need to skip */
 		p = *++args;
 		l = strlen(p) - 1;
 		if (*p == '(' && p[l] == ')') {
@@ -1277,20 +1275,15 @@ parse_cadef(char *nam, char **args)
 		} else
 		    axor = NULL;
 		ret->set = doset = tricat(p, "-", "");
-		ret->sname = ztrdup(p);
 		state = 1;
-	    } else {
+	    } else { /* starting new set */
 		setp = args;
 		state = 0;
 		args = sargs - 1;
 		doset = NULL;
-		ret->nopts = nopts;
-		ret->ndopts = ndopts;
-		ret->nodopts = nodopts;
 		set_cadef_opts(ret);
 		ret = ret->snext = alloc_cadef(NULL, single, match, nonarg, flags);
 		optp = &(ret->opts);
-		nopts = ndopts = nodopts = 0;
 		anum = 1;
 	    }
 	    continue;
@@ -1526,13 +1519,13 @@ parse_cadef(char *nam, char **args)
 	    opt->xor = (again == 1 && xor ? zarrdup(xor) : xor);
 	    opt->type = otype;
 	    opt->args = oargs;
-	    opt->num = nopts++;
+	    opt->num = ret->nopts++;
 	    opt->not = not;
 
 	    if (otype == CAO_DIRECT || otype == CAO_EQUAL)
-		ndopts++;
+		ret->ndopts++;
 	    else if (otype == CAO_ODIRECT || otype == CAO_OEQUAL)
-		nodopts++;
+		ret->nodopts++;
 
 	    /* If this is for single-letter option we also store a
 	     * pointer for the definition in the array for fast lookup.
@@ -1584,7 +1577,7 @@ parse_cadef(char *nam, char **args)
 		continue;
 
 	    if ((direct = idigit(*p))) {
-		/* Argment number is given. */
+		/* Argument number is given. */
 		int num = 0;
 
 		while (*p && idigit(*p))
@@ -1630,9 +1623,6 @@ parse_cadef(char *nam, char **args)
 		ret->args = arg;
 	}
     }
-    ret->nopts = nopts;
-    ret->ndopts = ndopts;
-    ret->nodopts = nodopts;
     set_cadef_opts(ret);
 
     return all;
@@ -1776,12 +1766,9 @@ ca_get_arg(Cadef d, int n)
  *   d: option definitions for a set
  *   pass either:
  *     xor: a list if exclusions
- *     opts: if set, all options excluded leaving only nornal/rest arguments
- * if ca_xor list initialised, exclusions are added to it */
+ *     opts: if set, all options excluded leaving only nornal/rest arguments */
 
-static LinkList ca_xor;
-
-static int
+static void
 ca_inactive(Cadef d, char **xor, int cur, int opts, char *optname)
 {
     if ((xor || opts) && cur <= compcurrent) {
@@ -1792,8 +1779,6 @@ ca_inactive(Cadef d, char **xor, int cur, int opts, char *optname)
 	for (; (x = (opts ? "-" : *xor)); xor++) {
             if (optname && optname[0] == x[0] && strcmp(optname, x))
                 continue;
-	    if (ca_xor)
-		addlinknode(ca_xor, x);
 	    set = 0;
 	    if (sl > 0) {
 		if (strpfx(d->set, x)) {
@@ -1846,7 +1831,6 @@ ca_inactive(Cadef d, char **xor, int cur, int opts, char *optname)
 		break;
 	}
     }
-    return 0;
 }
 
 /* State when parsing a command line. */
@@ -1875,7 +1859,6 @@ struct castate {
     int curpos;		/* current word position */
     int argend;         /* total number of words */
     int inopt;		/* set to current word pos if word is a recognised option */
-    int inrest;		/* unused */
     int inarg;          /* in a normal argument */
     int nth;		/* number of current normal arg */
     int doff;		/* length of current option */
@@ -1978,7 +1961,7 @@ ca_parse_line(Cadef d, int multi, int first)
     state.argbeg = state.optbeg = state.nargbeg = state.restbeg = state.actopts =
 	state.nth = state.inopt = state.inarg = state.opt = state.arg = 1;
     state.argend = argend = arrlen(compwords) - 1;
-    state.inrest = state.doff = state.singles = state.oopt = 0;
+    state.doff = state.singles = state.oopt = 0;
     state.curpos = compcurrent;
     state.args = znewlinklist();
     state.oargs = (LinkList *) zalloc(d->nopts * sizeof(LinkList));
@@ -2025,10 +2008,9 @@ ca_parse_line(Cadef d, int multi, int first)
         remnulargs(line);
         untokenize(line);
 
-	if (ca_inactive(d, argxor, cur, 0, NULL) ||
-	    ((d->flags & CDF_SEP) && cur != compcurrent && !strcmp(line, "--"))) {
-	    if (ca_inactive(d, NULL, cur, 1, NULL))
-		return 1;
+	ca_inactive(d, argxor, cur, 0, NULL);
+	if ((d->flags & CDF_SEP) && cur != compcurrent && !strcmp(line, "--")) {
+	    ca_inactive(d, NULL, cur, 1, NULL);
 	    continue;
 	}
 
@@ -2104,9 +2086,8 @@ ca_parse_line(Cadef d, int multi, int first)
 	    if (!state.oargs[state.curopt->num])
 		state.oargs[state.curopt->num] = znewlinklist();
 
-	    if (ca_inactive(d, state.curopt->xor, cur, 0,
-                            (cur == compcurrent ? state.curopt->name : NULL)))
-		return 1;
+	    ca_inactive(d, state.curopt->xor, cur, 0,
+		    (cur == compcurrent ? state.curopt->name : NULL));
 
 	    /* Collect the argument strings. Maybe. */
 
@@ -2159,9 +2140,8 @@ ca_parse_line(Cadef d, int multi, int first)
 		    if (!state.oargs[tmpopt->num])
 			state.oargs[tmpopt->num] = znewlinklist();
 
-		    if (ca_inactive(d, tmpopt->xor, cur, 0,
-                                    (cur == compcurrent ? tmpopt->name : NULL)))
-			return 1;
+		    ca_inactive(d, tmpopt->xor, cur, 0,
+			    (cur == compcurrent ? tmpopt->name : NULL));
 		}
 	    }
 	    if (state.def &&
@@ -2194,9 +2174,8 @@ ca_parse_line(Cadef d, int multi, int first)
 	else if (state.arg &&
 		 (!napat || cur <= compcurrent || !pattry(napat, line))) {
 	    /* Otherwise it's a normal argument. */
-	    if (napat && cur <= compcurrent &&
-		    ca_inactive(d, NULL, cur + 1, 1, NULL))
-		return 1;
+	    if (napat && cur <= compcurrent)
+		ca_inactive(d, NULL, cur + 1, 1, NULL);
 
 	    arglast = 1;
 	    /* if this is the first normal arg after an option, may have been
@@ -2231,7 +2210,6 @@ ca_parse_line(Cadef d, int multi, int first)
 		if (ca_laststate.def)
 		    break;
 
-		state.inrest = 0;
 		state.opt = (cur == state.nargbeg + 1 &&
 			     (!multi || !*line || 
 			      *line == '-' || *line == '+'));
@@ -2539,45 +2517,27 @@ bin_comparguments(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 	if (compcurrent > 1 && compwords[0]) {
 	    Cadef def;
 	    int cap = ca_parsed, multi, first = 1, use, ret = 0;
-	    LinkList cax = ca_xor, nx;
-	    LinkNode node;
 	    Castate states = NULL, sp;
-	    char *xor[2];
 
 	    ca_parsed = 0;
-	    xor[1] = NULL;
 
 	    if (!(def = get_cadef(nam, args + 1)))
 		return 1;
 
 	    multi = !!def->snext; /* if we have sets */
 	    ca_parsed = cap;
-	    ca_xor = (multi ? newlinklist() : NULL);
 
 	    while (def) { /* for each set */
 		use = !ca_parse_line(def, multi, first);
-		nx = ca_xor;
-		ca_xor = NULL; /* don't want to duplicate the xors in the list */
-		while ((def = def->snext)) {
-		    if (nx) {
-			for (node = firstnode(nx); node; incnode(node)) {
-			    xor[0] = (char *) getdata(node);
-			    if (!strcmp(xor[0], def->sname) ||
-				ca_inactive(def, xor, compcurrent, 0, NULL))
-				break; /* exclude this whole set */
-			}
-			if (!node) /* continue with this set */
-			    break;
-		    }
-		    /* entire set was excluded, continue to next set */
-		}
-		ca_xor = nx;
+		def = def->snext;
 		if (use && def) {
+		    /* entry needed so save it into list */
 		    sp = (Castate) zalloc(sizeof(*sp));
 		    memcpy(sp, &ca_laststate, sizeof(*sp));
 		    sp->snext = states;
 		    states = sp;
 		} else if (!use && !def) {
+		    /* final entry not needed */
 		    if (states) {
 			freecastate(&ca_laststate);
 			memcpy(&ca_laststate, states, sizeof(*sp));
@@ -2589,7 +2549,6 @@ bin_comparguments(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 		}
 		first = 0;
 	    }
-	    ca_xor = cax;
 	    ca_parsed = 1;
 	    ca_laststate.snext = states;
 
@@ -2602,7 +2561,7 @@ bin_comparguments(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
          * 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. */
+         * there are arguments to complete at all. */
 	{
 	    LinkList descr, act, subc;
 	    Caarg arg;
diff --git a/Test/Y03arguments.ztst b/Test/Y03arguments.ztst
index d09b118..0b797ac 100644
--- a/Test/Y03arguments.ztst
+++ b/Test/Y03arguments.ztst
@@ -64,6 +64,20 @@
 >line: {tst arg1 }{}
 >MESSAGE:{no more arguments}
 
+ tst_arguments -a '2:desc2:(arg2)'
+ comptest $'tst a1\t \t'
+0:second argument but no first argument
+>line: {tst a1}{}
+>MESSAGE:{no more arguments}
+>line: {tst a1 arg2 }{}
+
+ tst_arguments '2:desc2:(arg2)' '*:rest:(rest)'
+ comptest $'tst \t\t\t'
+0:second and rest arguments but no first argument
+>line: {tst rest }{}
+>line: {tst rest arg2 }{}
+>line: {tst rest arg2 rest }{}
+
  tst_arguments '-\+[opt]'
  comptest $'tst -\C-d'
 0:-+
@@ -82,6 +96,14 @@
 >line: {tst +o -o }{}
 >MESSAGE:{no arguments}
 
+ tst_arguments +-o
+ comptest $'tst \t'
+0:option beginning with + and -, specified the other way around
+>line: {tst }{}
+>DESCRIPTION:{option}
+>NO:{+o}
+>NO:{-o}
+
  tst_arguments '-o:1:(a):2:(b)'
  comptest $'tst \t\t\t'
 0:two option arguments
@@ -206,6 +228,15 @@
 0:opt_args with multiple arguments and quoting of colons and backslashes
 >line: {tst -a 1:x \2 1\:x:\\2 }{}
 
+ tst_arguments -a -b
+ comptest $'tst  rest -\t\C-w\eb\C-b-\t'
+0:option completion with rest arguments on the line but not in the specs
+>line: {tst  rest -}{}
+>line: {tst -}{ rest }
+>DESCRIPTION:{option}
+>NO:{-a}
+>NO:{-b}
+
  tst_arguments '-a' '*::rest:{compadd - -b}'
  comptest $'tst arg -\t'
 0:rest arguments
@@ -409,7 +440,6 @@
 0:exclude own set from an option
 >line: {tst -m -a }{}
 
-# the following two tests only verify the current questionable behaviour
  tst_arguments - set1 '(set2)-a' -m -n - set2 -a -t -u
  comptest $'tst -a -\t'
 0:exclude later set from an option common to both
@@ -417,17 +447,24 @@
 >DESCRIPTION:{option}
 >NO:{-m}
 >NO:{-n}
-
- tst_arguments - set2 -a -t -u - set1 '(set2)-a' -m -n
- comptest $'tst -a -\t'
-0:exclude later set from an option common to both
->line: {tst -a -}{}
->DESCRIPTION:{option}
->NO:{-m}
->NO:{-n}
 >NO:{-t}
 >NO:{-u}
 
+ tst_arguments - set2 -a -t -u - set1 '(set2)-a' -m -n
+ comptest $'tst -a -\t'
+0:exclude earlier set from an option common to both
+>line: {tst -a -}{}
+>DESCRIPTION:{option}
+>NO:{-m}
+>NO:{-n}
+>NO:{-t}
+>NO:{-u}
+
+ tst_arguments -x - '(set1)' -a -b - '(set2)' -m -n
+ comptest $'tst -m -\t'
+0:single option sets are still mutually exclusive
+>line: {tst -m -x }{}
+
  tst_arguments '(-)-h' -a -b -c
  comptest $'tst -h -\t'
 0:exclude all other options
@@ -459,6 +496,24 @@ F:shouldn't offer -b as it is already on the command-line
 >NO:{-d}
 F:the first tab press shouldn't offer -d since -a is on the command line
 
+ tst_arguments -s : -ad '(-d)-a' -b -ca -d
+ comptest $'tst -ad\t-b\t'
+0:option clumping mixed with real option that looks like clump
+>line: {tst -ad }{}
+>line: {tst -ad -b}{}
+>DESCRIPTION:{option}
+>NO:{-a}
+>NO:{-d}
+
+ tst_arguments -s : '(-d)-a+:arg:(b)' '(-c)-b' -c -d
+ comptest $'tst -ab\t-\t'
+0:option clumping mixed with direct argument
+>line: {tst -ab }{}
+>line: {tst -ab -}{}
+>DESCRIPTION:{option}
+>NO:{-b}
+>NO:{-c}
+
  tst_arguments '-a:arg' -b '(-b)-c'
  comptest $'tst -a -c -\t'
 0:exclusion with option argument that looks like an option