'compadd -P' matches $PREFIX greedily

'compadd -P' matches $PREFIX greedily

[Daniel Shahaf]

The following completion function:

% compdef 'compadd -P p_____ papa mama nana aaaa' f

gives:

% f ma<TAB>   → p_____mama
% f na<TAB>   → p_____nana
% f pa<TAB>   → p_____aaaa
%
% f pm<TAB>   → p_____mama
% f pn<TAB>   → p_____nana
% f pp<TAB>   → p_____papa
%
% f p_p<TAB>  → p_____papa

I expected the third case to offer «p_____papa» as a completion, by
analogy with the first two cases.

The reason it doesn't is that the argument to the -P option («p_____»)
is matched with the command-line word («pa») and their common prefix is
discarded from the command-line word before completion proceeds.  For
«pa<TAB>» the common prefix is «p», leaving «a» as the command-line
word, so completion looks for matches that start with «p_____a»; while
for «ma<TAB>» the common prefix is «» (empty string) so completion looks
for matches that start with «p_____ma».

This patch makes skipping the -P prefix an all-or-nothing affair: the -P
prefix will only be skipped if it the longest common prefix it and
${PREFIX} have is equal to the -P prefix.

diff --git a/Src/Zle/compcore.c b/Src/Zle/compcore.c
index 2f9fb33..05d2706 100644
--- a/Src/Zle/compcore.c
+++ b/Src/Zle/compcore.c
@@ -2029,7 +2029,7 @@ addmatches(Cadata dat, char **argv)
     char **aign = NULL, **dparr = NULL, *oaq = autoq, *oppre = dat->ppre;
     char *oqp = qipre, *oqs = qisuf, qc, **disp = NULL, *ibuf = NULL;
     char **arrays = NULL;
-    int lpl, lsl, pl, sl, bcp = 0, bcs = 0, bpadd = 0, bsadd = 0;
+    int lpl, lsl, sl, bcp = 0, bcs = 0, bpadd = 0, bsadd = 0;
     int ppl = 0, psl = 0, ilen = 0;
     int llpl = 0, llsl = 0, nm = mnum, gflags = 0, ohp = haspattern;
     int isexact, doadd, ois = instring, oib = inbackt;
@@ -2193,9 +2193,12 @@ addmatches(Cadata dat, char **argv)
 
 	    /* Test if there is an existing -P prefix. */
 	    if (dat->pre && *dat->pre) {
-		pl = pfxlen(dat->pre, lpre);
-		llpl -= pl;
-		lpre += pl;
+		int prefix_length = pfxlen(dat->pre, lpre);
+		if (dat->pre[prefix_length] == '\0') {
+		    /* $compadd_args[-P] is a prefix of ${PREFIX}. */
+		    llpl -= prefix_length;
+		    lpre += prefix_length;
+		}
 	    }
 	}
 	/* Now duplicate the strings we have from the command line. */

(Here lpre is ${PREFIX} and llpl is its strlen().)

Thoughts?

Cheers,

Daniel

Re: 'compadd -P' matches $PREFIX greedily

[Bart Schaefer]

On Sep 17,  6:32am, Daniel Shahaf wrote:
}
} This patch makes skipping the -P prefix an all-or-nothing affair: the -P
} prefix will only be skipped if it the longest common prefix it and
} ${PREFIX} have is equal to the -P prefix.

I had to read that sentence three times before I figured out that you
were saying what I thought you ought to be saying. :-)

} Thoughts?

Seems sensible to me, I can't think of any expected behavior that it
would break.

Re: 'compadd -P' matches $PREFIX greedily

[Oliver Kiddle]

Daniel Shahaf wrote:
> The following completion function:
>
> % compdef 'compadd -P p_____ papa mama nana aaaa' f
> % f pa<TAB>   → p_____aaaa

Out of interest, was there a real prefix where you came across this.
I was trying to think of more realistic prefixes to see how this
might affect them:

% compdef 'compadd -P file:// /papa //mama /nana aaaa filter' f

before: f fil<tab> → file://
after : f fil<tab> → file://filter

That now seems a bit too aggressive.

How about allowing the longest common prefix to be equal to either
the -P prefix or equal to $PREFIX.

Oliver

Re: 'compadd -P' matches $PREFIX greedily

[Daniel Shahaf]

Oliver Kiddle wrote on Sun, Sep 18, 2016 at 03:28:07 +0200:
> Daniel Shahaf wrote:
> > The following completion function:
> >
> > % compdef 'compadd -P p_____ papa mama nana aaaa' f
> > % f pa<TAB>   → p_____aaaa
> 
> Out of interest, was there a real prefix where you came across this.

Yes, I ran into it in _bts.  I'll send that patch separately for X-Seq
purposes, but the tl;dr is:

    _f() { _email_addresses -c -P "from:" }
    _email-foo() { compadd eee fee }
    f fe<TAB>  → from:eee

where «from:fee» was the expected result.

> I was trying to think of more realistic prefixes to see how this
> might affect them:
> 
> % compdef 'compadd -P file:// /papa //mama /nana aaaa filter' f
> 
> before: f fil<tab> → file://
> after : f fil<tab> → file://filter
> 
> That now seems a bit too aggressive.
> 

Agreed.  The ability to complete the prefix "file://" shouldn't depend
on what hostnames (matches) are available.  Especially as hostnames (and
email addresses) are added and removed over time.

> How about allowing the longest common prefix to be equal to either
> the -P prefix or equal to $PREFIX.

In both of our examples, the last character of $compadd_args[-P] is
a delimiter.

For fil<TAB> I'm not sure what's right.  Perhaps we should offer both
«file://<CURSOR>» (offering all five matches) and «file://filter».  I'm
not certain how the UI for this would look.  Conceptually, it would
involve trying two different values for the common prefix: both 0 (the
empty string) and min(${#PREFIX}, ${#compadd_args[-P]}) if one of those
two is a prefix of the other.  I'm not sure how easy or hard trying two
values would be to implement.

In the meantime, an interdiff implementing your suggestion is attached.
I'm inclined to commit it, and we can tweak it further later if need be.

Cheers,

Daniel

Interdiff:
diff --git a/Src/Zle/compcore.c b/Src/Zle/compcore.c
index 05d2706..5443018 100644
--- a/Src/Zle/compcore.c
+++ b/Src/Zle/compcore.c
@@ -2194,8 +2194,10 @@ addmatches(Cadata dat, char **argv)
 	    /* Test if there is an existing -P prefix. */
 	    if (dat->pre && *dat->pre) {
 		int prefix_length = pfxlen(dat->pre, lpre);
-		if (dat->pre[prefix_length] == '\0') {
-		    /* $compadd_args[-P] is a prefix of ${PREFIX}. */
+		if (dat->pre[prefix_length] == '\0' ||
+		    lpre[prefix_length] == '\0') {
+		    /* $compadd_args[-P] is a prefix of ${PREFIX}, or
+		     * vice-versa. */
 		    llpl -= prefix_length;
 		    lpre += prefix_length;
 		}

[PATCH] _bts: Complete more argument types for 'cache' and 'show'.

[Daniel Shahaf]

---
 Completion/Debian/Command/_bts | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Completion/Debian/Command/_bts b/Completion/Debian/Command/_bts
index 75085a9..d7aac9e 100644
--- a/Completion/Debian/Command/_bts
+++ b/Completion/Debian/Command/_bts
@@ -41,8 +41,9 @@ case "$words[1]" in
   (show|bugs)
     if [[ CURRENT -eq 2 ]]; then
       _alternative \
-	'packages:package:_deb_packages avail' \
-        'emails:package maintainer:compadd $DEBEMAIL'
+        'packages:package:_deb_packages avail' \
+        'emails:package maintainer:compadd $DEBEMAIL' \
+        'bugnum:bug number:_debbugs_bugnumber'
     else
       _wanted sep expl 'separator' compadd -S ' ' , .
     fi
@@ -199,8 +200,10 @@ case "$words[1]" in
   ;;
   (cache)
      _alternative \
-       'package:package:_deb_packages avail' \
-       'email:email address:_email_addresses -c' \
+       'source-packages:source package:_deb_packages -P "src:" source' \
+       'package:binary package:_deb_packages avail' \
+       'email:email address:_email_addresses -c -P "from:"' \
+       'bugnum:bug number:_debbugs_bugnumber' \
        'rc:rc:compadd release-critical'
   ;;
   (cleancache)