Re: [RFC PATCH] unpack-trees.c: handle empty deleted ita files

["René Scharfe" <l.s.r@web.de>]

Am 13.08.19 um 18:03 schrieb Varun Naik:
> It is possible to delete a committed file from the index and then add it
> as intent-to-add. Several variations of "reset" and "checkout" should
> resurrect the file in the index from HEAD. "merge", "cherry-pick", and
> "revert" should all fail with an error message. This patch provides the
> desired behavior even when the file is empty in HEAD.
>
> The affected commands all compare two cache entries by calling
> unpack-trees.c:same(). A cache entry for an ita file and a cache entry
> for an empty file have the same oid. So, the cache entry for an empty
> deleted ita file was previously considered the "same" as the cache entry
> for an empty file. This fix adds a comparison of the intent-to-add bits
> so that the two cache entries are no longer considered the "same".
>
> Signed-off-by: Varun Naik <vcnaik94@gmail.com>
> ---
> I am marking this patch as RFC because it is changing code deep in
> unpack-trees.c, and I'm sure it will generate some controversy :)

Lacking experience with intent-to-add I don't see why this would be
controversial.  Copying Duy and quoting in full, as he might have more
to say on that topic.

I have just one silly question below.

>
> The affected "reset" and "checkout" commands call
> unpack-trees.c:oneway_merge(), which calls same(). The affected "merge",
> "cherry-pick", and "revert" commands call
> unpack-trees.c:threeway_merge(), which calls same(). "stash" also calls
> oneway_merge(), and "rebase" also calls threeway_merge(), but they are
> not included in the test cases because their behaviors have not changed.
>
> The new tests are not comprehensive. In particular, they don't call
> plumbing commands, such as "read-tree". But hopefully they provide
> enough coverage to prevent most regressions.
>
> The new test cases for "cherry-pick" and "revert" grep for the single
> word "overwritten" rather than a more precise error message because the
> error message for an empty deleted ita file changes slightly if the
> patch in [0] is also applied. In retrospect, the commands affected by
> [0], [1], and this patch were all intertwined, and it would have been
> better to create a single large patch instead of three smaller patches.
>
> [0]: https://public-inbox.org/git/20190801161558.12838-1-vcnaik94@gmail.com/
> [1]: https://public-inbox.org/git/20190802162852.14498-1-vcnaik94@gmail.com/
>
>  t/t3030-merge-recursive.sh    | 25 +++++++++++++++---
>  t/t3501-revert-cherry-pick.sh | 49 ++++++++++++++++++++++++++++++++++-
>  t/t7104-reset-hard.sh         | 11 ++++++++
>  t/t7110-reset-merge.sh        | 31 ++++++++++++++++++++++
>  t/t7201-co.sh                 | 12 +++++++++
>  unpack-trees.c                |  1 +
>  6 files changed, 125 insertions(+), 4 deletions(-)
>
> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
> index ff641b348a..8aebb829a6 100755
> --- a/t/t3030-merge-recursive.sh
> +++ b/t/t3030-merge-recursive.sh
> @@ -303,13 +303,32 @@ test_expect_success 'fail if the index has unresolved entries' '
>  	git checkout -f "$c1" &&
>
>  	test_must_fail git merge "$c5" &&
> -	test_must_fail git merge "$c5" 2> out &&
> +	test_must_fail git merge "$c5" 2>out &&
>  	test_i18ngrep "not possible because you have unmerged files" out &&
>  	git add -u &&
> -	test_must_fail git merge "$c5" 2> out &&
> +	test_must_fail git merge "$c5" 2>out &&
>  	test_i18ngrep "You have not concluded your merge" out &&
>  	rm -f .git/MERGE_HEAD &&
> -	test_must_fail git merge "$c5" 2> out &&
> +	test_must_fail git merge "$c5" 2>out &&
> +	test_i18ngrep "Your local changes to the following files would be overwritten by merge:" out
> +'
> +
> +test_expect_success 'fail if a deleted intent-to-add file exists in the index' '
> +	git checkout -f "$c1" &&
> +	echo "nonempty" >nonempty &&
> +	git add nonempty &&
> +	git commit -m "create file to be deleted" &&
> +	git rm --cached nonempty &&
> +	git add -N nonempty &&
> +	test_must_fail git merge "$c5" 2>out &&
> +	test_i18ngrep "Your local changes to the following files would be overwritten by merge:" out &&
> +	git checkout -f "$c1" &&
> +	>empty &&
> +	git add empty &&
> +	git commit -m "create file to be deleted" &&
> +	git rm --cached empty &&
> +	git add -N empty &&
> +	test_must_fail git merge "$c5" 2>out &&
>  	test_i18ngrep "Your local changes to the following files would be overwritten by merge:" out
>  '
>
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index d1c68af8c5..45d816fc0c 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -91,16 +91,63 @@ test_expect_success 'cherry-pick on stat-dirty working tree' '
>  	)
>  '
>
> -test_expect_success 'revert forbidden on dirty working tree' '
> +test_expect_success 'cherry-pick forbidden on dirty working tree' '
>
> +	git checkout -b temp &&
>  	echo content >extra_file &&
>  	git add extra_file &&
> +	test_must_fail git cherry-pick rename2 2>errors &&
> +	test_i18ngrep "your local changes would be overwritten by " errors
> +
> +'
> +
> +test_expect_success 'revert forbidden on dirty working tree' '
> +
>  	test_must_fail git revert HEAD 2>errors &&
>  	test_i18ngrep "your local changes would be overwritten by " errors
>
>  '
>
> +test_expect_success 'cherry-pick fails if a deleted intent-to-add file exists in the index' '
> +	git reset --hard rename1 &&
> +	echo "nonempty" >nonempty &&
> +	git add nonempty &&
> +	git commit -m "create file to be deleted" &&
> +	git rm --cached nonempty &&
> +	git add -N nonempty &&
> +	test_must_fail git cherry-pick rename2 2>errors &&
> +	test_i18ngrep "overwritten" errors &&
> +	git reset --hard rename1 &&
> +	>empty &&
> +	git add empty &&
> +	git commit -m "create file to be deleted" &&
> +	git rm --cached empty &&
> +	git add -N empty &&
> +	test_must_fail git cherry-pick rename2 2>errors &&
> +	test_i18ngrep "overwritten" errors
> +'
> +
> +test_expect_success 'revert fails if a deleted intent-to-add file exists in the index' '
> +	git reset --hard rename1 &&
> +	echo "nonempty" >nonempty &&
> +	git add nonempty &&
> +	git commit -m "create file to be deleted" &&
> +	git rm --cached nonempty &&
> +	git add -N nonempty &&
> +	test_must_fail git revert rename2 2>errors &&
> +	test_i18ngrep "overwritten" errors &&
> +	git reset --hard rename1 &&
> +	>empty &&
> +	git add empty &&
> +	git commit -m "create file to be deleted" &&
> +	git rm --cached empty &&
> +	git add -N empty &&
> +	test_must_fail git revert rename2 2>errors &&
> +	test_i18ngrep "overwritten" errors
> +'
> +
>  test_expect_success 'cherry-pick on unborn branch' '
> +	git checkout -f rename1 &&
>  	git checkout --orphan unborn &&
>  	git rm --cached -r . &&
>  	rm -rf * &&
> diff --git a/t/t7104-reset-hard.sh b/t/t7104-reset-hard.sh
> index 16faa07813..96a0b779e7 100755
> --- a/t/t7104-reset-hard.sh
> +++ b/t/t7104-reset-hard.sh
> @@ -43,4 +43,15 @@ test_expect_success 'reset --hard did not corrupt index or cached-tree' '
>
>  '
>
> +test_expect_success 'reset --hard adds deleted intent-to-add file back to index' '
> +	echo "nonempty" >nonempty &&
> +	>empty &&
> +	git add nonempty empty &&
> +	git commit -m "create files to be deleted" &&
> +	git rm --cached nonempty empty &&
> +	git add -N nonempty empty &&
> +	git reset --hard &&
> +	git diff --cached --exit-code nonempty empty
> +'
> +
>  test_done
> diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
> index a82a07a04a..3346759375 100755
> --- a/t/t7110-reset-merge.sh
> +++ b/t/t7110-reset-merge.sh
> @@ -292,4 +292,35 @@ test_expect_success '--keep fails with added/deleted merge' '
>      test_i18ngrep "middle of a merge" err.log
>  '
>
> +test_expect_success 'reset --merge adds deleted intent-to-add file back to index' '
> +    git reset --hard initial &&
> +    echo "nonempty" >nonempty &&
> +    git add nonempty &&
> +    git commit -m "create file to be deleted" &&
> +    git rm --cached nonempty &&
> +    git add -N nonempty &&
> +    test_must_fail git reset --merge HEAD 2>err.log &&
> +    grep nonempty err.log | grep "not uptodate" &&
> +    git reset --hard initial &&
> +    >empty &&
> +    git add empty &&
> +    git commit -m "create file to be deleted" &&
> +    git rm --cached empty &&
> +    git add -N empty &&
> +    test_must_fail git reset --merge HEAD 2>err.log &&
> +    grep empty err.log | grep "not uptodate"
> +'
> +
> +test_expect_success 'reset --keep adds deleted intent-to-add file back to index' '
> +    git reset --hard initial &&
> +    echo "nonempty" >nonempty &&
> +    >empty &&
> +    git add nonempty empty &&
> +    git commit -m "create files to be deleted" &&
> +    git rm --cached nonempty empty &&
> +    git add -N nonempty empty &&
> +    git reset --keep HEAD &&
> +    git diff --cached --exit-code nonempty empty
> +'
> +
>  test_done
> diff --git a/t/t7201-co.sh b/t/t7201-co.sh
> index 5990299fc9..4c0c33ce33 100755
> --- a/t/t7201-co.sh
> +++ b/t/t7201-co.sh
> @@ -674,4 +674,16 @@ test_expect_success 'custom merge driver with checkout -m' '
>  	test_cmp expect arm
>  '
>
> +test_expect_success 'checkout -f HEAD adds deleted intent-to-add file back to index' '
> +	git reset --hard master &&
> +	echo "nonempty" >nonempty &&
> +	>empty &&
> +	git add nonempty empty &&
> +	git commit -m "create files to be deleted" &&
> +	git rm --cached nonempty empty &&
> +	git add -N nonempty empty &&
> +	git checkout -f HEAD &&
> +	git diff --cached --exit-code nonempty empty
> +'
> +
>  test_done
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 50189909b8..9b7e6b01c4 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1661,6 +1661,7 @@ static int same(const struct cache_entry *a, const struct cache_entry *b)
>  	if ((a->ce_flags | b->ce_flags) & CE_CONFLICTED)
>  		return 0;
>  	return a->ce_mode == b->ce_mode &&
> +	       !ce_intent_to_add(a) == !ce_intent_to_add(b) &&

Why the bangs?  This would work just as well and be slightly easier to
read without negating both sides, wouldn't it?

>  	       oideq(&a->oid, &b->oid);
>  }
>
>