PATCH: grouping _arguments specs to simplify exclusion lists

[Oliver Kiddle <okiddle@xxxxxxxxxxx>]

This patch adds an option group concept to _arguments. Superficially,
these look a lot like sets but there's no mutual exclusion between
groups. They also don't add much overhead (unlike sets, which Sven
warned us off using, both in the docs and e.g. _zpty).

Syntax is with + instead of - as follows:

  _arguments \
     '(group3--x)-a' \
    + group1 \
      -m \
      '(group2)-n' \
    + group2 \
      -m -n \
    + '(group3)' \
      -x -y

The idea stemmed from the realisation that the - '(set)' form tends
to be completely useless because you're excluding just about
everything. Often, you get more than one bunch of options that are
mutually exclusive but the bunches can be mixed. Groups of the +
'(group)' form let you have the internal mutual exclusion on its
own.

Otherwise, all this is really doing is giving a name to a cluster
of options.  But you can use those names in exclusion lists much
as you can for sets which is often nicer than creating lots of
variables that list options.

All specs contained in groups are common to all sets, you can't
have a group in a set or a set in a group. That keeps things simple
and allows this to piggyback much of the existing code for sets.

Groups don't imply any separation in completion listings either.
That may be possible as a future extension but the entire guts of
compdescribe would need to be deciphered and extended to support
multiple tags.

With this patch comparguments also now produces an error if you try
to use any empty string as the name of a set. Blocking it is less
effort than trying to understand and fix the implications.

I've inserted documentation before sets and moved much of the
documentation of common elements (the '(name)' form and name--opt
exclusions) to groups because they're a simpler concept. I also
noticed two errors that had been made in relatively recent updates
to _arguments documentation: specs for normal arguments do not need
to follow options. That's no more than a convention. Also, there
was some confusion in the wording refering to first and remaining
sets: which didn't look valid. If you do _arguments -a - set -b,
you still have just one set and all you've done is give a name that
covers the -b option.

Oliver

diff --git a/Doc/Zsh/compsys.yo b/Doc/Zsh/compsys.yo
index 953d51c..2a112ed 100644
--- a/Doc/Zsh/compsys.yo
+++ b/Doc/Zsh/compsys.yo
@@ -3573,13 +3573,11 @@ This function can be used to give a complete specification for completion
 for a command whose arguments follow standard UNIX option and argument
 conventions.
 
-em(Options overview)
+em(Options Overview)
 
 Options to tt(_arguments) itself must be in separate words, i.e. tt(-s -w),
 not tt(-sw).  The options are followed by var(spec)s that describe options and
-arguments of the analyzed command.  var(spec)s that describe option flags must
-precede var(spec)s that describe non-option ("positional" or "normal")
-arguments of the analyzed line.  To avoid ambiguity, all
+arguments of the analyzed command.  To avoid ambiguity, all
 options to tt(_arguments) itself may be separated from the var(spec) forms
 by a single colon.
 
@@ -3997,18 +3995,48 @@ example(local curcontext="$curcontext")
 This is useful where it is not possible for multiple states to be valid
 together.
 
-em(Specifying multiple sets of options)
+em(Grouping Options)
 
-It is possible to specify multiple sets of options and
-arguments with the sets separated by single hyphens.  The specifications
-before the first hyphen (if any) are shared by all the remaining sets.
-The first word in every other set provides a name for the
-set which may appear in exclusion lists in specifications,
-either alone or before one of the possible values described above.
-In the second case a `tt(-)' should appear between this name and the
-remainder.
+Options can be grouped to simplify exclusion lists. A group is
+introduced with `tt(PLUS())' followed by a name for the group in the
+subsequent word. Whole groups can then be referenced in an exclusion
+list or a group name can be used to disambiguate between two forms of
+the same option. For example:
 
-For example:
+example(_arguments \ 
+    '(group2--x)-a' \ 
+  PLUS() group1 \ 
+    -m \ 
+    '(group2)-n' \ 
+  PLUS() group2 \ 
+    -x -y)
+
+If the name of a group is specified in the form
+`tt(LPAR())var(name)tt(RPAR())' then only one value from that group
+will ever be completed; more formally, all specifications are mutually
+exclusive to all other specifications in that group. This is useful for
+defining options that are aliases for each other. For example:
+
+example(_arguments \ 
+    -a -b \ 
+  PLUS() '(operation)' \ 
+    {-c,--compress}'[compress]' \ 
+    {-d,--decompress}'[decompress]' \ 
+    {-l,--list}'[list]')
+
+If an option in a group appears on the command line, it is stored in the
+associative array `tt(opt_args)' with 'var(group)tt(-)var(option)'
+as a key.  In the example above, a key `tt(operation--c)' is used if the option
+`tt(-c)' is present on the command line.
+
+em(Specifying Multiple Sets of Arguments)
+
+It is possible to specify multiple sets of options and arguments with
+the sets separated by single hyphens. This differs from groups in that
+sets are considered to be mutually exclusive of each other.
+
+Specifications before the first set and from any group are common to
+all sets. For example:
 
 example(_arguments \ 
     -a \ 
@@ -4024,28 +4052,11 @@ possible completions.  When it contains `tt(-d)' or an argument, the
 option `tt(-c)' will not be considered.  However, after `tt(-a)'
 both sets will still be considered valid.
 
-If an option in a set appears on the command line, it is stored in the
-associative array `tt(opt_args)' with 'var(set)tt(-)var(option)'
-as a key.  In the example above, a key `tt(set1--c)' is used if the option
-`tt(-c)' is on the command line.
+As for groups, the name of a set may appear in exclusion lists, either
+alone or preceding a normal option or argument specification.
 
-If the name given for one of the mutually exclusive sets is of the form
-`tt(LPAR())var(name)tt(RPAR())' then only one value from each set will ever
-be completed; more formally, all specifications are mutually
-exclusive to all other specifications in the same set.  This is
-useful for defining multiple sets of options which are mutually
-exclusive and in which the options are aliases for each other.  For
-example:
-
-example(_arguments \ 
-    -a -b \ 
-  - '(compress)' \ 
-    {-c,--compress}'[compress]' \ 
-  - '(uncompress)' \ 
-    {-d,--decompress}'[decompress]')
-
-As the completion code has to parse the command line separately for each
-set this form of argument is slow and should only be used when necessary.
+The completion code has to parse the command line separately for each
+set. This can be slow so sets should only be used when necessary.
 A useful alternative is often an option specification with rest-arguments
 (as in `tt(-foo:*:...)'); here the option tt(-foo) swallows up all
 remaining arguments as described by the var(optarg) definitions.
diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index 12aa895..7bf9535 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -934,7 +934,7 @@ struct caopt {
     Caarg args;			/* option arguments */
     int active;			/* still allowed on command line */
     int num;			/* it's the num'th option */
-    char *set;			/* set name, shared */
+    char *gsname;		/* group or set name, shared */
     int not;			/* don't complete this option (`!...') */
 };
 
@@ -958,7 +958,7 @@ struct caarg {
     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 */
+    char *gsname;		/* group or set name, shared */
 };
 
 #define CAA_NORMAL 1
@@ -1096,7 +1096,7 @@ parse_caarg(int mult, int type, int num, int opt, char *oname, char **def,
     ret->type = type;
     ret->opt = ztrdup(oname);
     ret->direct = 0;
-    ret->set = set;
+    ret->gsname = set;
 
     /* Get the description. */
 
@@ -1183,10 +1183,10 @@ parse_cadef(char *nam, char **args)
     Cadef all, ret;
     Caopt *optp;
     char **orig_args = args, *p, *q, *match = "r:|[_-]=* r:|=*", **xor, **sargs;
-    char *adpre, *adsuf, *axor = NULL, *doset = NULL, **setp = NULL;
+    char *adpre, *adsuf, *axor = NULL, *doset = NULL, **pendset = NULL, **curset = NULL;
     char *nonarg = NULL;
     int single = 0, anum = 1, xnum, flags = 0;
-    int state = 0, not = 0;
+    int foreignset = 0, not = 0;
 
     /* First string is the auto-description definition. */
 
@@ -1249,7 +1249,6 @@ parse_cadef(char *nam, char **args)
 
     if (nonarg)
 	tokenize(nonarg = dupstring(nonarg));
-
     /* Looks good. Optimistically allocate the cadef structure. */
 
     all = ret = alloc_cadef(orig_args, single, match, nonarg, flags);
@@ -1258,36 +1257,64 @@ parse_cadef(char *nam, char **args)
 
     /* Get the definitions. */
 
-    for (; *args; args++) {
+    for (; *args || pendset; args++) {
+	if (!*args) {
+	    /* start new set */
+	    args = sargs; /* go back and repeat parse of common options */
+	    doset = NULL;
+	    set_cadef_opts(ret);
+	    ret = ret->snext = alloc_cadef(NULL, single, match, nonarg, flags);
+	    optp = &(ret->opts);
+	    anum = 1;
+	    foreignset = 0;
+	    curset = pendset;
+	    pendset = 0;
+        }
         if (args[0][0] == '-' && !args[0][1] && args[1]) {
-	    if (!state) {
-		char *p;
-		int l;
+	    if ((foreignset = curset && args != curset)) {
+		if (!pendset && args > curset)
+		    pendset = args; /* mark pointer to next pending set */
+		++args;
+	    } else {
+		/* Carrying on: this is the current set */
+		char *p = *++args;
+		int l = strlen(p) - 1;
 
-		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] == ')') {
 		    axor = p = dupstring(p + 1);
 		    p[l - 1] = '\0';
 		} else
 		    axor = NULL;
+		if (!*p) {
+		    freecadef(all);
+		    zwarnnam(nam, "empty set name");
+		    return NULL;
+		}
 		ret->set = doset = tricat(p, "-", "");
-		state = 1;
-	    } else { /* starting new set */
-		setp = args;
-		state = 0;
-		args = sargs - 1;
-		doset = NULL;
-		set_cadef_opts(ret);
-		ret = ret->snext = alloc_cadef(NULL, single, match, nonarg, flags);
-		optp = &(ret->opts);
-		anum = 1;
+		curset = args; /* needed for the first set */
 	    }
 	    continue;
-	}
+	} else if (args[0][0] == '+' && !args[0][1] && args[1]) {
+	    char *p;
+	    int l;
+
+	    foreignset = 0; /* group not in any set, don't want to skip it */
+	    p = *++args;
+	    l = strlen(p) - 1;
+	    if (*p == '(' && p[l] == ')') {
+		axor = p = dupstring(p + 1);
+		p[l - 1] = '\0';
+	    } else
+		axor = NULL;
+	    if (!*p) {
+		freecadef(all);
+		zwarnnam(nam, "empty group name");
+		return NULL;
+	    }
+	    doset = tricat(p, "-", "");
+	    continue;
+	} else if (foreignset) /* skipping over a different set */
+	    continue;
 	p = dupstring(*args);
 	xnum = 0;
 	if ((not = (*p == '!')))
@@ -1499,7 +1526,7 @@ parse_cadef(char *nam, char **args)
 	    optp = &((*optp)->next);
 
 	    opt->next = NULL;
-	    opt->set = doset;
+	    opt->gsname = doset;
 	    opt->name = ztrdup(rembslashcolon(name));
 	    if (descr)
 		opt->descr = ztrdup(descr);
@@ -1795,63 +1822,85 @@ ca_inactive(Cadef d, char **xor, int cur, int opts)
     if ((xor || opts) && cur <= compcurrent) {
 	Caopt opt;
 	char *x;
-	int sl = (d->set ? (int)strlen(d->set) : -1), set = 0;
         /* current word could be a prefix of a longer one so only do
 	 * exclusions for single-letter options (for option clumping) */
 	int single = (cur == compcurrent);
 
 	for (; (x = (opts ? "-" : *xor)); xor++) {
-	    set = 0;
-	    if (sl > 0) {
-		if (strpfx(d->set, x)) {
-		    x += sl;
-		    set = 1;
-		} else if (!strncmp(d->set, x, sl - 1)) {
-		    Caopt p;
+	    int excludeall = 0;
+	    char *grp = NULL;
+	    size_t grplen;
+	    char *next, *sep = x;
 
-		    for (p = d->opts; p; p = p->next)
-			if (p->set && !(single && *p->name && p->name[1] && p->name[2]))
-			    p->active = 0;
-			
-		    x = ":";
-		    set = 1;
+	    while (*sep != '+' && *sep != '-' && *sep != ':' && *sep != '*' && !idigit(*sep)) {
+		if (!(next = strchr(sep, '-')) || !*++next) {
+		    /* exclusion is just the name of a set or group */
+		    excludeall = 1; /* excluding options and args */
+		    sep += strlen(sep);
+		    /* A trailing '-' is included in the various gsname fields but is not
+		     * there for this branch. This is why we add excludeall to grplen
+		     * when checking for the null in a few places below */
+		    break;
 		}
+		sep = next;
 	    }
-	    if (x[0] == ':' && !x[1]) {
-		if (set) {
+	    if (sep > x) { /* exclusion included a set or group name */
+		grp = x;
+		grplen = sep - grp;
+		x = sep;
+	    }
+
+	    if (excludeall || (x[0] == ':' && !x[1])) {
+		if (grp) {
 		    Caarg a;
 
 		    for (a = d->args; a; a = a->next)
-			if (a->set)
+			if (a->gsname && !strncmp(a->gsname, grp, grplen) &&
+				!a->gsname[grplen + excludeall])
 			    a->active = 0;
-		    if (d->rest && (!set || d->rest->set))
+		    if (d->rest && d->rest->gsname &&
+			    !strncmp(d->rest->gsname, grp, grplen) &&
+			    !d->rest->gsname[grplen + excludeall])
 			d->rest->active = 0;
 		} else
 		    d->argsactive = 0;
-	    } else if (x[0] == '-' && !x[1]) {
+	    }
+
+	    if (excludeall || (x[0] == '-' && !x[1])) {
 		Caopt p;
 
 		for (p = d->opts; p; p = p->next)
-		    if ((!set || p->set) && !(single && *p->name && p->name[1] && p->name[2]))
+		    if ((!grp || (p->gsname && !strncmp(p->gsname, grp, grplen) &&
+			    !p->gsname[grplen + excludeall])) &&
+			    !(single && *p->name && p->name[1] && p->name[2]))
 			p->active = 0;
-	    } else if (x[0] == '*' && !x[1]) {
-		if (d->rest && (!set || d->rest->set))
+	    }
+
+	    if (excludeall || (x[0] == '*' && !x[1])) {
+		if (d->rest && (!grp || (d->rest->gsname &&
+			!strncmp(d->rest->gsname, grp, grplen) &&
+			!d->rest->gsname[grplen + excludeall])))
 		    d->rest->active = 0;
-	    } else if (idigit(x[0])) {
-		int n = atoi(x);
-		Caarg a = d->args;
+            }
 
-		while (a && a->num < n)
-		    a = a->next;
+	    if (!excludeall) {
+		if (idigit(x[0])) {
+		    int n = atoi(x);
+		    Caarg a = d->args;
 
-		if (a && a->num == n && (!set || a->set))
-		    a->active = 0;
-	    } else if ((opt = ca_get_opt(d, x, 1, NULL)) && (!set || opt->set) &&
-		    !(single && *opt->name && opt->name[1] && opt->name[2]))
-		opt->active = 0;
+		    while (a && a->num < n)
+			a = a->next;
 
-	    if (opts)
-		break;
+		    if (a && a->num == n && (!grp || (a->gsname &&
+			    !strncmp(a->gsname, grp, grplen))))
+			a->active = 0;
+		} else if ((opt = ca_get_opt(d, x, 1, NULL)) &&
+			(!grp || (opt->gsname && !strncmp(opt->gsname, grp, grplen))) &&
+			!(single && *opt->name && opt->name[1] && opt->name[2]))
+		    opt->active = 0;
+		if (opts)
+		    break;
+	    }
 	}
     }
 }
@@ -2414,19 +2463,19 @@ ca_set_data(LinkList descr, LinkList act, LinkList subc,
 		    restrict_range(ca_laststate.argbeg, ca_laststate.argend);
 	    }
 	    if (arg->opt) {
-		buf = (char *) zhalloc((arg->set ? strlen(arg->set) : 0) +
+		buf = (char *) zhalloc((arg->gsname ? strlen(arg->gsname) : 0) +
 				       strlen(arg->opt) + 40);
 		if (arg->num > 0 && arg->type < CAA_REST)
 		    sprintf(buf, "%soption%s-%d",
-			    (arg->set ? arg->set : ""), arg->opt, arg->num);
+			    (arg->gsname ? arg->gsname : ""), arg->opt, arg->num);
 		else
 		    sprintf(buf, "%soption%s-rest",
-			    (arg->set ? arg->set : ""), arg->opt);
+			    (arg->gsname ? arg->gsname : ""), arg->opt);
 	    } else if (arg->num > 0) {
 		sprintf(nbuf, "argument-%d", arg->num);
-		buf = (arg->set ? dyncat(arg->set, nbuf) : dupstring(nbuf));
+		buf = (arg->gsname ? dyncat(arg->gsname, nbuf) : dupstring(nbuf));
 	    } else
-		buf = (arg->set ? dyncat(arg->set, "argument-rest") :
+		buf = (arg->gsname ? dyncat(arg->gsname, "argument-rest") :
 		       dupstring("argument-rest"));
 
 	    addlinknode(subc, buf);
@@ -2779,7 +2828,7 @@ bin_comparguments(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 	    for (s = lstate; s; s = s->snext)
 		for (o = s->d->opts, a = s->oargs; o; o = o->next, a++)
 		    if (*a) {
-			*p++ = (o->set ? tricat(o->set, o->name, "") :
+			*p++ = (o->gsname ? tricat(o->gsname, o->name, "") :
 				ztrdup(o->name));
 			*p++ = ca_colonlist(*a);
 		    }
diff --git a/Test/Y03arguments.ztst b/Test/Y03arguments.ztst
index 5957bc3..25bb96b 100644
--- a/Test/Y03arguments.ztst
+++ b/Test/Y03arguments.ztst
@@ -463,6 +463,14 @@
 0:single option sets are still mutually exclusive
 >line: {tst -m -x }{}
 
+ tst_arguments '(set-c set-g)-a' '(set)-b' -c + grp -g - set -s
+ comptest $'tst -a -b -\t'
+0:excluding a set doesn't exclude common options as part of the set
+>line: {tst -a -b -}{}
+>DESCRIPTION:{option}
+>NO:{-c}
+>NO:{-g}
+
  tst_arguments '(-)-h' -a -b -c
  comptest $'tst -h -\t'
 0:exclude all other options
@@ -536,6 +544,75 @@ F:>DESCRIPTION:{option}
 F:>NO:{-b}
 F:>NO:{-c}
 
+ tst_arguments + grp1 -a -b - onlyset '(-a grp3--y grp2 grp4--)-m' -n + grp2 -u -v + grp3 -x -y + grp4 -0 ':rest'
+ comptest $'tst -m -\t'
+0:exclude group options
+>line: {tst -m -}{}
+>DESCRIPTION:{rest}
+>DESCRIPTION:{option}
+>NO:{-b}
+>NO:{-n}
+>NO:{-x}
+
+ tst_arguments -x + '(grp1)' -a -b + '(grp2)' -m -n
+ comptest $'tst -m -\t'
+0:single option groups are not mutually exclusive
+>line: {tst -m -}{}
+>DESCRIPTION:{option}
+>NO:{-a}
+>NO:{-b}
+>NO:{-x}
+
+ tst_arguments '(grp1 grp2-2)-a' '(grp1-*)-b' + grp1 ':first' '*:rest' + grp2 ':second'
+ comptest $'tst -a \t\eb\C-w\C-e x y \t'
+0:exclude rest args listed within a group
+>line: {tst -a -b }{}
+>line: {tst -b x y -a }{}
+
+ tst_arguments '(grp--m)-a' + grp '-m:value:(a b c)' + agrp '-m:other:(1 2 3)'
+ comptest $'tst -m \t\C-w-a -m \t'
+0:two forms of same option in different groups
+>line: {tst -m }{}
+>DESCRIPTION:{value}
+>NO:{a}
+>NO:{b}
+>NO:{c}
+>line: {tst -a -m }{}
+>DESCRIPTION:{other}
+>NO:{1}
+>NO:{2}
+>NO:{3}
+F:should offer both sets of arguments in first case
+
+ tst_arguments '(grp--m)-x' + '(grp)' -a -b -c ':val:(1 2 3)'
+ comptest $'tst 1 -\t'
+0:normal argument excludes options in internally mutually exclusive group
+>line: {tst 1 -x }{}
+
+ tst_arguments -s -a - set1 -c -s - set2 -c -t + grp -d
+ comptest $'tst -s\t -\t'
+0:mix sets, groups and option stacking
+>line: {tst -s}{}
+>DESCRIPTION:{option}
+>NO:{-a}
+>NO:{-c}
+>NO:{-d}
+>NO:{-t}
+>line: {tst -s -}{}
+>DESCRIPTION:{option}
+>NO:{-a}
+>NO:{-c}
+>NO:{-d}
+F:shouldn't offer -t in the first case (with stacked options)
+
+ tst_arguments -s '(set-a set--b grp-a grp--b grp-)-a' - set-a -s - set--b -t + grp-a -g + grp--b -h + grp- -i
+ comptest $'tst -a\t'
+0:sets and groups with - in their name
+>line: {tst -a}{}
+>DESCRIPTION:{option}
+>NO:{-h}
+>NO:{-t}
+
  tst_arguments --abc --aah :arg:
  comptesteval 'setopt bashautolist automenu'
  comptest $'tst --a\t\t\t'