{"thread":{"id":"51530","subject":"[PATCH] checkout.c: unstage empty deleted ita files","startedAt":"2019-07-26T04:57:09Z","lastAt":"2019-08-02T16:29:26Z","messageCount":11,"participants":["Varun Naik","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"379326","messageId":"20190726045645.2437-1-vcnaik94@gmail.com","threadId":"51530","inReplyTo":null,"subject":"[PATCH] checkout.c: unstage empty deleted ita files","fromName":"Varun Naik","fromEmail":"vcnaik94@gmail.com","sentAt":"2019-07-26T04:56:45Z","receivedAt":"2019-07-26T04:57:09Z","isPatch":true,"sender":{"key":"vcnaik94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2382501?v=4"},"body":"It is possible to delete a committed file from the index and then add it\nas intent-to-add. After `git checkout HEAD` or `git restore --staged`,\nthe file should be identical in the index and HEAD. This patch provides\nthe desired behavior even when the file is empty in the index.\n\nSigned-off-by: Varun Naik \n---\nCC Jeff because you wrote the code that I am changing now.\n\ncheckout.c:update_some() discards the newly created cache entry when its\nmode and oid match those of the old entry. Since an ita file has the\nsame oid as an empty file, an empty deleted ita file passes both of\nthese checks, and the new entry is discarded. In this case, the file\nshould be added to the cache instead.\n\nThis change should not affect newly added ita files. For those, inside\ntree.c:read_tree_1(), tree_entry_interesting() returns\nentry_not_interesting, so fn (which points to update_some()) is never\ncalled.\n\nTo the best of my understanding, the only other command that makes\nchanges to the index differently for nonempty vs empty deleted ita files\nis \"reset\", which I am fixing in [0]. I am separating the two changes\nbecause this change affects \"restore\", which has not reached maint yet.\n\n[0]: https://public-inbox.org/git/20190726044806.2216-1-vcnaik94@gmail.com/\n\n builtin/checkout.c | 1 +\n t/t2022-checkout-paths.sh | 11 +++++++++++\n t/t2070-restore.sh | 11 +++++++++++\n 3 files changed, 23 insertions(+)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 91f8509f85..27daa09c3c 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -126,6 +126,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base,\n \tif (pos >= 0) {\n \t\tstruct cache_entry *old = active_cache[pos];\n \t\tif (ce->ce_mode == old->ce_mode &&\n+\t\t !ce_intent_to_add(old) &&\n \t\t oideq(&ce->oid, &old->oid)) {\n \t\t\told->ce_flags |= CE_UPDATE;\n \t\t\tdiscard_cache_entry(ce);\ndiff --git a/t/t2022-checkout-paths.sh b/t/t2022-checkout-paths.sh\nindex fc3eb43b89..74add853fd 100755\n--- a/t/t2022-checkout-paths.sh\n+++ b/t/t2022-checkout-paths.sh\n@@ -78,4 +78,15 @@ test_expect_success 'do not touch files that are already up-to-date' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'checkout HEAD adds deleted intent-to-add file back to index' '\n+\techo \"nonempty\" >nonempty &&\n+\t>empty &&\n+\tgit add nonempty empty &&\n+\tgit commit -m \"create files to be deleted\" &&\n+\tgit rm --cached nonempty empty &&\n+\tgit add -N nonempty empty &&\n+\tgit checkout HEAD nonempty empty &&\n+\tgit diff --staged --exit-code\n+'\n+\n test_done\ndiff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\nindex 2650df1966..09b1543a5b 100755\n--- a/t/t2070-restore.sh\n+++ b/t/t2070-restore.sh\n@@ -95,4 +95,15 @@ test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '\n \t)\n '\n \n+test_expect_success 'restore --staged adds deleted intent-to-add file back to index' '\n+\techo \"nonempty\" >nonempty &&\n+\t>empty &&\n+\tgit add nonempty empty &&\n+\tgit commit -m \"create files to be deleted\" &&\n+\tgit rm --cached nonempty empty &&\n+\tgit add -N nonempty empty &&\n+\tgit restore --staged nonempty empty &&\n+\tgit diff --staged --exit-code\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"379328","messageId":"CAK_rgsG2ghwqDfRzyAy9MvFujsR=-Hy7YRiAv=RH1DpoZ-06hA@mail.gmail.com","threadId":"51530","inReplyTo":"20190726045645.2437-1-vcnaik94@gmail.com","subject":"Re: [PATCH] checkout.c: unstage empty deleted ita files","fromName":"Varun Naik","fromEmail":"vcnaik94@gmail.com","sentAt":"2019-07-26T05:01:13Z","receivedAt":"2019-07-26T05:01:27Z","isPatch":true,"sender":{"key":"vcnaik94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2382501?v=4"},"body":"Got the CC list wrong in the first email...\n\nVarun\n\n\nOn Thu, Jul 25, 2019 at 9:57 PM Varun Naik wrote:\n>\n> It is possible to delete a committed file from the index and then add it\n> as intent-to-add. After `git checkout HEAD` or `git restore --staged`,\n> the file should be identical in the index and HEAD. This patch provides\n> the desired behavior even when the file is empty in the index.\n>\n> Signed-off-by: Varun Naik \n> ---\n> CC Jeff because you wrote the code that I am changing now.\n>\n> checkout.c:update_some() discards the newly created cache entry when its\n> mode and oid match those of the old entry. Since an ita file has the\n> same oid as an empty file, an empty deleted ita file passes both of\n> these checks, and the new entry is discarded. In this case, the file\n> should be added to the cache instead.\n>\n> This change should not affect newly added ita files. For those, inside\n> tree.c:read_tree_1(), tree_entry_interesting() returns\n> entry_not_interesting, so fn (which points to update_some()) is never\n> called.\n>\n> To the best of my understanding, the only other command that makes\n> changes to the index differently for nonempty vs empty deleted ita files\n> is \"reset\", which I am fixing in [0]. I am separating the two changes\n> because this change affects \"restore\", which has not reached maint yet.\n>\n> [0]: https://public-inbox.org/git/20190726044806.2216-1-vcnaik94@gmail.com/\n>\n> builtin/checkout.c | 1 +\n> t/t2022-checkout-paths.sh | 11 +++++++++++\n> t/t2070-restore.sh | 11 +++++++++++\n> 3 files changed, 23 insertions(+)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 91f8509f85..27daa09c3c 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -126,6 +126,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base,\n> if (pos >= 0) {\n> struct cache_entry *old = active_cache[pos];\n> if (ce->ce_mode == old->ce_mode &&\n> + !ce_intent_to_add(old) &&\n> oideq(&ce->oid, &old->oid)) {\n> old->ce_flags |= CE_UPDATE;\n> discard_cache_entry(ce);\n> diff --git a/t/t2022-checkout-paths.sh b/t/t2022-checkout-paths.sh\n> index fc3eb43b89..74add853fd 100755\n> --- a/t/t2022-checkout-paths.sh\n> +++ b/t/t2022-checkout-paths.sh\n> @@ -78,4 +78,15 @@ test_expect_success 'do not touch files that are already up-to-date' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'checkout HEAD adds deleted intent-to-add file back to index' '\n> + echo \"nonempty\" >nonempty &&\n> + >empty &&\n> + git add nonempty empty &&\n> + git commit -m \"create files to be deleted\" &&\n> + git rm --cached nonempty empty &&\n> + git add -N nonempty empty &&\n> + git checkout HEAD nonempty empty &&\n> + git diff --staged --exit-code\n> +'\n> +\n> test_done\n> diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\n> index 2650df1966..09b1543a5b 100755\n> --- a/t/t2070-restore.sh\n> +++ b/t/t2070-restore.sh\n> @@ -95,4 +95,15 @@ test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '\n> )\n> '\n>\n> +test_expect_success 'restore --staged adds deleted intent-to-add file back to index' '\n> + echo \"nonempty\" >nonempty &&\n> + >empty &&\n> + git add nonempty empty &&\n> + git commit -m \"create files to be deleted\" &&\n> + git rm --cached nonempty empty &&\n> + git add -N nonempty empty &&\n> + git restore --staged nonempty empty &&\n> + git diff --staged --exit-code\n> +'\n> +\n> test_done\n> --\n> 2.22.0\n>\n"},{"id":"379331","messageId":"20190726085756.GA20017@sigill.intra.peff.net","threadId":"51530","inReplyTo":"20190726045645.2437-1-vcnaik94@gmail.com","subject":"Re: [PATCH] checkout.c: unstage empty deleted ita files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-26T08:57:57Z","receivedAt":"2019-07-26T08:58:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 25, 2019 at 09:56:45PM -0700, Varun Naik wrote:\n\n> It is possible to delete a committed file from the index and then add it\n> as intent-to-add. After `git checkout HEAD` or `git restore --staged`,\n> the file should be identical in the index and HEAD. This patch provides\n> the desired behavior even when the file is empty in the index.\n\nOK, so the issue is that ITA entries have an empty-file sha1, so they\nconfuse the logic to decide if we can use the old entry. Your fix makes\nsense.\n\n> ---\n> CC Jeff because you wrote the code that I am changing now.\n> \n> checkout.c:update_some() discards the newly created cache entry when its\n> mode and oid match those of the old entry. Since an ita file has the\n> same oid as an empty file, an empty deleted ita file passes both of\n> these checks, and the new entry is discarded. In this case, the file\n> should be added to the cache instead.\n> \n> This change should not affect newly added ita files. For those, inside\n> tree.c:read_tree_1(), tree_entry_interesting() returns\n> entry_not_interesting, so fn (which points to update_some()) is never\n> called.\n\nThese two paragraphs would be a nice addition to the actual commit\nmessage.\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 91f8509f85..27daa09c3c 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -126,6 +126,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base,\n> \tif (pos >= 0) {\n> \t\tstruct cache_entry *old = active_cache[pos];\n> \t\tif (ce->ce_mode == old->ce_mode &&\n> +\t\t !ce_intent_to_add(old) &&\n> \t\t oideq(&ce->oid, &old->oid)) {\n> \t\t\told->ce_flags |= CE_UPDATE;\n> \t\t\tdiscard_cache_entry(ce);\n\nMy first thought here was that we could skip ITA entries here only when\nthe HEAD hash is also the empty blob, which would let us retain index\nresults in more cases. But it doesn't help. If the HEAD entry isn't the\nempty blob, then we'll have !oideq() and we'll skip anyway, because an\nITA entry must be the empty blob (if we `git add` some other content,\nthen it ceases to be ITA).\n\nSo it makes sense to just always skip this \"retain the old index entry\"\nblock for any ITA entry.\n\n> +test_expect_success 'checkout HEAD adds deleted intent-to-add file back to index' '\n> +\techo \"nonempty\" >nonempty &&\n> +\t>empty &&\n> +\tgit add nonempty empty &&\n> +\tgit commit -m \"create files to be deleted\" &&\n> +\tgit rm --cached nonempty empty &&\n> +\tgit add -N nonempty empty &&\n> +\tgit checkout HEAD nonempty empty &&\n> +\tgit diff --staged --exit-code\n> +'\n\nThis clearly demonstrates the problem. Nice.\n\n> +test_expect_success 'restore --staged adds deleted intent-to-add file back to index' '\n> +\techo \"nonempty\" >nonempty &&\n> +\t>empty &&\n> +\tgit add nonempty empty &&\n> +\tgit commit -m \"create files to be deleted\" &&\n> +\tgit rm --cached nonempty empty &&\n> +\tgit add -N nonempty empty &&\n> +\tgit restore --staged nonempty empty &&\n> +\tgit diff --staged --exit-code\n> +'\n\nHmm. This git-restore test means we don't apply to maint. But wouldn't\nwe want the fix for \"checkout\" there?\n\nI.e., I'd expect a patch to fix and test git-checkout, and then an\nadditional patch to be added on the merge of that plus master to test\ngit-restore.\n\nOther than that, the patch looks good to me.\n\n-Peff\n"},{"id":"379474","messageId":"CAK_rgsH6hL3g+PVfcMuq1dQLeNJcii=D_dQ8anmWLvYrShmkNg@mail.gmail.com","threadId":"51530","inReplyTo":"20190726085756.GA20017@sigill.intra.peff.net","subject":"Re: [PATCH] checkout.c: unstage empty deleted ita files","fromName":"Varun Naik","fromEmail":"vcnaik94@gmail.com","sentAt":"2019-07-29T06:54:38Z","receivedAt":"2019-07-29T06:54:51Z","isPatch":true,"sender":{"key":"vcnaik94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2382501?v=4"},"body":"On Fri, Jul 26, 2019 at 1:57 AM Jeff King wrote:\n>\n> On Thu, Jul 25, 2019 at 09:56:45PM -0700, Varun Naik wrote:\n>\n> > It is possible to delete a committed file from the index and then add it\n> > as intent-to-add. After `git checkout HEAD` or `git restore --staged`,\n> > the file should be identical in the index and HEAD. This patch provides\n> > the desired behavior even when the file is empty in the index.\n>\n> OK, so the issue is that ITA entries have an empty-file sha1, so they\n> confuse the logic to decide if we can use the old entry. Your fix makes\n> sense.\n>\n> > ---\n> > CC Jeff because you wrote the code that I am changing now.\n> >\n> > checkout.c:update_some() discards the newly created cache entry when its\n> > mode and oid match those of the old entry. Since an ita file has the\n> > same oid as an empty file, an empty deleted ita file passes both of\n> > these checks, and the new entry is discarded. In this case, the file\n> > should be added to the cache instead.\n> >\n> > This change should not affect newly added ita files. For those, inside\n> > tree.c:read_tree_1(), tree_entry_interesting() returns\n> > entry_not_interesting, so fn (which points to update_some()) is never\n> > called.\n>\n> These two paragraphs would be a nice addition to the actual commit\n> message.\n>\n\nI will add them to the commit message, with some minor changes.\n\n> > diff --git a/builtin/checkout.c b/builtin/checkout.c\n> > index 91f8509f85..27daa09c3c 100644\n> > --- a/builtin/checkout.c\n> > +++ b/builtin/checkout.c\n> > @@ -126,6 +126,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base,\n> > if (pos >= 0) {\n> > struct cache_entry *old = active_cache[pos];\n> > if (ce->ce_mode == old->ce_mode &&\n> > + !ce_intent_to_add(old) &&\n> > oideq(&ce->oid, &old->oid)) {\n> > old->ce_flags |= CE_UPDATE;\n> > discard_cache_entry(ce);\n>\n> My first thought here was that we could skip ITA entries here only when\n> the HEAD hash is also the empty blob, which would let us retain index\n> results in more cases. But it doesn't help. If the HEAD entry isn't the\n> empty blob, then we'll have !oideq() and we'll skip anyway, because an\n> ITA entry must be the empty blob (if we `git add` some other content,\n> then it ceases to be ITA).\n>\n> So it makes sense to just always skip this \"retain the old index entry\"\n> block for any ITA entry.\n>\n> > +test_expect_success 'checkout HEAD adds deleted intent-to-add file back to index' '\n> > + echo \"nonempty\" >nonempty &&\n> > + >empty &&\n> > + git add nonempty empty &&\n> > + git commit -m \"create files to be deleted\" &&\n> > + git rm --cached nonempty empty &&\n> > + git add -N nonempty empty &&\n> > + git checkout HEAD nonempty empty &&\n> > + git diff --staged --exit-code\n> > +'\n>\n> This clearly demonstrates the problem. Nice.\n>\n> > +test_expect_success 'restore --staged adds deleted intent-to-add file back to index' '\n> > + echo \"nonempty\" >nonempty &&\n> > + >empty &&\n> > + git add nonempty empty &&\n> > + git commit -m \"create files to be deleted\" &&\n> > + git rm --cached nonempty empty &&\n> > + git add -N nonempty empty &&\n> > + git restore --staged nonempty empty &&\n> > + git diff --staged --exit-code\n> > +'\n>\n> Hmm. This git-restore test means we don't apply to maint. But wouldn't\n> we want the fix for \"checkout\" there?\n>\n> I.e., I'd expect a patch to fix and test git-checkout, and then an\n> additional patch to be added on the merge of that plus master to test\n> git-restore.\n>\n\nTo make sure I understand, do you mean that I should omit the test\ncase for \"restore\" right now, wait for the patch to reach master, and\nthen create another patch for the \"restore\" test case?\n\n> Other than that, the patch looks good to me.\n>\n> -Peff\n\nVarun\n"},{"id":"379481","messageId":"20190729091146.GA1595@sigill.intra.peff.net","threadId":"51530","inReplyTo":"CAK_rgsH6hL3g+PVfcMuq1dQLeNJcii=D_dQ8anmWLvYrShmkNg@mail.gmail.com","subject":"Re: [PATCH] checkout.c: unstage empty deleted ita files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-29T09:11:47Z","receivedAt":"2019-07-29T09:11:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 28, 2019 at 11:54:38PM -0700, Varun Naik wrote:\n\n> > These two paragraphs would be a nice addition to the actual commit\n> > message.\n> \n> I will add them to the commit message, with some minor changes.\n\nThanks!\n\n> > Hmm. This git-restore test means we don't apply to maint. But wouldn't\n> > we want the fix for \"checkout\" there?\n> >\n> > I.e., I'd expect a patch to fix and test git-checkout, and then an\n> > additional patch to be added on the merge of that plus master to test\n> > git-restore.\n> >\n> \n> To make sure I understand, do you mean that I should omit the test\n> case for \"restore\" right now, wait for the patch to reach master, and\n> then create another patch for the \"restore\" test case?\n\nI think we could do it all right now. If you split it into two patches,\none for \"fix checkout\" and another for \"add restore tests\", then Junio\ncould do something like:\n\n git checkout -b vn/checkout-ita maint\n git am fix-checkout.patch\n\n git checkout -b vn/restore-ita-tests master\n git merge vn/checkout-ita\n git am restore-tests.patch\n\nThe justification for the commit message in the latter patch is\nsomething like \"this is fixed already, but let's add a regression test\nto make sure it remains fixed\".\n\n-Peff\n"},{"id":"379755","messageId":"20190801160756.12601-1-vcnaik94@gmail.com","threadId":"51530","inReplyTo":"20190726045645.2437-1-vcnaik94@gmail.com","subject":"[PATCH v2] checkout.c: unstage empty deleted ita files","fromName":"Varun Naik","fromEmail":"vcnaik94@gmail.com","sentAt":"2019-08-01T16:07:56Z","receivedAt":"2019-08-01T16:08:18Z","isPatch":true,"sender":{"key":"vcnaik94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2382501?v=4"},"body":"It is possible to delete a committed file from the index and then add it\nas intent-to-add. After `git checkout HEAD`, the file should be\nidentical in the index and HEAD. The command already works correctly if\nthe file has contents in HEAD. This patch provides the desired behavior\neven when the file is empty in HEAD.\n\n`git checkout HEAD` calls tree.c:read_tree_1(), with fn pointing to\ncheckout.c:update_some(). update_some() creates a new cache entry but\ndiscards it when its mode and oid match those of the old entry. A cache\nentry for an ita file and a cache entry for an empty file have the same\noid. Therefore, an empty deleted ita file previously passed both of\nthese checks, and the new entry was discarded, so the file remained\nunchanged in the index. After this fix, if the file is marked as ita in\nthe cache, then we avoid discarding the new entry and add the new entry\nto the cache instead.\n\nThis change should not affect newly added ita files. For those, inside\ntree.c:read_tree_1(), tree_entry_interesting() returns\nentry_not_interesting, so fn is never called.\n\nHelped-by: Jeff King \nSigned-off-by: Varun Naik \n---\nThis patch fixes and tests only \"checkout\", because \"restore\" has not\nreached maint yet. A second patch on the merge of this patch into master\nwith a test case for \"restore\" is coming.\n\n builtin/checkout.c | 1 +\n t/t2022-checkout-paths.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex ffa776c6e1..c41dfa53a9 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -105,6 +105,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base,\n \tif (pos >= 0) {\n \t\tstruct cache_entry *old = active_cache[pos];\n \t\tif (ce->ce_mode == old->ce_mode &&\n+\t\t !ce_intent_to_add(old) &&\n \t\t oideq(&ce->oid, &old->oid)) {\n \t\t\told->ce_flags |= CE_UPDATE;\n \t\t\tdiscard_cache_entry(ce);\ndiff --git a/t/t2022-checkout-paths.sh b/t/t2022-checkout-paths.sh\nindex fc3eb43b89..6844afafc0 100755\n--- a/t/t2022-checkout-paths.sh\n+++ b/t/t2022-checkout-paths.sh\n@@ -78,4 +78,15 @@ test_expect_success 'do not touch files that are already up-to-date' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'checkout HEAD adds deleted intent-to-add file back to index' '\n+\techo \"nonempty\" >nonempty &&\n+\t>empty &&\n+\tgit add nonempty empty &&\n+\tgit commit -m \"create files to be deleted\" &&\n+\tgit rm --cached nonempty empty &&\n+\tgit add -N nonempty empty &&\n+\tgit checkout HEAD nonempty empty &&\n+\tgit diff --cached --exit-code\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"379756","messageId":"20190801160910.12652-1-vcnaik94@gmail.com","threadId":"51530","inReplyTo":"20190726045645.2437-1-vcnaik94@gmail.com","subject":"[PATCH] restore: add test for deleted ita files","fromName":"Varun Naik","fromEmail":"vcnaik94@gmail.com","sentAt":"2019-08-01T16:09:10Z","receivedAt":"2019-08-01T16:09:29Z","isPatch":true,"sender":{"key":"vcnaik94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2382501?v=4"},"body":"`git restore --staged` uses the same machinery as `git checkout HEAD`,\nso there should be a similar test case for \"restore\" as the existing\ntest case for \"checkout\" with deleted ita files.\n\nHelped-by: Jeff King \nSigned-off-by: Varun Naik \n---\nThis is the \"restore\" patch based on the merge of the \"checkout\" patch\ninto master.\n\n t/t2070-restore.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\nindex 2650df1966..21c3f84459 100755\n--- a/t/t2070-restore.sh\n+++ b/t/t2070-restore.sh\n@@ -95,4 +95,15 @@ test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '\n \t)\n '\n \n+test_expect_success 'restore --staged adds deleted intent-to-add file back to index' '\n+\techo \"nonempty\" >nonempty &&\n+\t>empty &&\n+\tgit add nonempty empty &&\n+\tgit commit -m \"create files to be deleted\" &&\n+\tgit rm --cached nonempty empty &&\n+\tgit add -N nonempty empty &&\n+\tgit restore --staged nonempty empty &&\n+\tgit diff --cached --exit-code\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"379761","messageId":"xmqq4l30okmi.fsf@gitster-ct.c.googlers.com","threadId":"51530","inReplyTo":"20190801160756.12601-1-vcnaik94@gmail.com","subject":"Re: [PATCH v2] checkout.c: unstage empty deleted ita files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-01T17:34:13Z","receivedAt":"2019-08-01T17:34:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Varun Naik writes:\n\n> It is possible to delete a committed file from the index and then add it\n> as intent-to-add. After `git checkout HEAD`, the file should be\n> identical in the index and HEAD.\n\nWe should write this as `git checkout HEAD `; with the\ncommand without the form, the files changed in various\nways should not be changed with it at all.\n\n\t$ echo modified >>file1\n\t$ rm file2\n\t$ git rm --cached file3 && git add -N file3\n\t$ git checkout HEAD\n\tM\tfile1\n\tD\tfile2\n\tM\tfile3\n\n> `git checkout HEAD` calls tree.c:read_tree_1(), with fn pointing to\n> checkout.c:update_some(). update_some() creates a new cache entry but\n> discards it when its mode and oid match those of the old entry. A cache\n> entry for an ita file and a cache entry for an empty file have the same\n> oid. Therefore, an empty deleted ita file previously passed both of\n> these checks, and the new entry was discarded, so the file remained\n> unchanged in the index. After this fix, if the file is marked as ita in\n> the cache, then we avoid discarding the new entry and add the new entry\n> to the cache instead.\n\nThanks; the flow of thought above is quite straight-forward to\nfollow.\n\n\n"},{"id":"379785","messageId":"20190801215148.GA30137@sigill.intra.peff.net","threadId":"51530","inReplyTo":"20190801160756.12601-1-vcnaik94@gmail.com","subject":"Re: [PATCH v2] checkout.c: unstage empty deleted ita files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-01T21:51:48Z","receivedAt":"2019-08-01T21:51:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 01, 2019 at 09:07:56AM -0700, Varun Naik wrote:\n\n> It is possible to delete a committed file from the index and then add it\n> as intent-to-add. After `git checkout HEAD`, the file should be\n> identical in the index and HEAD. The command already works correctly if\n> the file has contents in HEAD. This patch provides the desired behavior\n> even when the file is empty in HEAD.\n> \n> `git checkout HEAD` calls tree.c:read_tree_1(), with fn pointing to\n> checkout.c:update_some(). update_some() creates a new cache entry but\n> discards it when its mode and oid match those of the old entry. A cache\n> entry for an ita file and a cache entry for an empty file have the same\n> oid. Therefore, an empty deleted ita file previously passed both of\n> these checks, and the new entry was discarded, so the file remained\n> unchanged in the index. After this fix, if the file is marked as ita in\n> the cache, then we avoid discarding the new entry and add the new entry\n> to the cache instead.\n> \n> This change should not affect newly added ita files. For those, inside\n> tree.c:read_tree_1(), tree_entry_interesting() returns\n> entry_not_interesting, so fn is never called.\n> \n> Helped-by: Jeff King \n> Signed-off-by: Varun Naik \n> ---\n> This patch fixes and tests only \"checkout\", because \"restore\" has not\n> reached maint yet. A second patch on the merge of this patch into master\n> with a test case for \"restore\" is coming.\n\nThanks, this version looks good to me!\n\n-Peff\n"},{"id":"379833","messageId":"xmqq1ry3mtkk.fsf@gitster-ct.c.googlers.com","threadId":"51530","inReplyTo":"20190801160910.12652-1-vcnaik94@gmail.com","subject":"Re: [PATCH] restore: add test for deleted ita files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-02T16:16:11Z","receivedAt":"2019-08-02T16:16:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Varun Naik writes:\n\n> `git restore --staged` uses the same machinery as `git checkout HEAD`,\n> so there should be a similar test case for \"restore\" as the existing\n> test case for \"checkout\" with deleted ita files.\n>\n> Helped-by: Jeff King \n> Signed-off-by: Varun Naik \n> ---\n> This is the \"restore\" patch based on the merge of the \"checkout\" patch\n> into master.\n\nMakes sense. Thanks.\n"},{"id":"379836","messageId":"20190802162852.14498-1-vcnaik94@gmail.com","threadId":"51530","inReplyTo":"20190726045645.2437-1-vcnaik94@gmail.com","subject":"[PATCH v3] checkout.c: unstage empty deleted ita files","fromName":"Varun Naik","fromEmail":"vcnaik94@gmail.com","sentAt":"2019-08-02T16:28:52Z","receivedAt":"2019-08-02T16:29:26Z","isPatch":true,"sender":{"key":"vcnaik94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2382501?v=4"},"body":"It is possible to delete a committed file from the index and then add it\nas intent-to-add. After `git checkout HEAD `, the file should\nbe identical in the index and HEAD. The command already works correctly\nif the file has contents in HEAD. This patch provides the desired\nbehavior even when the file is empty in HEAD.\n\n`git checkout HEAD ` calls tree.c:read_tree_1(), with fn\npointing to checkout.c:update_some(). update_some() creates a new cache\nentry but discards it when its mode and oid match those of the old\nentry. A cache entry for an ita file and a cache entry for an empty file\nhave the same oid. Therefore, an empty deleted ita file previously\npassed both of these checks, and the new entry was discarded, so the\nfile remained unchanged in the index. After this fix, if the file is\nmarked as ita in the cache, then we avoid discarding the new entry and\nadd the new entry to the cache instead.\n\nThis change should not affect newly added ita files. For those, inside\ntree.c:read_tree_1(), tree_entry_interesting() returns\nentry_not_interesting, so fn is never called.\n\nHelped-by: Jeff King \nSigned-off-by: Varun Naik \n---\nThis patch fixes and tests \"checkout\". New in v3 is an improved commit\nmessage that clarifies that the affected command is `git checkout HEAD\n`.\n\n builtin/checkout.c | 1 +\n t/t2022-checkout-paths.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex ffa776c6e1..c41dfa53a9 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -105,6 +105,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base,\n \tif (pos >= 0) {\n \t\tstruct cache_entry *old = active_cache[pos];\n \t\tif (ce->ce_mode == old->ce_mode &&\n+\t\t !ce_intent_to_add(old) &&\n \t\t oideq(&ce->oid, &old->oid)) {\n \t\t\told->ce_flags |= CE_UPDATE;\n \t\t\tdiscard_cache_entry(ce);\ndiff --git a/t/t2022-checkout-paths.sh b/t/t2022-checkout-paths.sh\nindex fc3eb43b89..6844afafc0 100755\n--- a/t/t2022-checkout-paths.sh\n+++ b/t/t2022-checkout-paths.sh\n@@ -78,4 +78,15 @@ test_expect_success 'do not touch files that are already up-to-date' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'checkout HEAD adds deleted intent-to-add file back to index' '\n+\techo \"nonempty\" >nonempty &&\n+\t>empty &&\n+\tgit add nonempty empty &&\n+\tgit commit -m \"create files to be deleted\" &&\n+\tgit rm --cached nonempty empty &&\n+\tgit add -N nonempty empty &&\n+\tgit checkout HEAD nonempty empty &&\n+\tgit diff --cached --exit-code\n+'\n+\n test_done\n-- \n2.22.0\n\n"}]}