{"thread":{"id":"41497","subject":"[PATCH] unpack-trees: do not delete i-t-a entries in worktree even when forced","startedAt":"2016-02-24T11:45:17Z","lastAt":"2016-02-25T10:45:55Z","messageCount":4,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"279150","messageId":"1456314317-30301-1-git-send-email-pclouds@gmail.com","threadId":"41497","inReplyTo":null,"subject":"[PATCH] unpack-trees: do not delete i-t-a entries in worktree even when forced","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-24T11:45:17Z","receivedAt":"2016-02-24T11:45:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Intent-to-add entries are basically \"I may want to commit these files\nlater, but for now they are untracked\". As such, when the user does \"git\nreset --hard \", which removes i-t-a entries from the index, i-t-a\nentries in worktree should be kept as untracked.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Lost two files today, luckily I had a backup.\n\n t/t2203-add-intent.sh | 15 +++++++++++++++\n unpack-trees.c | 2 +-\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 2a4a749..63086bf 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -82,5 +82,20 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'reset --hard leaves on-disk ita entries alone' '\n+\tgit init keep-ita &&\n+\t(\n+\tcd keep-ita &&\n+\techo abc >abc &&\n+\techo def >def &&\n+\tgit add abc &&\n+\tgit commit -m abc &&\n+\tgit add -N def &&\n+\tgit reset --hard HEAD &&\n+\techo def >expected &&\n+\ttest_cmp expected def\n+\t)\n+'\n+\n test_done\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 9f55cc2..1a2271b 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -104,7 +104,7 @@ static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n {\n \tclear |= CE_HASHED;\n \n-\tif (set & CE_REMOVE)\n+\tif ((set & CE_REMOVE) && !ce_intent_to_add(ce))\n \t\tset |= CE_WT_REMOVE;\n \n \tce->ce_flags = (ce->ce_flags & ~clear) | set;\n-- \n2.7.2.531.gc9e018c\n"},{"id":"279190","messageId":"xmqqa8mpsom6.fsf@gitster.mtv.corp.google.com","threadId":"41497","inReplyTo":"1456314317-30301-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] unpack-trees: do not delete i-t-a entries in worktree even when forced","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-24T22:57:37Z","receivedAt":"2016-02-24T22:57:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Intent-to-add entries are basically \"I may want to commit these files\n> later, but for now they are untracked\". As such, when the user does \"git\n> reset --hard \", which removes i-t-a entries from the index, i-t-a\n> entries in worktree should be kept as untracked.\n\nHmm, I can see that the control flow of \"reset --hard\" for an i-t-a\npath does pass through this function, but it is not very obvious\nto see how this will not negatively affect other uses of the\nunpack-trees machinery (e.g. \"checkout\" and \"merge\", especially when\nsuch a path needs to turn into a directory by getting removed).\n\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> Lost two files today, luckily I had a backup.\n>\n> t/t2203-add-intent.sh | 15 +++++++++++++++\n> unpack-trees.c | 2 +-\n> 2 files changed, 16 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\n> index 2a4a749..63086bf 100755\n> --- a/t/t2203-add-intent.sh\n> +++ b/t/t2203-add-intent.sh\n> @@ -82,5 +82,20 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'reset --hard leaves on-disk ita entries alone' '\n> +\tgit init keep-ita &&\n> +\t(\n> +\tcd keep-ita &&\n> +\techo abc >abc &&\n> +\techo def >def &&\n> +\tgit add abc &&\n> +\tgit commit -m abc &&\n> +\tgit add -N def &&\n> +\tgit reset --hard HEAD &&\n> +\techo def >expected &&\n> +\ttest_cmp expected def\n> +\t)\n> +'\n> +\n> test_done\n> \n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 9f55cc2..1a2271b 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -104,7 +104,7 @@ static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n> {\n> \tclear |= CE_HASHED;\n> \n> -\tif (set & CE_REMOVE)\n> +\tif ((set & CE_REMOVE) && !ce_intent_to_add(ce))\n> \t\tset |= CE_WT_REMOVE;\n> \n> \tce->ce_flags = (ce->ce_flags & ~clear) | set;\n"},{"id":"279226","messageId":"xmqq60xdsnfr.fsf@gitster.mtv.corp.google.com","threadId":"41497","inReplyTo":"xmqqa8mpsom6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] unpack-trees: do not delete i-t-a entries in worktree even when forced","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-24T23:23:04Z","receivedAt":"2016-02-24T23:23:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> Intent-to-add entries are basically \"I may want to commit these files\n>> later, but for now they are untracked\". As such, when the user does \"git\n>> reset --hard \", which removes i-t-a entries from the index, i-t-a\n>> entries in worktree should be kept as untracked.\n>\n> Hmm, I can see that the control flow of \"reset --hard\" for an i-t-a\n> path does pass through this function, but it is not very obvious\n> to see how this will not negatively affect other uses of the\n> unpack-trees machinery (e.g. \"checkout\" and \"merge\", especially when\n> such a path needs to turn into a directory by getting removed).\n>\n\nThinking about it more, I have to say that I do not agree with the\nbasic premise of this patch. I-T-A is not \"may want to commit, but\nthey are untracked\" at all. It is \"I know I want to add, I just\ncannot yet decide the exact contents\".\n\nThat is why \"git add -N newfile && git grep string\" would find the\nstring from newfile, and \"git add -N newfile && git diff HEAD newfile\"\nwould show the addition.\n\nSane people would expect that \"git reset --hard HEAD\" would behave\nas \"git diff HEAD | git apply --index -R\" when your index is fully\nmerged, but this change will break the expectation.\n\nEarlier we changed \"git commit\" to pretend as if an I-T-A entry does\nnot exist in \"git add -N newfile && git commit\", but I think that\nwas a mistake that was caused by the same fuzzy thinking.\n\n3f6d56de (commit: ignore intent-to-add entries instead of refusing,\n2012-02-07) does talk about the use of \"git add -N\" in conjunction\nwith \"git status\" and \"git diff\", but somehow nobody realized that\nit was introducing inconsistency in the semantics.\n"},{"id":"279310","messageId":"CACsJy8C1fgu584Uxte+x4aHHQwishOeAzofyU7BGx0V-Pav7Kg@mail.gmail.com","threadId":"41497","inReplyTo":"xmqq60xdsnfr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] unpack-trees: do not delete i-t-a entries in worktree even when forced","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-25T10:45:55Z","receivedAt":"2016-02-25T10:45:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 25, 2016 at 6:23 AM, Junio C Hamano wrote:\n> Thinking about it more, I have to say that I do not agree with the\n> basic premise of this patch. I-T-A is not \"may want to commit, but\n> they are untracked\" at all. It is \"I know I want to add, I just\n> cannot yet decide the exact contents\".\n>\n> That is why \"git add -N newfile && git grep string\" would find the\n> string from newfile, and \"git add -N newfile && git diff HEAD newfile\"\n> would show the addition.\n\nWe can agree to disagree. To me i-t-a is still a reminder, not a real\ntracked entry (with unknown content).\n\n> Sane people would expect that \"git reset --hard HEAD\" would behave\n> as \"git diff HEAD | git apply --index -R\" when your index is fully\n> merged, but this change will break the expectation.\n\n$ echo abc >abc\n$ git add -N abc\n$ git diff HEAD|LANG=C git apply --index -R\nerror: abc: does not match index\n$ cat abc\nabc\n\nOK this is just nit-picking. We could probably make it work, though\nI'm not interested in doing that.\n\n> Earlier we changed \"git commit\" to pretend as if an I-T-A entry does\n> not exist in \"git add -N newfile && git commit\", but I think that\n> was a mistake that was caused by the same fuzzy thinking.\n>\n> 3f6d56de (commit: ignore intent-to-add entries instead of refusing,\n> 2012-02-07) does talk about the use of \"git add -N\" in conjunction\n> with \"git status\" and \"git diff\", but somehow nobody realized that\n> it was introducing inconsistency in the semantics.\n-- \nDuy\n"}]}