Re: Bug#486785: tab completion broken

Re: Bug#486785: tab completion broken

[Clint Adams]

Using only the following zshrc, ls /tmp/.X2<tab> results in the entire
argument being elided (/tmp/.X* exists but /tmp/.X2* does not).

--8<--zshrc--8<--
autoload -U compinit
compinit

zstyle ':completion:*' completer _approximate

--8<--zshrc--8<--

Re: Bug#486785: tab completion broken

[martin f krafft]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

also sprach Clint Adams <schizo@debian.org> [2008.06.19.2315 +0200]:
> Using only the following zshrc, ls /tmp/.X2<tab> results in the entire
> argument being elided (/tmp/.X* exists but /tmp/.X2* does not).
> 
> --8<--zshrc--8<--
> autoload -U compinit
> compinit
> 
> zstyle ':completion:*' completer _approximate
> 
> --8<--zshrc--8<--

confirmed.

-- 
 .''`.   martin f. krafft <madduck@debian.org>
: :'  :  proud Debian developer, author, administrator, and user
`. `'`   http://people.debian.org/~madduck - http://debiansystem.info
  `-  Debian - when you have better things to do than fixing systems
 
micro$oft windows psychic edition:
we will tell you where you are going tomorrow.

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Bug#486785: tab completion broken

[Peter Stephenson]

On Thu, 19 Jun 2008 21:15:39 +0000
Clint Adams <schizo@debian.org> wrote:
> Using only the following zshrc, ls /tmp/.X2<tab> results in the entire
> argument being elided (/tmp/.X* exists but /tmp/.X2* does not).
> 
> --8<--zshrc--8<--
> autoload -U compinit
> compinit
> 
> zstyle ':completion:*' completer _approximate
> 
> --8<--zshrc--8<--

It's something to do with using the -U argument to compadd when you've
got ambiguous completions: it works fine when there's only a single
completion.  It was triggered by adding the -U's to do approximation in
non-final path segments (even before the compmatch change).  The initial
"." is a red herring.  However, I don't know where it's going wrong
since when I do approximation after normal completion (but lots of other
options set, too) it works fine.  This triggers a different part of
_path_files which has compadd -U but slight differences in the other
arguments.  Internally the Cline struct that's used for generating the
command line looks a bit spartan in the failed case.  Needless to say I
haven't the faintest clue where this is leading.

pws

Re: Bug#486785: tab completion broken

[Clint Adams]

On Sun, Jun 22, 2008 at 11:23:42PM +0100, Peter Stephenson wrote:
> It's something to do with using the -U argument to compadd when you've
> got ambiguous completions: it works fine when there's only a single
> completion.  It was triggered by adding the -U's to do approximation in
> non-final path segments (even before the compmatch change).  The initial
> "." is a red herring.  However, I don't know where it's going wrong
> since when I do approximation after normal completion (but lots of other
> options set, too) it works fine.  This triggers a different part of
> _path_files which has compadd -U but slight differences in the other
> arguments.  Internally the Cline struct that's used for generating the
> command line looks a bit spartan in the failed case.  Needless to say I
> haven't the faintest clue where this is leading.

Further clues:

1) these behave the same way

zstyle ':completion:*' completer _complete _ignored _match _approximate

zstyle ':completion:*' completer _complete _correct _approximate        

zstyle ':completion:*' completer _complete _approximate

2) this one behaves the same way on the second tab

zstyle ':completion:*' completer _list _expand _complete _ignored _match _correct _approximate _prefix

I have not performed an exhaustive search by any means, but I have not
yet run across a combination where the inclusion of _approximate doesn't
trigger the surprise elision.

Re: Bug#486785: tab completion broken

[Peter Stephenson]

On Sun, 22 Jun 2008 23:23:42 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> On Thu, 19 Jun 2008 21:15:39 +0000
> Clint Adams <schizo@debian.org> wrote:
> > Using only the following zshrc, ls /tmp/.X2<tab> results in the entire
> > argument being elided (/tmp/.X* exists but /tmp/.X2* does not).
> > 
> > --8<--zshrc--8<--
> > autoload -U compinit
> > compinit
> > 
> > zstyle ':completion:*' completer _approximate
> > 
> > --8<--zshrc--8<--
> 
> It's something to do with using the -U argument to compadd when you've
> got ambiguous completions: it works fine when there's only a single
> completion.  It was triggered by adding the -U's to do approximation in
> non-final path segments (even before the compmatch change).

Slight cop out, but the "-M <matcher>" option is not documented as doing
anything useful with the -U option and removing that wherever it occurs
together with -U makes it better.  (In case you're worried, this is not
the matcher from the matcher list, which is passed down to compfiles
earlier on to generate the list of trial completions rather than to
compadd; the matcher here appears to be some kind of "it works better
that way" kludge, I think to help path completion by allowing anything
that matches completions of partial path prefixes.)  Strictly I think
the -M argument should be ignored, and it isn't, which is probably a
bug, but I don't have time to investigate bugs that can be easily worked
around, and I think given the -U the new code is more correct anyway.
I've only removed the -M where there is a -U option.

This improves things to the point where the unambiguous prefix is kept
and the trial completions are available.  I think the main difference in
my own set-up was having menucomplete set, but my experiences are a bit
ambiguous.

I'm now a little worried it should be going into menu completion at this
point even without the option being set; the documentation for
_approximate says "the completer will normally start menu completion"
but in the test case above it doesn't.  I'm not clear how it starts menu
completion, since _approximate only mentions menu completion in a
comment at the top, implying it is done somewhere within that function
that I can't find.  If someone who doesn't usually have menu completion
set can confirm that it's still not doing the right thing, I will see if
I can find any further clues to how this should be happening.

I couldn't find anything this change broke and I don't think it's
supposed to have any negative effect.

The reason this happens with _approximate, by the way, seems to be due
to addition of the default completion:
  compadd -V -default- -U -Q - /tmp/.X2 
to the array of normal matches (despite the fact there is no -M argument
here---possibly the discrepancy between the two is confusing it).  I
traced the bug down to the function (assuming you've got /tmp/.X2 on
the command line---the tmp1 contents aren't important, they're just what
I happen to have):

  local -a tmp1

  tmp1=( .X0-lock .X11-unix .Xfoo )
  compadd -J -default- -U -Qf -J -default- -p /tmp/ -s '' \
    -W /tmp/ -M 'r:|/=* r:|=*' -a tmp1
  compadd -V -default- -U -Q - /tmp/.X2

However, I don't think that helps us with the remaining problem
because that extra compadd can't explain how we get into menu
completion.

(I have kept this mail short by expunging phrases like "in an obscure
fashion" and "undocumented" and "incomprehensible", as well as sarcastic
asides like "of course" and "as I'm sure you'd guessed", and
long-suffering comments like "spending weeks of my life", from wherever
they naturally occur.  You can have fun reinserting them.  You can have
even more fun writing a lex programme to fill out a completion system
bug fix posting with the appropriate comments.)

Index: Completion/Unix/Type/_path_files
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Unix/Type/_path_files,v
retrieving revision 1.31
diff -u -r1.31 _path_files
--- Completion/Unix/Type/_path_files	21 Jun 2008 21:36:01 -0000	1.31
+++ Completion/Unix/Type/_path_files	30 Jun 2008 19:55:41 -0000
@@ -598,7 +598,7 @@
 	    compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \
 	            -s "/${tmp3#*/}$ISUFFIX" \
 	            -W "$prepath$realpath$testpath" \
-		    "$pfxsfx[@]" -M "r:|/=* r:|=*" \
+		    "$pfxsfx[@]" \
 		    $listopts \
 	            -a tmp1
           else
@@ -608,7 +608,7 @@
 	    compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \
 	            -s "$ISUFFIX" \
 	            -W "$prepath$realpath$testpath" \
-		    "$pfxsfx[@]" -M "r:|/=* r:|=*" \
+		    "$pfxsfx[@]" \
 	            $listopts \
 		    -a tmp1
           fi
@@ -617,7 +617,7 @@
 	  compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \
 	          -s "$ISUFFIX" \
 	          -W "$prepath$realpath$testpath" \
-		   "$pfxsfx[@]" -M "r:|/=* r:|=*" \
+		   "$pfxsfx[@]" \
 	           $listopts \
 		   -a tmp1
 	fi
@@ -627,7 +627,7 @@
 	  tmp4=( -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2"
 	         -s "$ISUFFIX"
 	         -W "$prepath$realpath$testpath"
-	         "$pfxsfx[@]" -M "r:|/=* r:|=*" )
+	         "$pfxsfx[@]" )
 	  if [[ -z "$listsfx" ]]; then
             for i in "$tmp1[@]"; do
 	      tmpdisp=("${i%%/*}")
@@ -647,7 +647,7 @@
 	  compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp2" \
 	          -s "$ISUFFIX" \
                   -W "$prepath$realpath$testpath" \
-		  "$pfxsfx[@]" -M "r:|/=* r:|=*" \
+		  "$pfxsfx[@]" \
                   $listopts \
 		  -a tmp1
         fi
@@ -716,7 +716,7 @@
         compadd -U -Qf "$mopts[@]" -p "$IPREFIX$linepath$tmp3/" \
 	        -s "/$tmp4$i$ISUFFIX" \
                 -W "$prepath$realpath${mid%/*/}/" \
-	        "$pfxsfx[@]" -M "r:|/=* r:|=*" $listopts - "$tmp2"
+	        "$pfxsfx[@]" $listopts - "$tmp2"
       done
     else
       if [[ "$osuf" = */* ]]; then
@@ -747,7 +747,7 @@
         compadd -U -Qf -p "$IPREFIX$linepath$tmp4" \
 	        -s "$ISUFFIX" \
 	        -W "$prepath$realpath$testpath" \
-	        "$pfxsfx[@]" "$mopts[@]" -M "r:|/=* r:|=*" $listopts -a tmp1
+	        "$pfxsfx[@]" "$mopts[@]" $listopts -a tmp1
       fi
     fi
   fi


-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: Bug#486785: tab completion broken

[Clint Adams]

On Tue, Jul 01, 2008 at 06:26:25PM +0200, martin f krafft wrote:
> Unfortunately, I cannot find the repository from which zsh-beta is
> built, so I can't bisect it...

You can bisect

git://git.debian.org/git/private/schizo/tailor/zsh-cvs/.git

Re: Bug#486785: tab completion broken

[Peter Stephenson]

On Mon, 30 Jun 2008 21:35:22 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> I'm now a little worried it should be going into menu completion at this
> point even without the option being set; the documentation for
> _approximate says "the completer will normally start menu completion"
> but in the test case above it doesn't.

Comparing with 4.3.4 confirms this problem.

> I'm not clear how it starts menu
> completion, since _approximate only mentions menu completion in a
> comment at the top, implying it is done somewhere within that function
> that I can't find.

It comes from the combination of

  compstate[pattern_match]='*'

in _approximate and something I haven't bothered to track down setting

  compstate[insert]=menu

somewhere else.  The need for the combination, which I don't really
understand, also underlies what's going on in the code.

See the comment in the fix.  I think I've said quite enough there.

Please do report any remaining problems, since I've at least half
convinced myself I've fixed it, however unlikely that seems.

Index: Src/Zle/compcore.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Zle/compcore.c,v
retrieving revision 1.94
diff -u -r1.94 compcore.c
--- Src/Zle/compcore.c	6 May 2008 16:01:19 -0000	1.94
+++ Src/Zle/compcore.c	6 Jul 2008 19:38:17 -0000
@@ -2277,6 +2277,45 @@
 			haspattern = 1;
 		}
 	    }
+	} else {
+	    /*
+	     * (This is called a "comment".  Given you've been
+	     * spending your time reading the completion time, you
+	     * may have forgotten what one is.  It's used to deconfuse
+	     * the poor so-and-so who's landed up having to maintain
+	     * the code.)
+	     *
+	     * So what's going on here then?  I'm glad you asked.  To test
+	     * whether we should start menu completion, we test whether
+	     * compstate[insert] has been set to "menu", but only if we found
+	     * patterns in the code.  It's not clear to me from the
+	     * documentation why the second condition would apply, but sure
+	     * enough if I remove it the test suite falls over.  (Testing
+	     * comppatmatch at the later point doesn't work because compstate
+	     * is likely to have been reset by the point we actually insert
+	     * the completions, after all functions have exited; this is at
+	     * least part of the problem.)  In the present case, we are not
+	     * doing matching on the code because all the clever stuff has
+	     * been done over our heads and we've simply between told to
+	     * insert it.  However, we still need to take account of ambiguous
+	     * completions properly.  To do this, we rely on the caller to
+	     * pass down the same prefix/suffix with the patterns that we
+	     * would get if we were doing matching, and test those for
+	     * patterns.  This gets us out of the hole apparently without
+	     * breaking anything.  The particular case where this is needed is
+	     * approximate file completion: this does its own matching but
+	     * _approximate still sets the prefix to include the pattern.
+	     */
+	    if (comppatmatch && *comppatmatch) {
+		int pflen = strlen(compprefix);
+		char *tmp = zhalloc(pflen + strlen(compsuffix) + 1);
+		strcpy(tmp, compprefix);
+		strcpy(tmp + pflen, compsuffix);
+		tokenize(tmp);
+		remnulargs(tmp);
+		if (haswilds(tmp))
+		    haspattern = 1;
+	    }
 	}
 	if (*argv) {
 	    if (dat->pre)


-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: Bug#486785: tab completion broken

[Richard Hartmann]

On Sun, Jul 6, 2008 at 21:51, Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:


> It's not clear to me from the
> +            * documentation why the second condition would apply, but sure
> +            * enough if I remove it the test suite falls over.

You will most likely not know _which_ test falls over in a few years. I would
suggest adding the name or some other reference for future reference.
Won't hurt and might the life of you or someone else a tiny bit easier, at
some point :)


Richard

Re: Bug#486785: tab completion broken

[Frank Terbeck]

Peter Stephenson <p.w.stephenson@ntlworld.com>:
[...]
> +	    /*
> +	     * (This is called a "comment".  Given you've been
> +	     * spending your time reading the completion time, you
                              (I guess you meant)    code

> +	     * may have forgotten what one is.  It's used to deconfuse
> +	     * the poor so-and-so who's landed up having to maintain
> +	     * the code.)
[...]

Regards, Frank

-- 
In protocol design, perfection has been reached not when there is
nothing left to add, but when there is nothing left to take away.
                                                  -- RFC 1925