Re: [PATCH] read-cache: disable renames in add_files_to_cache

[Junio C Hamano <gitster@pobox.com>]

blindmansion <blindmansion@gmail.com> writes:

> add_files_to_cache() refreshes the index from worktree changes and does
> not need rename detection. When unmerged entries and a deleted stage-0
> path are present together, rename detection can pair them and rewrite an
> unmerged diff pair to point at the deleted path.
>
> That later makes "git commit -a" and "git add -u" try to stat the
> deleted path and die with "unable to stat". Disable rename detection in
> this callback-driven staging path and add a regression test covering the
> crash.
> ---

Thanks for trying to help.

 * Documentation/SubmittingPatches::[[sign-off]]
 * Documentation/SubmittingPatches::[[real-name]]

Other than that, the contents of the proposed log message looks
perfect, which is rarely seen in a patch from somebody whose patch
we haven't seen around here yet.

> diff --git a/read-cache.c b/read-cache.c
> index 5049f9b..d938abc 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -4049,6 +4049,7 @@ int add_files_to_cache(struct repository *repo, const char *prefix,
>  	rev.diffopt.format_callback = update_callback;
>  	rev.diffopt.format_callback_data = &data;
>  	rev.diffopt.flags.override_submodule_config = 1;
> +	rev.diffopt.detect_rename = 0; /* staging worktree changes does not need renames */
>  	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
>  

The change itself looks sensible, but it makes me wonder if there
are other very similar glitches lying around.

> diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
> index 06e83d3..56c7e55 100755
> --- a/t/t2200-add-update.sh
> +++ b/t/t2200-add-update.sh
> @@ -200,6 +200,45 @@ test_expect_success 'add -u resolves unmerged paths' '
>  	test_cmp expect actual
>  '

I didn't see too many questionable steps in the new test, other than
the mixed use of "perl" plus "sed".

 - Can't we do this without relying on "perl"?  Also, why does those
   invocations of Perl insist on using single quotes, which is more
   awkward to write in our test framework, where double quotes should
   work equally well?

 - Can't the sample line be shortened so that patch fits comfortably
   on 80-column terminals?

 - Insisting on "add -u" showing nothing on its standard error
   stream makes the test more fragile than it is necessary.  We
   cannot dictate that nobody in the future will add progress
   eye-candy to the codepath and some folks run the test on overly
   loaded machine.  Checking that the command exits with status 0 is
   a good thing to do (which is already done here) and should be
   sufficient in this case.

 - Wouldn't running a single "git ls-files" with pathspec and
   comparing it with expected list of contents in the index make it
   more clear what is being tested and expected in the last part of
   the test where we see no bystander.txt is mentioned and
   conflict.txt is mentioned there?  I.e., something like

		git ls-files bystander.txt conflict.txt >actual &&
		cat >expect <<\-EOF &&
		conflict.txt
		EOF
		test_cmp expect actual

> +test_expect_success 'add -u avoids rename pairing on unmerged paths' '
> +	test_create_repo rename-crash &&
> +	(
> +		cd rename-crash &&
> +		test_seq 1 100 |
> +		sed "s/.*/line &: shared content that is identical across both files/" >conflict.txt &&
> +		cp conflict.txt bystander.txt &&
> +		git add conflict.txt bystander.txt &&
> +		git commit -m "initial: two files with identical content" &&
> +		main_branch=$(git symbolic-ref --short HEAD) &&
> +		git checkout -b feature &&
> +		perl -pe '\''s/^line 50:.*/line 50: FEATURE BRANCH CHANGE/'\'' \
> +			conflict.txt >conflict.txt.tmp &&
> +		mv conflict.txt.tmp conflict.txt &&
> +		git add conflict.txt &&
> +		git commit -m "feature: modify line 50" &&
> +		git checkout "$main_branch" &&
> +		perl -pe '\''s/^line 50:.*/line 50: MAIN BRANCH CHANGE/'\'' \
> +			conflict.txt >conflict.txt.tmp &&
> +		mv conflict.txt.tmp conflict.txt &&
> +		git add conflict.txt &&
> +		git commit -m "main: modify line 50 differently" &&
> +		test_must_fail git merge feature &&
> +		rm bystander.txt &&
> +		git add -u >out 2>err &&
> +		test_must_be_empty out &&
> +		test_must_be_empty err &&
> +		git ls-files -u >actual &&
> +		test_must_be_empty actual &&
> +		git ls-files bystander.txt >actual &&
> +		test_must_be_empty actual &&
> +		echo conflict.txt >expect &&
> +		git ls-files conflict.txt >actual &&
> +		test_cmp expect actual &&
> +		git diff-files --name-only >actual &&
> +		test_must_be_empty actual
> +	)
> +'
> +
>  test_expect_success '"add -u non-existent" should fail' '
>  	test_must_fail git add -u non-existent &&
>  	git ls-files >actual &&