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@xxxxxxxxx> > --- > 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@xxxxxxxxx/ > [1]: https://public-inbox.org/git/20190802162852.14498-1-vcnaik94@xxxxxxxxx/ > > 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); > } > >
