Another _path_files bug?

Another _path_files bug?

[Bart Schaefer]

I have my zsh build tree separate from my source tree.  I also have a
".zsh-test/.zshrc" file that does nothing but load compsys, so that I
can easily test completion issues.

schaefer[609] ZDOTDIR=$HOME/.zsh-test Src/zsh
torch% print $ZSH_VERSION $ZSH_PATCHLEVEL 
4.3.11-dev-1 1.5189
torch% ls Test
Makefile
torch% setopt completeinword

The completinword setting is necessary to cause the bug.

So now start completion:

torch% ls Test/

The "/" is bold (was auto-suffixed), so type a / so that it will remain.
Now back up so the cursor is on the "e". and complete again:

torch% ls Test//Test/Makefile

This only happens when there is only a single completion in the directory.
If the completion is ambiguous, it works correctly:

torch% touch Test/emptyfile  
torch% ls Test/
emptyfile  Makefile

(At this point the cursor has moved to be after the "/".)

The first significant difference seems to be at around line 740 - 760 of
_path_files where, in the completeinword case, $mid gets assigned on line
749.  I think, but am not yet sure, that when $tsuf contains but does not
begin with a "/", then at line 754 (and possibly also at line 752, but
again not sure) $cpre should NOT have a trailing slash appended.

Anyone else care to have a dive at this?

-- 

Re: Another _path_files bug?

[Bart Schaefer]

On Feb 10,  8:42am, Bart Schaefer wrote:
}
} torch% setopt completeinword
} torch% ls Test/
} torch% ls Test//Test/Makefile
} 
} This only happens when there is only a single completion in the directory.
} If the completion is ambiguous, it works correctly:
} 
} The first significant difference seems to be at around line 740 - 760 of
} _path_files where, in the completeinword case, $mid gets assigned on line
} 749.  I think, but am not yet sure, that when $tsuf contains but does not
} begin with a "/", then at line 754 (and possibly also at line 752, but
} again not sure) $cpre should NOT have a trailing slash appended.

That may or may not be the case, but I've narrowed this down to the compadd
at line 783.  In the failing case, this does:

    compadd -Qf -J -default- -J -default- -p Test/ \
            -s /Makefile -W Test// -M 'r:|/=* r:|=*' - Test

That is, it's attempting to make "Test" one of the completions, and at
the same time assert that "/Makefile" should be a suffix.  The trouble
is that it's also making "Test/" a prefix (-p).

The -p argument is "${Uopt:+$IPREFIX}$linepath$tmp3/" where Uopt and
IPREFIX and linepath are all empty.  tmp3="${mid%/*/}" from line 773,
which is just $mid because "Test/" does not begin with a slash.

I think it's pretty obvious that we don't want to be treating "Test"
as both a completion and a prefix, but we need the -p option in the
event that $linepath is not empty.  Also it's not clear that the final
slash should always be appended to the -p argument as it is there.

What's the right thing to do?

Re: Another _path_files bug?

[Peter Stephenson]

On Thu, 10 Feb 2011 09:28:20 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Feb 10,  8:42am, Bart Schaefer wrote:
> }
> } torch% setopt completeinword
> } torch% ls Test/
               ^completing here...
> } torch% ls Test//Test/Makefile
> } 
> } This only happens when there is only a single completion in the directory.
> } If the completion is ambiguous, it works correctly:
> } 
> } The first significant difference seems to be at around line 740 - 760 of
> } _path_files where, in the completeinword case, $mid gets assigned on line
> } 749.  I think, but am not yet sure, that when $tsuf contains but does not
> } begin with a "/", then at line 754 (and possibly also at line 752, but
> } again not sure) $cpre should NOT have a trailing slash appended.
> 
> That may or may not be the case, but I've narrowed this down to the compadd
> at line 783.  In the failing case, this does:
> 
>     compadd -Qf -J -default- -J -default- -p Test/ \
>             -s /Makefile -W Test// -M 'r:|/=* r:|=*' - Test

This is more an experiment than a suggested fix, but I wonder if the
problem is it's assuming there are three path sections: an initial path,
the middle bit, that the cursor's in the middle of, and then contents of
the directory at the end.  In the case here, we've only got two: the
"Test/" is being treated as both the initial part and the middle part.
So if we add a separate case for handling this it improves things.

Obvious we'll need to find out if the original problem was more general,
in which case splitting off this case may work but isn't the right fix,
or if this has repercussions in other simple cases.

The compquote stuff makes my hair stand on end, but I don't think
it comes in to play with simple file names like this.

By the way, we ought to take the opportunity to document further up that
this is handling completion in the middle of a path --- unless I'm the
only person in the world to feel that 'if [[ "$mid" = */ ]]' isn't
entirely self-explanatory.

Also, what does the 'if [[ -z "$tmp4" ]]' just above that signify?  I
think (from the comment above 729) it might just mean we've finished
collecting things and are about to add them to the completion ---
there's no 'else' in this case.  Even if we can't decide for sure a bit
of robust commentary speculation is probably better than the status quo.

I'm just imagining a Samuel Beckett character writing this file, so it's
time to stop and have supper.

Index: Completion/Unix/Type/_path_files
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Unix/Type/_path_files,v
retrieving revision 1.53
diff -p -u -r1.53 _path_files
--- Completion/Unix/Type/_path_files	6 Aug 2010 15:29:58 -0000	1.53
+++ Completion/Unix/Type/_path_files	11 Feb 2011 20:24:06 -0000
@@ -769,21 +769,35 @@ for prepath in "$prepaths[@]"; do
       SUFFIX="${osuf}"
 
       tmp4="${testpath#${mid}}"
-      tmp3="${mid%/*/}"
-      tmp2="${${mid%/}##*/}"
-      if [[ -n "$linepath" ]]; then
-        compquote -p tmp3
+      if [[ $mid = */*/* ]]; then
+	# Multiple levels of directory involved.
+	tmp3="${mid%/*/}"
+	tmp2="${${mid%/}##*/}"
+	if [[ -n "$linepath" ]]; then
+          compquote -p tmp3
+	else
+          compquote tmp3
+	fi
+	compquote tmp4 tmp2 tmp1
+	for i in "$tmp1[@]"; do
+	  _list_files tmp2 "$prepath$realpath${mid%/*/}"
+          compadd $Uopt -Qf "$mopts[@]" -p "${Uopt:+$IPREFIX}$linepath$tmp3/" \
+	    -s "/$tmp4$i${Uopt:+$ISUFFIX}" \
+            -W "$prepath$realpath${mid%/*/}/" \
+	    "$pfxsfx[@]" $Mopts $listopts - "$tmp2"
+	done
       else
-        compquote tmp3
+	# Simpler case with fewer directories: avoid double counting.
+	tmp2="${${mid%/}##*/}"
+	compquote tmp4 tmp2 tmp1
+	for i in "$tmp1[@]"; do
+	  _list_files tmp2 "$prepath$realpath${mid%/*/}"
+          compadd $Uopt -Qf "$mopts[@]" -p "${Uopt:+$IPREFIX}$linepath" \
+	    -s "/$tmp4$i${Uopt:+$ISUFFIX}" \
+            -W "$prepath$realpath" \
+	    "$pfxsfx[@]" $Mopts $listopts - "$tmp2"
+	done
       fi
-      compquote tmp4 tmp2 tmp1
-      for i in "$tmp1[@]"; do
-	_list_files tmp2 "$prepath$realpath${mid%/*/}"
-        compadd $Uopt -Qf "$mopts[@]" -p "${Uopt:+$IPREFIX}$linepath$tmp3/" \
-	        -s "/$tmp4$i${Uopt:+$ISUFFIX}" \
-                -W "$prepath$realpath${mid%/*/}/" \
-	        "$pfxsfx[@]" $Mopts $listopts - "$tmp2"
-      done
     else
       if [[ "$osuf" = */* ]]; then
         PREFIX="${opre}${osuf}"

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

Re: Another _path_files bug?

[Bart Schaefer]

On Feb 11,  8:39pm, Peter Stephenson wrote:
} Subject: Re: Another _path_files bug?
}
} On Thu, 10 Feb 2011 09:28:20 -0800
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > }
} > } torch% setopt completeinword
} > } torch% ls Test/
}                ^completing here...
} > } torch% ls Test//Test/Makefile
} > 
} >     compadd -Qf -J -default- -J -default- -p Test/ \
} >             -s /Makefile -W Test// -M 'r:|/=* r:|=*' - Test
} 
} This is more an experiment than a suggested fix, but I wonder if the
} problem is it's assuming there are three path sections: an initial path,
} the middle bit, that the cursor's in the middle of, and then contents of
} the directory at the end.  In the case here, we've only got two: the
} "Test/" is being treated as both the initial part and the middle part.

Yes, that does seem to describe the situation.

} So if we add a separate case for handling this it improves things.

This works for the test cases I could think of applying, such as putting
a full path at the front and (separately) where the tail is ambiguous.
 
} Obvious we'll need to find out if the original problem was more general,
} in which case splitting off this case may work but isn't the right fix,
} or if this has repercussions in other simple cases.

I haven't found any that break, but I don't have, e.g., any files with
multibyte characters in their names handy to try.

} Also, what does the 'if [[ -z "$tmp4" ]]' just above that signify?  I
} think (from the comment above 729) it might just mean we've finished
} collecting things and are about to add them to the completion ---

That's must be essentially correct -- when tmp4 is non-empty we go back
to the top of "for prepath in ..." at line 350, otherwise we enter this
block and (eventually) do one of "for i in "$tmp1[@]" ..." to add each
string from tmp1 individually, or "compadd ... -a tmp1" to add them all
at once.

In other cases, though, we compadd the contents of tmp1 in the body of
the "while true;" loop from line 595.  See comment at 597.  At the point
where tmp4 becomes empty, tmp1=( Makefile ) [the sole completion in the
Test subdir] so empty tmp4 appears to signify that we have in fact NOT
found a real file matching the current component (the current component
in this instance is the empty path tail after "Test/").

Re: Another _path_files bug?

[Bart Schaefer]

On Feb 12,  2:22pm, Bart Schaefer wrote:
}
} } Obvious we'll need to find out if the original problem was more general,
} } in which case splitting off this case may work but isn't the right fix,
} } or if this has repercussions in other simple cases.
} 
} I haven't found any that break, but I don't have, e.g., any files with
} multibyte characters in their names handy to try.

Hmm, not sure if this is related, but (with "|" representing the cursor
position):

schaefer<522> ls ../z/T/M|
schaefer<522> ls ../zsh-|/T/M

I then hit TAB again to get a listing:

schaefer<522> ls ../zsh-|/T/M
Completing files
zsh-3.1.6/        zsh-4.3/          zsh-4.3-build/    zsh-4.3-dynamic/

So far so good.  Now hit TAB a third time to cycle through choices:

schaefer<522> ls ../zsh-3.1.6/T/M|
Completing files
zsh-3.1.6/        zsh-4.3/          zsh-4.3-build/    zsh-4.3-dynamic/

Oops, the cursor has moved all the way to the end of the word.  I think
it should have been positioned right after the 6.  Ah, says I to myself,
I bet ALWAYS_TO_END is involved here.  Let's try again; nope, no diff.

Ah, but if I BOTH unsetopt alwaystoend AND start the completion here:

schaefer<526> ls ../z|/T/M

Then it works:

schaefer<526> ls ../zsh-3.1.6|/T/M
Completing files
zsh-3.1.6/        zsh-4.3/          zsh-4.3-build/    zsh-4.3-dynamic/

This much I can reproduce starting from zsh -f plus compsys.  However,
with my usual matcher-list style added, the above works but the
following breaks:

schaefer<528> ls ../|z/T/M
schaefer<528> ls ../z.|

The style is:

zstyle ':completion:*' matcher-list '' 'r:|[-._,]=** r:|=**' \
  'm:{a-zA-Z}={A-Za-z} r:|[-._,]=** r:|=**' 'r:|[-._,]=** r:|=** l:|=*'

Any clue why that causes the path tail to get discarded?  In the case
with no matcher-list, the compadd at line 493 finds nine possible
matches for (../)(z/), but with the matcher-list it finds none.

The actual files:

../zsh-2.4          ../zsh-3.1.6        ../zsh-4.3-dynamic
../zsh-3.0.5        ../zsh-4.3          ../zsh-forge
../zsh-3.0.8-pre    ../zsh-4.3-build    ../zsh-tarball

(Above are the nine that fail the compadd with the matcher-list.  Below
are the possible completions of the whole path.)

../zsh-3.1.6/Test/Makefile.in     ../zsh-4.3/Test/Makefile.in
../zsh-4.3-build/Test/Makefile    ../zsh-4.3/Test/README
../zsh-4.3-dynamic/Test/Makefile

Re: Another _path_files bug?

[Peter Stephenson]

On Sat, 12 Feb 2011 14:22:22 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> This works for the test cases I could think of applying, such as putting
> a full path at the front and (separately) where the tail is ambiguous.

I've committed it with some extra comments that are a bit vaguer than
would be ideal.

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