zsh-workers
 help / color / mirror / code / Atom feed
* [PATCH] fix option -A of _arguments
@ 2021-10-19 10:23 Jun T
  2021-10-20 17:49 ` Oliver Kiddle
  0 siblings, 1 reply; 3+ messages in thread
From: Jun T @ 2021-10-19 10:23 UTC (permalink / raw)
  To: zsh-workers

Consider the following example:

% _cmd () { _arguments -A '-*' : -a -b '*: :_file' }
% compdef _cmd cmd
cmd -x -<TAB>
No match for: `file'

But zshcompsys(1) says:

-A pat
  Do not complete options after the first non-option argument on the line.
  pat is a pattern matching all strings which are not to be taken as arguments.
  For example, to make _arguments stop completing options after the first
  normal argument, but ignoring all strings starting with a hyphen even if they
  are not described by one of the optspecs, the form is `-A "-*"'.

In the example above, -x should not be taken as a normal argument since it
matches the pattern '-*' (although it is not in the optspecs), and the
options -a and -b should still be offered here (if I understand the document
correctly).

With the patch below, ca_inactive() is called only if
-A pat is given, and
the word under inspection ('-x' in the example above) does not
match the pat so it can be considered as a normal argument, and
the word ('-x') is before the cursor.


diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index e08788e89..9d9a09543 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -1995,7 +1995,7 @@ ca_parse_line(Cadef d, Cadef all, int multi, int first)
     Caopt ptr, wasopt = NULL, dopt;
     struct castate state;
     char *line, *oline, *pe, **argxor = NULL;
-    int cur, doff, argend, arglast;
+    int cur, doff, argend, arglast, notmatch;
     Patprog endpat = NULL, napat = NULL;
     LinkList sopts = NULL;
 #if 0
@@ -2236,9 +2236,10 @@ ca_parse_line(Cadef d, Cadef all, int multi, int first)
 		&& (ca_foreign_opt(d, all, line)))
 	    return 1;
 	else if (state.arg &&
-		 (!napat || cur <= compcurrent || !pattry(napat, line))) {
+		 (!napat || (notmatch = !pattry(napat, line)) ||
+		  cur <= compcurrent)) {
 	    /* Otherwise it's a normal argument. */
-	    if (napat && cur <= compcurrent)
+	    if (napat && notmatch && cur <= compcurrent)
 		ca_inactive(d, NULL, cur + 1, 1);
 
 	    arglast = 1;





^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] fix option -A of _arguments
  2021-10-19 10:23 [PATCH] fix option -A of _arguments Jun T
@ 2021-10-20 17:49 ` Oliver Kiddle
  2021-10-22  4:00   ` Jun T
  0 siblings, 1 reply; 3+ messages in thread
From: Oliver Kiddle @ 2021-10-20 17:49 UTC (permalink / raw)
  To: Jun T; +Cc: zsh-workers

Jun T wrote:
> Consider the following example:
>
> % _cmd () { _arguments -A '-*' : -a -b '*: :_file' }
> % compdef _cmd cmd
> cmd -x -<TAB>
> No match for: `file'
>
> But zshcompsys(1) says:
>
> -A pat
>   Do not complete options after the first non-option argument on the line.
>   pat is a pattern matching all strings which are not to be taken as arguments.
>   For example, to make _arguments stop completing options after the first
>   normal argument, but ignoring all strings starting with a hyphen even if they
>   are not described by one of the optspecs, the form is `-A "-*"'.
>
> In the example above, -x should not be taken as a normal argument since it
> matches the pattern '-*' (although it is not in the optspecs), and the
> options -a and -b should still be offered here (if I understand the document
> correctly).

I agree with your understanding.

Experimenting with this, I found further issues with -A. I hope you
don't mind but the resulting patch to account for them also covers the
issue you describe but with a somewhat different fix. I've also added a
number of test cases.

In terms of behaviour of your example, I think -x should also not be
treated as the first normal argument. If you replace '*: :_file' with
'1: :_file' in your example, the -x should not exclude the first (1)
argument. It is generally best in completions if the behaviour
gracefully copes with options that it doesn't know about and the
interface was designed with a pattern that matches options (-*) not the
converse (^-*) so it is reasonable to expect that -x in your example is
a new option our completion doesn't know about rather than an unusual
normal argument. We can't know for sure but I think this is more useful
in practice.

> -		 (!napat || cur <= compcurrent || !pattry(napat, line))) {
> +		 (!napat || (notmatch = !pattry(napat, line)) ||
> +		  cur <= compcurrent)) {

This logic can be reorganised to keep the slow pattry() call
after the fast cur <= compcurrent test. If cur <= compcurrent then
we won't have done the pattry() so repeating the call won't result in it
being called twice.

However, after trying this with some further cases I think we can
further limit when we check the pattern. It doesn't make much sense to
compare it against the current (incomplete) word. Or subsequent words
once a word has failed to match. Or to words following -- with -S.
Similarly for duplicated -- words, only the first should count.

I also noticed the following odd behaviour:
_cmd() { _arguments -a -b -c -d '(-a)1:one' '(-b)2:two' '(-c)*:extra' }
cmd -<tab> x y z
one
option
-b  -c  -d

It actually calls ca_inactive to disable -a for each of the x, y and z
arguments.

Normally, exclusions only apply to later options. This is useful because
options can exclude each other mutually and one sided exclusions can
occur. Exclusions mostly aren't applied backwards from later words on
the command-line because it is tricky to implement and not always
desirable.

Finally, I think it is an error that inopt is initialised to 1 because
we're not completing options to an argument at the beginning. Starting
it at 0 still passes tests and cuts out some needless steps.

Oliver

diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index e08788e89..b311d6c5f 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -2031,9 +2031,9 @@ ca_parse_line(Cadef d, Cadef all, int multi, int first)
     state.def = state.ddef = NULL;
     state.curopt = state.dopt = NULL;
     state.argbeg = state.optbeg = state.nargbeg = state.restbeg = state.actopts =
-	state.nth = state.inopt = state.inarg = state.opt = state.arg = 1;
+	state.nth = state.inarg = state.opt = state.arg = 1;
     state.argend = argend = arrlen(compwords) - 1;
-    state.singles = state.oopt = 0;
+    state.inopt = state.singles = state.oopt = 0;
     state.curpos = compcurrent;
     state.args = znewlinklist();
     state.oargs = (LinkList *) zalloc(d->nopts * sizeof(LinkList));
@@ -2080,9 +2080,14 @@ ca_parse_line(Cadef d, Cadef all, int multi, int first)
         remnulargs(line);
         untokenize(line);
 
-	ca_inactive(d, argxor, cur - 1, 0);
-	if ((d->flags & CDF_SEP) && cur != compcurrent && !strcmp(line, "--")) {
+	if (argxor) {
+	    ca_inactive(d, argxor, cur - 1, 0);
+	    argxor = NULL;
+	}
+	if ((d->flags & CDF_SEP) && cur != compcurrent && state.actopts &&
+		!strcmp(line, "--")) {
 	    ca_inactive(d, NULL, cur, 1);
+	    state.actopts = 0;
 	    continue;
 	}
 
@@ -2235,11 +2240,17 @@ ca_parse_line(Cadef d, Cadef all, int multi, int first)
 	} else if (multi && (*line == '-' || *line == '+') && cur != compcurrent
 		&& (ca_foreign_opt(d, all, line)))
 	    return 1;
-	else if (state.arg &&
-		 (!napat || cur <= compcurrent || !pattry(napat, line))) {
+	else if (state.arg && cur <= compcurrent) {
 	    /* Otherwise it's a normal argument. */
-	    if (napat && cur <= compcurrent)
+
+	    /* test pattern passed to -A. if it matches treat this as an unknown
+	     * option and continue to the next word */
+	    if (napat && cur < compcurrent && state.actopts) {
+		if (pattry(napat, line))
+		    continue;
 		ca_inactive(d, NULL, cur + 1, 1);
+		state.actopts = 0;
+	    }
 
 	    arglast = 1;
 	    /* if this is the first normal arg after an option, may have been
@@ -2293,7 +2304,7 @@ ca_parse_line(Cadef d, Cadef all, int multi, int first)
             if (adef)
                 state.oopt = adef->num - state.nth;
 
-	    if (state.def)
+	    if (state.def && cur != compcurrent)
 		argxor = state.def->xor;
 
 	    if (state.def && state.def->type != CAA_NORMAL &&
diff --git a/Test/Y03arguments.ztst b/Test/Y03arguments.ztst
index bf41aead5..ef7a0ec56 100644
--- a/Test/Y03arguments.ztst
+++ b/Test/Y03arguments.ztst
@@ -359,6 +359,12 @@
 0:allowed option before --
 >line: {tst -x }{ --}
 
+ tst_arguments -S '1:one' '2:two'
+ comptest $'tst -- -- \t'
+0:only first of duplicate -- is ignored
+>line: {tst -- -- }{}
+>DESCRIPTION:{two}
+
  tst_arguments -x :word
  comptest $'tst word -\t'
 0:option after a word
@@ -390,6 +396,25 @@
 0:continue completion after rest argument that looks like an option
 >line: {tst -a -x more }{}
 
+ tst_arguments -A '-*' -a -b '*: :(words)'
+ comptest $'tst -x -\t'
+0:word matching -A pattern doesn't exclude options
+>line: {tst -x -}{}
+>DESCRIPTION:{option}
+>NO:{-a}
+>NO:{-b}
+
+ tst_arguments -A '-*' -a -b '1:word:(word)'
+ comptest $'tst -x \t'
+0:unrecognised word matching -A pattern not treated as a rest argument
+>line: {tst -x word }{}
+
+ tst_arguments -A "-*" '(3)-a' '1:one' '2:two' '3:three' '4:four' '*:extra'
+ comptest $'tst x -a \t'
+0:exclusion from option following word matching -A pattern should not apply
+>line: {tst x -a }{}
+>DESCRIPTION:{three}
+
  tst_arguments '*-v'
  comptest $'tst -v -\t'
 0:repeatable options
@@ -478,6 +503,16 @@
 >NO:{-b}
 >NO:{-v}
 
+ tst_arguments -a -b -c '(-a)1:one' '(-b)2:two' '(-c)*:extra'
+ comptest $'tst  x y z\e6\C-b-\t'
+0:exclude option from normal argument to the right of the cursor
+>line: {tst -}{ x y z}
+>DESCRIPTION:{one}
+>DESCRIPTION:{option}
+>NO:{-a}
+>NO:{-b}
+>NO:{-c}
+
  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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] fix option -A of _arguments
  2021-10-20 17:49 ` Oliver Kiddle
@ 2021-10-22  4:00   ` Jun T
  0 siblings, 0 replies; 3+ messages in thread
From: Jun T @ 2021-10-22  4:00 UTC (permalink / raw)
  To: zsh-workers


> 2021/10/21 2:49, Oliver Kiddle <opk@zsh.org> wrote:
> 
> Experimenting with this, I found further issues with -A.

Thanks for the patch; it solves lots of problems.


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-10-22  4:01 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19 10:23 [PATCH] fix option -A of _arguments Jun T
2021-10-20 17:49 ` Oliver Kiddle
2021-10-22  4:00   ` Jun T

Code repositories for project(s) associated with this inbox:

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).