40226: tidy up some of the _arguments set code

Remove old code for applying explicit exclusions between sets which
fixes some odd behaviour. Some struct members were unused. Also added
some comments and test cases.

3 files changed, 91 insertions, 72 deletions

diff --git a/ChangeLog b/ChangeLog
index 0170dc6ef..2f6023fdc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2017-01-04  Oliver Kiddle  <opk@zsh.org>
+
+	* 40226: Src/Zle/computil.c, Test/Y03arguments.ztst:
+	tidy up some of the _arguments set code
+
 2017-01-03  Peter Stephenson  <p.stephenson@samsung.com>
 
 	* 40265: Src/pattern.c: fix continuing problems with Meta characters
diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index d2f0c999b..cc879c445 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 */
-
-static LinkList ca_xor;
+ *     opts: if set, all options excluded leaving only nornal/rest arguments */
 
-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 d09b118a2..0b797ac03 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,10 +447,12 @@
 >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 later set from an option common to both
+0:exclude earlier set from an option common to both
 >line: {tst -a -}{}
 >DESCRIPTION:{option}
 >NO:{-m}
@@ -428,6 +460,11 @@
 >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