[PATCH] compvalues: support leading '!' in spec, handle escaping better

the docs imply that a leading '!' in a _values spec should work the same
way it does in an _arguments spec. but it does not. it would be nice if
it did, because _values stops completing a sequence as soon as it sees a
value it doesn't recognise

in adding that i realised that specs with escaped value names only
partially work -- they can't be matched correctly because we compare the
escaped name from the spec to the un-escaped word from the command line

so this patch adds the '!' syntax, fixes the escaping inconsistencies,
and adds tests for _values

i don't think the escaping change is an issue for existing functions, it
should only fix the matching

the '!' syntax is a 'breaking' change, but i'm not worried about it
because (a) the current behaviour contradicts the documentation, (b)
it's unusual to have a leading '!' in a value name, and (c) the only
negative consequence is that the value will be hidden. i will fix any
functions in the distribution that are impacted (only one afaict)

dana


diff --git a/README b/README
index 8d686a77c..b6408d882 100644
--- a/README
+++ b/README
@@ -149,6 +149,11 @@ Also, as a consequence of the zparseopts builtin now using standard
 argument parsing for its own options, long-option specs must be guarded
 using -- or similar.
 
+The _values completion helper now understands the leading '!' spec form
+supported by _arguments, bringing it in line with the documentation.  As
+a consequence, a leading '!' in a value name must now be escaped if it
+should be taken literally, as in: _values desc '!hidden' '\!literal'
+
 Incompatibilities between 5.8.1 and 5.9
 ---------------------------------------
 
diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index 55b0a9b9f..74370d0fc 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -1079,6 +1079,28 @@ bslashcolon(char *s)
     return r;
 }
 
+/* add backslashes before colons *and* other backslashes. this is suitable for
+ * re-escaping something that was un-escaped with rembslash(). basically it
+ * ensures that _describe handles a literal '\' correctly */
+
+static char *
+bslashcolon2(char *s)
+{
+    char *p, *r;
+
+    r = p = zhalloc((2 * strlen(s)) + 1);
+
+    while (*s) {
+	if (*s == ':' || *s == '\\')
+	    *p++ = '\\';
+	*p++ = *s++;
+    }
+    *p = '\0';
+
+    return r;
+}
+
+
 /* Get an index into the single array used in struct cadef
  * opt is the option letter and pre is either - or +
  * we only keep an array for the 94 ASCII characters from ! to ~ so
@@ -2944,6 +2966,7 @@ struct cvval {
     int type;			/* CVV_* below */
     Caarg arg;			/* argument definition */
     int active;			/* still allowed */
+    int not;			/* don't complete this value (`!...') */
 };
 
 #define CVV_NOARG 0
@@ -2989,7 +3012,7 @@ parse_cvdef(char *nam, char **args)
     Cvval val, *valp;
     Caarg arg;
     char **oargs = args, sep = '\0', asep = '=', *name, *descr, *p, *q, **xor, c;
-    int xnum, multi, vtype, hassep = 0, words = 0;
+    int xnum, multi, vtype, hassep = 0, words = 0, not = 0;
 
     while (args && args[0] && args[1] &&
            args[0][0] == '-' &&
@@ -3031,6 +3054,8 @@ parse_cvdef(char *nam, char **args)
 	p = dupstring(*args);
 	xnum = 0;
 
+	if ((not = (*p == '!')))
+	    p++;
 	/* xor list? */
 	if (*p == '(') {
 	    LinkList list = newlinklist();
@@ -3049,7 +3074,7 @@ parse_cvdef(char *nam, char **args)
 
 		sav = *p;
 		*p = '\0';
-		addlinknode(list, dupstring(q));
+		addlinknode(list, rembslash(q));
 		xnum++;
 		*p = sav;
 	    }
@@ -3133,17 +3158,18 @@ parse_cvdef(char *nam, char **args)
 		xor = (char **) zalloc(2 * sizeof(char *));
 		xor[1] = NULL;
 	    }
-	    xor[xnum] = ztrdup(name);
+	    xor[xnum] = ztrdup(rembslash(name));
 	}
 	*valp = val = (Cvval) zalloc(sizeof(*val));
 	valp = &((*valp)->next);
 
 	val->next = NULL;
-	val->name = ztrdup(name);
+	val->name = ztrdup(rembslash(name));
 	val->descr = ztrdup(descr);
 	val->xor = xor;
 	val->type = vtype;
 	val->arg = arg;
+	val->not = not;
     }
     return ret;
 }
@@ -3571,21 +3597,23 @@ bin_compvalues(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 	    char *str;
 
 	    for (p = cv_laststate.d->vals; p; p = p->next) {
-		if (p->active) {
+		if (p->active && !p->not) {
 		    switch (p->type) {
 		    case CVV_NOARG: l = noarg; break;
 		    case CVV_ARG:   l = arg;   break;
 		    default:        l = opt;   break;
 		    }
 		    if (p->descr) {
-			int len = strlen(p->name) + strlen(p->descr) + 2;
+			// see note on bslashcolon2()
+			char *n = bslashcolon2(p->name);
+			size_t len = strlen(n) + strlen(p->descr) + 2;
 
 			str = (char *) zhalloc(len);
-			strcpy(str, p->name);
+			strcpy(str, n);
 			strcat(str, ":");
 			strcat(str, p->descr);
 		    } else
-			str = p->name;
+			str = bslashcolon2(p->name);
 		    addlinknode(l, str);
 		}
 	    }
diff --git a/Test/Y06values.ztst b/Test/Y06values.ztst
new file mode 100644
index 000000000..b80cc2927
--- /dev/null
+++ b/Test/Y06values.ztst
@@ -0,0 +1,238 @@
+# tests for _values
+
+%prep
+
+  if ( zmodload -s zsh/zpty ); then
+    source $ZTST_srcdir/comptest
+    mkdir comp.tmp
+    cd comp.tmp
+    comptestinit -z $ZTST_testdir/../Src/zsh && {
+      comptesteval 'compdef _tst tst'
+      tst_values() { comptesteval "_tst() { _values ${${(@q+)@}} }" }
+    }
+  else
+    ZTST_unimplemented='the zsh/zpty module is not available'
+  fi
+
+%test
+
+  tst_values desc a b
+  comptest $'tst \t'
+0:basic value
+>line: {tst }{}
+>DESCRIPTION:{desc}
+>NO:{a}
+>NO:{b}
+
+  tst_values desc a b
+  comptest $'tst a \t'
+0:basic value, next word
+>line: {tst a }{}
+>DESCRIPTION:{desc}
+>NO:{a}
+>NO:{b}
+
+  tst_values desc 'a[adesc]' 'b[bdesc]'
+  comptest $'tst \t'
+0:basic value with description
+>line: {tst }{}
+>DESCRIPTION:{desc}
+>NO:{a  -- adesc}
+>NO:{b  -- bdesc}
+
+  tst_values desc 'a\[b' '\!x' '\*y' '\(z' '\\\.\:\]\&'
+  comptest $'tst \t'
+0:display of escaped char in value name
+>line: {tst }{}
+>DESCRIPTION:{desc}
+>NO:{!x}
+>NO:{(z}
+>NO:{*y}
+>NO:{\.:]&}
+>NO:{a[b}
+
+  tst_values desc '!a' b c
+  comptest $'tst \t'
+0:display of hidden value
+>line: {tst }{}
+>DESCRIPTION:{desc}
+>NO:{b}
+>NO:{c}
+
+  tst_values -s, desc a b c
+  comptest $'tst b,\t'
+0:basic sequence
+>line: {tst b,}{}
+>DESCRIPTION:{desc}
+>NO:{a}
+>NO:{c}
+
+  tst_values -s, desc a b c d e
+  comptest $'tst \'a,b,c,\t'
+  comptest $'tst "a,b,c,\t'
+0:sequence in quotes
+>line: {tst 'a,b,c,}{}
+>DESCRIPTION:{desc}
+>NO:{d}
+>NO:{e}
+>line: {tst "a,b,c,}{}
+>DESCRIPTION:{desc}
+>NO:{d}
+>NO:{e}
+
+  tst_values -s, desc '!a' b c
+  comptest $'tst a,\t'
+0:sequence continues after hidden value
+>line: {tst a,}{}
+>DESCRIPTION:{desc}
+>NO:{b}
+>NO:{c}
+
+  tst_values -s, desc '\*a' b c
+  comptest $'tst \\*a,\t'
+0:sequence continues after value with escaped meta char
+>line: {tst \*a,}{}
+>DESCRIPTION:{desc}
+>NO:{b}
+>NO:{c}
+
+  tst_values -s, desc '\:a' b c
+  comptest $'tst :a,\t'
+0:sequence continues after value with escaped non-meta char
+>line: {tst :a,}{}
+>DESCRIPTION:{desc}
+>NO:{b}
+>NO:{c}
+
+  tst_values -s, desc '*a' b c
+  comptest $'tst a,a,\t'
+0:multi value offered again
+>line: {tst a,a,}{}
+>DESCRIPTION:{desc}
+>NO:{a}
+>NO:{b}
+>NO:{c}
+
+  tst_values -s, desc '(b c)a' b c d e
+  comptest $'tst a,\t'
+0:xor value not offered
+>line: {tst a,}{}
+>DESCRIPTION:{desc}
+>NO:{d}
+>NO:{e}
+
+  tst_values -s, desc '(d\:e)a' b c 'd\:e'
+  comptest $'tst a,\t'
+0:xor value with escaped char not offered
+>line: {tst a,}{}
+>DESCRIPTION:{desc}
+>NO:{b}
+>NO:{c}
+
+  tst_values desc 'a:val'
+  comptest $'tst a=\t'
+  tst_values desc 'a:val:'
+  comptest $'tst a=\t'
+0:value with argument not enumerated
+>line: {tst a=}{}
+>DESCRIPTION:{val}
+>line: {tst a=}{}
+>DESCRIPTION:{val}
+
+  tst_values desc 'a:val:(x y z)'
+  comptest $'tst a=\t'
+0:value with argument enumerated
+>line: {tst a=}{}
+>DESCRIPTION:{val}
+>NO:{x}
+>NO:{y}
+>NO:{z}
+
+# _arguments requires the ':'s to be escaped but i guess it's unnecessary here
+  tst_values desc 'a:val:((x:descx y:descy z:descz))'
+  comptest $'tst a=\t'
+0:value with argument enumerated with description
+>line: {tst a=}{}
+>DESCRIPTION:{val}
+>NO:{x  -- descx}
+>NO:{y  -- descy}
+>NO:{z  -- descz}
+
+# this is the syntax required by _arguments (see above)
+  tst_values desc 'a:val:((x\:descx y\:descy z\:descz))'
+  comptest $'tst a=\t'
+0:value with argument enumerated with description, escaped colon
+>line: {tst a=}{}
+>DESCRIPTION:{val}
+>NO:{x  -- descx}
+>NO:{y  -- descy}
+>NO:{z  -- descz}
+
+# should be the same as above
+  tst_values desc 'a:val:((x\\:descx y\\:descy z\\:descz))'
+  comptest $'tst a=\t'
+0:value with argument enumerated with description, escaped colon with \\
+>line: {tst a=}{}
+>DESCRIPTION:{val}
+>NO:{x  -- descx}
+>NO:{y  -- descy}
+>NO:{z  -- descz}
+
+  tst_values desc 'a:val:(("x\\:x":descx y\:descy z\:descz))'
+  comptest $'tst a=\t'
+0:value with argument enumerated with description, escaped colon in arg name
+>line: {tst a=}{}
+>DESCRIPTION:{val}
+>NO:{x:x  -- descx}
+>NO:{y    -- descy}
+>NO:{z    -- descz}
+
+  tst_values desc 'a:val:((x:x:descx y\:descy z\:descz))'
+  comptest $'tst a=\t'
+0:value with argument enumerated with description, un-escaped colon in arg desc
+>line: {tst a=}{}
+>DESCRIPTION:{val}
+>NO:{x  -- x:descx}
+>NO:{y  -- descy}
+>NO:{z  -- descz}
+
+  tst_values desc 'a: :{ _message msg }'
+  comptest $'tst a=\t'
+0:value with argument eval string, braces
+>line: {tst a=}{}
+>MESSAGE:{msg}
+
+  tst_values desc 'a: : _message msg'
+  comptest $'tst a=\t'
+0:value with argument eval string, leading space
+>line: {tst a=}{}
+>MESSAGE:{msg}
+
+  tst_values -s: -S. desc 'a:val:(x y z)' b c
+  comptest $'tst b:a.\t'
+0:-s and -S
+>line: {tst b:a.}{}
+>DESCRIPTION:{val}
+>NO:{x}
+>NO:{y}
+>NO:{z}
+
+  tst_values -w desc a b c
+  comptest $'tst b \t'
+0:-w without -s
+>line: {tst b }{}
+>DESCRIPTION:{desc}
+>NO:{a}
+>NO:{c}
+
+  tst_values -ws, desc a b c d
+  comptest $'tst b,c \t'
+0:-w with -s
+>line: {tst b,c }{}
+>DESCRIPTION:{desc}
+>NO:{a}
+>NO:{d}
+
+%clean
+
+  zmodload -ui zsh/zpty