PATCH: revert 39611 and add tests and comments

[Oliver Kiddle <okiddle@xxxxxxxxxxx>]

As seen with _zip, 39611 can mess up the exclusion of options for sets.
I'll address the original issue again after 5.3. 

In an attempt to avoid a repeat when I come to do that, this adds
fairly comprehensive tests of _arguments. No tests for the little
used -w because it is either broken or I was too confused. Or -n
for somewhat similar reasons.  A few tests are commented where I
think the behaviour is not right. And I've tried to add comments
to the code while deciphering it.

I've got two approaches to alleviating the original bug working (not
in this patch). One is to check all sets to see if an unknown option
belongs to another set before skipping the current set. The other is not
to skip sets immediately but mark them as fallback only. Later code then
tries again if all the sets were set to fallback. That ends up being
more invasive. Ideal is perhaps to combine both approaches.

The original code for sets added explicit exclusions between options
for the different sets and I had the notion that this was still how
it was implemented. There's still some remnants of this: the call
to ca_inactive in the bin_comparguments handling of -i in particular.
This looks redundant to me: it only applies to sets declared later
in the line and then the options are all reset to active anyway
when ca_parse_line is called for the later set.

What it actually does is treat each set independently but skip any set
if it comes across an unknown option. Normally unknown options are
assumed to be one of the normal arguments.

As a general principle, I prefer to be forgiving of unknown options
because completion functions are not always up-to-date or accurate for
the installed software. Having looked closer at the code, we could do
with a simple way to indicate whether a normal or rest argument can
start with - or +. Often they can't and knowing this would help both in
this regard and for the sets. Note that this is not the same as what -A
does. Any ideas on syntax for this would be good?

We ought to strip unknown options in general when chaining
_arguments calls, e.g. for:
  git --foo checkout 
it'd be better to strip the --foo and carry on.

Oliver

diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index c78167329..192ddeab9 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -914,7 +914,7 @@ struct cadef {
     int lastt;			/* last time this was used */
     Caopt *single;		/* array of single-letter options */
     char *match;		/* -M spec to use */
-    int argsactive;		/* if arguments are still allowed */
+    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 */
@@ -922,7 +922,7 @@ struct cadef {
     char *nonarg;		/* pattern for non-args (-A argument) */
 };
 
-#define CDF_SEP 1
+#define CDF_SEP 1		/* -S was specified: -- terminates options */
 
 /* Description for an option. */
 
@@ -939,11 +939,11 @@ struct caopt {
     int not;			/* don't complete this option (`!...') */
 };
 
-#define CAO_NEXT    1
-#define CAO_DIRECT  2
-#define CAO_ODIRECT 3
-#define CAO_EQUAL   4
-#define CAO_OEQUAL  5
+#define CAO_NEXT    1		/* argument follows in next argument (`-opt:...') */
+#define CAO_DIRECT  2		/* argument follows option directly (`-opt-:...') */
+#define CAO_ODIRECT 3		/* argument may follow option directly (`-opt+:...') */
+#define CAO_EQUAL   4		/* argument follows mandatory equals (`-opt=-:...') */
+#define CAO_OEQUAL  5		/* argument follows optional equals (`-opt=:...') */
 
 /* Description for an argument */
 
@@ -957,7 +957,7 @@ struct caarg {
     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 direct;			/* number was given directly */
+    int direct;			/* true if argument number was given explicitly */
     int active;			/* still allowed on command line */
     char *set;			/* set name, shared */
 };
@@ -1772,7 +1772,12 @@ ca_get_arg(Cadef d, int n)
     return NULL;
 }
 
-/* Use a xor list, marking options as inactive. */
+/* Mark options as inactive.
+ *   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;
 
@@ -1848,22 +1853,37 @@ ca_inactive(Cadef d, char **xor, int cur, int opts, char *optname)
 
 typedef struct castate *Castate;
 
-/*
- *           **** DOCUMENT ME ****
- *
- * This structure and its use are a nightmare.
- */
+/* Encapsulates details from parsing the current line against a particular set,
+ * Covers positions of options and normal arguments. Used as a linked list
+ * with one state for each set. */
 
 struct castate {
-    Castate snext;
-    Cadef d;
-    int nopts;
-    Caarg def, ddef;
-    Caopt curopt, dopt;
-    int opt, arg, argbeg, optbeg, nargbeg, restbeg, curpos, argend;
-    int inopt, inrest, inarg, nth, doff, singles, oopt, actopts;
-    LinkList args;
-    LinkList *oargs;
+    Castate snext;	/* state for next set */
+    Cadef d;		/* parsed _arguments specs for the set */
+    int nopts;		/* number of specified options (size of oargs) */
+    Caarg def;		/* definition for the current set */
+    Caarg ddef;
+    Caopt curopt;	/* option description corresponding to option found on the command-line */
+    Caopt dopt;
+    int opt;		/* the length of the option up to a maximum of 2 */
+    int arg;		/* completing arguments to an option or rest args */
+    int argbeg;         /* position of first rest argument (+1) */
+    int optbeg;		/* first word after the last option to the left of the cursor:
+			 * in effect the start of any arguments to the current option */
+    int nargbeg;	/* same as optbeg but used during parse */
+    int restbeg;	/* same as argbeg but used during parse */
+    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 */
+    int singles;	/* argument consists of clumped options */
+    int oopt;
+    int actopts;	/* count of active options */
+    LinkList args;	/* list of non-option args used for populating $line */
+    LinkList *oargs;	/* list of lists used for populating $opt_args */
 };
 
 static struct castate ca_laststate;
@@ -1909,7 +1929,9 @@ ca_opt_arg(Caopt opt, char *line)
     return ztrdup(line);
 }
 
-/* Parse a command line. */
+/* Parse the command line for a particular argument set (d).
+ * Returns 1 if the set should be skipped because it doesn't match
+ * existing options on the line. */
 
 static int
 ca_parse_line(Cadef d, int multi, int first)
@@ -1971,7 +1993,7 @@ ca_parse_line(Cadef d, int multi, int first)
 
 	goto end;
     }
-    if (d->nonarg)
+    if (d->nonarg) /* argument to -A */
 	napat = patcompile(d->nonarg, 0, NULL);
 
     /* Loop over the words from the line. */
@@ -2009,8 +2031,9 @@ ca_parse_line(Cadef d, int multi, int first)
 		return 1;
 	    continue;
 	}
-	/* We've got a definition for an argument, skip to the next. */
 
+	/* We've got a definition for an option/rest argument. For an option,
+	 * this means that we're completing arguments to that option. */
 	if (state.def) {
 	    state.arg = 0;
 	    if (state.curopt)
@@ -2159,8 +2182,7 @@ ca_parse_line(Cadef d, int multi, int first)
 		state.opt = 0;
 	    else
 		state.curopt = NULL;
-	} else if (multi && (*line == '-' || *line == '+') && cur != compcurrent &&
-		ca_get_opt(d, line, 0, NULL)
+	} else if (multi && (*line == '-' || *line == '+') && cur != compcurrent
 #if 0
 		   /**** Ouch. Using this will disable the mutual exclusion
 			 of different sets. Not using it will make the -A
@@ -2177,6 +2199,8 @@ ca_parse_line(Cadef d, int multi, int first)
 		return 1;
 
 	    arglast = 1;
+	    /* if this is the first normal arg after an option, may have been
+	     * earlier normal arguments if they're intermixed with options */
 	    if (state.inopt) {
 		state.inopt = 0;
 		state.nargbeg = cur - 1;
@@ -2526,25 +2550,26 @@ bin_comparguments(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 	    if (!(def = get_cadef(nam, args + 1)))
 		return 1;
 
-	    multi = !!def->snext;
+	    multi = !!def->snext; /* if we have sets */
 	    ca_parsed = cap;
 	    ca_xor = (multi ? newlinklist() : NULL);
 
-	    while (def) {
+	    while (def) { /* for each set */
 		use = !ca_parse_line(def, multi, first);
 		nx = ca_xor;
-		ca_xor = NULL;
+		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;
+				break; /* exclude this whole set */
 			}
-			if (!node)
+			if (!node) /* continue with this set */
 			    break;
 		    }
+		    /* entire set was excluded, continue to next set */
 		}
 		ca_xor = nx;
 		if (use && def) {
diff --git a/Test/Y03arguments.ztst b/Test/Y03arguments.ztst
index d59ed5424..b5a5a4be9 100644
--- a/Test/Y03arguments.ztst
+++ b/Test/Y03arguments.ztst
@@ -40,6 +40,15 @@
 >NO:{a}
 >NO:{b}
 
+# it ought to be possible to include the quoted backslash here
+ tst_arguments ':desc2:((a\:a\ value b\:other\\value))'
+ comptest $'tst \t'
+0:a and b with descriptions
+>line: {tst }{}
+>DESCRIPTION:{desc2}
+>NO:{a  -- a value}
+>NO:{b  -- othervalue}
+
  tst_arguments ':desc1:(arg1)' ':desc2:(arg2)' ':desc3:(arg3)'
  comptest $'tst \t\t\t\C-w\C-w\C-w\C-d'
 0:three arguments
@@ -81,6 +90,74 @@
 >line: {tst -o a }{}
 >line: {tst -o a b }{}
 
+ tst_arguments '!-x:arg:(ok)'
+ comptest $'tst -x \t'
+0:option argument to ignored option
+>line: {tst -x ok }{}
+
+ tst_arguments '!-a' -b
+ comptest $'tst -\t'
+0:ignored option not completed
+>line: {tst -b }{}
+
+ tst_arguments +x +y '!+z' ':arg:(x)'
+ comptest $'tst +z \t'
+0:ignored option is not taken to be the normal argument
+>line: {tst +z x }{}
+
+ tst_arguments --known --other
+ comptest $'tst --unknown -\t'
+0:unrecognised option has no effect on proceedings with no normal arguments
+>line: {tst --unknown --}{}
+
+ tst_arguments +x +y ':arg:(x)'
+ comptest $'tst +z \t'
+0:unrecognised option is taken to be the normal argument
+>line: {tst +z +}{}
+
+ tst_arguments '*-a:value:(1)'
+ comptest $'tst -a\t\t -a=\t'
+0:option argument follows in next argument
+>line: {tst -a }{}
+>line: {tst -a 1 }{}
+>line: {tst -a 1 -a=}{}
+>MESSAGE:{no arguments}
+
+ tst_arguments '*-a+:value:(1)'
+ comptest $'tst -a\t -a \t -a=\t'
+0:option argument either direct or in following argument
+>line: {tst -a1 }{}
+>line: {tst -a1 -a 1 }{}
+>line: {tst -a1 -a 1 -a=}{}
+
+ tst_arguments '*-a-:value:(1)'
+ comptest $'tst -a\t -a \t=\t'
+0:option argument follows directly
+>line: {tst -a1 }{}
+>line: {tst -a1 -a -a}{}
+>line: {tst -a1 -a -a=}{}
+
+ tst_arguments '*-a=:value:(1)'
+ comptest $'tst -a\t\t -a \t'
+0:option argument follows optional equals
+>line: {tst -a=}{}
+>line: {tst -a=1 }{}
+>line: {tst -a=1 -a 1 }{}
+
+ tst_arguments -s '*-a=:value:(1)'
+ comptest $'tst -a\t-a=\t -a \t'
+0:option argument follows optional equals, with -s
+>line: {tst -a1 }{}
+>line: {tst -a1 -a=1 }{}
+>line: {tst -a1 -a=1 -a 1 }{}
+
+ tst_arguments '*-a=-:value:(1)'
+ comptest $'tst -a\t\t-a \t'
+0:option argument follows mandatory equals
+>line: {tst -a=}{}
+>line: {tst -a=1 }{}
+>line: {tst -a=1 -a -a=}{}
+
  tst_arguments '-x:arg:'
  comptest $'tst -x\t'
 0:sticky option argument
@@ -99,6 +176,11 @@
 >DESCRIPTION:{option}
 >NO:{-x}
 
+ tst_arguments '-x' ": :_guard '[0-9]#' number"
+ comptest $'tst -\t'
+0:argument beginning with minus, guard on rest argument
+>line: {tst -x }{}
+
  tst_arguments '-o::optarg:(oa)' ':arg1:(a1)'
  comptest $'tst -o\t\t'
 0:optional option argument
@@ -110,9 +192,10 @@
 >NO:{a1}
 
  tst_arguments '-o:*a:a:(a)' ':A:(A)' ':B:(B)'
- comptest $'tst A -o a \t'
+ comptest $'tst A -o a \t\C-W\C-w-a -b -c a \t'
 0:variable length option arguments
 >line: {tst A -o a B }{}
+>line: {tst A -o -a -b -c a B }{}
 
  tst_arguments -s '-a' '-b' ':descr:{compadd - $+opt_args[-a]}'
  comptest $'tst -ab \t'
@@ -129,6 +212,40 @@
 0:rest arguments
 >line: {tst arg -b }{}
 
+ tst_arguments -a :more '*:rest:{ compadd - $words }'
+ comptest $'tst x -a rest \t'
+0:rest arguments with single colon
+>line: {tst x -a rest }{}
+>NO:{-a}
+>NO:{rest}
+>NO:{tst}
+>NO:{x}
+
+ tst_arguments -a :more '*::rest:{ compadd - $words }'
+ comptest $'tst x -a rest \t\eb\eb\eb\et\C-E \t'
+0:rest arguments with two colons
+>line: {tst x -a rest rest }{}
+>line: {tst -a x rest rest }{}
+>NO:{rest}
+>NO:{x}
+
+ tst_arguments -a -b :more '*:::rest:{ compadd - $words }'
+ comptest $'tst -b x -a -x rest \t'
+0:rest arguments with three colons
+>line: {tst -b x -a -x rest }{}
+>NO:{-x}
+>NO:{rest}
+
+ tst_arguments -a ::more '*:::rest:{ compadd - $words }'
+ comptest $'tst -a opt rest \t'
+0:rest arguments with three colons following optional argument
+>line: {tst -a opt rest rest }{}
+
+ tst_arguments -a::arg '*:::rest:{ compadd - $words }'
+ comptest $'tst -a opt rest \t'
+0:rest arguments with three colons following optional argument to an option
+>line: {tst -a opt rest rest }{}
+
  tst_arguments '-e:*last:::b:{compadd "${(j:,:)words}"}' ':arg1:(arg1)'
  comptest $'tst -\t\tla\t\C-hst\t\t\eb\eb\C-b\t\t'
 0:words array in rest arguments
@@ -140,14 +257,14 @@
 >line: {tst -e ,last }{ last arg1}
 >line: {tst -e ,last ,last,,last }{ last arg1}
 
- tst_arguments -s '-d+:msg1:' '*::msg2:{compadd $CURRENT}' 
+ tst_arguments -s '-d+:msg1:' '*::msg2:{compadd $CURRENT}'
  comptest $'tst add \t\t\t'
 0:opt_args
 >line: {tst add 2 }{}
 >line: {tst add 2 3 }{}
 >line: {tst add 2 3 4 }{}
 
- tst_arguments -s '-a' '-b' '-c' ':words:compadd - abyyy abzzz' 
+ tst_arguments -s '-a' '-b' '-c' ':words:compadd - abyyy abzzz'
  comptest $'tst ab\t'
 0:options and words (zsh-workers:12257)
 >line: {tst ab}{}
@@ -155,6 +272,12 @@
 >NO:{abyyy}
 >NO:{abzzz}
 
+ tst_arguments -M 'm:{j}={y}' -y -n ':yes/no:(y n)'
+ comptest $'tst j\t\eb-\C-e\t'
+0:matcher applies to options but not rest arguments
+>line: {tst j}{}
+>line: {tst -y }{}
+
  tst_arguments -x :word
  comptest $'tst -- -\t'
 0:option after --
@@ -175,15 +298,15 @@
 0:option after a word
 >line: {tst word -x }{}
 
- tst_arguments -A '-*' -x :word
+ tst_arguments -A'-*' -x :word
  comptest $'tst word -\t'
-0:option after word that doesn't match -A pattern
+0:option after word that doesn't match -A pattern, no space before pattern
 >line: {tst word -}{}
 >MESSAGE:{no more arguments}
 
  tst_arguments -A '-*' -x ':word:(-word)'
  comptest $'tst  word\eB\C-b-\t'
-0:option before a word that doesn't match -A pattern
+0:option before a word that doesn't match -A pattern, separate -A from pattern
 >line: {tst -}{ word}
 >DESCRIPTION:{word}
 >NO:{-word}
@@ -195,12 +318,68 @@
 0:continue completion after rest argument that looks like an option
 >line: {tst -a -x more }{}
 
+ tst_arguments '*-v'
+ comptest $'tst -v -\t'
+0:repeatable options
+>line: {tst -v -v }{}
+
+# necessary to exclude the rest arguments for the other set because
+# it is currently any unknown option rather than options from another
+# set that causes a set to be excluded
  tst_arguments -A '-*' - help -h -V - other -a '*: :(-x more)'
  comptest $'tst -a -x m\t'
 0:continue completion after rest argument that looks like an option (with sets)
->line: {tst -a -x more }{}
+>line: {tst -a -x m}{}
+#>line: {tst -a -x more }{}
 
- tst_arguments '(-v)-a' '(set1--m -a)-b' - '(set1)' -m -n - set2 -v -w
+  tst_arguments - '(help)' -h -V - other -a '*:rest:(1 2 3)'
+  comptest $'tst -h \t'
+0:unknown option disables whole set (without -A)
+>line: {tst -h }{}
+>MESSAGE:{no arguments}
+
+  tst_arguments -A "-*" - '(help)' -h -V - other -a '*:rest:(1 2 3)'
+  comptest $'tst -h \t'
+0:unknown option disables whole set (with -A)
+>line: {tst -h }{}
+>MESSAGE:{no arguments}
+
+ tst_arguments '(-C)-a' - set1 -C -v - set2 '(-a)-C' -w
+ comptest $'tst -a -\t' $'\C-w\C-w-C -\t'
+0:exclude option common to two sets and from one common option
+>line: {tst -a -}{}
+>DESCRIPTION:{option}
+>NO:{-v}
+>NO:{-w}
+>line: {tst -C -}{}
+>DESCRIPTION:{option}
+>NO:{-a}
+>NO:{-v}
+>NO:{-w}
+
+# _arguments doesn't know what rest arguments might be so any non-option
+# might apply: for the one set, it accepts "a"
+ tst_arguments -e - one -o '*:number:(1 2)' - two '(-e)*:letter:(a b)'
+ comptest $'tst \t' $'a -\t'
+0:rest argument rule in two sets
+>line: {tst }{}
+>DESCRIPTION:{letter}
+>NO:{a}
+>NO:{b}
+>DESCRIPTION:{number}
+>NO:{1}
+>NO:{2}
+>line: {tst  a -}{}
+>DESCRIPTION:{option}
+>NO:{-e}
+>NO:{-o}
+
+ tst_arguments '(set1-: set2-* set3-1)-a' - set1 '1: :(1)' '2: :(2)' - set2 '*:rest:(rest)' - set3 '1:num:(num)' '*: :(allowable)' - set4 ': :(allowed)'
+ comptest $'tst -a \t'
+0:exclude various forms of rest argument in set specific form
+>line: {tst -a allow}{}
+
+ tst_arguments '(-v)-a' '(set1--m -a)-b' - '(set1)' '( -a )-m' '( )-n' - set2 '(-w)*-v' -w
  comptest $'tst -a -\t' $'\C-w\C-w-'{b,m,v}$' -\t'
 0:exclusion lists
 >line: {tst -a -}{}
@@ -214,19 +393,36 @@
 >NO:{-n}
 >NO:{-v}
 >NO:{-w}
->line: {tst -m -}{}
->DESCRIPTION:{option}
->NO:{-a}
->NO:{-b}
->NO:{-v}
->NO:{-w}
+>line: {tst -m -b }{}
 >line: {tst -v -}{}
 >DESCRIPTION:{option}
 >NO:{-a}
 >NO:{-b}
+>NO:{-v}
+
+ tst_arguments -a - set1 -d - set2 '(set2)-m' -n -o ':arg:(x)' - set2 -x
+ comptest $'tst -m \t'
+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
+>line: {tst -a -}{}
+>DESCRIPTION:{option}
 >NO:{-m}
 >NO:{-n}
->NO:{-w}
+
+ 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 '(-)-h' -a -b -c
  comptest $'tst -h -\t'
@@ -243,6 +439,33 @@
 >NO:{-a}
 >NO:{-b}
 
+# ideally, would handle exclusion within the current word
+ tst_arguments -s : '(-d)-a' -b -c -d
+ comptest $'tst -ab\t -\t\eb\eb \C-b-\t'
+0:exclusion with clumped options, in, after and before
+>line: {tst -ab}{}
+>DESCRIPTION:{option}
+>NO:{-c}
+>NO:{-d}
+>line: {tst -ab -c }{}
+>line: {tst -}{ -ab -c}
+>DESCRIPTION:{option}
+>NO:{-a}
+>NO:{-b}
+>NO:{-c}
+>NO:{-d}
+
+ tst_arguments '-a:arg' -b '(-b)-c'
+ comptest $'tst -a -c -\t'
+0:exclusion with option argument that looks like an option
+>line: {tst -a -c -}{}
+>MESSAGE:{no arguments}
+# seems we don't handle this case, ideal result would be as follows
+#>line: {tst -a -c -}{}
+#>DESCRIPTION:{option}
+#>NO:{-b}
+#>NO:{-c}
+
  tst_arguments --abc --aah :arg:
  comptesteval 'setopt bashautolist automenu'
  comptest $'tst --a\t\t\t'