{"thread":{"id":"39692","subject":"apply --cached --whitespace=fix now failing on items added with \"add -N\"","startedAt":"2015-06-22T14:29:49Z","lastAt":"2015-11-30T19:17:07Z","messageCount":28,"participants":["Patrick Higgins","Duy Nguyen","Junio C Hamano","Nguyễn Thái Ngọc Duy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"264536","messageId":"CACfKtTAvH7FH2AkC5hUNFEQ620gF401SNYaULLy62iE8S55-7A@mail.gmail.com","threadId":"39692","inReplyTo":null,"subject":"apply --cached --whitespace=fix now failing on items added with \"add -N\"","fromName":"Patrick Higgins","fromEmail":"phiggins@google.com","sentAt":"2015-06-22T14:29:49Z","receivedAt":"2015-06-22T14:29:49Z","isPatch":false,"sender":{"key":"phiggins@google.com","avatar":null},"body":"I like to use git to remove trailing whitespace from my files. I use\nthe following ~/.gitconfig to make this convenient:\n\n[alias]\n wsadd = \"!sh -c 'git diff -- \\\"$@\\\" | git apply --cached\n--whitespace=fix;\\\n git checkout -- ${1-.} \\\"$@\\\"' -\"\n\nThe wsadd alias doesn't work with new files, so I have to use \"git add\n-N\" on them first. As of a week or two ago, the \"apply --cached\" step\nnow fails with the following, assuming a new file named bar containing\n\"foo \" has been added with \"add -N\":\n\n$ git diff -- \"$@\" | git apply --cached --whitespace=fix\n:7: trailing whitespace.\nfoo\nerror: bar: already exists in index\n\nThe final \"git checkout\" at the end of wsadd truncates my file. I've\nchanged the \";\" to a \"&&\" to fix the truncation.\n\nWere there any recent changes to \"git apply\" that might have caused this?\n\n$ git --version\ngit version 2.4.3.573.g4eafbef\n$ uname -a\nLinux 3.13.0-53-generic #89-Ubuntu SMP Wed May 20 10:34:39\nUTC 2015 x86_64 x86_64 x86_64 GNU/Linux\n"},{"id":"264538","messageId":"CACsJy8BBxWme=y6jF_O74Jz37qPy=Sqf4w0cg-QLYzpxM5iwVw@mail.gmail.com","threadId":"39692","inReplyTo":"CACfKtTAvH7FH2AkC5hUNFEQ620gF401SNYaULLy62iE8S55-7A@mail.gmail.com","subject":"Re: apply --cached --whitespace=fix now failing on items added with \"add -N\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-22T14:45:22Z","receivedAt":"2015-06-22T14:45:22Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jun 22, 2015 at 9:29 PM, Patrick Higgins wrote:\n> I like to use git to remove trailing whitespace from my files. I use\n> the following ~/.gitconfig to make this convenient:\n>\n> [alias]\n> wsadd = \"!sh -c 'git diff -- \\\"$@\\\" | git apply --cached\n> --whitespace=fix;\\\n> git checkout -- ${1-.} \\\"$@\\\"' -\"\n>\n> The wsadd alias doesn't work with new files, so I have to use \"git add\n> -N\" on them first. As of a week or two ago, the \"apply --cached\" step\n> now fails with the following, assuming a new file named bar containing\n> \"foo \" has been added with \"add -N\":\n>\n> $ git diff -- \"$@\" | git apply --cached --whitespace=fix\n> :7: trailing whitespace.\n> foo\n> error: bar: already exists in index\n>\n> The final \"git checkout\" at the end of wsadd truncates my file. I've\n> changed the \";\" to a \"&&\" to fix the truncation.\n>\n> Were there any recent changes to \"git apply\" that might have caused this?\n\nProbably fallout from this one, merged to 'master' about 5 weeks ago.\nI'll have a look at it tomorrow unless somebody does it first\n\nd95d728 (diff-lib.c: adjust position of i-t-a entries in diff - 2015-03-16)\n-- \nDuy\n"},{"id":"264571","messageId":"xmqqa8vrisfl.fsf@gitster.dls.corp.google.com","threadId":"39692","inReplyTo":"CACsJy8BBxWme=y6jF_O74Jz37qPy=Sqf4w0cg-QLYzpxM5iwVw@mail.gmail.com","subject":"Re: apply --cached --whitespace=fix now failing on items added with \"add -N\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-22T17:06:22Z","receivedAt":"2015-06-22T17:06:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Mon, Jun 22, 2015 at 9:29 PM, Patrick Higgins wrote:\n>> I like to use git to remove trailing whitespace from my files. I use\n>> the following ~/.gitconfig to make this convenient:\n>>\n>> [alias]\n>> wsadd = \"!sh -c 'git diff -- \\\"$@\\\" | git apply --cached\n>> --whitespace=fix;\\\n>> git checkout -- ${1-.} \\\"$@\\\"' -\"\n>>\n>> The wsadd alias doesn't work with new files, so I have to use \"git add\n>> -N\" on them first. As of a week or two ago, the \"apply --cached\" step\n>> now fails with the following, assuming a new file named bar containing\n>> \"foo \" has been added with \"add -N\":\n>>\n>> $ git diff -- \"$@\" | git apply --cached --whitespace=fix\n>> :7: trailing whitespace.\n>> foo\n>> error: bar: already exists in index\n>>\n>> The final \"git checkout\" at the end of wsadd truncates my file. I've\n>> changed the \";\" to a \"&&\" to fix the truncation.\n>>\n>> Were there any recent changes to \"git apply\" that might have caused this?\n>\n> Probably fallout from this one, merged to 'master' about 5 weeks ago.\n> I'll have a look at it tomorrow unless somebody does it first\n>\n> d95d728 (diff-lib.c: adjust position of i-t-a entries in diff - 2015-03-16)\n\nYeah, the world order has changed by that commit, and I would expect\nto see some more fallouts.\n\nAfter \"add -N\", \"git diff\" used to pretend that an empty blob was in\nthe index, and showed a modification between an existing \"empty\" and\nthe full file contents, and \"git apply --cached\" was happy to take\nthat modification. In the new world order, \"git diff\" is instructed\nto pretend as if the path that was added by \"add -N\" does not exist\nyet in the index at all, but the index still knows about the path,\nwhich is a strange half-born state. It shows an addition of the\nfull file contents as a new file. \"git apply --cached\" sees an\naddition of a new path, which requires that the path does not exist\nin the index. In the new world order, it should be taught to\npretend that these \"add -N\" paths do not exist in the index, but\nthat was not done.\n\nSomething like the attached (totally untested) may be a good\nstarting point.\n\nFor another potential fallout, try this:\n\n $ cp COPYING RENAMING\n $ cp COPYING UNTRACKED\n $ >EMPTY\n $ git add -N RENAMING EMPTY\n $ git rm UNTRACKED\n fatal: pathspec 'UNTRACKED' did not match any files\n $ git rm EMPTY RENAMING\n error: the following file has staged content different from both the\n file and the HEAD:\n EMPTY\n RENAMING\n (use -f to force removal)\n\nOne could argue that these three should behave the same way, if the\nnew world order is \"path added by 'add -N' does not exist in the\nindex\".\n\nI however think the new world order should be slightly different\nfrom \"does not exist in the index\", but should be more like \"the\nindex is aware of its presence but has not been told about its\ncontents yet\". The consequences of this are:\n\n - \"git rm RENAMING\" shouldn't say 'did not match any files'.\n\n - \"git rm RENAMING\" does not know about 'staged content', so\n complaining about \"staged contents different from file and HEAD\"\n feels wrong.\n\nHaving said that, I do think erroring out and requiring -f is the\nright thing when remiving RENAMING and EMPTY. Unlike contents added\nby \"git add\" without \"-N\", we do not know what is in the working\ntree file at all. Given that we check and refuse when the contents\nare different between the working tree file, the index and the HEAD\neven when we know what was in the working tree when \"git add\"\nwithout \"-N\" was done, we should keep the safety to prevent\naccidental removal of the working tree file, which has the only\nexisting copy of the user content.\n\nOn the other hand, I am also OK if the behaviour was like this:\n\n $ git rm --cached RENAMING\n ... removed without complaints ...\n $ git rm RENAMING\n error: file 'RENAMING' does not have staged content yet.\n (use -f to force removal)\n\nIn any case, here is a \"how about this\" weather-balloon patch for\n\"git apply\"\n\n builtin/apply.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 146be97..653191e 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3546,12 +3546,23 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n #define EXISTS_IN_INDEX 1\n #define EXISTS_IN_WORKTREE 2\n \n+static int exists_in_index(const char *new_name)\n+{\n+\tint pos = cache_name_pos(new_name, strlen(new_name));\n+\n+\tif (pos < 0)\n+\t\treturn 0;\n+\tif (active_cache[pos]->ce_flags & CE_INTENT_TO_ADD)\n+\t\treturn 0;\n+\treturn 1;\n+}\n+\n static int check_to_create(const char *new_name, int ok_if_exists)\n {\n \tstruct stat nst;\n \n \tif (check_index &&\n-\t cache_name_pos(new_name, strlen(new_name)) >= 0 &&\n+\t exists_in_index(new_name) &&\n \t !ok_if_exists)\n \t\treturn EXISTS_IN_INDEX;\n \tif (cached)\n"},{"id":"264628","messageId":"1435062855-26274-1-git-send-email-pclouds@gmail.com","threadId":"39692","inReplyTo":"CACfKtTAvH7FH2AkC5hUNFEQ620gF401SNYaULLy62iE8S55-7A@mail.gmail.com","subject":"[PATCH] apply: fix adding new files on i-t-a entries","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-23T12:34:15Z","receivedAt":"2015-06-23T12:34:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Since d95d728 (diff-lib.c: adjust position of i-t-a entries in diff -\n2015-03-16), a normal \"git diff\" on i-t-a entries would produce a diff\nthat _adds_ those files, not just adding content to existing and empty\nfiles like before.\n\nThis is correct. Unfortunately, applying such a patch on the same\nrepository that has the same i-t-a entries will fail with message\n\"already exists in index\" because git-apply checks, sees those i-t-a\nentries and aborts. git-apply does not realize those are for\nbookkeeping only, they do not really exist in the index.\n\nThis patch tightens the \"exists in index\" check, ignoring i-t-a\nentries. For fixing the above problem, only the change in\ncheck_to_create() is needed. For other changes,\n\n - load_current(), reporting \"not exists in index\" is better than \"does\n not match index\"\n\n - check_preimage(), similar to load_current(), but it may also use\n ce_mode from i-t-a entry which is always zero\n\n - get_current_sha1(), or actually build_fake_ancestor(), we should not\n add i-t-a entries to the temporary index, at least not without also\n adding i-t-a flag back\n\nSince \"git add -p\" and \"git am\" use \"git apply\" underneath, they are\nbroken too before this patch and fixed now.\n\nReported-by: Patrick Higgins \nReported-by: Bjørnar Snoksrud \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n I think I'm opening a can of worms with d95d728. There's nothing\n wrong with that patch per se, but with this issue popping up, I need\n to go over all {cache,index}_name_pos call sites and see what would be\n the sensible behavior when i-t-a entries are involved.\n \n So far blame, rm and checkout-entry and \"checkout \" are on my\n to-think-or-fix list. But this patch can get in early to fix a real\n regression instead of waiting for one big series. A lot more\n discussions will be had before that series gets in good shape.\n\n builtin/apply.c | 8 ++++----\n cache.h | 2 ++\n read-cache.c | 12 ++++++++++++\n t/t2203-add-intent.sh | 10 ++++++++++\n 4 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 146be97..4f813ac 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3344,7 +3344,7 @@ static int load_current(struct image *image, struct patch *patch)\n \tif (!patch->is_new)\n \t\tdie(\"BUG: patch to %s is not a creation\", patch->old_name);\n \n-\tpos = cache_name_pos(name, strlen(name));\n+\tpos = exists_in_cache(name, strlen(name));\n \tif (pos < 0)\n \t\treturn error(_(\"%s: does not exist in index\"), name);\n \tce = active_cache[pos];\n@@ -3497,7 +3497,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n \t}\n \n \tif (check_index && !previous) {\n-\t\tint pos = cache_name_pos(old_name, strlen(old_name));\n+\t\tint pos = exists_in_cache(old_name, strlen(old_name));\n \t\tif (pos < 0) {\n \t\t\tif (patch->is_new < 0)\n \t\t\t\tgoto is_new;\n@@ -3551,7 +3551,7 @@ static int check_to_create(const char *new_name, int ok_if_exists)\n \tstruct stat nst;\n \n \tif (check_index &&\n-\t cache_name_pos(new_name, strlen(new_name)) >= 0 &&\n+\t exists_in_cache(new_name, strlen(new_name)) >= 0 &&\n \t !ok_if_exists)\n \t\treturn EXISTS_IN_INDEX;\n \tif (cached)\n@@ -3824,7 +3824,7 @@ static int get_current_sha1(const char *path, unsigned char *sha1)\n \n \tif (read_cache() < 0)\n \t\treturn -1;\n-\tpos = cache_name_pos(path, strlen(path));\n+\tpos = exists_in_cache(path, strlen(path));\n \tif (pos < 0)\n \t\treturn -1;\n \thashcpy(sha1, active_cache[pos]->sha1);\ndiff --git a/cache.h b/cache.h\nindex 571c98f..6a44cb6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -341,6 +341,7 @@ extern void free_name_hash(struct index_state *istate);\n #define discard_cache() discard_index(&the_index)\n #define unmerged_cache() unmerged_index(&the_index)\n #define cache_name_pos(name, namelen) index_name_pos(&the_index,(name),(namelen))\n+#define exists_in_cache(name, namelen) exists_in_index(&the_index,(name),(namelen))\n #define add_cache_entry(ce, option) add_index_entry(&the_index, (ce), (option))\n #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))\n #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))\n@@ -512,6 +513,7 @@ extern int verify_path(const char *path);\n extern struct cache_entry *index_dir_exists(struct index_state *istate, const char *name, int namelen);\n extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);\n extern int index_name_pos(const struct index_state *, const char *name, int namelen);\n+extern int exists_in_index(const struct index_state *, const char *name, int namelen);\n #define ADD_CACHE_OK_TO_ADD 1\t\t/* Ok to add */\n #define ADD_CACHE_OK_TO_REPLACE 2\t/* Ok to replace file/directory */\n #define ADD_CACHE_SKIP_DFCHECK 4\t/* Ok to skip DF conflict checks */\ndiff --git a/read-cache.c b/read-cache.c\nindex 5dee4e2..d3b50cb 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -506,6 +506,18 @@ int index_name_pos(const struct index_state *istate, const char *name, int namel\n \treturn index_name_stage_pos(istate, name, namelen, 0);\n }\n \n+/* This is the same as index_name_pos, except that i-t-a entries are invisible */\n+int exists_in_index(const struct index_state *istate, const char *name, int namelen)\n+{\n+\tint pos = index_name_stage_pos(istate, name, namelen, 0);\n+\n+\tif (pos < 0)\n+\t\treturn pos;\n+\tif (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD)\n+\t\treturn -pos-1;\n+\treturn pos;\n+}\n+\n /* Remove entry, return true if there are more entries to go.. */\n int remove_index_entry_at(struct index_state *istate, int pos)\n {\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 7c641bf..a10a96a 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -97,5 +97,15 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'apply on i-t-a entries' '\n+\tgit init apply &&\n+\t(\n+\t\tcd apply &&\n+\t\techo content >file &&\n+\t\tgit add -N file &&\n+\t\tgit diff | git apply --cached\n+\t)\n+'\n+\n test_done\n \n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"264632","messageId":"xmqqoak6e5dx.fsf@gitster.dls.corp.google.com","threadId":"39692","inReplyTo":"1435062855-26274-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] apply: fix adding new files on i-t-a entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-23T16:50:02Z","receivedAt":"2015-06-23T16:50:02Z","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> Since d95d728 (diff-lib.c: adjust position of i-t-a entries in diff -\n> 2015-03-16), a normal \"git diff\" on i-t-a entries would produce a diff\n> that _adds_ those files, not just adding content to existing and empty\n> files like before.\n>\n> This is correct. Unfortunately, applying such a patch on the same\n> repository that has the same i-t-a entries will fail with message\n> \"already exists in index\" because git-apply checks, sees those i-t-a\n> entries and aborts. git-apply does not realize those are for\n> bookkeeping only, they do not really exist in the index.\n\n> This patch tightens the \"exists in index\" check, ignoring i-t-a\n> entries. For fixing the above problem, only the change in\n> check_to_create() is needed.\n\nAnd the first thing I noticed and found a bit disturbing was that\nthis change (which I think is correct, and happens to match what I\nsent out earlier) was the only thing necessary to make your new test\npass. IOW, the other changes in this patch have no test coverage.\n\n> For other changes,\n>\n> - load_current(), reporting \"not exists in index\" is better than \"does\n> not match index\"\n\nIs that error reporting the only side effect from this change?\n\nThis is only used when falling back to three-way merge while\napplying a creation patch.\n\n> - check_preimage(), similar to load_current(), but it may also use\n> ce_mode from i-t-a entry which is always zero\n\nThis is for the normal (non three-way) application and the idea is\nthe same as load_current() as you said above.\n\n> - get_current_sha1(), or actually build_fake_ancestor(), we should not\n> add i-t-a entries to the temporary index, at least not without also\n> adding i-t-a flag back\n\nThis is part of \"am\" three-way fallback codepath. I do not think\nthe merge-recursive three-way merge code knows and cares about, is\ncapable of handling, or would even want to deal with i-t-a entries\nin the first place, so adding an entry as i-t-a bit would not help.\nWhat the ultimate caller wants from us in this codepath is a tree\nobject, and that is written out from the temporary index---and that\ncodepath ignores i-t-a entries, so it is correct to omit them from\nthe temporary index in the first place. Unlike the previous two\nchanges, I think this change deserves a new test.\n\n> I think I'm opening a can of worms with d95d728. There's nothing\n> wrong with that patch per se, but with this issue popping up, I need\n> to go over all {cache,index}_name_pos call sites and see what would be\n> the sensible behavior when i-t-a entries are involved.\n\nYeah, I agree that d95d728 should have been a part of a larger\nseries that changes the world order, instead of a single change that\nbrings inconsistency to the system.\n\nI cannot offhand convince myself that \"apply\" is the only casualty;\nassuming it is, I think a reasonable way forward is to keep d95d728\nand adjust \"apply\" to the new world order. Otherwise, i.e. if there\nare wider fallouts from d95d728, we may instead want to temporarily\nrevert it off from 'master', deal with fallouts to \"apply\" and other\nthings, before resurrecting it.\n\nAnything that internally uses \"diff-index\" is suspect, I think.\n\nWhat do others think? You seem to ...\n\n> So far blame, rm and checkout-entry and \"checkout \" are on my\n> to-think-or-fix list. But this patch can get in early to fix a real\n> regression instead of waiting for one big series. A lot more\n> discussions will be had before that series gets in good shape.\n\n... think that the damage could be quite extensive, so I am inclined\nto say that we first revert d95d728 before going forward.\n\n> builtin/apply.c | 8 ++++----\n> cache.h | 2 ++\n> read-cache.c | 12 ++++++++++++\n> t/t2203-add-intent.sh | 10 ++++++++++\n> 4 files changed, 28 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 146be97..4f813ac 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -3344,7 +3344,7 @@ static int load_current(struct image *image, struct patch *patch)\n> \tif (!patch->is_new)\n> \t\tdie(\"BUG: patch to %s is not a creation\", patch->old_name);\n> \n> -\tpos = cache_name_pos(name, strlen(name));\n> +\tpos = exists_in_cache(name, strlen(name));\n\nSomething that is named as if it would return yes/no that returns a\nreal value is not a very welcome idea.\n\n> +/* This is the same as index_name_pos, except that i-t-a entries are invisible */\n> +int exists_in_index(const struct index_state *istate, const char *name, int namelen)\n> +{\n> +\tint pos = index_name_stage_pos(istate, name, namelen, 0);\n> +\n> +\tif (pos < 0)\n> +\t\treturn pos;\n> +\tif (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD)\n> +\t\treturn -pos-1;\n\nThis is a useless complexity. Your callers cannot use the returned\nvalue like this:\n\n\tpos = exists_in_cache(...);\n if (pos < 0) {\n \tif (active_cache[-pos-1]->ce_flags & CE_INTENT_TO_ADD)\n\t\t\t; /* ah it actually exists but it is i-t-a */\n\t\telse\n ; /* no it does not really exist */\n\t} else {\n \t; /* yes it is really there at pos */\n\t}\n\nbecause they cannot tell two cases apart: (1) you do have i-t-a with\nthe given name, (2) you do not have the entry but the location you\nwould insert an entry with such a name is occupied by an unrelated\nentry (i.e. with a name that sorts adjacent) that happens to be\ni-t-a.\n\n> +\treturn pos;\n> +}\n"},{"id":"264644","messageId":"xmqqh9pye378.fsf@gitster.dls.corp.google.com","threadId":"39692","inReplyTo":"xmqqoak6e5dx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] apply: fix adding new files on i-t-a entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-23T17:37:15Z","receivedAt":"2015-06-23T17:37:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> I think I'm opening a can of worms with d95d728....\n>\n> I cannot offhand convince myself that \"apply\" is the only casualty;\n> assuming it is, I think a reasonable way forward is to keep d95d728\n> and adjust \"apply\" to the new world order. Otherwise, i.e. if there\n> are wider fallouts from d95d728, we may instead want to temporarily\n> revert it off from 'master', deal with fallouts to \"apply\" and other\n> things, before resurrecting it.\n>\n> Anything that internally uses \"diff-index\" is suspect, I think.\n>\n> What do others think? You seem to ...\n>\n>> So far blame, rm and checkout-entry and \"checkout \" are on my\n>> to-think-or-fix list. But this patch can get in early to fix a real\n>> regression instead of waiting for one big series. A lot more\n>> discussions will be had before that series gets in good shape.\n>\n> ... think that the damage could be quite extensive, so I am inclined\n> to say that we first revert d95d728 before going forward.\n\nLet's do this on 'nd/diff-i-t-a' topic and merge the result\nimmediately to 'master' for now.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Tue, 23 Jun 2015 10:27:47 -0700\nSubject: [PATCH] Revert \"diff-lib.c: adjust position of i-t-a entries in diff\"\n\nThis reverts commit d95d728aba06a34394d15466045cbdabdada58a2.\n\nIt turns out that many other commands that need to interact with the\nresult of running diff-files and diff-index, e.g. \"git apply\", \"git\nrm\", etc., need to be adjusted to the new world order it brings in.\nFor example, it would break this sequence to correct a whitespace\nbreakage in the parts you changed:\n\n\tgit add -N file\n\tgit diff --cached file | git apply --cached --whitespace=fix\n\tgit checkout file\n\nIn the old world order, \"diff\" showed a patch to modify an existing\nempty file by adding its full contents, and \"apply\" updated the\nindex by modifying the existing empty blob (which is what an\nIntent-to-Add entry records in the index) with that patch.\n\nIn the new world order, \"diff\" shows a patch to create a new file\nwith its full contents, but because \"apply\" thinks that the i-t-a\nentry already exists in the index, it refused to accept a creation.\n\nAdjusting \"apply\" to this new world order is easy, but we need to\nassess the extent of the damage to the rest of the system the new\nworld order brought in before going forward and adjust them all,\nafter which we can resurrect the commit being reverted here.\n---\n builtin/add.c | 1 -\n diff-lib.c | 12 ------------\n t/t2203-add-intent.sh | 23 ++++-------------------\n t/t4011-diff-symlink.sh | 10 ++++------\n 4 files changed, 8 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex ee370b0..3390933 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -63,7 +63,6 @@ static void update_callback(struct diff_queue_struct *q,\n \t\tswitch (fix_unmerged_status(p, data)) {\n \t\tdefault:\n \t\t\tdie(_(\"unexpected diff status %c\"), p->status);\n-\t\tcase DIFF_STATUS_ADDED:\n \t\tcase DIFF_STATUS_MODIFIED:\n \t\tcase DIFF_STATUS_TYPE_CHANGED:\n \t\t\tif (add_file_to_index(&the_index, path, data->flags)) {\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 714501a..a85c497 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -212,11 +212,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\t\t ce->sha1, !is_null_sha1(ce->sha1),\n \t\t\t\t\t ce->name, 0);\n \t\t\t\tcontinue;\n-\t\t\t} else if (ce->ce_flags & CE_INTENT_TO_ADD) {\n-\t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n-\t\t\t\t\t EMPTY_BLOB_SHA1_BIN, 0,\n-\t\t\t\t\t ce->name, 0);\n-\t\t\t\tcontinue;\n \t\t\t}\n \n \t\t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n@@ -381,13 +376,6 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \tstruct rev_info *revs = o->unpack_data;\n \tint match_missing, cached;\n \n-\t/* i-t-a entries do not actually exist in the index */\n-\tif (idx && (idx->ce_flags & CE_INTENT_TO_ADD)) {\n-\t\tidx = NULL;\n-\t\tif (!tree)\n-\t\t\treturn;\t/* nothing to diff.. */\n-\t}\n-\n \t/* if the entry is not checked out, don't examine work tree */\n \tcached = o->index_only ||\n \t\t(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 7c641bf..2a4a749 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -5,24 +5,10 @@ test_description='Intent to add'\n . ./test-lib.sh\n \n test_expect_success 'intent to add' '\n-\ttest_commit 1 &&\n-\tgit rm 1.t &&\n-\techo hello >1.t &&\n \techo hello >file &&\n \techo hello >elif &&\n \tgit add -N file &&\n-\tgit add elif &&\n-\tgit add -N 1.t\n-'\n-\n-test_expect_success 'git status' '\n-\tgit status --porcelain | grep -v actual >actual &&\n-\tcat >expect <<-\\EOF &&\n-\tDA 1.t\n-\tA elif\n-\t A file\n-\tEOF\n-\ttest_cmp expect actual\n+\tgit add elif\n '\n \n test_expect_success 'check result of \"add -N\"' '\n@@ -57,8 +43,7 @@ test_expect_success 'i-t-a entry is simply ignored' '\n \tgit add -N nitfol &&\n \tgit commit -m second &&\n \ttest $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&\n-\ttest $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&\n-\ttest $(git diff --name-only -- nitfol | wc -l) = 1\n+\ttest $(git diff --name-only HEAD -- nitfol | wc -l) = 1\n '\n \n test_expect_success 'can commit with an unrelated i-t-a entry in index' '\n@@ -87,13 +72,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '\n \t: >dir/bar &&\n \tgit add -N dir/bar &&\n \tgit diff --cached --name-only >actual &&\n-\t>expect &&\n+\techo dir/bar >expect &&\n \ttest_cmp expect actual &&\n \n \tgit write-tree >/dev/null &&\n \n \tgit diff --cached --name-only >actual &&\n-\t>expect &&\n+\techo dir/bar >expect &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh\nindex 7452fce..13e7f62 100755\n--- a/t/t4011-diff-symlink.sh\n+++ b/t/t4011-diff-symlink.sh\n@@ -139,13 +139,11 @@ test_expect_success SYMLINKS 'setup symlinks with attributes' '\n test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' '\n \tcat >expect <<-\\EOF &&\n \tdiff --git a/file.bin b/file.bin\n-\tnew file mode 100644\n-\tindex 0000000..d95f3ad\n-\tBinary files /dev/null and b/file.bin differ\n+\tindex e69de29..d95f3ad 100644\n+\tBinary files a/file.bin and b/file.bin differ\n \tdiff --git a/link.bin b/link.bin\n-\tnew file mode 120000\n-\tindex 0000000..dce41ec\n-\t--- /dev/null\n+\tindex e69de29..dce41ec 120000\n+\t--- a/link.bin\n \t+++ b/link.bin\n \t@@ -0,0 +1 @@\n \t+file.bin\n-- \n2.4.4-598-g4ab0ce8\n"},{"id":"264702","messageId":"CAPc5daXgEt==wpWH7CBXQFFZVSMuTR1PUMZqwQDmRG+QTJyt0g@mail.gmail.com","threadId":"39692","inReplyTo":"xmqqoak6e5dx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] apply: fix adding new files on i-t-a entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-24T04:48:33Z","receivedAt":"2015-06-24T04:48:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Jun 23, 2015 at 9:50 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> - pos = cache_name_pos(name, strlen(name));\n>> + pos = exists_in_cache(name, strlen(name));\n>\n> Something that is named as if it would return yes/no that returns a\n> real value is not a very welcome idea.\n>\n>> +/* This is the same as index_name_pos, except that i-t-a entries are invisible */\n>> +int exists_in_index(const struct index_state *istate, const char *name, int namelen)\n>> +{\n>> + int pos = index_name_stage_pos(istate, name, namelen, 0);\n>> +\n>> + if (pos < 0)\n>> + return pos;\n>> + if (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD)\n>> + return -pos-1;\n>\n> This is a useless complexity. Your callers cannot use the returned\n> value like this:\n>\n> pos = exists_in_cache(...);\n> if (pos < 0) {\n> if (active_cache[-pos-1]->ce_flags & CE_INTENT_TO_ADD)\n> ; /* ah it actually exists but it is i-t-a */\n> else\n> ; /* no it does not really exist */\n> } else {\n> ; /* yes it is really there at pos */\n> }\n>\n> because they cannot tell two cases apart: (1) you do have i-t-a with\n> the given name, (2) you do not have the entry but the location you\n> would insert an entry with such a name is occupied by an unrelated\n> entry (i.e. with a name that sorts adjacent) that happens to be\n> i-t-a.\n\nAlso, the callers cannot even use that return value in the usual way they\nwould use the return value from index_name_pos(), either.\n\n pos = exists_in_cache(...);\n if (pos < 0) {\n /* ah, it does not exist, so... */\n pos = -1 - pos;\n /*\n * ... it is OK to shift active_cache[pos..] by one and add our\n * entry at active_cache[pos]\n */\n } else {\n /* it exists, so update in place */\n ;\n }\n\nSo, returning pos that smells like a return value from index_name_pos()\nonly has an effect of confusing callers into buggy code, I am afraid. The\ncallers that care need to be updated to check for ce_flags after finding the\nentry with index_name_pos() the usual way if you want to avoid search in\nthe index_state->cache[] twice, and the callers that are only interested in\nknowing if an entry \"exists\" are better off with an exists_in_cache() that\nreturns Yes/No and not a confusing and useless \"pos\", I think.\n"},{"id":"264716","messageId":"CACsJy8Ap4uNi3gXV8Y+S18xtLaZ1R6DscEf7wJKjt59ZLOAJ5Q@mail.gmail.com","threadId":"39692","inReplyTo":"xmqqoak6e5dx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] apply: fix adding new files on i-t-a entries","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-24T10:11:37Z","receivedAt":"2015-06-24T10:11:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jun 23, 2015 at 11:50 PM, Junio C Hamano wrote:\n>> This patch tightens the \"exists in index\" check, ignoring i-t-a\n>> entries. For fixing the above problem, only the change in\n>> check_to_create() is needed.\n>\n> And the first thing I noticed and found a bit disturbing was that\n> this change (which I think is correct, and happens to match what I\n> sent out earlier) was the only thing necessary to make your new test\n> pass. IOW, the other changes in this patch have no test coverage.\n\nBecause to be honest I don't understand these code well enough to know\nhow to trigger it :)\n\n>> For other changes,\n>>\n>> - load_current(), reporting \"not exists in index\" is better than \"does\n>> not match index\"\n>\n> Is that error reporting the only side effect from this change?\n\nThe only thing that I can see. If an i-t-a entry is returned, it can't\nget past verify_index_match because ce_match_stat(). Hmm.. no the\non-disk version is gone, we'll get to checkout_target() where it will\n\"restore\" the entry with an empty file. This is related to checkout\nthat I will continue later below.\n\n>> - get_current_sha1(), or actually build_fake_ancestor(), we should not\n>> add i-t-a entries to the temporary index, at least not without also\n>> adding i-t-a flag back\n>\n> This is part of \"am\" three-way fallback codepath. I do not think\n> the merge-recursive three-way merge code knows and cares about, is\n> capable of handling, or would even want to deal with i-t-a entries\n> in the first place, so adding an entry as i-t-a bit would not help.\n> What the ultimate caller wants from us in this codepath is a tree\n> object, and that is written out from the temporary index---and that\n> codepath ignores i-t-a entries, so it is correct to omit them from\n> the temporary index in the first place. Unlike the previous two\n> changes, I think this change deserves a new test.\n\nWill do, after I study some more about this apply.c.\n\n>\n>> I think I'm opening a can of worms with d95d728. There's nothing\n>> wrong with that patch per se, but with this issue popping up, I need\n>> to go over all {cache,index}_name_pos call sites and see what would be\n>> the sensible behavior when i-t-a entries are involved.\n>\n> Yeah, I agree that d95d728 should have been a part of a larger\n> series that changes the world order, instead of a single change that\n> brings inconsistency to the system.\n>\n> I cannot offhand convince myself that \"apply\" is the only casualty;\n> assuming it is, I think a reasonable way forward is to keep d95d728\n> and adjust \"apply\" to the new world order. Otherwise, i.e. if there\n> are wider fallouts from d95d728, we may instead want to temporarily\n> revert it off from 'master', deal with fallouts to \"apply\" and other\n> things, before resurrecting it.\n>\n> Anything that internally uses \"diff-index\" is suspect, I think.\n\nYeah that's one or two more grep runs and more reading.\n\n> What do others think? You seem to ...\n>\n>> So far blame, rm and checkout-entry and \"checkout \" are on my\n>> to-think-or-fix list. But this patch can get in early to fix a real\n>> regression instead of waiting for one big series. A lot more\n>> discussions will be had before that series gets in good shape.\n>\n> ... think that the damage could be quite extensive, so I am inclined\n> to say that we first revert d95d728 before going forward.\n\nI'm not opposed to reverting if you think it's the safest option and I\nwill report back soon after grepping diff-index. But those I mentioned\nabove have more to do with the fact that an i-t-a entry does exist in\nthe index in a normal way, so reverting does not help.\n\nTake checkout for example, when you do \"git checkout -- foo\" where foo\nis i-t-a, the file foo on disk will be emptied because the SHA-1 in\nthe i-t-a entry is an empty blob, mostly to help \"git diff\". I think\nit should behave as if foo is not i-t-a: checkout should error out\nabout not matching pathspec, or at least not destroy \"foo\" on disk. To\nme, when \"ce\" is an i-t-a entry, only i-t-a flag and ce_name are\nvalid, the rest of \"ce\" should never be accessed.\n\nblame.c's situation is close to check_preimage() where it may read\nzero from ce_mode. It may be ok for check_preimage() to take zero as\nmode, but I think this is like fixed size buffer vs strbuf again. It\nworks now, but if the code is reorganized or refactored, then it may\nor may not work. Better be safe than sorry and avoid reading something\nwe should not read in the first place.\n\n>> builtin/apply.c | 8 ++++----\n>> cache.h | 2 ++\n>> read-cache.c | 12 ++++++++++++\n>> t/t2203-add-intent.sh | 10 ++++++++++\n>> 4 files changed, 28 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/builtin/apply.c b/builtin/apply.c\n>> index 146be97..4f813ac 100644\n>> --- a/builtin/apply.c\n>> +++ b/builtin/apply.c\n>> @@ -3344,7 +3344,7 @@ static int load_current(struct image *image, struct patch *patch)\n>> if (!patch->is_new)\n>> die(\"BUG: patch to %s is not a creation\", patch->old_name);\n>>\n>> - pos = cache_name_pos(name, strlen(name));\n>> + pos = exists_in_cache(name, strlen(name));\n>\n> Something that is named as if it would return yes/no that returns a\n> real value is not a very welcome idea.\n\nYeah. But I don't want the caller to call cache_name_pos again to get\n\"pos\". If I can't find a better name, I'll probably go with\ncache_name_pos_without_ita().\n\n>> +/* This is the same as index_name_pos, except that i-t-a entries are invisible */\n>> +int exists_in_index(const struct index_state *istate, const char *name, int namelen)\n>> +{\n>> + int pos = index_name_stage_pos(istate, name, namelen, 0);\n>> +\n>> + if (pos < 0)\n>> + return pos;\n>> + if (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD)\n>> + return -pos-1;\n>\n> This is a useless complexity. Your callers cannot use the returned\n> value like this:\n>\n> pos = exists_in_cache(...);\n> if (pos < 0) {\n> if (active_cache[-pos-1]->ce_flags & CE_INTENT_TO_ADD)\n> ; /* ah it actually exists but it is i-t-a */\n> else\n> ; /* no it does not really exist */\n> } else {\n> ; /* yes it is really there at pos */\n> }\n>\n> because they cannot tell two cases apart: (1) you do have i-t-a with\n> the given name, (2) you do not have the entry but the location you\n> would insert an entry with such a name is occupied by an unrelated\n> entry (i.e. with a name that sorts adjacent) that happens to be\n> i-t-a.\n\nOK so either return -1 when the entry does not exist or is i-t-a and\nnon-negative otherwise.\n-- \nDuy\n"},{"id":"264738","messageId":"xmqqzj3p9gv7.fsf@gitster.dls.corp.google.com","threadId":"39692","inReplyTo":"CACsJy8Ap4uNi3gXV8Y+S18xtLaZ1R6DscEf7wJKjt59ZLOAJ5Q@mail.gmail.com","subject":"Re: [PATCH] apply: fix adding new files on i-t-a entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-24T17:05:32Z","receivedAt":"2015-06-24T17:05:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Take checkout for example, when you do \"git checkout -- foo\" where foo\n> is i-t-a, the file foo on disk will be emptied because the SHA-1 in\n> the i-t-a entry is an empty blob, mostly to help \"git diff\". I think\n> it should behave as if foo is not i-t-a: checkout should error out\n> about not matching pathspec, or at least not destroy \"foo\" on disk. To\n> me, when \"ce\" is an i-t-a entry, only i-t-a flag and ce_name are\n> valid, the rest of \"ce\" should never be accessed.\n>\n> blame.c's situation is close to check_preimage() where it may read\n> zero from ce_mode. It may be ok for check_preimage() to take zero as\n> mode, but I think this is like fixed size buffer vs strbuf again. It\n> works now, but if the code is reorganized or refactored, then it may\n> or may not work. Better be safe than sorry and avoid reading something\n> we should not read in the first place.\n\nAll of the above say there _are_ some codepaths that want to treat\nthe existence of a path in the index not as \nboolean but as tristate. I do not\nthink we disagree on that.\n\nBut that is different from saying that it is always OK to treat\ni-t-a entries the same way as \"does not exist\" non-entries.\n\nInternal \"diff-index --cached\" is used for another reason you did\nnot mention (and scripted Porcelains literally use that externally\nfor the same reason). When we start a mergy operation, we say it is\nOK if the working tree has local changes relative to the index, but\nwe require the index does not have any modifications since the HEAD\nwas read.\n\n\tSide note: some codepaths insist that \"diff-index --cached\"\n and \"diff-files\" are both clean, so d95d728a is harmless;\n the former may say \"clean\" on i-t-a entries more than\n before, but the latter will still catch the difference\n between the index and the working tree and stop the caller\n from going forward.\n\nWith d95d728a (diff-lib.c: adjust position of i-t-a entries in diff,\n2015-03-16)'s world view, an empty output from \"diff-index --cached\"\nno longer means that. Entries added with any \"git add -N\" are not\nreported, so we would go ahead to record the merge result on top of\nthat \"half-dirty\" index.\n\n\tSide note: a merge based on unpack-trees has an extra layer\n\tof safety that unpack_trees() does not ignore i-t-a entry as\n\t\"not exist (yet)\" and notices that the path does exist in\n\tthe index but not in HEAD. But that just shows that some\n\tparts of the world do need to consider that having an i-t-a\n\tin the index makes it different from HEAD.\n\nIf the mergy operation goes without any conflict, the next thing we\ndo typically is to write the index out as a tree (to record in a new\ncommit, etc.) and we are OK only in that particular case, because\ni-t-a entries are ignored. But what would happen when the mergy\noperation conflicts? I haven't thought about that fully, but I\ndoubt it is a safe thing to do in general.\n\nBut that is just one example that there are also other codepaths\nthat do not want to be fooled into thinking that i-t-a entries do\nnot exist in the index at all.\n\nAll we learned from the above discussion is that unconditionally\ndeclaring that adding i-t-a entries to the index without doing\nanything else should keep the index compare the same to HEAD.\n\nIf d95d728a were only about what wt_status.c sees (and gets reported\nin \"git status\" output), which was what the change wanted to address\nanyway, we didn't have to have this discussion. Without realizing\nthat two kinds of callers want different view out of \"diff-index\n--cached\" and \"diff-files\", we broke half the world by changing the\nworld order unconditionally for everybody, I am afraid.\n\nPerhaps a good and safe way forward to resurrect what d95d728a\nwanted to do is to first add an option to tell run_diff_index() and\nrun_diff_files() which behaviour the caller wants to see, add that\nonly to the caller in wt-status.c? Then incrementally pass that\noption from more callsites that we are absolutely certain that want\nthis different worldview with respect to i-t-a?\n"},{"id":"264841","messageId":"CACsJy8AVeGxOS4pWfkcbPnxRQ0-gJRC7B1NSs+Ci81x46ieDzQ@mail.gmail.com","threadId":"39692","inReplyTo":"xmqqzj3p9gv7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] apply: fix adding new files on i-t-a entries","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-25T12:26:17Z","receivedAt":"2015-06-25T12:26:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 25, 2015 at 12:05 AM, Junio C Hamano wrote:\n> Internal \"diff-index --cached\" is used for another reason you did\n> not mention (and scripted Porcelains literally use that externally\n> for the same reason). When we start a mergy operation, we say it is\n> OK if the working tree has local changes relative to the index, but\n> we require the index does not have any modifications since the HEAD\n> was read.\n>\n> Side note: some codepaths insist that \"diff-index --cached\"\n> and \"diff-files\" are both clean, so d95d728a is harmless;\n> the former may say \"clean\" on i-t-a entries more than\n> before, but the latter will still catch the difference\n> between the index and the working tree and stop the caller\n> from going forward.\n>\n> With d95d728a (diff-lib.c: adjust position of i-t-a entries in diff,\n> 2015-03-16)'s world view, an empty output from \"diff-index --cached\"\n> no longer means that. Entries added with any \"git add -N\" are not\n> reported, so we would go ahead to record the merge result on top of\n> that \"half-dirty\" index.\n>\n> Side note: a merge based on unpack-trees has an extra layer\n> of safety that unpack_trees() does not ignore i-t-a entry as\n> \"not exist (yet)\" and notices that the path does exist in\n> the index but not in HEAD. But that just shows that some\n> parts of the world do need to consider that having an i-t-a\n> in the index makes it different from HEAD.\n>\n> If the mergy operation goes without any conflict, the next thing we\n> do typically is to write the index out as a tree (to record in a new\n> commit, etc.) and we are OK only in that particular case, because\n> i-t-a entries are ignored. But what would happen when the mergy\n> operation conflicts? I haven't thought about that fully, but I\n> doubt it is a safe thing to do in general.\n>\n> But that is just one example that there are also other codepaths\n> that do not want to be fooled into thinking that i-t-a entries do\n> not exist in the index at all.\n\nI think it's clear that you need to revert that commit. I didn't see\nthis at all when I made the commit.\n\n> All we learned from the above discussion is that unconditionally\n> declaring that adding i-t-a entries to the index without doing\n> anything else should keep the index compare the same to HEAD.\n>\n> If d95d728a were only about what wt_status.c sees (and gets reported\n> in \"git status\" output), which was what the change wanted to address\n> anyway, we didn't have to have this discussion. Without realizing\n> that two kinds of callers want different view out of \"diff-index\n> --cached\" and \"diff-files\", we broke half the world by changing the\n> world order unconditionally for everybody, I am afraid.\n>\n> Perhaps a good and safe way forward to resurrect what d95d728a\n> wanted to do is to first add an option to tell run_diff_index() and\n> run_diff_files() which behaviour the caller wants to see, add that\n> only to the caller in wt-status.c? Then incrementally pass that\n> option from more callsites that we are absolutely certain that want\n> this different worldview with respect to i-t-a?\n\nAgreed.\n-- \nDuy\n"},{"id":"264845","messageId":"xmqqy4j80wdb.fsf@gitster.dls.corp.google.com","threadId":"39692","inReplyTo":"CACsJy8AVeGxOS4pWfkcbPnxRQ0-gJRC7B1NSs+Ci81x46ieDzQ@mail.gmail.com","subject":"Re: [PATCH] apply: fix adding new files on i-t-a entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-25T13:07:44Z","receivedAt":"2015-06-25T13:07:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> I think it's clear that you need to revert that commit. I didn't see\n> this at all when I made the commit.\n\nI didn't either, and no other reviewers did. But now we know it was\nnot sufficient, so let's see...\n\n>> Perhaps a good and safe way forward to resurrect what d95d728a\n>> wanted to do is to first add an option to tell run_diff_index() and\n>> run_diff_files() which behaviour the caller wants to see, add that\n>> only to the caller in wt-status.c? Then incrementally pass that\n>> option from more callsites that we are absolutely certain that want\n>> this different worldview with respect to i-t-a?\n>\n> Agreed.\n\nOK. Perhaps then first I should do that revert and we'll\nincrementally rebuild on top.\n\nThanks.\n"},{"id":"268474","messageId":"1440205700-19749-1-git-send-email-pclouds@gmail.com","threadId":"39692","inReplyTo":"xmqqy4j80wdb.fsf@gitster.dls.corp.google.com","subject":"[PATCH 0/8] Resurrect \"diff-lib.c: adjust position of i-t-a entries in diff\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-22T01:08:03Z","receivedAt":"2015-08-22T01:08:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 25, 2015 at 8:07 PM, Junio C Hamano wrote:\n>>> Perhaps a good and safe way forward to resurrect what d95d728a\n>>> wanted to do is to first add an option to tell run_diff_index() and\n>>> run_diff_files() which behaviour the caller wants to see, add that\n>>> only to the caller in wt-status.c?  Then incrementally pass that\n>>> option from more callsites that we are absolutely certain that want\n>>> this different worldview with respect to i-t-a?\n>>\n>> Agreed.\n>\n> OK.  Perhaps then first I should do that revert and we'll\n> incrementally rebuild on top.\n\nHere comes the rebuild. This adds --shift-ita option (and an internal\nflag) to enable this behavior. Those only take the last two patches.\nThe remaining is to make sure we handle i-t-a entries correctly in\nsome commands.\n\nNguyễn Thái Ngọc Duy (8):\n blame: remove obsolete comment\n Add and use convenient macro ce_intent_to_add()\n apply: fix adding new files on i-t-a entries\n apply: make sure check_preimage() does not leave empty file on error\n checkout(-index): do not checkout i-t-a entries\n grep: make it clear i-t-a entries are ignored\n diff.h: extend \"flags\" field to 64 bits because we're out of bits\n Resurrect \"diff-lib.c: adjust position of i-t-a entries in diff\"\n\n Documentation/diff-options.txt | 6 +++\n builtin/apply.c | 13 ++++---\n builtin/blame.c | 5 ---\n builtin/checkout-index.c | 5 ++-\n builtin/checkout.c | 2 +\n builtin/commit.c | 2 +-\n builtin/grep.c | 2 +-\n builtin/rm.c | 2 +-\n cache-tree.c | 2 +-\n cache.h | 1 +\n diff-lib.c | 18 ++++++++-\n diff.c | 4 +-\n diff.h | 9 +++--\n read-cache.c | 4 +-\n t/t2203-add-intent.sh | 83 +++++++++++++++++++++++++++++++++++++++++-\n wt-status.c | 2 +\n 16 files changed, 134 insertions(+), 26 deletions(-)\n\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"268475","messageId":"1440205700-19749-2-git-send-email-pclouds@gmail.com","threadId":"39692","inReplyTo":"1440205700-19749-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/8] blame: remove obsolete comment","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-22T01:08:04Z","receivedAt":"2015-08-22T01:08:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"That \"someday\" in the comment happened two years later in\nb65982b (Optimize \"diff-index --cached\" using cache-tree - 2009-05-20)\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/blame.c | 5 -----\n 1 file changed, 5 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 4db01c1..942f302 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2379,11 +2379,6 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \tce->ce_mode = create_ce_mode(mode);\n \tadd_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n \n-\t/*\n-\t * We are not going to write this out, so this does not matter\n-\t * right now, but someday we might optimize diff-index --cached\n-\t * with cache-tree information.\n-\t */\n \tcache_tree_invalidate_path(&the_index, path);\n \n \treturn commit;\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"268476","messageId":"1440205700-19749-3-git-send-email-pclouds@gmail.com","threadId":"39692","inReplyTo":"1440205700-19749-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/8] Add and use convenient macro ce_intent_to_add()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-22T01:08:05Z","receivedAt":"2015-08-22T01:08:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/rm.c | 2 +-\n cache-tree.c | 2 +-\n cache.h | 1 +\n read-cache.c | 4 ++--\n 4 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 80b972f..8829b09 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -211,7 +211,7 @@ static int check_local_mod(unsigned char *head, int index_only)\n \t\t * \"intent to add\" entry.\n \t\t */\n \t\tif (local_changes && staged_changes) {\n-\t\t\tif (!index_only || !(ce->ce_flags & CE_INTENT_TO_ADD))\n+\t\t\tif (!index_only || !ce_intent_to_add(ce))\n \t\t\t\tstring_list_append(&files_staged, name);\n \t\t}\n \t\telse if (!index_only) {\ndiff --git a/cache-tree.c b/cache-tree.c\nindex feace8b..c865bd2 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -377,7 +377,7 @@ static int update_one(struct cache_tree *it,\n \t\t * they are not part of generated trees. Invalidate up\n \t\t * to root to force cache-tree users to read elsewhere.\n \t\t */\n-\t\tif (ce->ce_flags & CE_INTENT_TO_ADD) {\n+\t\tif (ce_intent_to_add(ce)) {\n \t\t\tto_invalidate = 1;\n \t\t\tcontinue;\n \t\t}\ndiff --git a/cache.h b/cache.h\nindex 4e25271..41d811c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -241,6 +241,7 @@ static inline unsigned create_ce_flags(unsigned stage)\n #define ce_uptodate(ce) ((ce)->ce_flags & CE_UPTODATE)\n #define ce_skip_worktree(ce) ((ce)->ce_flags & CE_SKIP_WORKTREE)\n #define ce_mark_uptodate(ce) ((ce)->ce_flags |= CE_UPTODATE)\n+#define ce_intent_to_add(ce) ((ce)->ce_flags & CE_INTENT_TO_ADD)\n \n #define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)\n static inline unsigned int create_ce_mode(unsigned int mode)\ndiff --git a/read-cache.c b/read-cache.c\nindex 89dbc08..f849cbd 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -327,7 +327,7 @@ int ie_match_stat(const struct index_state *istate,\n \t * by definition never matches what is in the work tree until it\n \t * actually gets added.\n \t */\n-\tif (ce->ce_flags & CE_INTENT_TO_ADD)\n+\tif (ce_intent_to_add(ce))\n \t\treturn DATA_CHANGED | TYPE_CHANGED | MODE_CHANGED;\n \n \tchanged = ce_match_stat_basic(ce, st);\n@@ -1251,7 +1251,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \n \t\t\tif (cache_errno == ENOENT)\n \t\t\t\tfmt = deleted_fmt;\n-\t\t\telse if (ce->ce_flags & CE_INTENT_TO_ADD)\n+\t\t\telse if (ce_intent_to_add(ce))\n \t\t\t\tfmt = added_fmt; /* must be before other checks */\n \t\t\telse if (changed & TYPE_CHANGED)\n \t\t\t\tfmt = typechange_fmt;\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"268477","messageId":"1440205700-19749-4-git-send-email-pclouds@gmail.com","threadId":"39692","inReplyTo":"1440205700-19749-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/8] apply: fix adding new files on i-t-a entries","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-22T01:08:06Z","receivedAt":"2015-08-22T01:08:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Applying a patch that adds a file when that file is registered with \"git\nadd -N\" will fail with message \"already exists in index\" because\ngit-apply checks, sees those i-t-a entries and aborts. git-apply does\nnot realize those are for bookkeeping only, they do not really exist in\nthe index. This patch tightens the \"exists in index\" check, ignoring\ni-t-a entries.\n\nReported-by: Patrick Higgins \nReported-by: Bjørnar Snoksrud \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/apply.c | 9 +++++----\n t/t2203-add-intent.sh | 13 +++++++++++++\n 2 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 54aba4e..76b58a1 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3553,10 +3553,11 @@ static int check_to_create(const char *new_name, int ok_if_exists)\n {\n \tstruct stat nst;\n \n-\tif (check_index &&\n-\t cache_name_pos(new_name, strlen(new_name)) >= 0 &&\n-\t !ok_if_exists)\n-\t\treturn EXISTS_IN_INDEX;\n+\tif (check_index && !ok_if_exists) {\n+\t\tint pos = cache_name_pos(new_name, strlen(new_name));\n+\t\tif (pos >= 0 && !ce_intent_to_add(active_cache[pos]))\n+\t\t\treturn EXISTS_IN_INDEX;\n+\t}\n \tif (cached)\n \t\treturn 0;\n \ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 2a4a749..bb5ef2b 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -82,5 +82,18 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'apply adds new file on i-t-a entry' '\n+\tgit init apply &&\n+\t(\n+\t\tcd apply &&\n+\t\techo newcontent >newfile &&\n+\t\tgit add newfile &&\n+\t\tgit diff --cached >patch &&\n+\t\trm .git/index &&\n+\t\tgit add -N newfile &&\n+\t\tgit apply --cached patch\n+\t)\n+'\n+\n test_done\n \n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"268478","messageId":"1440205700-19749-5-git-send-email-pclouds@gmail.com","threadId":"39692","inReplyTo":"1440205700-19749-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 4/8] apply: make sure check_preimage() does not leave empty file on error","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-22T01:08:07Z","receivedAt":"2015-08-22T01:08:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The test case probably describes the test scenario the best. We have a\npatch to modify some file but the base file is gone. Because\ncheck_preimage() finds an index entry with the same old_name, it tries\nto restore the on-disk base file with cached content with\ncheckout_target() and move on.\n\nIf this old_name is i-t-a, before this patch \"apply -3\" will call\ncheckout_target() which will write an empty file (i-t-a entries always\nhave \"empty blob\" SHA-1), then it'll fail at verify_index_match()\n(i-t-a entries are always \"modified\") and the operation is aborted.\n\nThis empty file should not be created. Compare to the case where\nold_name does not exist in index, \"apply -3\" fails with a different\nerror message \"...: does not exist\" but it does not touch\nworktree. This patch makes sure the same happens to i-t-a entries.\n\nload_current() shares the same code pattern (look up an index entry,\nif on-disk version is not found, restore using the cached\nversion). Fix it too (even though it's not exercised in any test case)\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/apply.c | 4 ++--\n t/t2203-add-intent.sh | 16 ++++++++++++++++\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 76b58a1..b185c97 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3348,7 +3348,7 @@ static int load_current(struct image *image, struct patch *patch)\n \t\tdie(\"BUG: patch to %s is not a creation\", patch->old_name);\n \n \tpos = cache_name_pos(name, strlen(name));\n-\tif (pos < 0)\n+\tif (pos < 0 || ce_intent_to_add(active_cache[pos]))\n \t\treturn error(_(\"%s: does not exist in index\"), name);\n \tce = active_cache[pos];\n \tif (lstat(name, &st)) {\n@@ -3501,7 +3501,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n \n \tif (check_index && !previous) {\n \t\tint pos = cache_name_pos(old_name, strlen(old_name));\n-\t\tif (pos < 0) {\n+\t\tif (pos < 0 || ce_intent_to_add(active_cache[pos])) {\n \t\t\tif (patch->is_new < 0)\n \t\t\t\tgoto is_new;\n \t\t\treturn error(_(\"%s: does not exist in index\"), old_name);\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex bb5ef2b..96c8755 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -95,5 +95,21 @@ test_expect_success 'apply adds new file on i-t-a entry' '\n \t)\n '\n \n+test_expect_success 'apply:check_preimage() not creating empty file' '\n+\tgit init check-preimage &&\n+\t(\n+\t\tcd check-preimage &&\n+\t\techo oldcontent >newfile &&\n+\t\tgit add newfile &&\n+\t\techo newcontent >newfile &&\n+\t\tgit diff >patch &&\n+\t\trm .git/index &&\n+\t\tgit add -N newfile &&\n+\t\trm newfile &&\n+\t\ttest_must_fail git apply -3 patch &&\n+\t\t! test -f newfile\n+\t)\n+'\n+\n test_done\n \n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"268479","messageId":"1440205700-19749-6-git-send-email-pclouds@gmail.com","threadId":"39692","inReplyTo":"1440205700-19749-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 5/8] checkout(-index): do not checkout i-t-a entries","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-22T01:08:08Z","receivedAt":"2015-08-22T01:08:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The cached blob of i-t-a entries are empty blob. By checkout, we delete\nthe content we have. Don't do it.\n\nThis is done higher up instead of inside checkout_entry() because we\nwould have limited options in there: silently ignore, loudly ignore,\ndie. At higher level we can do better reporting. For example, \"git\ncheckout -- foo\" will complain that \"foo\" does not match pathspec, just\nlike when \"foo\" is not registered with \"git add -N\"\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/checkout-index.c | 5 ++++-\n builtin/checkout.c | 2 ++\n t/t2203-add-intent.sh | 34 ++++++++++++++++++++++++++++++++++\n 3 files changed, 40 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex 8028c37..eca975d 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -56,7 +56,8 @@ static int checkout_file(const char *name, const char *prefix)\n \twhile (pos < active_nr) {\n \t\tstruct cache_entry *ce = active_cache[pos];\n \t\tif (ce_namelen(ce) != namelen ||\n-\t\t memcmp(ce->name, name, namelen))\n+\t\t memcmp(ce->name, name, namelen) ||\n+\t\t ce_intent_to_add(ce))\n \t\t\tbreak;\n \t\thas_same_name = 1;\n \t\tpos++;\n@@ -99,6 +100,8 @@ static void checkout_all(const char *prefix, int prefix_length)\n \t\tif (ce_stage(ce) != checkout_stage\n \t\t && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))\n \t\t\tcontinue;\n+\t\tif (ce_intent_to_add(ce))\n+\t\t\tcontinue;\n \t\tif (prefix && *prefix &&\n \t\t (ce_namelen(ce) <= prefix_length ||\n \t\t memcmp(prefix, ce->name, prefix_length)))\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex e1403be..02889d4 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\t\t * anything to this entry at all.\n \t\t\t */\n \t\t\tcontinue;\n+\t\tif (ce_intent_to_add(ce))\n+\t\t\tcontinue;\n \t\t/*\n \t\t * Either this entry came from the tree-ish we are\n \t\t * checking the paths out of, or we are checking out\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 96c8755..d0f36a4 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -111,5 +111,39 @@ test_expect_success 'apply:check_preimage() not creating empty file' '\n \t)\n '\n \n+test_expect_success 'checkout ignores i-t-a' '\n+\tgit init checkout &&\n+\t(\n+\t\tcd checkout &&\n+\t\techo data >file &&\n+\t\tgit add -N file &&\n+\t\ttest_must_fail git checkout -- file &&\n+\t\techo data >expected &&\n+\t\ttest_cmp expected file\n+\t)\n+'\n+\n+test_expect_success 'checkout-index ignores i-t-a' '\n+\t(\n+\t\tcd checkout &&\n+\t\tgit checkout-index file &&\n+\t\techo data >expected &&\n+\t\ttest_cmp expected file\n+\t)\n+'\n+\n+test_expect_success 'checkout-index --all ignores i-t-a' '\n+\t(\n+\t\tcd checkout &&\n+\t\techo data >anotherfile &&\n+\t\tgit add anotherfile &&\n+\t\trm anotherfile &&\n+\t\tgit checkout-index --all &&\n+\t\techo data >expected &&\n+\t\ttest_cmp expected file &&\n+\t\ttest_cmp expected anotherfile\n+\t)\n+'\n+\n test_done\n \n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"268480","messageId":"1440205700-19749-7-git-send-email-pclouds@gmail.com","threadId":"39692","inReplyTo":"1440205700-19749-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 6/8] grep: make it clear i-t-a entries are ignored","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-22T01:08:09Z","receivedAt":"2015-08-22T01:08:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The expression \"!S_ISREG(ce)\" covers i-t-a entries as well because\nce->ce_mode would be zero then. I could make a comment saying that, but\nit's probably better just to comment with code, in case i-t-a entry\ncontent changes in future.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/grep.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex d04f440..f508179 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -375,7 +375,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n \n \tfor (nr = 0; nr < active_nr; nr++) {\n \t\tconst struct cache_entry *ce = active_cache[nr];\n-\t\tif (!S_ISREG(ce->ce_mode))\n+\t\tif (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce))\n \t\t\tcontinue;\n \t\tif (!ce_path_match(ce, pathspec, NULL))\n \t\t\tcontinue;\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"268481","messageId":"1440205874-20295-1-git-send-email-pclouds@gmail.com","threadId":"39692","inReplyTo":"1440205700-19749-7-git-send-email-pclouds@gmail.com","subject":"[PATCH 7/8] diff.h: extend \"flags\" field to 64 bits because we're out of bits","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-22T01:11:14Z","receivedAt":"2015-08-22T01:11:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"I renamed both \"flags\" and \"touched_flags\" fields while making this\npatch to make sure I was aware of how these flags were manipulated\n(besides DIFF_OPT* macros). So hopefully I didn't miss anything.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/commit.c | 2 +-\n diff-lib.c | 4 ++--\n diff.c | 2 +-\n diff.h | 8 +++++---\n 4 files changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 4cbd5ff..95f7296 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -895,7 +895,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t * submodules which were manually staged, which would\n \t\t\t * be really confusing.\n \t\t\t */\n-\t\t\tint diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n+\t\t\tdiff_flags_t diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n \t\t\tif (ignore_submodule_arg &&\n \t\t\t !strcmp(ignore_submodule_arg, \"all\"))\n \t\t\t\tdiff_flags |= DIFF_OPT_IGNORE_SUBMODULES;\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 241a843..ae09034 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -71,7 +71,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n {\n \tint changed = ce_match_stat(ce, st, ce_option);\n \tif (S_ISGITLINK(ce->ce_mode)) {\n-\t\tunsigned orig_flags = diffopt->flags;\n+\t\tdiff_flags_t orig_flags = diffopt->flags;\n \t\tif (!DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG))\n \t\t\tset_diffopt_flags_from_submodule_config(diffopt, ce->name);\n \t\tif (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES))\n@@ -516,7 +516,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)\n \treturn 0;\n }\n \n-int index_differs_from(const char *def, int diff_flags)\n+int index_differs_from(const char *def, diff_flags_t diff_flags)\n {\n \tstruct rev_info rev;\n \tstruct setup_revision_opt opt;\ndiff --git a/diff.c b/diff.c\nindex 7deac90..2485870 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4912,7 +4912,7 @@ int diff_can_quit_early(struct diff_options *opt)\n static int is_submodule_ignored(const char *path, struct diff_options *options)\n {\n \tint ignored = 0;\n-\tunsigned orig_flags = options->flags;\n+\tdiff_flags_t orig_flags = options->flags;\n \tif (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG))\n \t\tset_diffopt_flags_from_submodule_config(options, path);\n \tif (DIFF_OPT_TST(options, IGNORE_SUBMODULES))\ndiff --git a/diff.h b/diff.h\nindex f7208ad..4241aa5 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -110,13 +110,15 @@ enum diff_words_type {\n \tDIFF_WORDS_COLOR\n };\n \n+typedef uint64_t diff_flags_t;\n+\n struct diff_options {\n \tconst char *orderfile;\n \tconst char *pickaxe;\n \tconst char *single_follow;\n \tconst char *a_prefix, *b_prefix;\n-\tunsigned flags;\n-\tunsigned touched_flags;\n+\tdiff_flags_t flags;\n+\tdiff_flags_t touched_flags;\n \n \t/* diff-filter bits */\n \tunsigned int filter;\n@@ -347,7 +349,7 @@ extern int diff_result_code(struct diff_options *, int);\n \n extern void diff_no_index(struct rev_info *, int, const char **, const char *);\n \n-extern int index_differs_from(const char *def, int diff_flags);\n+extern int index_differs_from(const char *def, diff_flags_t diff_flags);\n \n extern size_t fill_textconv(struct userdiff_driver *driver,\n \t\t\t struct diff_filespec *df,\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"268657","messageId":"xmqqh9nn1ewd.fsf@gitster.dls.corp.google.com","threadId":"39692","inReplyTo":"1440205700-19749-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/8] apply: fix adding new files on i-t-a entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T17:01:06Z","receivedAt":"2015-08-25T17:01:06Z","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> Applying a patch that adds a file when that file is registered with \"git\n> add -N\" will fail with message \"already exists in index\" because\n> git-apply checks, sees those i-t-a entries and aborts. git-apply does\n> not realize those are for bookkeeping only, they do not really exist in\n> the index. This patch tightens the \"exists in index\" check, ignoring\n> i-t-a entries.\n\nSuppose that you came up with some contents to register at path F in\nyour working tree, told Git about your intention with \"add -N F\",\nand then tried to apply a patch that wants to _create_ F:\n\nWithout this patch, we would say \"F already exists so a patch to\ncreate is incompatible with our current state\".\n\nWith this patch, we do not say that, but instead we'll hopefully\ntrigger \"does it exist in the working tree\" check, unless you are\nrunning under \"--cached\".\n\nWhich means that this change will not lead to data loss in the\n\"untracked\" file F in the working tree that was merely added to the\nindex with i-t-a bit.\n\nAs you did not say the equivalent of the last paragraph above in the\nlog message, my knee-jerk reaction was \"why could this possibly be a\ngood idea?\"\n\nPlease try again with a better log message.\n\nThanks.\n"},{"id":"268659","messageId":"xmqqd1yb1e7a.fsf@gitster.dls.corp.google.com","threadId":"39692","inReplyTo":"1440205700-19749-5-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 4/8] apply: make sure check_preimage() does not leave empty file on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T17:16:09Z","receivedAt":"2015-08-25T17:16:09Z","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> The test case probably describes the test scenario the best. We have a\n> patch to modify some file but the base file is gone. Because\n> check_preimage() finds an index entry with the same old_name, it tries\n> to restore the on-disk base file with cached content with\n> checkout_target() and move on.\n>\n> If this old_name is i-t-a, before this patch \"apply -3\" will call\n> checkout_target() which will write an empty file (i-t-a entries always\n> have \"empty blob\" SHA-1), then it'll fail at verify_index_match()\n> (i-t-a entries are always \"modified\") and the operation is aborted.\n>\n> This empty file should not be created. Compare to the case where\n> old_name does not exist in index, \"apply -3\" fails with a different\n> error message \"...: does not exist\" but it does not touch\n> worktree. This patch makes sure the same happens to i-t-a entries.\n\nThis part (unlike 3/8) is very well explained, and makes sense to\nme.\n\n> load_current() shares the same code pattern (look up an index entry,\n> if on-disk version is not found, restore using the cached\n> version).\n\nThe \"fall-back to the cached one\" is very much deliberate to allow\nus to work with an empty working tree as long as the index is\ncorrect, so this perhaps makes sense. If the user said \"add -N F\",\nand F is involved in the patch application, we do not want to\nconsider F exists in the index and we would instead want to pretend\nthat we do not have F ourselves. It does not even make sense to do\n\"apply -3\" for such a path, and rejecting with \"does not exist\" is a\ngood thing to do.\n\nGood.\n\n> Fix it too (even though it's not exercised in any test case)\n\nHmm, as this is adding new test for the other case, perhaps this\ncase is covered by another one, too?\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/apply.c | 4 ++--\n> t/t2203-add-intent.sh | 16 ++++++++++++++++\n> 2 files changed, 18 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 76b58a1..b185c97 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -3348,7 +3348,7 @@ static int load_current(struct image *image, struct patch *patch)\n> \t\tdie(\"BUG: patch to %s is not a creation\", patch->old_name);\n> \n> \tpos = cache_name_pos(name, strlen(name));\n> -\tif (pos < 0)\n> +\tif (pos < 0 || ce_intent_to_add(active_cache[pos]))\n> \t\treturn error(_(\"%s: does not exist in index\"), name);\n> \tce = active_cache[pos];\n> \tif (lstat(name, &st)) {\n> @@ -3501,7 +3501,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n> \n> \tif (check_index && !previous) {\n> \t\tint pos = cache_name_pos(old_name, strlen(old_name));\n> -\t\tif (pos < 0) {\n> +\t\tif (pos < 0 || ce_intent_to_add(active_cache[pos])) {\n> \t\t\tif (patch->is_new < 0)\n> \t\t\t\tgoto is_new;\n> \t\t\treturn error(_(\"%s: does not exist in index\"), old_name);\n> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\n> index bb5ef2b..96c8755 100755\n> --- a/t/t2203-add-intent.sh\n> +++ b/t/t2203-add-intent.sh\n> @@ -95,5 +95,21 @@ test_expect_success 'apply adds new file on i-t-a entry' '\n> \t)\n> '\n> \n> +test_expect_success 'apply:check_preimage() not creating empty file' '\n> +\tgit init check-preimage &&\n> +\t(\n> +\t\tcd check-preimage &&\n> +\t\techo oldcontent >newfile &&\n> +\t\tgit add newfile &&\n> +\t\techo newcontent >newfile &&\n> +\t\tgit diff >patch &&\n> +\t\trm .git/index &&\n> +\t\tgit add -N newfile &&\n> +\t\trm newfile &&\n> +\t\ttest_must_fail git apply -3 patch &&\n> +\t\t! test -f newfile\n> +\t)\n> +'\n> +\n> test_done\n"},{"id":"268666","messageId":"xmqq8u8z1d8r.fsf@gitster.dls.corp.google.com","threadId":"39692","inReplyTo":"1440205700-19749-6-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 5/8] checkout(-index): do not checkout i-t-a entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T17:36:52Z","receivedAt":"2015-08-25T17:36:52Z","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> The cached blob of i-t-a entries are empty blob. By checkout, we delete\n> the content we have. Don't do it.\n>\n> This is done higher up instead of inside checkout_entry() because we\n> would have limited options in there: silently ignore, loudly ignore,\n> die. At higher level we can do better reporting. For example, \"git\n> checkout -- foo\" will complain that \"foo\" does not match pathspec, just\n> like when \"foo\" is not registered with \"git add -N\"\n\nHmm, I am not sure about the example, though. The user _told_ Git\nthat 'foo' is a path she cares about so \"does not match pathspec\" is\na very unfriendly thing to say. \"foo does not exist in the index\nhence we cannot check it out\" is a very good thing to say, though\n(and of course, I agree that not touching the working tree is a good\nthing to do).\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/checkout-index.c | 5 ++++-\n> builtin/checkout.c | 2 ++\n> t/t2203-add-intent.sh | 34 ++++++++++++++++++++++++++++++++++\n> 3 files changed, 40 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\n> index 8028c37..eca975d 100644\n> --- a/builtin/checkout-index.c\n> +++ b/builtin/checkout-index.c\n> @@ -56,7 +56,8 @@ static int checkout_file(const char *name, const char *prefix)\n> \twhile (pos < active_nr) {\n> \t\tstruct cache_entry *ce = active_cache[pos];\n> \t\tif (ce_namelen(ce) != namelen ||\n> -\t\t memcmp(ce->name, name, namelen))\n> +\t\t memcmp(ce->name, name, namelen) ||\n> +\t\t ce_intent_to_add(ce))\n> \t\t\tbreak;\n> \t\thas_same_name = 1;\n> \t\tpos++;\n\n'pos' here comes from cache_name_pos(), and it is either the\nlocation an entry with the same name exists, or the location a new\nentry with the given name would be inserted at.\n\nWhen we detect that the entry at pos is different from the given\nname, we break out of the loop, because we _know_ that an entry with\nthe same name cannot exist. Does the same hold for an i-t-a entry\nthat exactly records the given name?\n\nThe answer is yes, as you cannot have an unmerged i-t-a entry. If a\npath marked as i-t-a is in the index, no other entry with the same\nname would exist.\n\nSo this change is good. I would actually have introduced another\nbool has_i_t_a to make the reporting richer, as it is your primary\nreason why you are touching this part of the code instead of\ncheckout_entry(). The reporting part then would become\n\n\tfprintf(stderr, \"git checkout-index: %s \", name);\n-\tif (!has_same_name)\n+\tif (has_i_t_a)\n+ \tfprintf(stderr, \"is not yet in the cache);\n\telse if (!has_same_name)\n\t\tfprintf(stderr, \"is not in the cache\");\n\n;-)\n\n> @@ -99,6 +100,8 @@ static void checkout_all(const char *prefix, int prefix_length)\n> \t\tif (ce_stage(ce) != checkout_stage\n> \t\t && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))\n> \t\t\tcontinue;\n> +\t\tif (ce_intent_to_add(ce))\n> +\t\t\tcontinue;\n\nThis one is obviously good ;-)\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index e1403be..02889d4 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts,\n> \t\t\t * anything to this entry at all.\n> \t\t\t */\n> \t\t\tcontinue;\n> +\t\tif (ce_intent_to_add(ce))\n> +\t\t\tcontinue;\n> \t\t/*\n> \t\t * Either this entry came from the tree-ish we are\n> \t\t * checking the paths out of, or we are checking out\n\nHmm, while this does prevent the later code from checking it out, I\nam not sure how well this interacts with ps_matched[] logic here.\nIf the user told Git that 'foo' is a path that she cares about with\n\"add -N foo\", and said \"git checkout -- foo\", should we be somehow\nsaying that 'foo' did match but there is nothing to check out, or\nsomething?\n"},{"id":"268668","messageId":"xmqq4mjn1d7s.fsf@gitster.dls.corp.google.com","threadId":"39692","inReplyTo":"1440205700-19749-7-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 6/8] grep: make it clear i-t-a entries are ignored","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T17:37:27Z","receivedAt":"2015-08-25T17:37:27Z","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> The expression \"!S_ISREG(ce)\" covers i-t-a entries as well because\n> ce->ce_mode would be zero then. I could make a comment saying that, but\n> it's probably better just to comment with code, in case i-t-a entry\n> content changes in future.\n\nOK. Thansk.\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/grep.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index d04f440..f508179 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -375,7 +375,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n> \n> \tfor (nr = 0; nr < active_nr; nr++) {\n> \t\tconst struct cache_entry *ce = active_cache[nr];\n> -\t\tif (!S_ISREG(ce->ce_mode))\n> +\t\tif (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce))\n> \t\t\tcontinue;\n> \t\tif (!ce_path_match(ce, pathspec, NULL))\n> \t\t\tcontinue;\n"},{"id":"268667","messageId":"xmqq37z71d7o.fsf@gitster.dls.corp.google.com","threadId":"39692","inReplyTo":"1440205700-19749-7-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 6/8] grep: make it clear i-t-a entries are ignored","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T17:37:31Z","receivedAt":"2015-08-25T17:37:31Z","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> The expression \"!S_ISREG(ce)\" covers i-t-a entries as well because\n> ce->ce_mode would be zero then. I could make a comment saying that, but\n> it's probably better just to comment with code, in case i-t-a entry\n> content changes in future.\n\nOK. Thanks.\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/grep.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index d04f440..f508179 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -375,7 +375,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n> \n> \tfor (nr = 0; nr < active_nr; nr++) {\n> \t\tconst struct cache_entry *ce = active_cache[nr];\n> -\t\tif (!S_ISREG(ce->ce_mode))\n> +\t\tif (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce))\n> \t\t\tcontinue;\n> \t\tif (!ce_path_match(ce, pathspec, NULL))\n> \t\t\tcontinue;\n"},{"id":"268669","messageId":"xmqqy4gzz2qa.fsf@gitster.dls.corp.google.com","threadId":"39692","inReplyTo":"1440205874-20295-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 7/8] diff.h: extend \"flags\" field to 64 bits because we're out of bits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-25T17:39:57Z","receivedAt":"2015-08-25T17:39:57Z","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> I renamed both \"flags\" and \"touched_flags\" fields while making this\n> patch to make sure I was aware of how these flags were manipulated\n> (besides DIFF_OPT* macros). So hopefully I didn't miss anything.\n\nIt is a bad taste to use user_defined_t typedef (I think it actually\nis a standard violation), isn't it?\n\nThe diff-struct is not like objects where we need million copies of\nin-core while running. What do you need many more flags for?\n"},{"id":"268981","messageId":"CACsJy8BaCWgQa=1bc-_6BHQJLUk4_GsiKQchJRpFMTrxp4Np6g@mail.gmail.com","threadId":"39692","inReplyTo":"xmqqy4gzz2qa.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 7/8] diff.h: extend \"flags\" field to 64 bits because we're out of bits","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-31T10:22:37Z","receivedAt":"2015-08-31T10:22:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 26, 2015 at 12:39 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> I renamed both \"flags\" and \"touched_flags\" fields while making this\n>> patch to make sure I was aware of how these flags were manipulated\n>> (besides DIFF_OPT* macros). So hopefully I didn't miss anything.\n>\n> It is a bad taste to use user_defined_t typedef (I think it actually\n> is a standard violation), isn't it?\n\nYeah I think you posted a patch somewhere updating CodingGuidelines about this..\n\n> The diff-struct is not like objects where we need million copies of\n> in-core while running. What do you need many more flags for?\n\nWe already use all 32 bit flags and I need one more flag. I guess I go\nwith flags because it's how we add features in diff struct. Adding a\nnew field instead of extending flags could be dangerous: elsewhere\npeople copy flags out to a temporary place, do something then restore.\nIf it's a separate field, it's left in place and bad things could\nhappen.\n-- \nDuy\n"},{"id":"273820","messageId":"20151129153122.GA3719@duynguyen-vnpc.dek-tpc.internal","threadId":"39692","inReplyTo":"xmqq8u8z1d8r.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 5/8] checkout(-index): do not checkout i-t-a entries","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-11-29T15:31:22Z","receivedAt":"2015-11-29T15:31:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Sorry for this waaay too late response, everything (of the series\nnd/ita-cleanup) is addressed so far except this..\n\nOn Tue, Aug 25, 2015 at 10:36:52AM -0700, Junio C Hamano wrote:\n> > diff --git a/builtin/checkout.c b/builtin/checkout.c\n> > index e1403be..02889d4 100644\n> > --- a/builtin/checkout.c\n> > +++ b/builtin/checkout.c\n> > @@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts,\n> > \t\t\t * anything to this entry at all.\n> > \t\t\t */\n> > \t\t\tcontinue;\n> > +\t\tif (ce_intent_to_add(ce))\n> > +\t\t\tcontinue;\n> > \t\t/*\n> > \t\t * Either this entry came from the tree-ish we are\n> > \t\t * checking the paths out of, or we are checking out\n> \n> Hmm, while this does prevent the later code from checking it out, I\n> am not sure how well this interacts with ps_matched[] logic here.\n> If the user told Git that 'foo' is a path that she cares about with\n> \"add -N foo\", and said \"git checkout -- foo\", should we be somehow\n> saying that 'foo' did match but there is nothing to check out, or\n> something?\n\nHow about this? It does not mess with ps_matched logic. But it does\nnot say \"nothing to checkout\" at the end either. While we could do\nthat (in general case, not just because all we are checking out is ita\nentries), I'm not sure if such verbosity helps anyone. I'm thinking of\ndropping the new warning I added here too..\n\n-- 8< --\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3e141fc..c11fe71 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -328,12 +328,17 @@ static int checkout_paths(const struct checkout_opts *opts,\n \tif (opts->merge)\n \t\tunmerge_marked_index(&the_index);\n \n-\t/* Any unmerged paths? */\n \tfor (pos = 0; pos < active_nr; pos++) {\n \t\tconst struct cache_entry *ce = active_cache[pos];\n \t\tif (ce->ce_flags & CE_MATCHED) {\n-\t\t\tif (!ce_stage(ce))\n+\t\t\tif (!ce_stage(ce)) {\n+\t\t\t\tif (ce_intent_to_add(ce)) {\n+\t\t\t\t\twarning(_(\"path '%s' is only intended to add\"), ce->name);\n+\t\t\t\t\tce->ce_flags &= ~CE_MATCHED;\n+\t\t\t\t}\n \t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\t/* Any unmerged paths? */\n \t\t\tif (opts->force) {\n \t\t\t\twarning(_(\"path '%s' is unmerged\"), ce->name);\n \t\t\t} else if (opts->writeout_stage) {\n-- 8< --\n--\nDuy\n"},{"id":"273836","messageId":"xmqq1tb78fgs.fsf@gitster.mtv.corp.google.com","threadId":"39692","inReplyTo":"20151129153122.GA3719@duynguyen-vnpc.dek-tpc.internal","subject":"Re: [PATCH 5/8] checkout(-index): do not checkout i-t-a entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-30T19:17:07Z","receivedAt":"2015-11-30T19:17:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Sorry for this waaay too late response, everything (of the series\n> nd/ita-cleanup) is addressed so far except this..\n>\n> On Tue, Aug 25, 2015 at 10:36:52AM -0700, Junio C Hamano wrote:\n>> > diff --git a/builtin/checkout.c b/builtin/checkout.c\n>> > index e1403be..02889d4 100644\n>> > --- a/builtin/checkout.c\n>> > +++ b/builtin/checkout.c\n>> > @@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts,\n>> > \t\t\t * anything to this entry at all.\n>> > \t\t\t */\n>> > \t\t\tcontinue;\n>> > +\t\tif (ce_intent_to_add(ce))\n>> > +\t\t\tcontinue;\n>> > \t\t/*\n>> > \t\t * Either this entry came from the tree-ish we are\n>> > \t\t * checking the paths out of, or we are checking out\n>> \n>> Hmm, while this does prevent the later code from checking it out, I\n>> am not sure how well this interacts with ps_matched[] logic here.\n>> If the user told Git that 'foo' is a path that she cares about with\n>> \"add -N foo\", and said \"git checkout -- foo\", should we be somehow\n>> saying that 'foo' did match but there is nothing to check out, or\n>> something?\n>\n> How about this? It does not mess with ps_matched logic. But it does\n> not say \"nothing to checkout\" at the end either. While we could do\n> that (in general case, not just because all we are checking out is ita\n> entries), I'm not sure if such verbosity helps anyone. I'm thinking of\n> dropping the new warning I added here too..\n\nI agree that these warnings are unwanted when you run \"checkout .\"\nin a repository with tons of i-t-a paths (but on the other hand,\nhaving tons of i-t-a paths is unusual so the user might want to be\nreminded of them--I dunno).\n\nWith or without the new warning(), this one looks an improvement\nover the previous one to me.\n\nThanks.\n\n> -- 8< --\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 3e141fc..c11fe71 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -328,12 +328,17 @@ static int checkout_paths(const struct checkout_opts *opts,\n> \tif (opts->merge)\n> \t\tunmerge_marked_index(&the_index);\n> \n> -\t/* Any unmerged paths? */\n> \tfor (pos = 0; pos < active_nr; pos++) {\n> \t\tconst struct cache_entry *ce = active_cache[pos];\n> \t\tif (ce->ce_flags & CE_MATCHED) {\n> -\t\t\tif (!ce_stage(ce))\n> +\t\t\tif (!ce_stage(ce)) {\n> +\t\t\t\tif (ce_intent_to_add(ce)) {\n> +\t\t\t\t\twarning(_(\"path '%s' is only intended to add\"), ce->name);\n> +\t\t\t\t\tce->ce_flags &= ~CE_MATCHED;\n> +\t\t\t\t}\n> \t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t\t/* Any unmerged paths? */\n> \t\t\tif (opts->force) {\n> \t\t\t\twarning(_(\"path '%s' is unmerged\"), ce->name);\n> \t\t\t} else if (opts->writeout_stage) {\n> -- 8< --\n> --\n> Duy\n"}]}