[PATCH 1/2] completion: improve ls-files filter performance

[PATCH 1/2] completion: improve ls-files filter performance

[Clemens Buchacher]

From the output of ls-files, we remove all but the leftmost path
component and then we eliminate duplicates. We do this in a while loop,
which is a performance bottleneck when the number of iterations is large
(e.g. for 60000 files in linux.git).

$ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git

real    0m11.876s
user    0m4.685s
sys     0m6.808s

Using an equivalent sed script improves performance significantly:

$ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git

real    0m1.372s
user    0m0.263s
sys     0m0.167s

The measurements were done with mingw64 bash, which is used by Git for
Windows.

Signed-off-by: Clemens Buchacher <drizzd@gmx.net>
---
 contrib/completion/git-completion.bash | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6da95b8..e3ddf27 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -384,12 +384,7 @@ __git_index_files ()
 	local root="${2-.}" file
 
 	__git_ls_files_helper "$root" "$1" |
-	while read -r file; do
-		case "$file" in
-		?*/*) echo "${file%%/*}" ;;
-		*) echo "$file" ;;
-		esac
-	done | sort | uniq
+	sed -e '/^\//! s#/.*##' | sort | uniq
 }
 
 # Lists branches from the local repository.
-- 
2.7.4

[PATCH 2/2] completion: simplify ls-files filter

[Clemens Buchacher]

When filtering the ls-files output we take care not to touch absolute
paths. This is redundant, because ls-files will never output absolute
paths. Furthermore, sorting the output is also redundant, because the
output of ls-files is already sorted.

Remove the unnecessary operations.

Signed-off-by: Clemens Buchacher <drizzd@gmx.net>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e3ddf27..394c3df 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -384,7 +384,7 @@ __git_index_files ()
 	local root="${2-.}" file
 
 	__git_ls_files_helper "$root" "$1" |
-	sed -e '/^\//! s#/.*##' | sort | uniq
+	cut -f1 -d/ | uniq
 }
 
 # Lists branches from the local repository.
-- 
2.7.4

Re: [PATCH 1/2] completion: improve ls-files filter performance

[Junio C Hamano]

Clemens Buchacher <drizzd@gmx.net> writes:

> From the output of ls-files, we remove all but the leftmost path
> component and then we eliminate duplicates. We do this in a while loop,
> which is a performance bottleneck when the number of iterations is large
> (e.g. for 60000 files in linux.git).
>
> $ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git
>
> real    0m11.876s
> user    0m4.685s
> sys     0m6.808s
>
> Using an equivalent sed script improves performance significantly:
>
> $ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git
>
> real    0m1.372s
> user    0m0.263s
> sys     0m0.167s
>
> The measurements were done with mingw64 bash, which is used by Git for
> Windows.
>
> Signed-off-by: Clemens Buchacher <drizzd@gmx.net>
> ---
>  contrib/completion/git-completion.bash | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 6da95b8..e3ddf27 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -384,12 +384,7 @@ __git_index_files ()
>  	local root="${2-.}" file
>  
>  	__git_ls_files_helper "$root" "$1" |
> -	while read -r file; do
> -		case "$file" in
> -		?*/*) echo "${file%%/*}" ;;
> -		*) echo "$file" ;;
> -		esac
> -	done | sort | uniq
> +	sed -e '/^\//! s#/.*##' | sort | uniq

Micronit: perhaps lose SP after '!'?

cf. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html

"""A function can be preceded by a '!' character, in which case the
function shall be applied if the addresses do not select the pattern
space. Zero or more <blank> characters shall be accepted before the
'!' character. It is unspecified whether <blank> characters can
follow the '!' character, and conforming applications shall not
follow the '!' character with <blank> characters."""


>  }
>  
>  # Lists branches from the local repository.

Re: [PATCH 2/2] completion: simplify ls-files filter

[Junio C Hamano]

Clemens Buchacher <drizzd@gmx.net> writes:

> When filtering the ls-files output we take care not to touch absolute
> paths. This is redundant, because ls-files will never output absolute
> paths. Furthermore, sorting the output is also redundant, because the
> output of ls-files is already sorted.
>
> Remove the unnecessary operations.
>
> Signed-off-by: Clemens Buchacher <drizzd@gmx.net>
> ---

Makes sense, and I think you can and should just directly jump to
this concluding state without having an intermediate "sed" version.
The fact that the code does not have to worry about absolute paths
and unsorted input is shared with the original version, too, so the
proposed log message for this one applies equally well to such a
squashed patch.



>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index e3ddf27..394c3df 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -384,7 +384,7 @@ __git_index_files ()
>  	local root="${2-.}" file
>  
>  	__git_ls_files_helper "$root" "$1" |
> -	sed -e '/^\//! s#/.*##' | sort | uniq
> +	cut -f1 -d/ | uniq
>  }
>  
>  # Lists branches from the local repository.

Re: [PATCH 2/2] completion: simplify ls-files filter

[SZEDER Gábor]

> When filtering the ls-files output we take care not to touch absolute
> paths. This is redundant, because ls-files will never output absolute
> paths. Furthermore, sorting the output is also redundant, because the
> output of ls-files is already sorted.
> 
> Remove the unnecessary operations.

You didn't run the test suite, did you? ;)

First, neither 'git ls-files' nor 'git diff-index' produce quite the
same order as the 'sort' utility does, e.g.:

  $ touch foo.c foo-zzz.c
  $ git add foo*
  $ git diff-index --name-only HEAD
  foo-zzz.c
  foo.c
  $ git diff-index --name-only HEAD |sort
  foo.c
  foo-zzz.c

Second, the output of 'git ls-files' is kind of "block-sorted": if you
were to invoke it with the options '--cached --modified --others',
then it will first list all untracked files in order, then all cached
files in order, and finally all modified files in order.  Note the
implications:

  - A file could theoretically be listed twice, because a modified
    file is inherently cached as well.  I believe this doesn't happen
    currently, because no path completions use the combination of
    '--modified --cached', but we use a lot of options when completing
    paths for 'git status', and I haven't thought that through.

  - A directory name is repeated in two (or more) blocks, if it
    contains modified and untracked files as well.  We do use the
    combination of '--modified --others' for 'git add', and '--cached
    --others' for 'git mv', so this does happen.

Note also that there can be any number of other files between the same
directory listed in two different blocks.  That 'sort' that this patch
is about to remove took care of this, but without that 'sort' the same
directory name can be listed more than once even after 'uniq'.
Consequently, the subsequent filtering of paths matching the current
word to be completed might have twice as much work to do.

All this leads to the failure of an enormous test in t9902, hence my
rethorical question at the beginning of my reply.


I have a short patch series collecting dust somewhere for a long
while, which pulls a couple more tricks to make git-aware path
completion faster, but haven't submitted it yet, because it doesn't
work quite that well when filenames require quoting.  Though, arguably
the current version doesn't work quite that well with quoted filenames
either, so...
Will try to dig up those patches.


> Signed-off-by: Clemens Buchacher <drizzd@gmx.net>
> ---
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index e3ddf27..394c3df 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -384,7 +384,7 @@ __git_index_files ()
>  	local root="${2-.}" file
>  
>  	__git_ls_files_helper "$root" "$1" |
> -	sed -e '/^\//! s#/.*##' | sort | uniq
> +	cut -f1 -d/ | uniq
>  }
>  
>  # Lists branches from the local repository.
> -- 
> 2.7.4
> 
> 

Re: [PATCH 2/2] completion: simplify ls-files filter

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> First, neither 'git ls-files' nor 'git diff-index' produce quite the
> same order as the 'sort' utility does, e.g.:
>
>   $ touch foo.c foo-zzz.c
>   $ git add foo*
>   $ git diff-index --name-only HEAD
>   foo-zzz.c
>   foo.c
>   $ git diff-index --name-only HEAD |sort
>   foo.c
>   foo-zzz.c

Doesn't this depend on your locale?

    $ printf "foo%s\n" .c -zzz.c /c | LC_ALL=C sort
    foo-zzz.c
    foo.c
    foo/c

    $ printf "foo%s\n" .c -zzz.c /c | LC_ALL=en_US.UTF-8 sort
    foo/c
    foo.c
    foo-zzz.c

> Second, the output of 'git ls-files' is kind of "block-sorted": if you
> were to invoke it with the options '--cached --modified --others',
> then it will first list all untracked files in order, then all cached
> files in order, and finally all modified files in order.

This is a lot more important consideration.

> I have a short patch series collecting dust somewhere for a long
> while, which pulls a couple more tricks to make git-aware path
> completion faster, but haven't submitted it yet, because it doesn't
> work quite that well when filenames require quoting.  Though, arguably
> the current version doesn't work quite that well with quoted filenames
> either, so...
> Will try to dig up those patches.

Thanks.

Re: [PATCH 1/2] completion: improve ls-files filter performance

[Johannes Schindelin]

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

Hi drizzd,

first of all: thank you so much for working on this. I am sure it will
be noticeable to many Windows users, and also make my life easier.

On Sat, 17 Mar 2018, Clemens Buchacher wrote:

> From the output of ls-files, we remove all but the leftmost path
> component and then we eliminate duplicates. We do this in a while loop,
> which is a performance bottleneck when the number of iterations is large
> (e.g. for 60000 files in linux.git).
> 
> $ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git
> 
> real    0m11.876s
> user    0m4.685s
> sys     0m6.808s
> 
> Using an equivalent sed script improves performance significantly:
> 
> $ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git
> 
> real    0m1.372s
> user    0m0.263s
> sys     0m0.167s
> 
> The measurements were done with mingw64 bash, which is used by Git for
> Windows.

Technically, it is not the *mingw64* bash, but it is an MSYS2 Bash. This
does make a little bit of a difference because of the penalty incurred by
the POSIX emulation layer provided by the MSYS2 runtime.

(And it also addresses Gabór's question whether you ran the test suite, I
guess... it takes multiple hours to run it even once on a regular
computer.)

Ciao,
Dscho

[PATCH] completion: improve ls-files filter performance

[Clemens Buchacher]

From the output of ls-files, we remove all but the leftmost path
component and then we eliminate duplicates. We do this in a while loop,
which is a performance bottleneck when the number of iterations is large
(e.g. for 60000 files in linux.git).

$ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git

real    0m11.876s
user    0m4.685s
sys     0m6.808s

Replacing the loop with the cut command improves performance
significantly:

$ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git

real    0m1.372s
user    0m0.263s
sys     0m0.167s

The measurements were done with Msys2 bash, which is used by Git for
Windows.

When filtering the ls-files output we take care not to touch absolute
paths. This is redundant, because ls-files will never output absolute
paths. Remove the unnecessary operations.

The issue was reported here:
https://github.com/git-for-windows/git/issues/1533

Signed-off-by: Clemens Buchacher <drizzd@gmx.net>
---

On Sun, Mar 18, 2018 at 02:26:18AM +0100, SZEDER Gábor wrote:
> 
> You didn't run the test suite, did you? ;)

My bad. I put the sort back in. Test t9902 is now pass. I did not run
the other tests. I think the completion script is not used there.

I also considered Junio's and Johannes' comments.

> I have a short patch series collecting dust somewhere for a long
> while, [...]
> Will try to dig up those patches.

Cool. Bash completion can certainly use more performance improvements.

 contrib/completion/git-completion.bash | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6da95b8..69a2d41 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -384,12 +384,7 @@ __git_index_files ()
 	local root="${2-.}" file
 
 	__git_ls_files_helper "$root" "$1" |
-	while read -r file; do
-		case "$file" in
-		?*/*) echo "${file%%/*}" ;;
-		*) echo "$file" ;;
-		esac
-	done | sort | uniq
+	cut -f1 -d/ | sort | uniq
 }
 
 # Lists branches from the local repository.
-- 
2.7.4

Re: [PATCH] completion: improve ls-files filter performance

[Johannes Schindelin]

Hi drizzd,

On Wed, 4 Apr 2018, Clemens Buchacher wrote:

> From the output of ls-files, we remove all but the leftmost path
> component and then we eliminate duplicates. We do this in a while loop,
> which is a performance bottleneck when the number of iterations is large
> (e.g. for 60000 files in linux.git).
> 
> $ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git
> 
> real    0m11.876s
> user    0m4.685s
> sys     0m6.808s
> 
> Replacing the loop with the cut command improves performance
> significantly:
> 
> $ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git
> 
> real    0m1.372s
> user    0m0.263s
> sys     0m0.167s
> 
> The measurements were done with Msys2 bash, which is used by Git for
> Windows.

Those are nice numbers right there, so I am eager to get this into Git for
Windows as quickly as it stabilizes (i.e. when it hits `next` or so).

I was wondering about one thing, though:

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 6da95b8..69a2d41 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -384,12 +384,7 @@ __git_index_files ()
>  	local root="${2-.}" file
>  
>  	__git_ls_files_helper "$root" "$1" |
> -	while read -r file; do
> -		case "$file" in
> -		?*/*) echo "${file%%/*}" ;;

This is a bit different from the `cut -f1 -d/` logic, as it does *not
necessarily* strip a leading slash: for `/abc` the existing code would
return the string unmodified, for `/abc/def` it would return an empty
string!

Now, I think that this peculiar behavior is most likely bogus as `git
ls-files` outputs only relative paths (that I know of). In any case,
reducing paths to an empty string seems fishy.

I looked through the history of that code and tracked it all the way back
to
https://public-inbox.org/git/1357930123-26310-1-git-send-email-manlio.perillo@gmail.com/
(that is the reason why you are Cc:ed, Manlio). Manlio, do you remember
why you put the `?` in front of `?*/*` here? I know, it's been more than
five years...

Out of curiosity, would the numbers change a lot if you replaced the `cut
-f1 -d/` call by a `sed -e 's/^\//' -e 's/\/.*//'` one?

I am not proposing to change the patch, though, because we really do not
need to expect `ls-files` to print lines with leading slashes.

> -		*) echo "$file" ;;
> -		esac
> -	done | sort | uniq
> +	cut -f1 -d/ | sort | uniq
>  }
>  
>  # Lists branches from the local repository.

Ciao,
Dscho

[PATCH 00/11] completion: path completion improvements: speedup and quoted paths

[SZEDER Gábor]

So, the highlights of this patch series are:

  - At the top of the worktree in linux.git with over 62k files the time
    needed for 'git rm <TAB>' goes down from 2.15s to 0.052s.

  - Same place, uniquely completing Makefile with 'git rm Mak<TAB>' goes
    down from 2.16s to 0.033s.

  - Completing paths containing escaped or quoted characters (except
    newline) will work properly, and won't fall back to Bash's filename
    completion, e.g. git add File\\w<TAB>' will list
    'File\with\backslashes.c', but not the corresponding ignored object
    file.

Well, at least it appears to be working for me, but dealing with quoting
especially is nuts.  If someone knows simpler ways to remove escaping
and quoting than in the functions added in patches 5 and 10, then
please, I would really love to learn something :)

Someone using ZSH regularly should have a look and test it.

SZEDER Gábor (11):
  t9902-completion: add tests demonstrating issues with quoted pathnames
  completion: move __git_complete_index_file() next to its helpers
  completion: simplify prefix path component handling during path
    completion
  completion: support completing non-ASCII pathnames
  completion: improve handling quoted paths on the command line
  completion: let 'ls-files' and 'diff-index' filter matching paths
  completion: use 'awk' to strip trailing path components
  t9902-completion: ignore COMPREPLY element order in some tests
  completion: remove repeated dirnames with 'awk' during path completion
  completion: improve handling quoted paths in 'git ls-files's output
  completion: fill COMPREPLY directly when completing paths

 contrib/completion/git-completion.bash | 218 +++++++++++++++++++++----
 contrib/completion/git-completion.zsh  |   9 +
 t/t9902-completion.sh                  | 153 ++++++++++++++++-
 3 files changed, 348 insertions(+), 32 deletions(-)

-- 
2.17.0.366.gbe216a3084

[PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames

[SZEDER Gábor]

Completion functions see all words on the command line verbatim,
including any backslash-escapes, single and double quotes that might
be there.  Furthermore, git commands quote pathnames if they contain
certain special characters.  All these create various issues when
doing git-aware path completion.

Add a couple of failing tests to demonstrate these issues.

Later patches in this series will discuss these issues in detail as
they fix them.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

Notes:
    Do any more new tests need FUNNYNAMES* prereq?

 t/t9902-completion.sh | 91 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index b7f5b1e632..ff2e4a8f5f 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1427,6 +1427,97 @@ test_expect_success 'complete files' '
 	test_completion "git add mom" "momified"
 '
 
+# The next tests only care about how the completion script deals with
+# unusual characters in path names.  By defining a custom completion
+# function to list untracked files they won't be influenced by future
+# changes of the completion functions of real git commands, and we
+# don't have to bother with adding files to the index in these tests.
+_git_test_path_comp ()
+{
+	__git_complete_index_file --others
+}
+
+test_expect_failure 'complete files - escaped characters on cmdline' '
+	test_when_finished "rm -rf \"New|Dir\"" &&
+	mkdir "New|Dir" &&
+	>"New|Dir/New&File.c" &&
+
+	test_completion "git test-path-comp N" \
+			"New|Dir" &&	# Bash will turn this into "New\|Dir/"
+	test_completion "git test-path-comp New\\|D" \
+			"New|Dir" &&
+	test_completion "git test-path-comp New\\|Dir/N" \
+			"New|Dir/New&File.c" &&	# Bash will turn this into
+						# "New\|Dir/New\&File.c "
+	test_completion "git test-path-comp New\\|Dir/New\\&F" \
+			"New|Dir/New&File.c"
+'
+
+test_expect_failure 'complete files - quoted characters on cmdline' '
+	test_when_finished "rm -r \"New(Dir\"" &&
+	mkdir "New(Dir" &&
+	>"New(Dir/New)File.c" &&
+
+	test_completion "git test-path-comp \"New(D" "New(Dir" &&
+	test_completion "git test-path-comp \"New(Dir/New)F" \
+			"New(Dir/New)File.c"
+'
+
+test_expect_failure 'complete files - UTF-8 in ls-files output' '
+	test_when_finished "rm -r árvíztűrő" &&
+	mkdir árvíztűrő &&
+	>"árvíztűrő/Сайн яваарай" &&
+
+	test_completion "git test-path-comp á" "árvíztűrő" &&
+	test_completion "git test-path-comp árvíztűrő/С" \
+			"árvíztűrő/Сайн яваарай"
+'
+
+if test_have_prereq !MINGW &&
+   mkdir 'New\Dir' 2>/dev/null &&
+   touch 'New\Dir/New"File.c' 2>/dev/null
+then
+	test_set_prereq FUNNYNAMES_BS_DQ
+else
+	say "Your filesystem does not allow \\ and \" in filenames."
+	rm -rf 'New\Dir'
+fi
+test_expect_failure FUNNYNAMES_BS_DQ \
+    'complete files - C-style escapes in ls-files output' '
+	test_when_finished "rm -r \"New\\\\Dir\"" &&
+
+	test_completion "git test-path-comp N" "New\\Dir" &&
+	test_completion "git test-path-comp New\\\\D" "New\\Dir" &&
+	test_completion "git test-path-comp New\\\\Dir/N" \
+			"New\\Dir/New\"File.c" &&
+	test_completion "git test-path-comp New\\\\Dir/New\\\"F" \
+			"New\\Dir/New\"File.c"
+'
+
+if test_have_prereq !MINGW &&
+   mkdir $'New\034Special\035Dir' 2>/dev/null &&
+   touch $'New\034Special\035Dir/New\036Special\037File' 2>/dev/null
+then
+	test_set_prereq FUNNYNAMES_SEPARATORS
+else
+	say 'Your filesystem does not allow special separator characters (FS, GS, RS, US) in filenames.'
+	rm -rf $'New\034Special\035Dir'
+fi
+test_expect_failure FUNNYNAMES_SEPARATORS \
+    'complete files - \nnn-escaped control characters in ls-files output' '
+	test_when_finished "rm -r '$'New\034Special\035Dir''" &&
+
+	# Note: these will be literal separator characters on the cmdline.
+	test_completion "git test-path-comp N" "'$'New\034Special\035Dir''" &&
+	test_completion "git test-path-comp '$'New\034S''" \
+			"'$'New\034Special\035Dir''" &&
+	test_completion "git test-path-comp '$'New\034Special\035Dir/''" \
+			"'$'New\034Special\035Dir/New\036Special\037File''" &&
+	test_completion "git test-path-comp '$'New\034Special\035Dir/New\036S''" \
+			"'$'New\034Special\035Dir/New\036Special\037File''"
+'
+
+
 test_expect_success "completion uses <cmd> completion for alias: !sh -c 'git <cmd> ...'" '
 	test_config alias.co "!sh -c '"'"'git checkout ...'"'"'" &&
 	test_completion "git co m" <<-\EOF
-- 
2.17.0.366.gbe216a3084

[PATCH 02/11] completion: move __git_complete_index_file() next to its helpers

[SZEDER Gábor]

It's much easier to read, understand and modify the functions related
to git-aware path completion when they are right next to each other.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 39 +++++++++++++-------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b09c8a2362..36d3c6f928 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -396,6 +396,25 @@ __git_index_files ()
 	done | sort | uniq
 }
 
+# __git_complete_index_file requires 1 argument:
+# 1: the options to pass to ls-file
+#
+# The exception is --committable, which finds the files appropriate commit.
+__git_complete_index_file ()
+{
+	local pfx="" cur_="$cur"
+
+	case "$cur_" in
+	?*/*)
+		pfx="${cur_%/*}"
+		cur_="${cur_##*/}"
+		pfx="${pfx}/"
+		;;
+	esac
+
+	__gitcomp_file "$(__git_index_files "$1" ${pfx:+"$pfx"})" "$pfx" "$cur_"
+}
+
 # Lists branches from the local repository.
 # 1: A prefix to be added to each listed branch (optional).
 # 2: List only branches matching this word (optional; list all branches if
@@ -712,26 +731,6 @@ __git_complete_revlist_file ()
 	esac
 }
 
-
-# __git_complete_index_file requires 1 argument:
-# 1: the options to pass to ls-file
-#
-# The exception is --committable, which finds the files appropriate commit.
-__git_complete_index_file ()
-{
-	local pfx="" cur_="$cur"
-
-	case "$cur_" in
-	?*/*)
-		pfx="${cur_%/*}"
-		cur_="${cur_##*/}"
-		pfx="${pfx}/"
-		;;
-	esac
-
-	__gitcomp_file "$(__git_index_files "$1" ${pfx:+"$pfx"})" "$pfx" "$cur_"
-}
-
 __git_complete_file ()
 {
 	__git_complete_revlist_file
-- 
2.17.0.366.gbe216a3084

[PATCH 03/11] completion: simplify prefix path component handling during path completion

[SZEDER Gábor]

Once upon a time 'git -C "" cmd' errored out with "Cannot change to
'': No such file or directory", therefore the completion script took
extra steps to run 'git -C "." cmd' instead; see fca416a41e
(completion: use "git -C $there" instead of (cd $there && git ...),
2014-10-09).

Those extra steps are not needed since 6a536e2076 (git: treat "git -C
'<path>'" as a no-op when <path> is empty, 2015-03-06), so remove
them.

While at it, also simplify how the trailing '/' is appended to the
variable holding the prefix path components.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 36d3c6f928..72cd3add19 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -385,7 +385,7 @@ __git_ls_files_helper ()
 #    slash.
 __git_index_files ()
 {
-	local root="${2-.}" file
+	local root="$2" file
 
 	__git_ls_files_helper "$root" "$1" |
 	while read -r file; do
@@ -406,13 +406,12 @@ __git_complete_index_file ()
 
 	case "$cur_" in
 	?*/*)
-		pfx="${cur_%/*}"
+		pfx="${cur_%/*}/"
 		cur_="${cur_##*/}"
-		pfx="${pfx}/"
 		;;
 	esac
 
-	__gitcomp_file "$(__git_index_files "$1" ${pfx:+"$pfx"})" "$pfx" "$cur_"
+	__gitcomp_file "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_"
 }
 
 # Lists branches from the local repository.
-- 
2.17.0.366.gbe216a3084

[PATCH 04/11] completion: support completing non-ASCII pathnames

[SZEDER Gábor]

Unless the user has 'core.quotePath=false' somewhere in the
configuration, both 'git ls-files' and 'git diff-index' will by
default quote any pathnames that contain bytes with values higher than
0x80, and escape those bytes as '\nnn' octal values.  This prevents
completing paths when the current path component to be completed
contains any non-ASCII, most notably UTF-8, characters, because none
of the listed quoted paths will match the current word on the command
line.

Set 'core.quotePath=false' for those 'git ls-files' and 'git
diff-index' invocations, so they won't consider bytes higher than 0x80
as "unusual", and won't quote pathnames containing such characters.

Note that pathnames containing backslash, double quote, or control
characters will still be quoted; a later patch in this series will
deal with those.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 6 ++++--
 t/t9902-completion.sh                  | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 72cd3add19..7072555960 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -369,10 +369,12 @@ __gitcomp_file ()
 __git_ls_files_helper ()
 {
 	if [ "$2" == "--committable" ]; then
-		__git -C "$1" diff-index --name-only --relative HEAD
+		__git -C "$1" -c core.quotePath=false diff-index \
+			--name-only --relative HEAD
 	else
 		# NOTE: $2 is not quoted in order to support multiple options
-		__git -C "$1" ls-files --exclude-standard $2
+		__git -C "$1" -c core.quotePath=false ls-files \
+			--exclude-standard $2
 	fi
 }
 
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index ff2e4a8f5f..a4f2c03b93 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1463,7 +1463,7 @@ test_expect_failure 'complete files - quoted characters on cmdline' '
 			"New(Dir/New)File.c"
 '
 
-test_expect_failure 'complete files - UTF-8 in ls-files output' '
+test_expect_success 'complete files - UTF-8 in ls-files output' '
 	test_when_finished "rm -r árvíztűrő" &&
 	mkdir árvíztűrő &&
 	>"árvíztűrő/Сайн яваарай" &&
-- 
2.17.0.366.gbe216a3084

[PATCH 05/11] completion: improve handling quoted paths on the command line

[SZEDER Gábor]

Our git-aware path completion doesn't work when it has to complete a
word already containing quoted and/or backslash-escaped characters on
the command line.  The root cause of the issue is that completion
functions see all words on the command line verbatim, i.e. including
all backslash, single and double quote characters that the shell would
eventually remove when executing the finished command.  These
quoting/escaping characters cause different issues depending on which
path component of the word to be completed contains them:

  - The quoting/escaping is in the prefix path component(s).

    Let's suppose we have a directory called 'New Dir', containing two
    untracked files 'file.c' and 'file.o', and we have a gitignore
    rule ignoring object files.  In this case all of these:

      git add New\ Dir/<TAB>
      git add "New Dir/<TAB>
      git add 'New Dir/<TAB>

    should uniquely complete 'file.c' right away, but Bash offers both
    'file.c' and 'file.o' instead.  The reason for this behavior is
    that our completion script uses the prefix directory name like
    'git -C "New\ Dir/" ls-files ...", i.e. with the backslash inside
    double quotes.  Git then tries to enter a directory called
    'New\ Dir', which (most likely) fails because such a directory
    doesn't exists.  As a result our completion script doesn't list
    any files, leaves the COMPREPLY array empty, which in turn causes
    Bash to fall back to its simple filename completion and lists all
    files in that directory, i.e. both 'file.c' and 'file.o'.

  - The quoting/escaping is in the path component to be completed.

    Let's suppose we have two untracked files 'New File.c' and
    'New File.o', and we have a gitignore rule ignoring object files.
    In this case all of these:

      git add New\ Fi<TAB>
      git add "New Fi<TAB>
      git add 'New Fi<TAB>

    should uniquely complete 'New File.c' right away, but Bash offers
    both 'New File.c' and 'New File.o' instead.  The reason for this
    behavior is that our completion script uses this 'New\ Fi' or
    '"New Fi' etc. word to filter matching paths, and of course none
    of the potential filenames will match because of the included
    backslash or double quote.  The end result is the same as above:
    the completion script doesn't list any files, Bash falls back to
    its filename completion, which then lists the matching object file
    as well.

Add the new helper function __git_dequote() [1], which removes (most
of[2]) the quoting and escaping from the word it gets as argument.  To
minimize the overhead of calling this function, store its result in
the variable $dequoted_word, supposed to be declared local in the
caller; simply printing the result would require a command
substitution imposing the overhead of fork()ing a subshell.  Use this
function in __git_complete_index_file() to dequote the current word,
i.e. the path, to be completed, to avoid the above described
quoting-related issues, thereby fixing two of the failing quoted path
completion tests.

[1] The bash-completion project already has a dequote() function,
    which I hoped I could borrow to deal with this, but unfortunately
    it doesn't work quite well for this purpose (perhaps that's why
    even the bash-completion project only rarely uses it).  The main
    issue is that their dequote() is implemented as:

      eval printf %s "$1" 2> /dev/null

    where $1 would contain the word to be completed.  While it's a
    short and sweet one-liner, the use of 'eval' requires that $1 is a
    syntactically valid string, which is not the case when quoting the
    path like 'git add "New Dir/<TAB>'.  This causes 'eval' to fail,
    because it can't find the matching closing double quote, and the
    function returns nothing.  The result is totally broken behavior,
    as if the current word were empty, and the completion script would
    then list all files from the current directory.  This is why one
    of the quoted path completion tests specifically checks the
    completion of a path with an opening but without a corresponding
    closing double quote character.  Furthermore, the 'eval' performs
    all kinds of expansions, which may or may not be desired; I think
    it's the latter.  Finally, using this function would require a
    command substitution.

[2] Bash understands the $'string' quoting as well, which "expands to
    'string', with backslash-escaped characters replaced as specified
    by the ANSI C standard" (quoted from Bash manpage).  Since shell
    metacharacters, field separators, globbing, etc. can all be easily
    entered using standard shell escaping or quoting, this type of
    quoting comes in handly when dealing with control characters that
    are otherwise difficult both to "type" and to see on the command
    line.  Because of this difficulty I would assume that people do
    avoid pathnames with such control characters anyway, so I didn't
    bother implementing it.  This function is already way too long as
    it is.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

Notes:
    What if someone on Windows were to use backslash as directory
    separator during path completion?  E.g. 'git add new-dir\subdir\<TAB>'
    Did that happen to work in the past?  Does this patch break it?

 contrib/completion/git-completion.bash | 76 ++++++++++++++++++++++++--
 t/t9902-completion.sh                  | 46 +++++++++++++++-
 2 files changed, 116 insertions(+), 6 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7072555960..ae6127155e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -92,6 +92,70 @@ __git ()
 		${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null
 }
 
+# Removes backslash escaping, single quotes and double quotes from a word,
+# stores the result in the variable $dequoted_word.
+# 1: The word to dequote.
+__git_dequote ()
+{
+	local rest="$1" len ch
+
+	dequoted_word=""
+
+	while test -n "$rest"; do
+		len=${#dequoted_word}
+		dequoted_word="$dequoted_word${rest%%[\\\'\"]*}"
+		rest="${rest:$((${#dequoted_word}-$len))}"
+
+		case "${rest:0:1}" in
+		\\)
+			ch="${rest:1:1}"
+			case "$ch" in
+			$'\n')
+				;;
+			*)
+				dequoted_word="$dequoted_word$ch"
+				;;
+			esac
+			rest="${rest:2}"
+			;;
+		\')
+			rest="${rest:1}"
+			len=${#dequoted_word}
+			dequoted_word="$dequoted_word${rest%%\'*}"
+			rest="${rest:$((${#dequoted_word}-$len+1))}"
+			;;
+		\")
+			rest="${rest:1}"
+			while test -n "$rest" ; do
+				len=${#dequoted_word}
+				dequoted_word="$dequoted_word${rest%%[\\\"]*}"
+				rest="${rest:$((${#dequoted_word}-$len))}"
+				case "${rest:0:1}" in
+				\\)
+					ch="${rest:1:1}"
+					case "$ch" in
+					\"|\\|\$|\`)
+						dequoted_word="$dequoted_word$ch"
+						;;
+					$'\n')
+						;;
+					*)
+						dequoted_word="$dequoted_word\\$ch"
+						;;
+					esac
+					rest="${rest:2}"
+					;;
+				\")
+					rest="${rest:1}"
+					break
+					;;
+				esac
+			done
+			;;
+		esac
+	done
+}
+
 # The following function is based on code from:
 #
 #   bash_completion - programmable completion functions for bash 3.2+
@@ -404,13 +468,17 @@ __git_index_files ()
 # The exception is --committable, which finds the files appropriate commit.
 __git_complete_index_file ()
 {
-	local pfx="" cur_="$cur"
+	local dequoted_word pfx="" cur_
 
-	case "$cur_" in
+	__git_dequote "$cur"
+
+	case "$dequoted_word" in
 	?*/*)
-		pfx="${cur_%/*}/"
-		cur_="${cur_##*/}"
+		pfx="${dequoted_word%/*}/"
+		cur_="${dequoted_word##*/}"
 		;;
+	*)
+		cur_="$dequoted_word"
 	esac
 
 	__gitcomp_file "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_"
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a4f2c03b93..6856b263f8 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -400,6 +400,46 @@ test_expect_success '__gitdir - remote as argument' '
 	test_cmp expected "$actual"
 '
 
+
+test_expect_success '__git_dequote - plain unquoted word' '
+	__git_dequote unquoted-word &&
+	verbose test unquoted-word = "$dequoted_word"
+'
+
+# input:    b\a\c\k\'\\\"s\l\a\s\h\es
+# expected: back'\"slashes
+test_expect_success '__git_dequote - backslash escaped' '
+	__git_dequote "b\a\c\k\\'\''\\\\\\\"s\l\a\s\h\es" &&
+	verbose test "back'\''\\\"slashes" = "$dequoted_word"
+'
+
+# input:    sin'gle\' '"quo'ted
+# expected: single\ "quoted
+test_expect_success '__git_dequote - single quoted' '
+	__git_dequote "'"sin'gle\\\\' '\\\"quo'ted"'" &&
+	verbose test '\''single\ "quoted'\'' = "$dequoted_word"
+'
+
+# input:    dou"ble\\" "\"\quot"ed
+# expected: double\ "\quoted
+test_expect_success '__git_dequote - double quoted' '
+	__git_dequote '\''dou"ble\\" "\"\quot"ed'\'' &&
+	verbose test '\''double\ "\quoted'\'' = "$dequoted_word"
+'
+
+# input: 'open single quote
+test_expect_success '__git_dequote - open single quote' '
+	__git_dequote "'\''open single quote" &&
+	verbose test "open single quote" = "$dequoted_word"
+'
+
+# input: "open double quote
+test_expect_success '__git_dequote - open double quote' '
+	__git_dequote "\"open double quote" &&
+	verbose test "open double quote" = "$dequoted_word"
+'
+
+
 test_expect_success '__gitcomp_direct - puts everything into COMPREPLY as-is' '
 	sed -e "s/Z$//g" >expected <<-EOF &&
 	with-trailing-space Z
@@ -1437,7 +1477,7 @@ _git_test_path_comp ()
 	__git_complete_index_file --others
 }
 
-test_expect_failure 'complete files - escaped characters on cmdline' '
+test_expect_success 'complete files - escaped characters on cmdline' '
 	test_when_finished "rm -rf \"New|Dir\"" &&
 	mkdir "New|Dir" &&
 	>"New|Dir/New&File.c" &&
@@ -1453,11 +1493,13 @@ test_expect_failure 'complete files - escaped characters on cmdline' '
 			"New|Dir/New&File.c"
 '
 
-test_expect_failure 'complete files - quoted characters on cmdline' '
+test_expect_success 'complete files - quoted characters on cmdline' '
 	test_when_finished "rm -r \"New(Dir\"" &&
 	mkdir "New(Dir" &&
 	>"New(Dir/New)File.c" &&
 
+	# Testing with an opening but without a corresponding closing
+	# double quote is important.
 	test_completion "git test-path-comp \"New(D" "New(Dir" &&
 	test_completion "git test-path-comp \"New(Dir/New)F" \
 			"New(Dir/New)File.c"
-- 
2.17.0.366.gbe216a3084

[PATCH 06/11] completion: let 'ls-files' and 'diff-index' filter matching paths

[SZEDER Gábor]

During git-aware path completion, e.g. 'git rm dir/fil<TAB>', both
'git ls-files' and 'git diff-index' list all paths in the given 'dir/'
matching certain criteria (cached, modified, untracked, etc.)
appropriate for the given git command, even paths whose names don't
begin with 'fil'.  This comes with a considerable performance
penalty when the directory in question contains a lot of paths, but
the current word can be uniquely completed or when only a handful of
those paths match the current word.

Reduce the number of iterations in this codepath from the number of
paths to the number of matching paths by specifying an appropriate
globbing pattern to 'git ls-files' and 'git diff-index' to list only
paths that match the current word to be completed.

Note that both commands treat backslashes as escape characters in
their file arguments, e.g. to preserve the literal meaning of globbing
characters, so we have to double every backslash in the globbing
pattern.  This is why one of the path completion tests specifically
checks the completion of a path containing a literal backslash
character (that test still fails, though, because both commands output
such paths enclosed in double quotes and the special characters
escaped; a later patch in this series will deal with those).

This speeds up path completion considerably when there are a lot of
non-matching paths to be filtered out.  Uniquely completing a tracked
filename at the top of the worktree in linux.git (over 62k files),
i.e. what's doing all the hard work behind 'git rm Mak<TAB>' to
complete 'Makefile':

  Before this patch, best of five, on Linux:

    $ time cur=Mak __git_complete_index_file

    real    0m2.159s
    user    0m1.299s
    sys     0m1.089s

  After:

    real    0m0.033s
    user    0m0.023s
    sys     0m0.015s

  Difference: -98.5%
  Speedup:     65.4x

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 11 ++++++-----
 t/t9902-completion.sh                  |  1 +
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ae6127155e..3948265d32 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -434,11 +434,11 @@ __git_ls_files_helper ()
 {
 	if [ "$2" == "--committable" ]; then
 		__git -C "$1" -c core.quotePath=false diff-index \
-			--name-only --relative HEAD
+			--name-only --relative HEAD -- "${3//\\/\\\\}*"
 	else
 		# NOTE: $2 is not quoted in order to support multiple options
 		__git -C "$1" -c core.quotePath=false ls-files \
-			--exclude-standard $2
+			--exclude-standard $2 -- "${3//\\/\\\\}*"
 	fi
 }
 
@@ -449,11 +449,12 @@ __git_ls_files_helper ()
 #    If provided, only files within the specified directory are listed.
 #    Sub directories are never recursed.  Path must have a trailing
 #    slash.
+# 3: List only paths matching this path component (optional).
 __git_index_files ()
 {
-	local root="$2" file
+	local root="$2" match="$3" file
 
-	__git_ls_files_helper "$root" "$1" |
+	__git_ls_files_helper "$root" "$1" "$match" |
 	while read -r file; do
 		case "$file" in
 		?*/*) echo "${file%%/*}" ;;
@@ -481,7 +482,7 @@ __git_complete_index_file ()
 		cur_="$dequoted_word"
 	esac
 
-	__gitcomp_file "$(__git_index_files "$1" "$pfx")" "$pfx" "$cur_"
+	__gitcomp_file "$(__git_index_files "$1" "$pfx" "$cur_")" "$pfx" "$cur_"
 }
 
 # Lists branches from the local repository.
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 6856b263f8..f7a9dd446d 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1515,6 +1515,7 @@ test_expect_success 'complete files - UTF-8 in ls-files output' '
 			"árvíztűrő/Сайн яваарай"
 '
 
+# Testing with a path containing a backslash is important.
 if test_have_prereq !MINGW &&
    mkdir 'New\Dir' 2>/dev/null &&
    touch 'New\Dir/New"File.c' 2>/dev/null
-- 
2.17.0.366.gbe216a3084

[PATCH 07/11] completion: use 'awk' to strip trailing path components

[SZEDER Gábor]

During git-aware path completion we complete one path component at a
time, i.e. 'git add <TAB>' offers only 'dir/' at first, not
'dir/subdir/file' right away, just like Bash's own filename
completion.  However, since both 'git ls-files' and 'git diff-index'
dive deep into subdirectories, we have to strip all trailing path
components from the listed paths, keeping only the leading path
component.  This stripping is currently done in a shell loop in
__git_index_files(), which can take a significant amount of time when
it has to iterate through a large number of paths.

Replace this shell loop with a little 'awk' script using '/' as input
field separator and printing the first field, which produces the same
output much faster.

Listing all tracked files (12) and directories (23) at the top of the
worktree in linux.git (over 62k files), i.e. what's doing all the hard
work behind 'git rm <TAB>':

  Before this patch, best of five, using GNU awk on Linux:

    $ time cur= __git_complete_index_file

    real    0m2.149s
    user    0m1.307s
    sys     0m1.086s

  After:

    real    0m0.067s
    user    0m0.089s
    sys     0m0.023s

  Difference: -96.9%
  Speedup:     32.1x

Note that this could be done with 'sed', or even with 'cut', just as
well, but the upcoming patches require 'awk's scriptability.

Note also that this change means one more fork()+exec()ed process
during path completion, adding more overhead especially on Windows,
but a later patch will more than make up for it by eliminating two
other processes in the same function.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3948265d32..0abba88462 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -452,15 +452,12 @@ __git_ls_files_helper ()
 # 3: List only paths matching this path component (optional).
 __git_index_files ()
 {
-	local root="$2" match="$3" file
+	local root="$2" match="$3"
 
 	__git_ls_files_helper "$root" "$1" "$match" |
-	while read -r file; do
-		case "$file" in
-		?*/*) echo "${file%%/*}" ;;
-		*) echo "$file" ;;
-		esac
-	done | sort | uniq
+	awk -F / '{
+		print $1
+	}' | sort | uniq
 }
 
 # __git_complete_index_file requires 1 argument:
-- 
2.17.0.366.gbe216a3084

[PATCH 08/11] t9902-completion: ignore COMPREPLY element order in some tests

[SZEDER Gábor]

The order or possible completion words in the COMPREPLY array doesn't
actually matter, as long as all the right words are in there, because
Bash will sort them anyway.  Yet, our tests looking at the elements of
COMPREPLY always expect them to be in a specific order.

Now, this hasn't been an issue before, but the next patch is about to
optimize a bit more our git-aware path completion, and as a harmless
side effect the order of elements in COMPREPLY will change.  Worse,
the order will be downright undefined, because after the next patch
path components will come directly from iterating through an
associative array in 'awk', and the order of iteration over the
elements in those arrays is undefined, and indeed different 'awk'
implementations produce different order.  Consequently, we can't get
away with simply adjusting the expected results in the affected tests.

Modify the 'test_completion' helper function to sort both the expected
and the actual results, i.e. the elements in COMPREPLY, before
comparing them, so the tests using this helper function will work
regardless of the order of elements.

Note that this change still leaves a bunch of tests depending on the
order of elements in COMPREPLY, tests that focus on a specific helper
function and therefore don't use the 'test_completion' helper.  I
would rather deal with those later, when (if ever) the need actually
arises, than create unnecessary code churn now.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t9902-completion.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index f7a9dd446d..a747998d6c 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -84,10 +84,11 @@ test_completion ()
 	then
 		printf '%s\n' "$2" >expected
 	else
-		sed -e 's/Z$//' >expected
+		sed -e 's/Z$//' |sort >expected
 	fi &&
 	run_completion "$1" &&
-	test_cmp expected out
+	sort out >out_sorted &&
+	test_cmp expected out_sorted
 }
 
 # Test __gitcomp.
@@ -1405,6 +1406,7 @@ test_expect_success 'complete files' '
 
 	echo "expected" > .gitignore &&
 	echo "out" >> .gitignore &&
+	echo "out_sorted" >> .gitignore &&
 
 	git add .gitignore &&
 	test_completion "git commit " ".gitignore" &&
-- 
2.17.0.366.gbe216a3084

[PATCH 09/11] completion: remove repeated dirnames with 'awk' during path completion

[SZEDER Gábor]

During git-aware path completion, after all the trailing path
components have been removed from the output of 'git ls-files' and
'git diff-index' (see previous patch), each directory name is repeated
as many times as the number of listed paths it contains.  This can be
a lot of repetitions, especially when invoking path completion close
to the root of a big worktree, which would cause a considerable
overhead downstream of __git_index_files(), in particular in the shell
loop that fills the COMPREPLY array.  To reduce this overhead,
__git_index_files() runs the classic '... |sort |uniq' pattern to
remove those repetitions from the function's output.

While removing repeated directory names is effective in reducing the
number of iterations in that shell loop, it still imposes the overhead
of fork()+exec()ing two external processes, and two additional stages
in the pipeline, where potentially relatively large amount of data can
be passed between two subsequent pipeline stages.

Extend __git_index_files()'s 'awk' script to remove repeated path
components by first creating and filling an associative array indexed
by all encountered path components (after the trailing path components
have been removed), and then iterating over this array and printing
the indices, i.e. unique path components.  This way we can remove the
'|sort |uniq' pipeline stages, and their eliminated overhead results
in faster path completion.

Listing all tracked files (12) and directories (23) at the top of the
worktree in linux.git (over 62k files), i.e. what's doing all the hard
work behind 'git rm <TAB>':

  Before this patch, best of five, using GNU awk on Linux:

    real    0m0.069s
    user    0m0.089s
    sys     0m0.026s

  After:

    real    0m0.052s
    user    0m0.072s
    sys     0m0.014s

  Difference: -24.6%

Note that this changes order of elements in __git_index_files()'s
output.  This is not an issue, because this function was only ever
intended to feed paths into the COMPREPLY array, and Bash will sort
its elements (according to the users locale) anyway.

Note also that using 'awk' to remove repeated path components is also
beneficial for the performance of the next two patches:

  - The first will extend this 'awk' script to dequote quoted paths in
    the output of 'git ls-files' and 'git diff-index'.  With this
    patch it will only have to dequote unique path components, not
    all.

  - The second will, among other things, extend this 'awk' script to
    prepend prefix path components from the command line to the
    currently completed path component.  Consequently, each line in
    'awk's output will grow longer.  Without this patch that '|sort
    |uniq' would have to exchange and process that much more data.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0abba88462..70bc75dfc7 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -456,8 +456,12 @@ __git_index_files ()
 
 	__git_ls_files_helper "$root" "$1" "$match" |
 	awk -F / '{
-		print $1
-	}' | sort | uniq
+		paths[$1] = 1
+	}
+	END {
+		for (p in paths)
+			print p
+	}'
 }
 
 # __git_complete_index_file requires 1 argument:
-- 
2.17.0.366.gbe216a3084

[PATCH 10/11] completion: improve handling quoted paths in 'git ls-files's output

[SZEDER Gábor]

If any pathname contains backslash, double quote, tab, newline, or any
control characters, 'git ls-files' and 'git diff-index' will enclose
that pathname in double quotes and escape those special characters
using C-style one-character escape sequences or \nnn octal values.
This prevents those files from being listed during git-aware path
completion, because due to the quoting they will never match the
current word to be completed.

Extend __git_index_files()'s 'awk' script to remove all that quoting
and escaping from unique path components, so even paths containing
(almost all) such special characters can be completed.

Paths containing newline characters are still an issue, though.  We
use newlines as separator character when filling the COMPREPLY array,
so a path with one or more newline will end up split to two or more
elements in COMPREPLY, basically breaking completion.  There is
nothing we can do about it without a significant performance hit, so
let's just ignore such paths for now.  As far as paths with newlines
are concerned, this isn't any different from the previous behavior,
because those paths were always omitted, though in the past they were
omitted because due to the quoting they didn't match the current word
to be completed.  Anyway, Bash's own filename completion (Meta-/) can
complete even those paths, if need be.

Note:

  - We don't dequote path components right away as they are coming in,
    because then we would have to dequote each directory name
    repeatedly, as many times as it appears in the input, i.e. as many
    times as the number of listed paths it contains.  Instead, we
    dequote them at the end, as we print unique path components.

  - Even when a directory name itself does not contain any special
    characters, it will still be quoted if any of its trailing path
    components do.  If a directory contains paths both with and
    without special characters, then the name of that directory will
    appear both quoted and unquoted in the output of 'git ls-files'
    and 'git diff-index'.  Consequently, we will add such a directory
    name to the deduplicating associative array twice: once quoted and
    once unquoted.

    This means that we have to be careful after dequoting a directory
    name, and only print it if we haven't seen the same directory name
    unquoted.

  - It would be wonderful if we could just pass '-z' to those git
    commands to output \0-separated unquoted paths, and use \0 as
    record separator in the 'awk' script processing their output...
    this patch would be so much simpler, almost trivial even.
    Unfortunately, however, POSIX and most 'awk' implementations don't
    support \0 as record separator (GNU awk does support it).

  - This patch makes the earlier change to list paths with
    'core.quotePath=false' basically redundant, because this could
    decode any \nnn-escaped non-ASCII character just fine, as well.
    However, I suspect that 'git ls-files' can deal with those
    non-ASCII characters faster than this updated 'awk' script; just
    in case someone is burdened with tons of pathnames containing
    non-ASCII characters.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 66 +++++++++++++++++++++++++-
 t/t9902-completion.sh                  | 17 ++++++-
 2 files changed, 79 insertions(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 70bc75dfc7..e97d57024b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -459,8 +459,70 @@ __git_index_files ()
 		paths[$1] = 1
 	}
 	END {
-		for (p in paths)
-			print p
+		for (p in paths) {
+			if (substr(p, 1, 1) != "\"") {
+				# No special characters, easy!
+				print p
+				continue
+			}
+
+			# The path is quoted.
+			p = dequote(p)
+			if (p == "")
+				continue
+
+			# Even when a directory name itself does not contain
+			# any special characters, it will still be quoted if
+			# any of its (stripped) trailing path components do.
+			# Because of this we may have seen the same direcory
+			# both quoted and unquoted.
+			if (p in paths)
+				# We have seen the same directory unquoted,
+				# skip it.
+				continue
+			else
+				print p
+		}
+	}
+	function dequote(p,    bs_idx, out, esc, esc_idx, dec) {
+		# Skip opening double quote.
+		p = substr(p, 2)
+
+		# Interpret backslash escape sequences.
+		while ((bs_idx = index(p, "\\")) != 0) {
+			out = out substr(p, 1, bs_idx - 1)
+			esc = substr(p, bs_idx + 1, 1)
+			p = substr(p, bs_idx + 2)
+
+			if ((esc_idx = index("abtvfr\"\\", esc)) != 0) {
+				# C-style one-character escape sequence.
+				out = out substr("\a\b\t\v\f\r\"\\",
+						 esc_idx, 1)
+			} else if (esc == "n") {
+				# Uh-oh, a newline character.
+				# We cant reliably put a pathname
+				# containing a newline into COMPREPLY,
+				# and the newline would create a mess.
+				# Skip this path.
+				return ""
+			} else {
+				# Must be a \nnn octal value, then.
+				dec = esc             * 64 + \
+				      substr(p, 1, 1) * 8  + \
+				      substr(p, 2, 1)
+				out = out sprintf("%c", dec)
+				p = substr(p, 3)
+			}
+		}
+		# Drop closing double quote, if there is one.
+		# (There isnt any if this is a directory, as it was
+		# already stripped with the trailing path components.)
+		if (substr(p, length(p), 1) == "\"")
+			out = out substr(p, 1, length(p) - 1)
+		else
+			out = out p
+
+		return out
 	}'
 }
 
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a747998d6c..b9879576ab 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1527,7 +1527,7 @@ else
 	say "Your filesystem does not allow \\ and \" in filenames."
 	rm -rf 'New\Dir'
 fi
-test_expect_failure FUNNYNAMES_BS_DQ \
+test_expect_success FUNNYNAMES_BS_DQ \
     'complete files - C-style escapes in ls-files output' '
 	test_when_finished "rm -r \"New\\\\Dir\"" &&
 
@@ -1548,7 +1548,7 @@ else
 	say 'Your filesystem does not allow special separator characters (FS, GS, RS, US) in filenames.'
 	rm -rf $'New\034Special\035Dir'
 fi
-test_expect_failure FUNNYNAMES_SEPARATORS \
+test_expect_success FUNNYNAMES_SEPARATORS \
     'complete files - \nnn-escaped control characters in ls-files output' '
 	test_when_finished "rm -r '$'New\034Special\035Dir''" &&
 
@@ -1562,6 +1562,19 @@ test_expect_failure FUNNYNAMES_SEPARATORS \
 			"'$'New\034Special\035Dir/New\036Special\037File''"
 '
 
+test_expect_success FUNNYNAMES_BS_DQ \
+    'complete files - removing repeated quoted path components' '
+	test_when_finished rm -rf NewDir &&
+	mkdir NewDir &&    # A dirname which in itself would not be quoted ...
+	>NewDir/0-file &&
+	>NewDir/1\"file && # ... but here the file makes the dirname quoted ...
+	>NewDir/2-file &&
+	>NewDir/3\"file && # ... and here, too.
+
+	# Still, we should only list it once.
+	test_completion "git test-path-comp New" "NewDir"
+'
+
 
 test_expect_success "completion uses <cmd> completion for alias: !sh -c 'git <cmd> ...'" '
 	test_config alias.co "!sh -c '"'"'git checkout ...'"'"'" &&
-- 
2.17.0.366.gbe216a3084

[PATCH 11/11] completion: fill COMPREPLY directly when completing paths

[SZEDER Gábor]

During git-aware path completion, when a lot of path components have
to be listed, a significant amount of time is spent in
__gitcomp_file(), or more accurately in the shell loop of
__gitcompappend(), iterating over all the path components filtering
path components matching the current word to be completed, adding
prefix path components, and placing the resulting matching paths into
the COMPREPLY array.

Now, a previous patch in this series made 'git ls-files' and 'git
diff-index' list only paths matching the current word to be completed,
so an additional filtering in __gitcomp_file() is not necessary
anymore.  Adding the prefix path components could be done much more
efficiently in __git_index_files()'s 'awk' script while stripping
trailing path components and removing duplicates and quoting.  And
then the resulting paths won't require any more filtering or
processing before being handed over to Bash, so we could fill the
COMPREPLY array directly.

Unfortunately, we can't simply use the __gitcomp_direct() helper
function to do that, because __gitcomp_file() does one additional
thing: it tells Bash that we are doing filename completion, so the
shell will kindly do four important things for us:

  1. Append a trailing space to all filenames.
  2. Append a trailing '/' to all directory names.
  3. Escape any meta, globbing, separator, etc. characters.
  4. List only the current path component when listing possible
     completions (i.e. 'dir/subdir/f<TAB>' will list 'file1', 'file2',
     etc. instead of the whole 'dir/subdir/file1',
     'dir/subdir/file2').

While we could let __git_index_files()'s 'awk' script take care of the
first two points, the third one gets tricky, and we absolutely need
the shell's support for the fourth.

Add the helper function __gitcomp_file_direct(), which, just like
__gitcomp_direct(), fills the COMPREPLY array with prefiltered and
preprocessed paths without any additional processing, without a shell
loop, with just one single compound assignment, and, similar to
__gitcomp_file(), tells Bash and ZSH that we are doing filename
completion.  Extend __git_index_files()'s 'awk' script a bit to
prepend any prefix path components to all listed paths.  Finally,
modify __git_complete_index_file() to feed __git_index_files()'s
output to ___gitcomp_file_direct() instead of __gitcomp_file().

After this patch there is no shell loop left in the path completion
code path.

This speeds up path completion when there are a lot of paths matching
the current word to be completed.  In a pathological repository with
100k files in a single directory, listing all those files:

  Before this patch, best of five, using GNU awk on Linux:

    $ time cur=dir/ __git_complete_index_file

    real    0m0.983s
    user    0m1.004s
    sys     0m0.033s

  After:

    real    0m0.313s
    user    0m0.341s
    sys     0m0.029s

  Difference: -68.2%
  Speedup:      3.1x

  To see the benefits of the whole patch series, the same command with
  v2.17.0:

    real    0m2.736s
    user    0m2.472s
    sys     0m0.610s

  Difference: -88.6%
  Speedup:      8.7x

Note that this patch changes the output of the __git_index_files()
helper function by unconditionally prepending the prefix path
components to every listed path.  This would break users' completion
scriptlets that directly run:

  __gitcomp_file "$(__git_index_files ...)" "$pfx" "$cur_"

because that would add the prefix path components once more.
However, __git_index_files() is kind of a "helper function of a helper
function", and users' completion scriptlets should have been using
__git_complete_index_file() for git-aware path completion in the first
place, so this is likely doesn't worth worrying about.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 34 +++++++++++++++++++++++---
 contrib/completion/git-completion.zsh  |  9 +++++++
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e97d57024b..360ee9ca51 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -404,6 +404,23 @@ __gitcomp_nl ()
 	__gitcomp_nl_append "$@"
 }
 
+# Fills the COMPREPLY array with prefiltered paths without any additional
+# processing.
+# Callers must take care of providing only paths that match the current path
+# to be completed and adding any prefix path components, if necessary.
+# 1: List of newline-separated matching paths, complete with all prefix
+#    path componens.
+__gitcomp_file_direct ()
+{
+	local IFS=$'\n'
+
+	COMPREPLY=($1)
+
+	# use a hack to enable file mode in bash < 4
+	compopt -o filenames +o nospace 2>/dev/null ||
+	compgen -f /non-existing-dir/ > /dev/null
+}
+
 # Generates completion reply with compgen from newline-separated possible
 # completion filenames.
 # It accepts 1 to 3 arguments:
@@ -455,14 +472,14 @@ __git_index_files ()
 	local root="$2" match="$3"
 
 	__git_ls_files_helper "$root" "$1" "$match" |
-	awk -F / '{
+	awk -F / -v pfx="${2//\\/\\\\}" '{
 		paths[$1] = 1
 	}
 	END {
 		for (p in paths) {
 			if (substr(p, 1, 1) != "\"") {
 				# No special characters, easy!
-				print p
+				print pfx p
 				continue
 			}
 
@@ -481,7 +498,7 @@ __git_index_files ()
 				# skip it.
 				continue
 			else
-				print p
+				print pfx p
 		}
 	}
 	function dequote(p,    bs_idx, out, esc, esc_idx, dec) {
@@ -545,7 +562,7 @@ __git_complete_index_file ()
 		cur_="$dequoted_word"
 	esac
 
-	__gitcomp_file "$(__git_index_files "$1" "$pfx" "$cur_")" "$pfx" "$cur_"
+	__gitcomp_file_direct "$(__git_index_files "$1" "$pfx" "$cur_")"
 }
 
 # Lists branches from the local repository.
@@ -3311,6 +3328,15 @@ if [[ -n ${ZSH_VERSION-} ]]; then
 		compadd -Q -S "${4- }" -p "${2-}" -- ${=1} && _ret=0
 	}
 
+	__gitcomp_file_direct ()
+	{
+		emulate -L zsh
+
+		local IFS=$'\n'
+		compset -P '*[=:]'
+		compadd -Q -f -- ${=1} && _ret=0
+	}
+
 	__gitcomp_file ()
 	{
 		emulate -L zsh
diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh
index c3521fbfc4..53cb0f934f 100644
--- a/contrib/completion/git-completion.zsh
+++ b/contrib/completion/git-completion.zsh
@@ -93,6 +93,15 @@ __gitcomp_nl_append ()
 	compadd -Q -S "${4- }" -p "${2-}" -- ${=1} && _ret=0
 }
 
+__gitcomp_file_direct ()
+{
+	emulate -L zsh
+
+	local IFS=$'\n'
+	compset -P '*[=:]'
+	compadd -Q -f -- ${=1} && _ret=0
+}
+
 __gitcomp_file ()
 {
 	emulate -L zsh
-- 
2.17.0.366.gbe216a3084

Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

>     Do any more new tests need FUNNYNAMES* prereq?

Hmph, all of these look like they involve some funnynames ;-)

> +test_expect_failure 'complete files - escaped characters on cmdline' '
> +	test_when_finished "rm -rf \"New|Dir\"" &&
> +	mkdir "New|Dir" &&
> +	>"New|Dir/New&File.c" &&
> +
> +	test_completion "git test-path-comp N" \
> +			"New|Dir" &&	# Bash will turn this into "New\|Dir/"
> +	test_completion "git test-path-comp New\\|D" \
> +			"New|Dir" &&
> +	test_completion "git test-path-comp New\\|Dir/N" \
> +			"New|Dir/New&File.c" &&	# Bash will turn this into
> +						# "New\|Dir/New\&File.c "
> +	test_completion "git test-path-comp New\\|Dir/New\\&F" \
> +			"New|Dir/New&File.c"
> +'
> +
> +test_expect_failure 'complete files - quoted characters on cmdline' '
> +	test_when_finished "rm -r \"New(Dir\"" &&

This does not use -rf unlike the previous one?

> +	mkdir "New(Dir" &&
> +	>"New(Dir/New)File.c" &&
> +
> +	test_completion "git test-path-comp \"New(D" "New(Dir" &&
> +	test_completion "git test-path-comp \"New(Dir/New)F" \
> +			"New(Dir/New)File.c"
> +'

OK.

> +test_expect_failure 'complete files - UTF-8 in ls-files output' '
> +	test_when_finished "rm -r árvíztűrő" &&
> +	mkdir árvíztűrő &&
> +	>"árvíztűrő/Сайн яваарай" &&
> +
> +	test_completion "git test-path-comp á" "árvíztűrő" &&
> +	test_completion "git test-path-comp árvíztűrő/С" \
> +			"árvíztűrő/Сайн яваарай"
> +'
> +
> +if test_have_prereq !MINGW &&
> +   mkdir 'New\Dir' 2>/dev/null &&
> +   touch 'New\Dir/New"File.c' 2>/dev/null
> +then
> +	test_set_prereq FUNNYNAMES_BS_DQ
> +else
> +	say "Your filesystem does not allow \\ and \" in filenames."
> +	rm -rf 'New\Dir'
> +fi
> +test_expect_failure FUNNYNAMES_BS_DQ \
> +    'complete files - C-style escapes in ls-files output' '
> +	test_when_finished "rm -r \"New\\\\Dir\"" &&
> +
> +	test_completion "git test-path-comp N" "New\\Dir" &&
> +	test_completion "git test-path-comp New\\\\D" "New\\Dir" &&
> +	test_completion "git test-path-comp New\\\\Dir/N" \
> +			"New\\Dir/New\"File.c" &&
> +	test_completion "git test-path-comp New\\\\Dir/New\\\"F" \
> +			"New\\Dir/New\"File.c"
> +'
> +
> +if test_have_prereq !MINGW &&
> +   mkdir $'New\034Special\035Dir' 2>/dev/null &&
> +   touch $'New\034Special\035Dir/New\036Special\037File' 2>/dev/null

The $'...' quote is bash-ism, but this is about testing bash
completion, so as long as we make sure non-bash shells won't touch
this part of the test, it is OK.

Do we want to test a more common case of a filename that is two
words with SP in between, i.e. 

	$ >'hello world' && git add hel<TAB>

or is it known to work just fine without quoting/escaping (because
the funny we care about is output from ls-files and SP is not special
in its one-item-at-a-time-on-a-line output) and not worth checking?

Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames

[SZEDER Gábor]

On Tue, Apr 17, 2018 at 5:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>>     Do any more new tests need FUNNYNAMES* prereq?
>
> Hmph, all of these look like they involve some funnynames ;-)

Well, I can' create a directory with a '|' in its name on FAT32 (on
Linux), so this needs FUNNYNAMES prereq, too.

>> +test_expect_failure 'complete files - escaped characters on cmdline' '
>> +     test_when_finished "rm -rf \"New|Dir\"" &&
>> +     mkdir "New|Dir" &&
>> +     >"New|Dir/New&File.c" &&
>> +
>> +     test_completion "git test-path-comp N" \
>> +                     "New|Dir" &&    # Bash will turn this into "New\|Dir/"
>> +     test_completion "git test-path-comp New\\|D" \
>> +                     "New|Dir" &&
>> +     test_completion "git test-path-comp New\\|Dir/N" \
>> +                     "New|Dir/New&File.c" && # Bash will turn this into
>> +                                             # "New\|Dir/New\&File.c "
>> +     test_completion "git test-path-comp New\\|Dir/New\\&F" \
>> +                     "New|Dir/New&File.c"
>> +'
>> +
>> +test_expect_failure 'complete files - quoted characters on cmdline' '
>> +     test_when_finished "rm -r \"New(Dir\"" &&
>
> This does not use -rf unlike the previous one?

Noted.

The lack of '-f' is leftover from early versions of these tests, when I
had a hard time getting the quoting-escaping right.  Without the '-f'
'rm' errored out when I messed up, and the error message helpfully
contained the path it wasn't able to delete.

>> +     mkdir "New(Dir" &&
>> +     >"New(Dir/New)File.c" &&
>> +
>> +     test_completion "git test-path-comp \"New(D" "New(Dir" &&
>> +     test_completion "git test-path-comp \"New(Dir/New)F" \
>> +                     "New(Dir/New)File.c"
>> +'
>
> OK.
>
>> +test_expect_failure 'complete files - UTF-8 in ls-files output' '
>> +     test_when_finished "rm -r árvíztűrő" &&
>> +     mkdir árvíztűrő &&
>> +     >"árvíztűrő/Сайн яваарай" &&
>> +
>> +     test_completion "git test-path-comp á" "árvíztűrő" &&
>> +     test_completion "git test-path-comp árvíztűrő/С" \
>> +                     "árvíztűrő/Сайн яваарай"
>> +'
>> +
>> +if test_have_prereq !MINGW &&
>> +   mkdir 'New\Dir' 2>/dev/null &&
>> +   touch 'New\Dir/New"File.c' 2>/dev/null
>> +then
>> +     test_set_prereq FUNNYNAMES_BS_DQ
>> +else
>> +     say "Your filesystem does not allow \\ and \" in filenames."
>> +     rm -rf 'New\Dir'
>> +fi
>> +test_expect_failure FUNNYNAMES_BS_DQ \
>> +    'complete files - C-style escapes in ls-files output' '
>> +     test_when_finished "rm -r \"New\\\\Dir\"" &&
>> +
>> +     test_completion "git test-path-comp N" "New\\Dir" &&
>> +     test_completion "git test-path-comp New\\\\D" "New\\Dir" &&
>> +     test_completion "git test-path-comp New\\\\Dir/N" \
>> +                     "New\\Dir/New\"File.c" &&
>> +     test_completion "git test-path-comp New\\\\Dir/New\\\"F" \
>> +                     "New\\Dir/New\"File.c"
>> +'
>> +
>> +if test_have_prereq !MINGW &&
>> +   mkdir $'New\034Special\035Dir' 2>/dev/null &&
>> +   touch $'New\034Special\035Dir/New\036Special\037File' 2>/dev/null
>
> The $'...' quote is bash-ism, but this is about testing bash
> completion, so as long as we make sure non-bash shells won't touch
> this part of the test, it is OK.
>
> Do we want to test a more common case of a filename that is two
> words with SP in between, i.e.
>
>         $ >'hello world' && git add hel<TAB>
>
> or is it known to work just fine without quoting/escaping (because
> the funny we care about is output from ls-files and SP is not special
> in its one-item-at-a-time-on-a-line output) and not worth checking?

This particular case already works, even without this patch series.

The problems start when you want to complete the filename after a space,
e.g. 'hello\ w<TAB', as discussed in detail in patch 5.  Actually, this
was the first thing I tried to write a test for, but it didn't work out:
inside the 'test_completion' helper function the space acts as
separator, and the completion script then sees 'hello\' and 'w' as two
separate words.

Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames

[SZEDER Gábor]

On Wed, Apr 18, 2018 at 1:32 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Tue, Apr 17, 2018 at 5:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> SZEDER Gábor <szeder.dev@gmail.com> writes:
>>
>>>     Do any more new tests need FUNNYNAMES* prereq?
>>
>> Hmph, all of these look like they involve some funnynames ;-)
>
> Well, I can' create a directory with a '|' in its name on FAT32 (on
> Linux), so this needs FUNNYNAMES prereq, too.

Or, on second thought, using a different, more widely usable character
would be better, so the test can be run on more platforms.


>> Do we want to test a more common case of a filename that is two
>> words with SP in between, i.e.
>>
>>         $ >'hello world' && git add hel<TAB>
>>
>> or is it known to work just fine without quoting/escaping (because
>> the funny we care about is output from ls-files and SP is not special
>> in its one-item-at-a-time-on-a-line output) and not worth checking?
>
> This particular case already works, even without this patch series.
>
> The problems start when you want to complete the filename after a space,
> e.g. 'hello\ w<TAB', as discussed in detail in patch 5.  Actually, this
> was the first thing I tried to write a test for, but it didn't work out:
> inside the 'test_completion' helper function the space acts as
> separator, and the completion script then sees 'hello\' and 'w' as two
> separate words.

On another second thought, a test for the already working 'hel<TAB>'
case could make sure that we won't mess up the value of IFS when filling
COMPREPLY.

Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

>>> +test_expect_failure 'complete files - quoted characters on cmdline' '
>>> +     test_when_finished "rm -r \"New(Dir\"" &&
>>
>> This does not use -rf unlike the previous one?
>
> Noted.
>
> The lack of '-f' is leftover from early versions of these tests, when I
> had a hard time getting the quoting-escaping right.  Without the '-f'
> 'rm' errored out when I messed up, and the error message helpfully
> contained the path it wasn't able to delete.

Sounds like we do not want 'f' from both tests, then?  I think it is
OK either way.

>>> +if test_have_prereq !MINGW &&
>>> +   mkdir $'New\034Special\035Dir' 2>/dev/null &&
>>> +   touch $'New\034Special\035Dir/New\036Special\037File' 2>/dev/null
>>
>> The $'...' quote is bash-ism, but this is about testing bash
>> completion, so as long as we make sure non-bash shells won't touch
>> this part of the test, it is OK.
>>
>> Do we want to test a more common case of a filename that is two
>> words with SP in between, i.e.
>>
>>         $ >'hello world' && git add hel<TAB>
>>
>> or is it known to work just fine without quoting/escaping (because
>> the funny we care about is output from ls-files and SP is not special
>> in its one-item-at-a-time-on-a-line output) and not worth checking?
>
> This particular case already works, even without this patch series.

I was more wondering about preventing regressions---"it worked
without this patch series, but now it is broken" is what I was
worried about.

> The problems start when you want to complete the filename after a space,
> e.g. 'hello\ w<TAB', as discussed in detail in patch 5.  Actually, this
> was the first thing I tried to write a test for, but it didn't work out:
> inside the 'test_completion' helper function the space acts as
> separator, and the completion script then sees 'hello\' and 'w' as two
> separate words.

Hmph.  That is somewhat unfortunate.

Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames

[Johannes Schindelin]

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

Hi Gábor,

On Tue, 17 Apr 2018, SZEDER Gábor wrote:

> Completion functions see all words on the command line verbatim,
> including any backslash-escapes, single and double quotes that might
> be there.  Furthermore, git commands quote pathnames if they contain
> certain special characters.  All these create various issues when
> doing git-aware path completion.
> 
> Add a couple of failing tests to demonstrate these issues.
> 
> Later patches in this series will discuss these issues in detail as
> they fix them.
> 
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> 
> Notes:
>     Do any more new tests need FUNNYNAMES* prereq?

Yes.

>  t/t9902-completion.sh | 91 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index b7f5b1e632..ff2e4a8f5f 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1427,6 +1427,97 @@ test_expect_success 'complete files' '
>  	test_completion "git add mom" "momified"
>  '
>  
> +# The next tests only care about how the completion script deals with
> +# unusual characters in path names.  By defining a custom completion
> +# function to list untracked files they won't be influenced by future
> +# changes of the completion functions of real git commands, and we
> +# don't have to bother with adding files to the index in these tests.
> +_git_test_path_comp ()
> +{
> +	__git_complete_index_file --others
> +}
> +
> +test_expect_failure 'complete files - escaped characters on cmdline' '
> +	test_when_finished "rm -rf \"New|Dir\"" &&
> +	mkdir "New|Dir" &&
> +	>"New|Dir/New&File.c" &&
> +
> +	test_completion "git test-path-comp N" \
> +			"New|Dir" &&	# Bash will turn this into "New\|Dir/"
> +	test_completion "git test-path-comp New\\|D" \
> +			"New|Dir" &&
> +	test_completion "git test-path-comp New\\|Dir/N" \
> +			"New|Dir/New&File.c" &&	# Bash will turn this into
> +						# "New\|Dir/New\&File.c "
> +	test_completion "git test-path-comp New\\|Dir/New\\&F" \
> +			"New|Dir/New&File.c"
> +'

This fails with:

2018-04-18T11:12:55.0436371Z expecting success: 
2018-04-18T11:12:55.0436665Z 	test_when_finished "rm -rf \"New|Dir\"" &&
2018-04-18T11:12:55.0436799Z 	mkdir "New|Dir" &&
2018-04-18T11:12:55.0436904Z 	>"New|Dir/New&File.c" &&
2018-04-18T11:12:55.0436972Z 
2018-04-18T11:12:55.0437158Z 	test_completion "git test-path-comp N" \
2018-04-18T11:12:55.0437296Z 			"New|Dir" &&	# Bash will turn this into "New\|Dir/"
2018-04-18T11:12:55.0437413Z 	test_completion "git test-path-comp New\\|D" \
2018-04-18T11:12:55.0437522Z 			"New|Dir" &&
2018-04-18T11:12:55.0437629Z 	test_completion "git test-path-comp New\\|Dir/N" \
2018-04-18T11:12:55.0437767Z 			"New|Dir/New&File.c" &&	# Bash will turn this into
2018-04-18T11:12:55.0438040Z 						# "New\|Dir/New\&File.c "
2018-04-18T11:12:55.0438152Z 	test_completion "git test-path-comp New\\|Dir/New\\&F" \
2018-04-18T11:12:55.0438504Z 			"New|Dir/New&File.c"
2018-04-18T11:12:55.0438742Z 
2018-04-18T11:12:55.0590984Z ++ test_when_finished 'rm -rf "New|Dir"'
2018-04-18T11:12:55.0591722Z ++ test 0 = 0
2018-04-18T11:12:55.0592001Z ++ test_cleanup='{ rm -rf "New|Dir"
2018-04-18T11:12:55.0592290Z 		} && (exit "$eval_ret"); eval_ret=$?; :'
2018-04-18T11:12:55.0592472Z ++ mkdir 'New|Dir'
2018-04-18T11:12:55.0717255Z ++ test_completion 'git test-path-comp N' 'New|Dir'
2018-04-18T11:12:55.0717680Z ++ test 2 -gt 1
2018-04-18T11:12:55.0718062Z ++ printf '%s\n' 'New|Dir'
2018-04-18T11:12:55.0718275Z ++ run_completion 'git test-path-comp N'
2018-04-18T11:12:55.0718447Z ++ local -a COMPREPLY _words
2018-04-18T11:12:55.0718631Z ++ local _cword
2018-04-18T11:12:55.0718806Z ++ _words=($1)
2018-04-18T11:12:55.0718965Z ++ test N = ' '
2018-04-18T11:12:55.0719124Z ++ ((  _cword = 3 - 1  ))
2018-04-18T11:12:55.0719286Z ++ __git_wrap__git_main
2018-04-18T11:12:55.0719467Z ++ __git_func_wrap __git_main
2018-04-18T11:12:55.0719633Z ++ local cur words cword prev
2018-04-18T11:12:55.0719801Z ++ _get_comp_words_by_ref -n =: cur words cword prev
2018-04-18T11:12:55.0720074Z ++ '[' 6 -gt 0 ']'
2018-04-18T11:12:55.0720239Z ++ case "$1" in
2018-04-18T11:12:55.0720406Z ++ shift
2018-04-18T11:12:55.0720584Z ++ '[' 5 -gt 0 ']'
2018-04-18T11:12:55.0720742Z ++ case "$1" in
2018-04-18T11:12:55.0720899Z ++ shift
2018-04-18T11:12:55.0721054Z ++ '[' 4 -gt 0 ']'
2018-04-18T11:12:55.0721240Z ++ case "$1" in
2018-04-18T11:12:55.0721392Z ++ cur=N
2018-04-18T11:12:55.0721547Z ++ shift
2018-04-18T11:12:55.0721717Z ++ '[' 3 -gt 0 ']'
2018-04-18T11:12:55.0721879Z ++ case "$1" in
2018-04-18T11:12:55.0722040Z ++ words=("${_words[@]}")
2018-04-18T11:12:55.0722201Z ++ shift
2018-04-18T11:12:55.0722396Z ++ '[' 2 -gt 0 ']'
2018-04-18T11:12:55.0722931Z ++ case "$1" in
2018-04-18T11:12:55.0723070Z ++ cword=2
2018-04-18T11:12:55.0723221Z ++ shift
2018-04-18T11:12:55.0723357Z ++ '[' 1 -gt 0 ']'
2018-04-18T11:12:55.0723575Z ++ case "$1" in
2018-04-18T11:12:55.0723735Z ++ prev=test-path-comp
2018-04-18T11:12:55.0723874Z ++ shift
2018-04-18T11:12:55.0724009Z ++ '[' 0 -gt 0 ']'
2018-04-18T11:12:55.0724397Z ++ __git_main
2018-04-18T11:12:55.0724984Z ++ local i c=1 command __git_dir __git_repo_path
2018-04-18T11:12:55.0725183Z ++ local __git_C_args C_args_count=0
2018-04-18T11:12:55.0725353Z ++ '[' 1 -lt 2 ']'
2018-04-18T11:12:55.0725537Z ++ i=test-path-comp
2018-04-18T11:12:55.0725712Z ++ case "$i" in
2018-04-18T11:12:55.0725882Z ++ command=test-path-comp
2018-04-18T11:12:55.0726057Z ++ break
2018-04-18T11:12:55.0726270Z ++ '[' -z test-path-comp ']'
2018-04-18T11:12:55.0726446Z ++ __git_complete_command test-path-comp
2018-04-18T11:12:55.0726621Z ++ local command=test-path-comp
2018-04-18T11:12:55.0726816Z ++ local completion_func=_git_test_path_comp
2018-04-18T11:12:55.0726992Z ++ declare -f _git_test_path_comp
2018-04-18T11:12:55.0727353Z ++ declare -f _git_test_path_comp
2018-04-18T11:12:55.0727547Z ++ _git_test_path_comp
2018-04-18T11:12:55.0727716Z ++ __git_complete_index_file --others
2018-04-18T11:12:55.0727890Z ++ local dequoted_word pfx= cur_
2018-04-18T11:12:55.0728234Z ++ __git_dequote N
2018-04-18T11:12:55.0728418Z ++ local rest=N len ch
2018-04-18T11:12:55.0728869Z ++ dequoted_word=
2018-04-18T11:12:55.0729020Z ++ test -n N
2018-04-18T11:12:55.0729152Z ++ len=0
2018-04-18T11:12:55.0729309Z ++ dequoted_word=N
2018-04-18T11:12:55.0729440Z ++ rest=
2018-04-18T11:12:55.0729666Z ++ case "${rest:0:1}" in
2018-04-18T11:12:55.0729822Z ++ test -n ''
2018-04-18T11:12:55.0729993Z ++ case "$dequoted_word" in
2018-04-18T11:12:55.0730133Z ++ cur_=N
2018-04-18T11:12:55.0782504Z +++ __git_index_files --others '' N
2018-04-18T11:12:55.0782805Z +++ local root= match=N
2018-04-18T11:12:55.0845235Z +++ __git_ls_files_helper '' --others N
2018-04-18T11:12:55.0845440Z +++ '[' --others == --committable ']'
2018-04-18T11:12:55.0845567Z +++ __git -C '' -c core.quotePath=false ls-files --exclude-standard --others -- 'N*'
2018-04-18T11:12:55.0845706Z +++ git -C '' -c core.quotePath=false ls-files --exclude-standard --others -- 'N*'
2018-04-18T11:12:55.0907632Z +++ awk -F / -v pfx= '{
2018-04-18T11:12:55.0907806Z 		paths[$1] = 1
2018-04-18T11:12:55.0908985Z 	}
2018-04-18T11:12:55.0942839Z 	END {
2018-04-18T11:12:55.0943072Z 		for (p in paths) {
2018-04-18T11:12:55.0949175Z 			if (substr(p, 1, 1) != "\"") {
2018-04-18T11:12:55.0949458Z 				# No special characters, easy!
2018-04-18T11:12:55.0949659Z 				print pfx p
2018-04-18T11:12:55.0949823Z 				continue
2018-04-18T11:12:55.0949999Z 			}
2018-04-18T11:12:55.0950121Z 
2018-04-18T11:12:55.0950335Z 			# The path is quoted.
2018-04-18T11:12:55.0950829Z 			p = dequote(p)
2018-04-18T11:12:55.0951171Z 			if (p == "")
2018-04-18T11:12:55.0951555Z 				continue
2018-04-18T11:12:55.0951672Z 
2018-04-18T11:12:55.0951856Z 			# Even when a directory name itself does not contain
2018-04-18T11:12:55.0952038Z 			# any special characters, it will still be quoted if
2018-04-18T11:12:55.0952213Z 			# any of its (stripped) trailing path components do.
2018-04-18T11:12:55.0952407Z 			# Because of this we may have seen the same direcory
2018-04-18T11:12:55.0952583Z 			# both quoted and unquoted.
2018-04-18T11:12:55.0952762Z 			if (p in paths)
2018-04-18T11:12:55.0952948Z 				# We have seen the same directory unquoted,
2018-04-18T11:12:55.0953117Z 				# skip it.
2018-04-18T11:12:55.0953276Z 				continue
2018-04-18T11:12:55.0953441Z 			else
2018-04-18T11:12:55.0953613Z 				print pfx p
2018-04-18T11:12:55.0953766Z 		}
2018-04-18T11:12:55.0953914Z 	}
2018-04-18T11:12:55.0954461Z 	function dequote(p,    bs_idx, out, esc, esc_idx, dec) {
2018-04-18T11:12:55.0954650Z 		# Skip opening double quote.
2018-04-18T11:12:55.0954813Z 		p = substr(p, 2)
2018-04-18T11:12:55.0954935Z 
2018-04-18T11:12:55.0955237Z 		# Interpret backslash escape sequences.
2018-04-18T11:12:55.0955415Z 		while ((bs_idx = index(p, "\\")) != 0) {
2018-04-18T11:12:55.0955533Z 			out = out substr(p, 1, bs_idx - 1)
2018-04-18T11:12:55.0955638Z 			esc = substr(p, bs_idx + 1, 1)
2018-04-18T11:12:55.0955743Z 			p = substr(p, bs_idx + 2)
2018-04-18T11:12:55.0955830Z 
2018-04-18T11:12:55.0955939Z 			if ((esc_idx = index("abtvfr\"\\", esc)) != 0) {
2018-04-18T11:12:55.0956079Z 				# C-style one-character escape sequence.
2018-04-18T11:12:55.0956513Z 				out = out substr("\a\b\t\v\f\r\"\\",
2018-04-18T11:12:55.0956631Z esc_idx, 1)
2018-04-18T11:12:55.0956745Z 			} else if (esc == "n") {
2018-04-18T11:12:55.0956853Z 				# Uh-oh, a newline character.
2018-04-18T11:12:55.0956973Z 				# We cant reliably put a pathname
2018-04-18T11:12:55.0957086Z 				# containing a newline into COMPREPLY,
2018-04-18T11:12:55.0957193Z 				# and the newline would create a mess.
2018-04-18T11:12:55.0957300Z 				# Skip this path.
2018-04-18T11:12:55.0957413Z 				return ""
2018-04-18T11:12:55.0957510Z 			} else {
2018-04-18T11:12:55.0957808Z 				# Must be a \nnn octal value, then.
2018-04-18T11:12:55.0958070Z 				dec = esc * 64 + \
2018-04-18T11:12:55.0958184Z 				      substr(p, 1, 1) * 8  + \
2018-04-18T11:12:55.0958274Z 				      substr(p, 2, 1)
2018-04-18T11:12:55.0958369Z 				out = out sprintf("%c", dec)
2018-04-18T11:12:55.0958587Z 				p = substr(p, 3)
2018-04-18T11:12:55.0958692Z 			}
2018-04-18T11:12:55.0958769Z 		}
2018-04-18T11:12:55.0958862Z 		# Drop closing double quote, if there is one.
2018-04-18T11:12:55.0958969Z 		# (There isnt any if this is a directory, as it was
2018-04-18T11:12:55.0959153Z 		# already stripped with the trailing path components.)
2018-04-18T11:12:55.0959256Z 		if (substr(p, length(p), 1) == "\"")
2018-04-18T11:12:55.0959356Z 			out = out substr(p, 1, length(p) - 1)
2018-04-18T11:12:55.0959441Z 		else
2018-04-18T11:12:55.0959541Z 			out = out p
2018-04-18T11:12:55.0959598Z 
2018-04-18T11:12:55.0959682Z 		return out
2018-04-18T11:12:55.0959763Z 	}'
2018-04-18T11:12:55.1182135Z ++ __gitcomp_file_direct $'New∩\201╝Dir'
2018-04-18T11:12:55.1182355Z ++ local 'IFS=
2018-04-18T11:12:55.1182439Z '
2018-04-18T11:12:55.1182518Z ++ COMPREPLY=($1)
2018-04-18T11:12:55.1182622Z ++ compopt -o filenames +o nospace
2018-04-18T11:12:55.1182877Z ++ compgen -f /non-existing-dir/
2018-04-18T11:12:55.1182979Z ++ return 0
2018-04-18T11:12:55.1183055Z ++ return
2018-04-18T11:12:55.1183147Z ++ print_comp
2018-04-18T11:12:55.1183224Z ++ local 'IFS=
2018-04-18T11:12:55.1183300Z '
2018-04-18T11:12:55.1183398Z ++ echo $'New∩\201╝Dir'
2018-04-18T11:12:55.1183508Z ++ sort out
2018-04-18T11:12:55.1183605Z ++ /usr/bin/sort out
2018-04-18T11:12:55.1306331Z ++ test_cmp expected out_sorted
2018-04-18T11:12:55.1306825Z ++ mingw_test_cmp expected out_sorted
2018-04-18T11:12:55.1307024Z ++ local test_cmp_a= test_cmp_b=
2018-04-18T11:12:55.1307233Z ++ local stdin_for_diff=
2018-04-18T11:12:55.1307401Z ++ test -s expected
2018-04-18T11:12:55.1307568Z ++ test -s out_sorted
2018-04-18T11:12:55.1307742Z ++ mingw_read_file_strip_cr_ test_cmp_a
2018-04-18T11:12:55.1308083Z ++ local line
2018-04-18T11:12:55.1308424Z ++ :
2018-04-18T11:12:55.1308566Z ++ IFS=$'\r'
2018-04-18T11:12:55.1308717Z ++ read -r -d '
2018-04-18T11:12:55.1308852Z ' line
2018-04-18T11:12:55.1317521Z ++ line='New|Dir
2018-04-18T11:12:55.1317784Z '
2018-04-18T11:12:55.1318257Z ++ eval 'test_cmp_a=$test_cmp_a$line'
2018-04-18T11:12:55.1318424Z +++ test_cmp_a='New|Dir
2018-04-18T11:12:55.1318569Z '
2018-04-18T11:12:55.1318724Z ++ :
2018-04-18T11:12:55.1318871Z ++ IFS=$'\r'
2018-04-18T11:12:55.1319027Z ++ read -r -d '
2018-04-18T11:12:55.1319170Z ' line
2018-04-18T11:12:55.1319334Z ++ test -z ''
2018-04-18T11:12:55.1319476Z ++ break
2018-04-18T11:12:55.1319628Z ++ mingw_read_file_strip_cr_ test_cmp_b
2018-04-18T11:12:55.1319797Z ++ local line
2018-04-18T11:12:55.1319939Z ++ :
2018-04-18T11:12:55.1320081Z ++ IFS=$'\r'
2018-04-18T11:12:55.1320240Z ++ read -r -d '
2018-04-18T11:12:55.1320384Z ' line
2018-04-18T11:12:55.1320555Z ++ line='NewDir
2018-04-18T11:12:55.1320915Z '
2018-04-18T11:12:55.1321099Z ++ eval 'test_cmp_b=$test_cmp_b$line'
2018-04-18T11:12:55.1321266Z +++ test_cmp_b='NewDir
2018-04-18T11:12:55.1321422Z '
2018-04-18T11:12:55.1321570Z ++ :
2018-04-18T11:12:55.1321705Z ++ IFS=$'\r'
2018-04-18T11:12:55.1321859Z ++ read -r -d '
2018-04-18T11:12:55.1321994Z ' line
2018-04-18T11:12:55.1322219Z ++ test -z ''
2018-04-18T11:12:55.1322361Z ++ break
2018-04-18T11:12:55.1322497Z ++ test -n 'New|Dir
2018-04-18T11:12:55.1322649Z '
2018-04-18T11:12:55.1322828Z ++ test -n 'NewDir
2018-04-18T11:12:55.1322977Z '
2018-04-18T11:12:55.1323109Z ++ test 'New|Dir
2018-04-18T11:12:55.1323397Z ' = 'NewDir
2018-04-18T11:12:55.1323540Z '
2018-04-18T11:12:55.1323680Z ++ eval 'diff -u "$@" '
2018-04-18T11:12:55.1323840Z +++ diff -u expected out_sorted
2018-04-18T11:12:55.1454977Z --- expected	2018-04-18 11:12:55.065444100 +0000
2018-04-18T11:12:55.1455785Z error: last command exited with $?=1
2018-04-18T11:12:55.1456722Z +++ out_sorted	2018-04-18 11:12:55.127568400 +0000
2018-04-18T11:12:55.1457211Z @@ -1 +1 @@
2018-04-18T11:12:55.1457408Z -New|Dir
2018-04-18T11:12:55.1457752Z +NewDir
2018-04-18T11:12:55.1457975Z not ok 111 - complete files - escaped characters on cmdline
2018-04-18T11:12:55.1645995Z #	
2018-04-18T11:12:55.1646221Z #		test_when_finished "rm -rf \"New|Dir\"" &&
2018-04-18T11:12:55.1646380Z #		mkdir "New|Dir" &&
2018-04-18T11:12:55.1646487Z #		>"New|Dir/New&File.c" &&
2018-04-18T11:12:55.1646583Z #	
2018-04-18T11:12:55.1646865Z #		test_completion "git test-path-comp N" \
2018-04-18T11:12:55.1646986Z #				"New|Dir" &&	# Bash will turn this into "New\|Dir/"
2018-04-18T11:12:55.1647108Z #		test_completion "git test-path-comp New\\|D" \
2018-04-18T11:12:55.1647212Z #				"New|Dir" &&
2018-04-18T11:12:55.1647346Z #		test_completion "git test-path-comp New\\|Dir/N" \
2018-04-18T11:12:55.1647510Z # "New|Dir/New&File.c" &&	# Bash will turn this into
2018-04-18T11:12:55.1647636Z # # "New\|Dir/New\&File.c "
2018-04-18T11:12:55.1647775Z #		test_completion "git test-path-comp New\\|Dir/New\\&F" \
2018-04-18T11:12:55.1647886Z # "New|Dir/New&File.c"

I suspect that the culprit is once again Cygwin's trick where illegal
characters are mapped into a private Unicode page. Cygwin (and therefore
MSYS2 runtime, and therefore the Bash used to run the test script) can use
those filenames all right, but Git cannot.

So even testing whether you could write an illegal file name via shell
script is *not* enough to determine whether the file system supports funny
characters.

As far as I can tell from a *really* cursory glance, this is the only
affected test case. Apparently your prereq catches, somehow, on Windows:

2018-04-18T11:12:43.0459702Z     Your filesystem does not allow \ and " in filenames.
2018-04-18T11:12:43.0459823Z     skipped: complete files - C-style escapes in ls-files output (missing FUNNYNAMES_BS_DQ)

Ciao,
Dscho

Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames

[SZEDER Gábor]

On Wed, Apr 18, 2018 at 2:31 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

> I suspect that the culprit is once again Cygwin's trick where illegal
> characters are mapped into a private Unicode page. Cygwin (and therefore
> MSYS2 runtime, and therefore the Bash used to run the test script) can use
> those filenames all right, but Git cannot.
>
> So even testing whether you could write an illegal file name via shell
> script is *not* enough to determine whether the file system supports funny
> characters.

I followed suit of all existing FUNNYNAMES checks:

  $ git grep -B3 'test_set_prereq FUNNYNAMES' master t/
  master:t/t3600-rm.sh-if test_have_prereq !MINGW && touch -- 'tab
   embedded' 'newline
  master:t/t3600-rm.sh-embedded' 2>/dev/null
  master:t/t3600-rm.sh-then
  master:t/t3600-rm.sh:   test_set_prereq FUNNYNAMES
  --
  master:t/t4135-apply-weird-filenames.sh-        if test_have_prereq !MINGW &&
  master:t/t4135-apply-weird-filenames.sh-                touch --
"tab   embedded.txt" '\''"quoteembedded".txt'\''
  master:t/t4135-apply-weird-filenames.sh-        then
  master:t/t4135-apply-weird-filenames.sh:
test_set_prereq FUNNYNAMES
  --
  master:t/t9903-bash-prompt.sh-
  master:t/t9903-bash-prompt.sh-if test_have_prereq !MINGW && mkdir
"$repo_with_newline" 2>/dev/null
  master:t/t9903-bash-prompt.sh-then
  master:t/t9903-bash-prompt.sh:  test_set_prereq FUNNYNAMES

How am I supposed to check this, then?


> As far as I can tell from a *really* cursory glance, this is the only
> affected test case. Apparently your prereq catches, somehow, on Windows:
>
> 2018-04-18T11:12:43.0459702Z     Your filesystem does not allow \ and " in filenames.
> 2018-04-18T11:12:43.0459823Z     skipped: complete files - C-style escapes in ls-files output (missing FUNNYNAMES_BS_DQ)

Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames

[SZEDER Gábor]

On Wed, Apr 18, 2018 at 3:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>>> Do we want to test a more common case of a filename that is two
>>> words with SP in between, i.e.
>>>
>>>         $ >'hello world' && git add hel<TAB>
>>>
>>> or is it known to work just fine without quoting/escaping (because
>>> the funny we care about is output from ls-files and SP is not special
>>> in its one-item-at-a-time-on-a-line output) and not worth checking?
>>
>> This particular case already works, even without this patch series.
>
> I was more wondering about preventing regressions---"it worked
> without this patch series, but now it is broken" is what I was
> worried about.
>
>> The problems start when you want to complete the filename after a space,
>> e.g. 'hello\ w<TAB', as discussed in detail in patch 5.  Actually, this
>> was the first thing I tried to write a test for, but it didn't work out:
>> inside the 'test_completion' helper function the space acts as
>> separator, and the completion script then sees 'hello\' and 'w' as two
>> separate words.
>
> Hmph.  That is somewhat unfortunate.

Actually, I used 'test_completion' in these new tests, because there
is that big test checking file completion for various commands, and it
already uses 'test_completion', so I just followed suit.  Now, that
test checks that the right type(s) of files are listed for various git
commands, e.g. modified and untracked for 'git add', IOW that the
caller of __git_complete_index_file() specifies the appropriate 'git
ls-files' options.  For those kind of checks 'test_completion' is
great.

These new tests, however, are primarily interested in the inner
workings of __git_complete_index_file() in the presence of escapes
and/or quotes in the path to be completed and/or in the output of 'git
ls-files'.  For these kind of tests we could simply invoke
__git_complete_index_file() directly, like we call __git_refs()
directly to test refs completion.  Then we could set the current path
to be completed to whatever we want, including spaces, because it
won't be subject to field splitting like the command line given to
'test_completion'.

So, I think for v2 I will rewrite these tests to call
__git_complete_index_file() directly instead of using
'test_completion', and will include a test with spaces in path names.

Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> These new tests, however, are primarily interested in the inner
> workings of __git_complete_index_file() in the presence of escapes
> and/or quotes in the path to be completed and/or in the output of 'git
> ls-files'.  For these kind of tests we could simply invoke
> __git_complete_index_file() directly, like we call __git_refs()
> directly to test refs completion.  Then we could set the current path
> to be completed to whatever we want, including spaces, because it
> won't be subject to field splitting like the command line given to
> 'test_completion'.
>
> So, I think for v2 I will rewrite these tests to call
> __git_complete_index_file() directly instead of using
> 'test_completion', and will include a test with spaces in path names.

Quite well thought-out reasoning.  Thanks.

[PATCH 0/2] Test improvements for 'sg/complete-paths'

[SZEDER Gábor]

> > So, I think for v2 I will rewrite these tests to call
> > __git_complete_index_file() directly instead of using
> > 'test_completion', and will include a test with spaces in path names.
> 
> Quite well thought-out reasoning.  Thanks.

Unfortunately I couldn't get around to it soon enough, and now the
topic 'sg/complete-paths' is already in next, so here are those test
improvements on top.


SZEDER Gábor (2):
  completion: don't return with error from __gitcomp_file_direct()
  t9902-completion: exercise __git_complete_index_file() directly

 contrib/completion/git-completion.bash |   6 +-
 t/t9902-completion.sh                  | 225 +++++++++++++------------
 2 files changed, 122 insertions(+), 109 deletions(-)

-- 
2.17.0.799.gd371044c7c

[PATCH 1/2] completion: don't return with error from __gitcomp_file_direct()

[SZEDER Gábor]

In __gitcomp_file_direct() we tell Bash that it should handle our
possible completion words as filenames with the following piece of
cleverness:

  # use a hack to enable file mode in bash < 4
  compopt -o filenames +o nospace 2>/dev/null ||
  compgen -f /non-existing-dir/ > /dev/null

Unfortunately, this makes this function always return with error
when it is not invoked in real completion, but e.g. in tests of
't9902-completion.sh':

  - First the 'compopt' line errors out
    - either because in Bash v3.x there is no such command,
    - or because in Bash v4.x it complains about "not currently
      executing completion function",

  - then 'compgen' just silently returns with error because of the
    non-existing directory.

Since __gitcomp_file_direct() is now the last command executed in
__git_complete_index_file(), that function returns with error as well,
which prevents it from being invoked in tests directly as is, and
would require extra steps in test to hide its error code.

So let's make sure that __gitcomp_file_direct() doesn't return with
error, because in the tests coming in the following patch we do want
to exercise __git_complete_index_file() directly,

__gitcomp_file() contains the same construct, and thus it, too, always
returns with error.  Update that function accordingly as well.

While at it, also remove the space from between the redirection
operator and the filename in both functions.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 816901f0f0..8bc79a5226 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -420,7 +420,8 @@ __gitcomp_file_direct ()
 
 	# use a hack to enable file mode in bash < 4
 	compopt -o filenames +o nospace 2>/dev/null ||
-	compgen -f /non-existing-dir/ > /dev/null
+	compgen -f /non-existing-dir/ >/dev/null ||
+	true
 }
 
 # Generates completion reply with compgen from newline-separated possible
@@ -442,7 +443,8 @@ __gitcomp_file ()
 
 	# use a hack to enable file mode in bash < 4
 	compopt -o filenames +o nospace 2>/dev/null ||
-	compgen -f /non-existing-dir/ > /dev/null
+	compgen -f /non-existing-dir/ >/dev/null ||
+	true
 }
 
 # Execute 'git ls-files', unless the --committable option is specified, in
-- 
2.17.0.799.gd371044c7c

[PATCH 2/2] t9902-completion: exercise __git_complete_index_file() directly

[SZEDER Gábor]

The tests added in 2f271cd9cf (t9902-completion: add tests
demonstrating issues with quoted pathnames, 2018-05-08) and in
2ab6eab4fe (completion: improve handling quoted paths in 'git
ls-files's output, 2018-03-28) have a few shortcomings:

  - All these test use the 'test_completion' helper function, thus
    they are exercising the whole completion machinery, although they
    are only interested in how git-aware path completion, specifically
    the __git_complete_index_file() function deals with unusual
    characters in pathnames and on the command line.

  - These tests can't satisfactorily test the case of pathnames
    containing spaces, because 'test_completion' gets the words on the
    command line as a single argument and it uses space as word
    separator.

  - Some of the tests are protected by different FUNNYNAMES_* prereqs
    depending on whether they put backslashes and double quotes or
    separator characters (FS, GS, RS, US) in pathnames, although a
    filesystem not allowing one likely doesn't allow the others
    either.

  - One of the tests operates on paths containing '|' and '&'
    characters without being protected by a FUNNYNAMES prereq, but
    some filesystems (notably on Windows) don't allow these characters
    in pathnames, either.

Replace these tests with basically equivalent, more focused tests that
call __git_complete_index_file() directly.  Since this function only
looks at the current word to be completed, i.e. the $cur variable, we
can easily include pathnames containing spaces in the tests, so use
such pathnames instead of pathnames containing '|' and '&'.  Finally,
use only a single FUNNYNAMES prereq for all kinds of special
characters.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t9902-completion.sh | 225 ++++++++++++++++++++++--------------------
 1 file changed, 118 insertions(+), 107 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 955932174c..1b6d275254 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1209,6 +1209,124 @@ test_expect_success 'teardown after ref completion' '
 	git remote remove other
 '
 
+
+test_path_completion ()
+{
+	test $# = 2 || error "bug in the test script: not 2 parameters to test_path_completion"
+
+	local cur="$1" expected="$2"
+	echo "$expected" >expected &&
+	(
+		# In the following tests calling this function we only
+		# care about how __git_complete_index_file() deals with
+		# unusual characters in path names.  By requesting only
+		# untracked files we dont have to bother adding any
+		# paths to the index in those tests.
+		__git_complete_index_file --others &&
+		print_comp
+	) &&
+	test_cmp expected out
+}
+
+test_expect_success 'setup for path completion tests' '
+	mkdir simple-dir \
+	      "spaces in dir" \
+	      árvíztűrő &&
+	touch simple-dir/simple-file \
+	      "spaces in dir/spaces in file" \
+	      "árvíztűrő/Сайн яваарай" &&
+	if test_have_prereq !MINGW &&
+	   mkdir BS\\dir \
+		 '$'separators\034in\035dir'' &&
+	   touch BS\\dir/DQ\"file \
+		 '$'separators\034in\035dir/sep\036in\037file''
+	then
+		test_set_prereq FUNNYNAMES
+	else
+		rm -rf BS\\dir '$'separators\034in\035dir''
+	fi
+'
+
+test_expect_success '__git_complete_index_file - simple' '
+	test_path_completion simple simple-dir &&  # Bash is supposed to
+						   # add the trailing /.
+	test_path_completion simple-dir/simple simple-dir/simple-file
+'
+
+test_expect_success \
+    '__git_complete_index_file - escaped characters on cmdline' '
+	test_path_completion spac "spaces in dir" &&  # Bash will turn this
+						      # into "spaces\ in\ dir"
+	test_path_completion "spaces\\ i" \
+			     "spaces in dir" &&
+	test_path_completion "spaces\\ in\\ dir/s" \
+			     "spaces in dir/spaces in file" &&
+	test_path_completion "spaces\\ in\\ dir/spaces\\ i" \
+			     "spaces in dir/spaces in file"
+'
+
+test_expect_success \
+    '__git_complete_index_file - quoted characters on cmdline' '
+	# Testing with an opening but without a corresponding closing
+	# double quote is important.
+	test_path_completion \"spac "spaces in dir" &&
+	test_path_completion "\"spaces i" \
+			     "spaces in dir" &&
+	test_path_completion "\"spaces in dir/s" \
+			     "spaces in dir/spaces in file" &&
+	test_path_completion "\"spaces in dir/spaces i" \
+			     "spaces in dir/spaces in file"
+'
+
+test_expect_success '__git_complete_index_file - UTF-8 in ls-files output' '
+	test_path_completion á árvíztűrő &&
+	test_path_completion árvíztűrő/С "árvíztűrő/Сайн яваарай"
+'
+
+test_expect_success FUNNYNAMES \
+    '__git_complete_index_file - C-style escapes in ls-files output' '
+	test_path_completion BS \
+			     BS\\dir &&
+	test_path_completion BS\\\\d \
+			     BS\\dir &&
+	test_path_completion BS\\\\dir/DQ \
+			     BS\\dir/DQ\"file &&
+	test_path_completion BS\\\\dir/DQ\\\"f \
+			     BS\\dir/DQ\"file
+'
+
+test_expect_success FUNNYNAMES \
+    '__git_complete_index_file - \nnn-escaped characters in ls-files output' '
+	test_path_completion sep '$'separators\034in\035dir'' &&
+	test_path_completion '$'separators\034i'' \
+			     '$'separators\034in\035dir'' &&
+	test_path_completion '$'separators\034in\035dir/sep'' \
+			     '$'separators\034in\035dir/sep\036in\037file'' &&
+	test_path_completion '$'separators\034in\035dir/sep\036i'' \
+			     '$'separators\034in\035dir/sep\036in\037file''
+'
+
+test_expect_success FUNNYNAMES \
+    '__git_complete_index_file - removing repeated quoted path components' '
+	test_when_finished rm -r repeated-quoted &&
+	mkdir repeated-quoted &&      # A directory whose name in itself
+				      # would not be quoted ...
+	>repeated-quoted/0-file &&
+	>repeated-quoted/1\"file &&   # ... but here the file makes the
+				      # dirname quoted ...
+	>repeated-quoted/2-file &&
+	>repeated-quoted/3\"file &&   # ... and here, too.
+
+	# Still, we shold only list the directory name only once.
+	test_path_completion repeated repeated-quoted
+'
+
+test_expect_success 'teardown after path completion tests' '
+	rm -rf simple-dir "spaces in dir" árvíztűrő \
+	       BS\\dir '$'separators\034in\035dir''
+'
+
+
 test_expect_success '__git_get_config_variables' '
 	cat >expect <<-EOF &&
 	name-1
@@ -1469,113 +1587,6 @@ test_expect_success 'complete files' '
 	test_completion "git add mom" "momified"
 '
 
-# The next tests only care about how the completion script deals with
-# unusual characters in path names.  By defining a custom completion
-# function to list untracked files they won't be influenced by future
-# changes of the completion functions of real git commands, and we
-# don't have to bother with adding files to the index in these tests.
-_git_test_path_comp ()
-{
-	__git_complete_index_file --others
-}
-
-test_expect_success 'complete files - escaped characters on cmdline' '
-	test_when_finished "rm -rf \"New|Dir\"" &&
-	mkdir "New|Dir" &&
-	>"New|Dir/New&File.c" &&
-
-	test_completion "git test-path-comp N" \
-			"New|Dir" &&	# Bash will turn this into "New\|Dir/"
-	test_completion "git test-path-comp New\\|D" \
-			"New|Dir" &&
-	test_completion "git test-path-comp New\\|Dir/N" \
-			"New|Dir/New&File.c" &&	# Bash will turn this into
-						# "New\|Dir/New\&File.c "
-	test_completion "git test-path-comp New\\|Dir/New\\&F" \
-			"New|Dir/New&File.c"
-'
-
-test_expect_success 'complete files - quoted characters on cmdline' '
-	test_when_finished "rm -r \"New(Dir\"" &&
-	mkdir "New(Dir" &&
-	>"New(Dir/New)File.c" &&
-
-	# Testing with an opening but without a corresponding closing
-	# double quote is important.
-	test_completion "git test-path-comp \"New(D" "New(Dir" &&
-	test_completion "git test-path-comp \"New(Dir/New)F" \
-			"New(Dir/New)File.c"
-'
-
-test_expect_success 'complete files - UTF-8 in ls-files output' '
-	test_when_finished "rm -r árvíztűrő" &&
-	mkdir árvíztűrő &&
-	>"árvíztűrő/Сайн яваарай" &&
-
-	test_completion "git test-path-comp á" "árvíztűrő" &&
-	test_completion "git test-path-comp árvíztűrő/С" \
-			"árvíztűrő/Сайн яваарай"
-'
-
-# Testing with a path containing a backslash is important.
-if test_have_prereq !MINGW &&
-   mkdir 'New\Dir' 2>/dev/null &&
-   touch 'New\Dir/New"File.c' 2>/dev/null
-then
-	test_set_prereq FUNNYNAMES_BS_DQ
-else
-	say "Your filesystem does not allow \\ and \" in filenames."
-	rm -rf 'New\Dir'
-fi
-test_expect_success FUNNYNAMES_BS_DQ \
-    'complete files - C-style escapes in ls-files output' '
-	test_when_finished "rm -r \"New\\\\Dir\"" &&
-
-	test_completion "git test-path-comp N" "New\\Dir" &&
-	test_completion "git test-path-comp New\\\\D" "New\\Dir" &&
-	test_completion "git test-path-comp New\\\\Dir/N" \
-			"New\\Dir/New\"File.c" &&
-	test_completion "git test-path-comp New\\\\Dir/New\\\"F" \
-			"New\\Dir/New\"File.c"
-'
-
-if test_have_prereq !MINGW &&
-   mkdir $'New\034Special\035Dir' 2>/dev/null &&
-   touch $'New\034Special\035Dir/New\036Special\037File' 2>/dev/null
-then
-	test_set_prereq FUNNYNAMES_SEPARATORS
-else
-	say 'Your filesystem does not allow special separator characters (FS, GS, RS, US) in filenames.'
-	rm -rf $'New\034Special\035Dir'
-fi
-test_expect_success FUNNYNAMES_SEPARATORS \
-    'complete files - \nnn-escaped control characters in ls-files output' '
-	test_when_finished "rm -r '$'New\034Special\035Dir''" &&
-
-	# Note: these will be literal separator characters on the cmdline.
-	test_completion "git test-path-comp N" "'$'New\034Special\035Dir''" &&
-	test_completion "git test-path-comp '$'New\034S''" \
-			"'$'New\034Special\035Dir''" &&
-	test_completion "git test-path-comp '$'New\034Special\035Dir/''" \
-			"'$'New\034Special\035Dir/New\036Special\037File''" &&
-	test_completion "git test-path-comp '$'New\034Special\035Dir/New\036S''" \
-			"'$'New\034Special\035Dir/New\036Special\037File''"
-'
-
-test_expect_success FUNNYNAMES_BS_DQ \
-    'complete files - removing repeated quoted path components' '
-	test_when_finished rm -rf NewDir &&
-	mkdir NewDir &&    # A dirname which in itself would not be quoted ...
-	>NewDir/0-file &&
-	>NewDir/1\"file && # ... but here the file makes the dirname quoted ...
-	>NewDir/2-file &&
-	>NewDir/3\"file && # ... and here, too.
-
-	# Still, we should only list it once.
-	test_completion "git test-path-comp New" "NewDir"
-'
-
-
 test_expect_success "completion uses <cmd> completion for alias: !sh -c 'git <cmd> ...'" '
 	test_config alias.co "!sh -c '"'"'git checkout ...'"'"'" &&
 	test_completion "git co m" <<-\EOF
-- 
2.17.0.799.gd371044c7c

Re: [PATCH 2/2] t9902-completion: exercise __git_complete_index_file() directly

[Eric Sunshine]

On Fri, May 18, 2018 at 10:17 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> The tests added in 2f271cd9cf (t9902-completion: add tests
> demonstrating issues with quoted pathnames, 2018-05-08) and in
> 2ab6eab4fe (completion: improve handling quoted paths in 'git
> ls-files's output, 2018-03-28) have a few shortcomings:
>
>   - All these test use the 'test_completion' helper function, thus

s/these test/&s/

>     they are exercising the whole completion machinery, although they
>     are only interested in how git-aware path completion, specifically
>     the __git_complete_index_file() function deals with unusual
>     characters in pathnames and on the command line.
>
>   - These tests can't satisfactorily test the case of pathnames
>     containing spaces, because 'test_completion' gets the words on the
>     command line as a single argument and it uses space as word
>     separator.
>
>   - Some of the tests are protected by different FUNNYNAMES_* prereqs
>     depending on whether they put backslashes and double quotes or
>     separator characters (FS, GS, RS, US) in pathnames, although a
>     filesystem not allowing one likely doesn't allow the others
>     either.
>
>   - One of the tests operates on paths containing '|' and '&'
>     characters without being protected by a FUNNYNAMES prereq, but
>     some filesystems (notably on Windows) don't allow these characters
>     in pathnames, either.
>
> Replace these tests with basically equivalent, more focused tests that
> call __git_complete_index_file() directly.  Since this function only
> looks at the current word to be completed, i.e. the $cur variable, we
> can easily include pathnames containing spaces in the tests, so use
> such pathnames instead of pathnames containing '|' and '&'.  Finally,
> use only a single FUNNYNAMES prereq for all kinds of special
> characters.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>

Re: [PATCH 0/2] Test improvements for 'sg/complete-paths'

[Johannes Schindelin]

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

Hi Gábor,

On Fri, 18 May 2018, SZEDER Gábor wrote:

> > > So, I think for v2 I will rewrite these tests to call
> > > __git_complete_index_file() directly instead of using
> > > 'test_completion', and will include a test with spaces in path
> > > names.
> > 
> > Quite well thought-out reasoning.  Thanks.
> 
> Unfortunately I couldn't get around to it soon enough, and now the topic
> 'sg/complete-paths' is already in next, so here are those test
> improvements on top.

I can verify that the weeks-long breakage of `pu` on Windows has been
addressed, probably by this patch series.

Ciao,
Dscho

Re: [PATCH 2/2] t9902-completion: exercise __git_complete_index_file() directly

[Johannes Schindelin]

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

Hi Gábor,

On Fri, 18 May 2018, SZEDER Gábor wrote:

> The tests added in 2f271cd9cf (t9902-completion: add tests
> demonstrating issues with quoted pathnames, 2018-05-08) and in
> 2ab6eab4fe (completion: improve handling quoted paths in 'git
> ls-files's output, 2018-03-28) have a few shortcomings:
> 
>   - All these test use the 'test_completion' helper function, thus
>     they are exercising the whole completion machinery, although they
>     are only interested in how git-aware path completion, specifically
>     the __git_complete_index_file() function deals with unusual
>     characters in pathnames and on the command line.
> 
>   - These tests can't satisfactorily test the case of pathnames
>     containing spaces, because 'test_completion' gets the words on the
>     command line as a single argument and it uses space as word
>     separator.
> 
>   - Some of the tests are protected by different FUNNYNAMES_* prereqs
>     depending on whether they put backslashes and double quotes or
>     separator characters (FS, GS, RS, US) in pathnames, although a
>     filesystem not allowing one likely doesn't allow the others
>     either.
> 
>   - One of the tests operates on paths containing '|' and '&'
>     characters without being protected by a FUNNYNAMES prereq, but
>     some filesystems (notably on Windows) don't allow these characters
>     in pathnames, either.
> 
> Replace these tests with basically equivalent, more focused tests that
> call __git_complete_index_file() directly.  Since this function only
> looks at the current word to be completed, i.e. the $cur variable, we
> can easily include pathnames containing spaces in the tests, so use
> such pathnames instead of pathnames containing '|' and '&'.  Finally,
> use only a single FUNNYNAMES prereq for all kinds of special
> characters.

Makes sense.

> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 955932174c..1b6d275254 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1209,6 +1209,124 @@ test_expect_success 'teardown after ref completion' '
>  	git remote remove other
>  '
>  
> +
> +test_path_completion ()
> +{
> +	test $# = 2 || error "bug in the test script: not 2 parameters to test_path_completion"

Maybe shorten this to

	test $# = 2 || error "BUG: test_path_completion requires 2 parameters"

in order to keep to the 80 columns/line limit?

> +
> +	local cur="$1" expected="$2"

I thought `local` was a Bash-ism we tried to avoid in the test scripts.
But I guess this file is already littered with `local` keywords...

> +	echo "$expected" >expected &&
> +	(
> +		# In the following tests calling this function we only
> +		# care about how __git_complete_index_file() deals with
> +		# unusual characters in path names.  By requesting only
> +		# untracked files we dont have to bother adding any
> +		# paths to the index in those tests.
> +		__git_complete_index_file --others &&
> +		print_comp
> +	) &&
> +	test_cmp expected out
> +}
> +
> +test_expect_success 'setup for path completion tests' '
> +	mkdir simple-dir \
> +	      "spaces in dir" \
> +	      árvíztűrő &&
> +	touch simple-dir/simple-file \
> +	      "spaces in dir/spaces in file" \
> +	      "árvíztűrő/Сайн яваарай" &&
> +	if test_have_prereq !MINGW &&
> +	   mkdir BS\\dir \
> +		 '$'separators\034in\035dir'' &&
> +	   touch BS\\dir/DQ\"file \
> +		 '$'separators\034in\035dir/sep\036in\037file''
> +	then
> +		test_set_prereq FUNNYNAMES
> +	else
> +		rm -rf BS\\dir '$'separators\034in\035dir''
> +	fi
> +'
> +
> +test_expect_success '__git_complete_index_file - simple' '
> +	test_path_completion simple simple-dir &&  # Bash is supposed to
> +						   # add the trailing /.
> +	test_path_completion simple-dir/simple simple-dir/simple-file
> +'
> +
> +test_expect_success \
> +    '__git_complete_index_file - escaped characters on cmdline' '
> +	test_path_completion spac "spaces in dir" &&  # Bash will turn this
> +						      # into "spaces\ in\ dir"
> +	test_path_completion "spaces\\ i" \
> +			     "spaces in dir" &&
> +	test_path_completion "spaces\\ in\\ dir/s" \
> +			     "spaces in dir/spaces in file" &&
> +	test_path_completion "spaces\\ in\\ dir/spaces\\ i" \
> +			     "spaces in dir/spaces in file"
> +'
> +
> +test_expect_success \
> +    '__git_complete_index_file - quoted characters on cmdline' '
> +	# Testing with an opening but without a corresponding closing
> +	# double quote is important.
> +	test_path_completion \"spac "spaces in dir" &&
> +	test_path_completion "\"spaces i" \
> +			     "spaces in dir" &&
> +	test_path_completion "\"spaces in dir/s" \
> +			     "spaces in dir/spaces in file" &&
> +	test_path_completion "\"spaces in dir/spaces i" \
> +			     "spaces in dir/spaces in file"
> +'
> +
> +test_expect_success '__git_complete_index_file - UTF-8 in ls-files output' '
> +	test_path_completion á árvíztűrő &&
> +	test_path_completion árvíztűrő/С "árvíztűrő/Сайн яваарай"
> +'
> +
> +test_expect_success FUNNYNAMES \
> +    '__git_complete_index_file - C-style escapes in ls-files output' '
> +	test_path_completion BS \
> +			     BS\\dir &&
> +	test_path_completion BS\\\\d \
> +			     BS\\dir &&
> +	test_path_completion BS\\\\dir/DQ \
> +			     BS\\dir/DQ\"file &&
> +	test_path_completion BS\\\\dir/DQ\\\"f \
> +			     BS\\dir/DQ\"file
> +'
> +
> +test_expect_success FUNNYNAMES \
> +    '__git_complete_index_file - \nnn-escaped characters in ls-files output' '
> +	test_path_completion sep '$'separators\034in\035dir'' &&
> +	test_path_completion '$'separators\034i'' \
> +			     '$'separators\034in\035dir'' &&
> +	test_path_completion '$'separators\034in\035dir/sep'' \
> +			     '$'separators\034in\035dir/sep\036in\037file'' &&
> +	test_path_completion '$'separators\034in\035dir/sep\036i'' \
> +			     '$'separators\034in\035dir/sep\036in\037file''
> +'
> +
> +test_expect_success FUNNYNAMES \
> +    '__git_complete_index_file - removing repeated quoted path components' '
> +	test_when_finished rm -r repeated-quoted &&
> +	mkdir repeated-quoted &&      # A directory whose name in itself
> +				      # would not be quoted ...
> +	>repeated-quoted/0-file &&
> +	>repeated-quoted/1\"file &&   # ... but here the file makes the
> +				      # dirname quoted ...
> +	>repeated-quoted/2-file &&
> +	>repeated-quoted/3\"file &&   # ... and here, too.
> +
> +	# Still, we shold only list the directory name only once.
> +	test_path_completion repeated repeated-quoted
> +'
> +
> +test_expect_success 'teardown after path completion tests' '
> +	rm -rf simple-dir "spaces in dir" árvíztűrő \
> +	       BS\\dir '$'separators\034in\035dir''
> +'
> +
> +
>  test_expect_success '__git_get_config_variables' '
>  	cat >expect <<-EOF &&
>  	name-1
> @@ -1469,113 +1587,6 @@ test_expect_success 'complete files' '

It made it a bit awkward to review this patch that the code was
move-edited. In this case, I cannot blame exclusively the hostile review
environment that is an email reader, but also sadly, `git show --color-moved
7d314073488ae81b8f5cdecb4d00a78529fa7dc3` helped only *so* much
*almost-touches-thumb-with-first-finger*.

>  	test_completion "git add mom" "momified"
>  '
>  
> -# The next tests only care about how the completion script deals with
> -# unusual characters in path names.  By defining a custom completion
> -# function to list untracked files they won't be influenced by future
> -# changes of the completion functions of real git commands, and we
> -# don't have to bother with adding files to the index in these tests.

We should keep the corresponding new comment also outside the function, as
it talks about the following tests inside a twice-indented code comment
inside a subshell.

> -_git_test_path_comp ()
> -{
> -	__git_complete_index_file --others
> -}

A new test case was inserted here, in the move-edited section:
'__git_complete_index_file - simple'.

> -
> -test_expect_success 'complete files - escaped characters on cmdline' '
> -	test_when_finished "rm -rf \"New|Dir\"" &&
> -	mkdir "New|Dir" &&
> -	>"New|Dir/New&File.c" &&
> -
> -	test_completion "git test-path-comp N" \
> -			"New|Dir" &&	# Bash will turn this into "New\|Dir/"
> -	test_completion "git test-path-comp New\\|D" \
> -			"New|Dir" &&
> -	test_completion "git test-path-comp New\\|Dir/N" \
> -			"New|Dir/New&File.c" &&	# Bash will turn this into
> -						# "New\|Dir/New\&File.c "
> -	test_completion "git test-path-comp New\\|Dir/New\\&F" \
> -			"New|Dir/New&File.c"
> -'

This corresponds to the new '__git_complete_index_file - escaped
characters on cmdline' test case, which looks different by necessity: it
avoids funny file names by using spaces in file names instead.

From what I can see, the new version is indeed equivalent to the old
version.

> -
> -test_expect_success 'complete files - quoted characters on cmdline' '
> -	test_when_finished "rm -r \"New(Dir\"" &&
> -	mkdir "New(Dir" &&
> -	>"New(Dir/New)File.c" &&
> -
> -	# Testing with an opening but without a corresponding closing
> -	# double quote is important.
> -	test_completion "git test-path-comp \"New(D" "New(Dir" &&
> -	test_completion "git test-path-comp \"New(Dir/New)F" \
> -			"New(Dir/New)File.c"
> -'

The move-edited code is essentially testing the same, and then two more
conditions: in addition to testing with a prefix without a space, it also
tests with prefixes with a space (when trying to complete "spaces in dir",
it should not matter whether we start writing "spac<TAB> or "spaces
i<TAB>.

Good.

> -
> -test_expect_success 'complete files - UTF-8 in ls-files output' '
> -	test_when_finished "rm -r árvíztűrő" &&
> -	mkdir árvíztűrő &&
> -	>"árvíztűrő/Сайн яваарай" &&
> -
> -	test_completion "git test-path-comp á" "árvíztűrő" &&
> -	test_completion "git test-path-comp árvíztűrő/С" \
> -			"árvíztűrő/Сайн яваарай"
> -'

This one is identical to the move-edited code (modulo the
no-longer-necessary directory/file).

> -
> -# Testing with a path containing a backslash is important.
> -if test_have_prereq !MINGW &&
> -   mkdir 'New\Dir' 2>/dev/null &&
> -   touch 'New\Dir/New"File.c' 2>/dev/null
> -then
> -	test_set_prereq FUNNYNAMES_BS_DQ
> -else
> -	say "Your filesystem does not allow \\ and \" in filenames."
> -	rm -rf 'New\Dir'
> -fi
> -test_expect_success FUNNYNAMES_BS_DQ \
> -    'complete files - C-style escapes in ls-files output' '
> -	test_when_finished "rm -r \"New\\\\Dir\"" &&
> -
> -	test_completion "git test-path-comp N" "New\\Dir" &&
> -	test_completion "git test-path-comp New\\\\D" "New\\Dir" &&
> -	test_completion "git test-path-comp New\\\\Dir/N" \
> -			"New\\Dir/New\"File.c" &&
> -	test_completion "git test-path-comp New\\\\Dir/New\\\"F" \
> -			"New\\Dir/New\"File.c"
> -'

The move-edited code uses BS\dir instead of New\Dir.

> -
> -if test_have_prereq !MINGW &&
> -   mkdir $'New\034Special\035Dir' 2>/dev/null &&
> -   touch $'New\034Special\035Dir/New\036Special\037File' 2>/dev/null
> -then
> -	test_set_prereq FUNNYNAMES_SEPARATORS
> -else
> -	say 'Your filesystem does not allow special separator characters (FS, GS, RS, US) in filenames.'
> -	rm -rf $'New\034Special\035Dir'
> -fi
> -test_expect_success FUNNYNAMES_SEPARATORS \
> -    'complete files - \nnn-escaped control characters in ls-files output' '
> -	test_when_finished "rm -r '$'New\034Special\035Dir''" &&
> -
> -	# Note: these will be literal separator characters on the cmdline.
> -	test_completion "git test-path-comp N" "'$'New\034Special\035Dir''" &&
> -	test_completion "git test-path-comp '$'New\034S''" \
> -			"'$'New\034Special\035Dir''" &&
> -	test_completion "git test-path-comp '$'New\034Special\035Dir/''" \
> -			"'$'New\034Special\035Dir/New\036Special\037File''" &&
> -	test_completion "git test-path-comp '$'New\034Special\035Dir/New\036S''" \
> -			"'$'New\034Special\035Dir/New\036Special\037File''"
> -'

The move-edited code uses the file name
`$separators<FS>in<GS>dir/sep<RS>in<US>file` instead of
`$New<FS>Special<GC>Dir/New<RS>Special<US>File`, but is otherwise
identical.

Too many differences for the --color-moved code to catch, though.

> -
> -test_expect_success FUNNYNAMES_BS_DQ \
> -    'complete files - removing repeated quoted path components' '
> -	test_when_finished rm -rf NewDir &&
> -	mkdir NewDir &&    # A dirname which in itself would not be quoted ...
> -	>NewDir/0-file &&
> -	>NewDir/1\"file && # ... but here the file makes the dirname quoted ...
> -	>NewDir/2-file &&
> -	>NewDir/3\"file && # ... and here, too.
> -
> -	# Still, we should only list it once.
> -	test_completion "git test-path-comp New" "NewDir"
> -'

The move-edited code uses `repeated` instead of `New` and
`repeated-quoted` instead of `NewDir`. I could not spot any other
differences.

Of course, `--color-moved` had no chance here, either.

> -
> -
>  test_expect_success "completion uses <cmd> completion for alias: !sh -c 'git <cmd> ...'" '
>  	test_config alias.co "!sh -c '"'"'git checkout ...'"'"'" &&
>  	test_completion "git co m" <<-\EOF

The move-edited code needed to insert a cleanup step at the end, because
the new directories and files need to live for more than one test case
(therefore `test_when_finished` is out of the game).

Note to self: should we ever switch to a modern test framework that allows
parallelizing tests, then these test cases need to be marked up with
@Before and @After to create/delete those directories/files.

Even if it was hard to review, I think this patch is essentially good, at
least after fixing the typo pointed out by Eric and then shortening the
long line.

Thank you for keeping on the ball!
Dscho

Re: [PATCH 0/2] Test improvements for 'sg/complete-paths'

[Johannes Schindelin]

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

Hi Junio,

On Mon, 21 May 2018, Johannes Schindelin wrote:

> On Fri, 18 May 2018, SZEDER Gábor wrote:
> 
> > > > So, I think for v2 I will rewrite these tests to call
> > > > __git_complete_index_file() directly instead of using
> > > > 'test_completion', and will include a test with spaces in path
> > > > names.
> > > 
> > > Quite well thought-out reasoning.  Thanks.
> > 
> > Unfortunately I couldn't get around to it soon enough, and now the topic
> > 'sg/complete-paths' is already in next, so here are those test
> > improvements on top.
> 
> I can verify that the weeks-long breakage of `pu` on Windows has been
> addressed, probably by this patch series.

Please note that as the branch that is fixed by these two patches was
already merged down to `next`, the Continuous Testing on Windows reports
that `next` is broken.

(I saw that breakage for over a week, but was too busy elsewhere to act on
it.)

It would be really lovely to see it fixed soon, so that other bugs cannot
be hidden by that breakage.

And I would also love to see sg/complete-paths to be merged down to
`master` *only* in conjunction with these two patches, not on its own
(because that would break the Continuous Testing on Windows of `master`,
which is something I really want us to avoid).

Thanks,
Dscho