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