Re: [PATCH] add--interactive: do not expand pathspecs with ls-files

From: Brandon Williams <bmwill@google.com>
Date: Mar 14, 2017, 17:25 UTC
Message-ID: <20170314172504.GA102212@google.com>
In-Reply-To: <20170314163024.et2cp5e3imrrrf7k@sigill.intra.peff.net>

On 03/14, Jeff King wrote:

Show 14 quoted lines

>   b. If the repository contains filenames with literal wildcard
>      characters (e.g., "foo*"), the original code expanded
>      them via "ls-files" and then fed those wildcard names
>      to "diff-index", which would have treated them as
>      wildcards. This was a bug, which is now fixed (though
>      unless you really go through some contortions with
>      ":(literal)", it's likely that your original pathspec
>      would match whatever the accidentally-expanded wildcard
>      would anyway).
> 
>      So this takes us one step closer to working correctly
>      with files whose names contain wildcard characters, but
>      it's likely that others remain (e.g., if "git add -i"
>      feeds the selected paths to "git add").

It definitely makes things more difficult when people use wildcard characters in filenames. Is there any reason people use wildcards as literal characters in filenames other than them not knowing any better?

Show 32 quoted lines

> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index f5c816e27..77b4ed53a 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -275,20 +275,11 @@ sub list_modified {
>  	my ($only) = @_;
>  	my (%data, @return);
>  	my ($add, $del, $adddel, $file);
> -	my @tracked = ();
> -
> -	if (@ARGV) {
> -		@tracked = map {
> -			chomp $_;
> -			unquote_path($_);
> -		} run_cmd_pipe(qw(git ls-files --), @ARGV);
> -		return if (!@tracked);
> -	}
>  
>  	my $reference = get_diff_reference($patch_mode_revision);
>  	for (run_cmd_pipe(qw(git diff-index --cached
>  			     --numstat --summary), $reference,
> -			     '--', @tracked)) {
> +			     '--', @ARGV)) {
>  		if (($add, $del, $file) =
>  		    /^([-\d]+)	([-\d]+)	(.*)/) {
>  			my ($change, $bin);
> @@ -313,7 +304,7 @@ sub list_modified {
>  		}
>  	}
>  
> -	for (run_cmd_pipe(qw(git diff-files --numstat --summary --raw --), @tracked)) {
> +	for (run_cmd_pipe(qw(git diff-files --numstat --summary --raw --), @ARGV)) {

Such a small and easy change for that big of a perf win.

Show 55 quoted lines

>  		if (($add, $del, $file) =
>  		    /^([-\d]+)	([-\d]+)	(.*)/) {
>  			$file = unquote_path($file);
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index aaa258daa..f9528fa00 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -412,4 +412,47 @@ test_expect_success 'patch-mode via -i prompts for files' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'add -p handles globs' '
> +	git reset --hard &&
> +
> +	mkdir -p subdir &&
> +	echo base >one.c &&
> +	echo base >subdir/two.c &&
> +	git add "*.c" &&
> +	git commit -m base &&
> +
> +	echo change >one.c &&
> +	echo change >subdir/two.c &&
> +	git add -p "*.c" <<-\EOF &&
> +	y
> +	y
> +	EOF
> +
> +	cat >expect <<-\EOF &&
> +	one.c
> +	subdir/two.c
> +	EOF
> +	git diff --cached --name-only >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'add -p does not expand argument lists' '
> +	git reset --hard &&
> +
> +	echo content >not-changed &&
> +	git add not-changed &&
> +	git commit -m "add not-changed file" &&
> +
> +	echo change >file &&
> +	GIT_TRACE=$(pwd)/trace.out git add -p . <<-\EOF &&
> +	y
> +	EOF
> +
> +	# we know that "file" must be mentioned since we actually
> +	# update it, but we want to be sure that our "." pathspec
> +	# was not expanded into the argument list of any command.
> +	# So look only for "not-changed".
> +	! grep not-changed trace.out
> +'
> +
>  test_done

Tests look sane to me.

-- 
Brandon Williams