* [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames
2018-04-16 22:41 ` [PATCH 00/11] completion: path completion improvements: speedup and quoted paths SZEDER Gábor
@ 2018-04-16 22:41 ` SZEDER Gábor
2018-04-17 3:48 ` Junio C Hamano
2018-04-18 12:31 ` [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames Johannes Schindelin
2018-04-16 22:41 ` [PATCH 02/11] completion: move __git_complete_index_file() next to its helpers SZEDER Gábor
` (8 subsequent siblings)
9 siblings, 2 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-16 22:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Clemens Buchacher, Johannes Schindelin,
Manlio Perillo, 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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames
2018-04-16 22:41 ` [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames SZEDER Gábor
@ 2018-04-17 3:48 ` Junio C Hamano
2018-04-17 23:32 ` SZEDER Gábor
2018-04-18 12:31 ` [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames Johannes Schindelin
1 sibling, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2018-04-17 3:48 UTC (permalink / raw)
To: SZEDER Gábor
Cc: git, Clemens Buchacher, Johannes Schindelin, Manlio Perillo
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?
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames
2018-04-17 3:48 ` Junio C Hamano
@ 2018-04-17 23:32 ` SZEDER Gábor
2018-04-17 23:41 ` SZEDER Gábor
2018-04-18 1:22 ` Junio C Hamano
0 siblings, 2 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-17 23:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git mailing list, Clemens Buchacher, Johannes Schindelin, Manlio Perillo
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames
2018-04-17 23:32 ` SZEDER Gábor
@ 2018-04-17 23:41 ` SZEDER Gábor
2018-04-18 1:22 ` Junio C Hamano
1 sibling, 0 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-17 23:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git mailing list, Clemens Buchacher, Johannes Schindelin, Manlio Perillo
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames
2018-04-17 23:32 ` SZEDER Gábor
2018-04-17 23:41 ` SZEDER Gábor
@ 2018-04-18 1:22 ` Junio C Hamano
2018-04-26 0:25 ` SZEDER Gábor
1 sibling, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2018-04-18 1:22 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Git mailing list, Clemens Buchacher, Johannes Schindelin, Manlio Perillo
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames
2018-04-18 1:22 ` Junio C Hamano
@ 2018-04-26 0:25 ` SZEDER Gábor
2018-04-26 2:11 ` Junio C Hamano
0 siblings, 1 reply; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-26 0:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git mailing list, Clemens Buchacher, Johannes Schindelin, Manlio Perillo
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames
2018-04-26 0:25 ` SZEDER Gábor
@ 2018-04-26 2:11 ` Junio C Hamano
2018-05-18 14:17 ` [PATCH 0/2] Test improvements for 'sg/complete-paths' SZEDER Gábor
0 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2018-04-26 2:11 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Git mailing list, Clemens Buchacher, Johannes Schindelin, Manlio Perillo
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 0/2] Test improvements for 'sg/complete-paths'
2018-04-26 2:11 ` Junio C Hamano
@ 2018-05-18 14:17 ` SZEDER Gábor
2018-05-18 14:17 ` [PATCH 1/2] completion: don't return with error from __gitcomp_file_direct() SZEDER Gábor
` (2 more replies)
0 siblings, 3 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-05-18 14:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Clemens Buchacher, Johannes Schindelin, Manlio Perillo,
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/2] completion: don't return with error from __gitcomp_file_direct()
2018-05-18 14:17 ` [PATCH 0/2] Test improvements for 'sg/complete-paths' SZEDER Gábor
@ 2018-05-18 14:17 ` SZEDER Gábor
2018-05-18 14:17 ` [PATCH 2/2] t9902-completion: exercise __git_complete_index_file() directly SZEDER Gábor
2018-05-21 11:35 ` [PATCH 0/2] Test improvements for 'sg/complete-paths' Johannes Schindelin
2 siblings, 0 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-05-18 14:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Clemens Buchacher, Johannes Schindelin, Manlio Perillo,
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 2/2] t9902-completion: exercise __git_complete_index_file() directly
2018-05-18 14:17 ` [PATCH 0/2] Test improvements for 'sg/complete-paths' SZEDER Gábor
2018-05-18 14:17 ` [PATCH 1/2] completion: don't return with error from __gitcomp_file_direct() SZEDER Gábor
@ 2018-05-18 14:17 ` SZEDER Gábor
2018-05-18 19:25 ` Eric Sunshine
2018-05-21 12:14 ` Johannes Schindelin
2018-05-21 11:35 ` [PATCH 0/2] Test improvements for 'sg/complete-paths' Johannes Schindelin
2 siblings, 2 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-05-18 14:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Clemens Buchacher, Johannes Schindelin, Manlio Perillo,
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] t9902-completion: exercise __git_complete_index_file() directly
2018-05-18 14:17 ` [PATCH 2/2] t9902-completion: exercise __git_complete_index_file() directly SZEDER Gábor
@ 2018-05-18 19:25 ` Eric Sunshine
2018-05-21 12:14 ` Johannes Schindelin
1 sibling, 0 replies; 36+ messages in thread
From: Eric Sunshine @ 2018-05-18 19:25 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Junio C Hamano, Git List, Clemens Buchacher, Johannes Schindelin,
Manlio Perillo
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>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] t9902-completion: exercise __git_complete_index_file() directly
2018-05-18 14:17 ` [PATCH 2/2] t9902-completion: exercise __git_complete_index_file() directly SZEDER Gábor
2018-05-18 19:25 ` Eric Sunshine
@ 2018-05-21 12:14 ` Johannes Schindelin
1 sibling, 0 replies; 36+ messages in thread
From: Johannes Schindelin @ 2018-05-21 12:14 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Junio C Hamano, git, Clemens Buchacher, Manlio Perillo
[-- 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/2] Test improvements for 'sg/complete-paths'
2018-05-18 14:17 ` [PATCH 0/2] Test improvements for 'sg/complete-paths' SZEDER Gábor
2018-05-18 14:17 ` [PATCH 1/2] completion: don't return with error from __gitcomp_file_direct() SZEDER Gábor
2018-05-18 14:17 ` [PATCH 2/2] t9902-completion: exercise __git_complete_index_file() directly SZEDER Gábor
@ 2018-05-21 11:35 ` Johannes Schindelin
2018-05-21 12:17 ` Johannes Schindelin
2 siblings, 1 reply; 36+ messages in thread
From: Johannes Schindelin @ 2018-05-21 11:35 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Junio C Hamano, git, Clemens Buchacher, Manlio Perillo
[-- 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/2] Test improvements for 'sg/complete-paths'
2018-05-21 11:35 ` [PATCH 0/2] Test improvements for 'sg/complete-paths' Johannes Schindelin
@ 2018-05-21 12:17 ` Johannes Schindelin
0 siblings, 0 replies; 36+ messages in thread
From: Johannes Schindelin @ 2018-05-21 12:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: SZEDER Gábor, git, Clemens Buchacher, Manlio Perillo
[-- 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames
2018-04-16 22:41 ` [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames SZEDER Gábor
2018-04-17 3:48 ` Junio C Hamano
@ 2018-04-18 12:31 ` Johannes Schindelin
2018-04-19 19:08 ` SZEDER Gábor
1 sibling, 1 reply; 36+ messages in thread
From: Johannes Schindelin @ 2018-04-18 12:31 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano, Clemens Buchacher, Manlio Perillo
[-- 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames
2018-04-18 12:31 ` [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames Johannes Schindelin
@ 2018-04-19 19:08 ` SZEDER Gábor
0 siblings, 0 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-19 19:08 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Git mailing list, Junio C Hamano, Clemens Buchacher, Manlio Perillo
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)
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 02/11] completion: move __git_complete_index_file() next to its helpers
2018-04-16 22:41 ` [PATCH 00/11] completion: path completion improvements: speedup and quoted paths SZEDER Gábor
2018-04-16 22:41 ` [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames SZEDER Gábor
@ 2018-04-16 22:41 ` SZEDER Gábor
2018-04-16 22:41 ` [PATCH 03/11] completion: simplify prefix path component handling during path completion SZEDER Gábor
` (7 subsequent siblings)
9 siblings, 0 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-16 22:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Clemens Buchacher, Johannes Schindelin,
Manlio Perillo, 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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 03/11] completion: simplify prefix path component handling during path completion
2018-04-16 22:41 ` [PATCH 00/11] completion: path completion improvements: speedup and quoted paths SZEDER Gábor
2018-04-16 22:41 ` [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames SZEDER Gábor
2018-04-16 22:41 ` [PATCH 02/11] completion: move __git_complete_index_file() next to its helpers SZEDER Gábor
@ 2018-04-16 22:41 ` SZEDER Gábor
2018-04-16 22:41 ` [PATCH 04/11] completion: support completing non-ASCII pathnames SZEDER Gábor
` (6 subsequent siblings)
9 siblings, 0 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-16 22:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Clemens Buchacher, Johannes Schindelin,
Manlio Perillo, 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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 04/11] completion: support completing non-ASCII pathnames
2018-04-16 22:41 ` [PATCH 00/11] completion: path completion improvements: speedup and quoted paths SZEDER Gábor
` (2 preceding siblings ...)
2018-04-16 22:41 ` [PATCH 03/11] completion: simplify prefix path component handling during path completion SZEDER Gábor
@ 2018-04-16 22:41 ` SZEDER Gábor
2018-04-16 22:41 ` [PATCH 05/11] completion: improve handling quoted paths on the command line SZEDER Gábor
` (5 subsequent siblings)
9 siblings, 0 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-16 22:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Clemens Buchacher, Johannes Schindelin,
Manlio Perillo, 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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 05/11] completion: improve handling quoted paths on the command line
2018-04-16 22:41 ` [PATCH 00/11] completion: path completion improvements: speedup and quoted paths SZEDER Gábor
` (3 preceding siblings ...)
2018-04-16 22:41 ` [PATCH 04/11] completion: support completing non-ASCII pathnames SZEDER Gábor
@ 2018-04-16 22:41 ` SZEDER Gábor
2018-04-16 22:41 ` [PATCH 06/11] completion: let 'ls-files' and 'diff-index' filter matching paths SZEDER Gábor
` (4 subsequent siblings)
9 siblings, 0 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-16 22:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Clemens Buchacher, Johannes Schindelin,
Manlio Perillo, 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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 06/11] completion: let 'ls-files' and 'diff-index' filter matching paths
2018-04-16 22:41 ` [PATCH 00/11] completion: path completion improvements: speedup and quoted paths SZEDER Gábor
` (4 preceding siblings ...)
2018-04-16 22:41 ` [PATCH 05/11] completion: improve handling quoted paths on the command line SZEDER Gábor
@ 2018-04-16 22:41 ` SZEDER Gábor
2018-04-16 22:41 ` [PATCH 07/11] completion: use 'awk' to strip trailing path components SZEDER Gábor
` (3 subsequent siblings)
9 siblings, 0 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-16 22:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Clemens Buchacher, Johannes Schindelin,
Manlio Perillo, 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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 07/11] completion: use 'awk' to strip trailing path components
2018-04-16 22:41 ` [PATCH 00/11] completion: path completion improvements: speedup and quoted paths SZEDER Gábor
` (5 preceding siblings ...)
2018-04-16 22:41 ` [PATCH 06/11] completion: let 'ls-files' and 'diff-index' filter matching paths SZEDER Gábor
@ 2018-04-16 22:41 ` SZEDER Gábor
2018-04-16 22:41 ` [PATCH 08/11] t9902-completion: ignore COMPREPLY element order in some tests SZEDER Gábor
` (2 subsequent siblings)
9 siblings, 0 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-16 22:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Clemens Buchacher, Johannes Schindelin,
Manlio Perillo, 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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 08/11] t9902-completion: ignore COMPREPLY element order in some tests
2018-04-16 22:41 ` [PATCH 00/11] completion: path completion improvements: speedup and quoted paths SZEDER Gábor
` (6 preceding siblings ...)
2018-04-16 22:41 ` [PATCH 07/11] completion: use 'awk' to strip trailing path components SZEDER Gábor
@ 2018-04-16 22:41 ` SZEDER Gábor
2018-04-16 22:41 ` [PATCH 09/11] completion: remove repeated dirnames with 'awk' during path completion SZEDER Gábor
2018-04-16 22:42 ` [PATCH 10/11] completion: improve handling quoted paths in 'git ls-files's output SZEDER Gábor
9 siblings, 0 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-16 22:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Clemens Buchacher, Johannes Schindelin,
Manlio Perillo, 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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 09/11] completion: remove repeated dirnames with 'awk' during path completion
2018-04-16 22:41 ` [PATCH 00/11] completion: path completion improvements: speedup and quoted paths SZEDER Gábor
` (7 preceding siblings ...)
2018-04-16 22:41 ` [PATCH 08/11] t9902-completion: ignore COMPREPLY element order in some tests SZEDER Gábor
@ 2018-04-16 22:41 ` SZEDER Gábor
2018-04-16 22:42 ` [PATCH 10/11] completion: improve handling quoted paths in 'git ls-files's output SZEDER Gábor
9 siblings, 0 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-16 22:41 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Clemens Buchacher, Johannes Schindelin,
Manlio Perillo, 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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 10/11] completion: improve handling quoted paths in 'git ls-files's output
2018-04-16 22:41 ` [PATCH 00/11] completion: path completion improvements: speedup and quoted paths SZEDER Gábor
` (8 preceding siblings ...)
2018-04-16 22:41 ` [PATCH 09/11] completion: remove repeated dirnames with 'awk' during path completion SZEDER Gábor
@ 2018-04-16 22:42 ` SZEDER Gábor
2018-04-16 22:42 ` [PATCH 11/11] completion: fill COMPREPLY directly when completing paths SZEDER Gábor
9 siblings, 1 reply; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-16 22:42 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Clemens Buchacher, Johannes Schindelin,
Manlio Perillo, 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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 11/11] completion: fill COMPREPLY directly when completing paths
2018-04-16 22:42 ` [PATCH 10/11] completion: improve handling quoted paths in 'git ls-files's output SZEDER Gábor
@ 2018-04-16 22:42 ` SZEDER Gábor
0 siblings, 0 replies; 36+ messages in thread
From: SZEDER Gábor @ 2018-04-16 22:42 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Clemens Buchacher, Johannes Schindelin,
Manlio Perillo, 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
^ permalink raw reply related [flat|nested] 36+ messages in thread