| Message ID | 20190726045645.2437-1-vcnaik94@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | checkout.c: unstage empty deleted ita files | expand |
Got the CC list wrong in the first email... Varun On Thu, Jul 25, 2019 at 9:57 PM Varun Naik <vcnaik94@gmail.com> wrote: > > It is possible to delete a committed file from the index and then add it > as intent-to-add. After `git checkout HEAD` or `git restore --staged`, > the file should be identical in the index and HEAD. This patch provides > the desired behavior even when the file is empty in the index. > > Signed-off-by: Varun Naik <vcnaik94@gmail.com> > --- > CC Jeff because you wrote the code that I am changing now. > > checkout.c:update_some() discards the newly created cache entry when its > mode and oid match those of the old entry. Since an ita file has the > same oid as an empty file, an empty deleted ita file passes both of > these checks, and the new entry is discarded. In this case, the file > should be added to the cache instead. > > This change should not affect newly added ita files. For those, inside > tree.c:read_tree_1(), tree_entry_interesting() returns > entry_not_interesting, so fn (which points to update_some()) is never > called. > > To the best of my understanding, the only other command that makes > changes to the index differently for nonempty vs empty deleted ita files > is "reset", which I am fixing in [0]. I am separating the two changes > because this change affects "restore", which has not reached maint yet. > > [0]: https://public-inbox.org/git/20190726044806.2216-1-vcnaik94@gmail.com/ > > builtin/checkout.c | 1 + > t/t2022-checkout-paths.sh | 11 +++++++++++ > t/t2070-restore.sh | 11 +++++++++++ > 3 files changed, 23 insertions(+) > > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 91f8509f85..27daa09c3c 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -126,6 +126,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base, > if (pos >= 0) { > struct cache_entry *old = active_cache[pos]; > if (ce->ce_mode == old->ce_mode && > + !ce_intent_to_add(old) && > oideq(&ce->oid, &old->oid)) { > old->ce_flags |= CE_UPDATE; > discard_cache_entry(ce); > diff --git a/t/t2022-checkout-paths.sh b/t/t2022-checkout-paths.sh > index fc3eb43b89..74add853fd 100755 > --- a/t/t2022-checkout-paths.sh > +++ b/t/t2022-checkout-paths.sh > @@ -78,4 +78,15 @@ test_expect_success 'do not touch files that are already up-to-date' ' > test_cmp expect actual > ' > > +test_expect_success 'checkout HEAD 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 checkout HEAD nonempty empty && > + git diff --staged --exit-code > +' > + > test_done > diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh > index 2650df1966..09b1543a5b 100755 > --- a/t/t2070-restore.sh > +++ b/t/t2070-restore.sh > @@ -95,4 +95,15 @@ test_expect_success 'restore --ignore-unmerged ignores unmerged entries' ' > ) > ' > > +test_expect_success 'restore --staged 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 restore --staged nonempty empty && > + git diff --staged --exit-code > +' > + > test_done > -- > 2.22.0 >
On Thu, Jul 25, 2019 at 09:56:45PM -0700, Varun Naik wrote: > It is possible to delete a committed file from the index and then add it > as intent-to-add. After `git checkout HEAD` or `git restore --staged`, > the file should be identical in the index and HEAD. This patch provides > the desired behavior even when the file is empty in the index. OK, so the issue is that ITA entries have an empty-file sha1, so they confuse the logic to decide if we can use the old entry. Your fix makes sense. > --- > CC Jeff because you wrote the code that I am changing now. > > checkout.c:update_some() discards the newly created cache entry when its > mode and oid match those of the old entry. Since an ita file has the > same oid as an empty file, an empty deleted ita file passes both of > these checks, and the new entry is discarded. In this case, the file > should be added to the cache instead. > > This change should not affect newly added ita files. For those, inside > tree.c:read_tree_1(), tree_entry_interesting() returns > entry_not_interesting, so fn (which points to update_some()) is never > called. These two paragraphs would be a nice addition to the actual commit message. > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 91f8509f85..27daa09c3c 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -126,6 +126,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base, > if (pos >= 0) { > struct cache_entry *old = active_cache[pos]; > if (ce->ce_mode == old->ce_mode && > + !ce_intent_to_add(old) && > oideq(&ce->oid, &old->oid)) { > old->ce_flags |= CE_UPDATE; > discard_cache_entry(ce); My first thought here was that we could skip ITA entries here only when the HEAD hash is also the empty blob, which would let us retain index results in more cases. But it doesn't help. If the HEAD entry isn't the empty blob, then we'll have !oideq() and we'll skip anyway, because an ITA entry must be the empty blob (if we `git add` some other content, then it ceases to be ITA). So it makes sense to just always skip this "retain the old index entry" block for any ITA entry. > +test_expect_success 'checkout HEAD 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 checkout HEAD nonempty empty && > + git diff --staged --exit-code > +' This clearly demonstrates the problem. Nice. > +test_expect_success 'restore --staged 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 restore --staged nonempty empty && > + git diff --staged --exit-code > +' Hmm. This git-restore test means we don't apply to maint. But wouldn't we want the fix for "checkout" there? I.e., I'd expect a patch to fix and test git-checkout, and then an additional patch to be added on the merge of that plus master to test git-restore. Other than that, the patch looks good to me. -Peff
On Fri, Jul 26, 2019 at 1:57 AM Jeff King <peff@peff.net> wrote: > > On Thu, Jul 25, 2019 at 09:56:45PM -0700, Varun Naik wrote: > > > It is possible to delete a committed file from the index and then add it > > as intent-to-add. After `git checkout HEAD` or `git restore --staged`, > > the file should be identical in the index and HEAD. This patch provides > > the desired behavior even when the file is empty in the index. > > OK, so the issue is that ITA entries have an empty-file sha1, so they > confuse the logic to decide if we can use the old entry. Your fix makes > sense. > > > --- > > CC Jeff because you wrote the code that I am changing now. > > > > checkout.c:update_some() discards the newly created cache entry when its > > mode and oid match those of the old entry. Since an ita file has the > > same oid as an empty file, an empty deleted ita file passes both of > > these checks, and the new entry is discarded. In this case, the file > > should be added to the cache instead. > > > > This change should not affect newly added ita files. For those, inside > > tree.c:read_tree_1(), tree_entry_interesting() returns > > entry_not_interesting, so fn (which points to update_some()) is never > > called. > > These two paragraphs would be a nice addition to the actual commit > message. > I will add them to the commit message, with some minor changes. > > diff --git a/builtin/checkout.c b/builtin/checkout.c > > index 91f8509f85..27daa09c3c 100644 > > --- a/builtin/checkout.c > > +++ b/builtin/checkout.c > > @@ -126,6 +126,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base, > > if (pos >= 0) { > > struct cache_entry *old = active_cache[pos]; > > if (ce->ce_mode == old->ce_mode && > > + !ce_intent_to_add(old) && > > oideq(&ce->oid, &old->oid)) { > > old->ce_flags |= CE_UPDATE; > > discard_cache_entry(ce); > > My first thought here was that we could skip ITA entries here only when > the HEAD hash is also the empty blob, which would let us retain index > results in more cases. But it doesn't help. If the HEAD entry isn't the > empty blob, then we'll have !oideq() and we'll skip anyway, because an > ITA entry must be the empty blob (if we `git add` some other content, > then it ceases to be ITA). > > So it makes sense to just always skip this "retain the old index entry" > block for any ITA entry. > > > +test_expect_success 'checkout HEAD 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 checkout HEAD nonempty empty && > > + git diff --staged --exit-code > > +' > > This clearly demonstrates the problem. Nice. > > > +test_expect_success 'restore --staged 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 restore --staged nonempty empty && > > + git diff --staged --exit-code > > +' > > Hmm. This git-restore test means we don't apply to maint. But wouldn't > we want the fix for "checkout" there? > > I.e., I'd expect a patch to fix and test git-checkout, and then an > additional patch to be added on the merge of that plus master to test > git-restore. > To make sure I understand, do you mean that I should omit the test case for "restore" right now, wait for the patch to reach master, and then create another patch for the "restore" test case? > Other than that, the patch looks good to me. > > -Peff Varun
On Sun, Jul 28, 2019 at 11:54:38PM -0700, Varun Naik wrote: > > These two paragraphs would be a nice addition to the actual commit > > message. > > I will add them to the commit message, with some minor changes. Thanks! > > Hmm. This git-restore test means we don't apply to maint. But wouldn't > > we want the fix for "checkout" there? > > > > I.e., I'd expect a patch to fix and test git-checkout, and then an > > additional patch to be added on the merge of that plus master to test > > git-restore. > > > > To make sure I understand, do you mean that I should omit the test > case for "restore" right now, wait for the patch to reach master, and > then create another patch for the "restore" test case? I think we could do it all right now. If you split it into two patches, one for "fix checkout" and another for "add restore tests", then Junio could do something like: git checkout -b vn/checkout-ita maint git am fix-checkout.patch git checkout -b vn/restore-ita-tests master git merge vn/checkout-ita git am restore-tests.patch The justification for the commit message in the latter patch is something like "this is fixed already, but let's add a regression test to make sure it remains fixed". -Peff
diff --git a/builtin/checkout.c b/builtin/checkout.c index 91f8509f85..27daa09c3c 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -126,6 +126,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base, if (pos >= 0) { struct cache_entry *old = active_cache[pos]; if (ce->ce_mode == old->ce_mode && + !ce_intent_to_add(old) && oideq(&ce->oid, &old->oid)) { old->ce_flags |= CE_UPDATE; discard_cache_entry(ce); diff --git a/t/t2022-checkout-paths.sh b/t/t2022-checkout-paths.sh index fc3eb43b89..74add853fd 100755 --- a/t/t2022-checkout-paths.sh +++ b/t/t2022-checkout-paths.sh @@ -78,4 +78,15 @@ test_expect_success 'do not touch files that are already up-to-date' ' test_cmp expect actual ' +test_expect_success 'checkout HEAD 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 checkout HEAD nonempty empty && + git diff --staged --exit-code +' + test_done diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh index 2650df1966..09b1543a5b 100755 --- a/t/t2070-restore.sh +++ b/t/t2070-restore.sh @@ -95,4 +95,15 @@ test_expect_success 'restore --ignore-unmerged ignores unmerged entries' ' ) ' +test_expect_success 'restore --staged 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 restore --staged nonempty empty && + git diff --staged --exit-code +' + test_done
It is possible to delete a committed file from the index and then add it as intent-to-add. After `git checkout HEAD` or `git restore --staged`, the file should be identical in the index and HEAD. This patch provides the desired behavior even when the file is empty in the index. Signed-off-by: Varun Naik <vcnaik94@gmail.com> --- CC Jeff because you wrote the code that I am changing now. checkout.c:update_some() discards the newly created cache entry when its mode and oid match those of the old entry. Since an ita file has the same oid as an empty file, an empty deleted ita file passes both of these checks, and the new entry is discarded. In this case, the file should be added to the cache instead. This change should not affect newly added ita files. For those, inside tree.c:read_tree_1(), tree_entry_interesting() returns entry_not_interesting, so fn (which points to update_some()) is never called. To the best of my understanding, the only other command that makes changes to the index differently for nonempty vs empty deleted ita files is "reset", which I am fixing in [0]. I am separating the two changes because this change affects "restore", which has not reached maint yet. [0]: https://public-inbox.org/git/20190726044806.2216-1-vcnaik94@gmail.com/ builtin/checkout.c | 1 + t/t2022-checkout-paths.sh | 11 +++++++++++ t/t2070-restore.sh | 11 +++++++++++ 3 files changed, 23 insertions(+)