{"thread":{"id":"38763","subject":"[PATCH 1/2] diff --cached: do not report i-t-a entries as \"new\"","startedAt":"2015-03-09T14:14:03Z","lastAt":"2015-03-24T17:00:43Z","messageCount":18,"participants":["Nguyễn Thái Ngọc Duy","Michael J Gruber","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"257401","messageId":"1425910445-27383-1-git-send-email-pclouds@gmail.com","threadId":"38763","inReplyTo":null,"subject":"[PATCH/RFC 0/2] Bug fixes regarding diff and \"git add -N\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-09T14:14:03Z","receivedAt":"2015-03-09T14:14:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The first one attempts to fix \"git status\" reporting intent-to-add\nfiles as \"changes to be committed\". The \"new file\" report is now moved\nto \"git diff-files\" aka \"changes not staged for commit\" instead.\n\nI just want to check if I'm going on the right direction as core diff\nmachinery is not really my area. I know these patches break t2203 and\nt4011 but from a quick glance, the tests simply rely on the old behavior,\nnot real breakages.\n\nNguyễn Thái Ngọc Duy (2):\n  diff --cached: do not report i-t-a entries as \"new\"\n  diff-files: mark i-t-a paths as \"new\"\n\n builtin/add.c | 1 +\n diff-lib.c    | 7 +++++++\n 2 files changed, 8 insertions(+)\n\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"257400","messageId":"1425910445-27383-2-git-send-email-pclouds@gmail.com","threadId":"38763","inReplyTo":"1425910445-27383-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/2] diff --cached: do not report i-t-a entries as \"new\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-09T14:14:04Z","receivedAt":"2015-03-09T14:14:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Paths marked by \"git add -N\" are simply a reminder to the user that\nthese files should be staged. If the user does not stage any of them,\n\"git commit\" will not record them.\n\nAlign the behavior of \"diff --cached\" and \"git commit\". The most\nprominent result of this patch is \"git status\" no longer reports i-t-a\npaths as \"Changes to be committed\".\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n diff-lib.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex a85c497..db0e6f8 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -400,6 +400,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \t * Something added to the tree?\n \t */\n \tif (!tree) {\n+\t\tif (idx && (idx->ce_flags & CE_INTENT_TO_ADD))\n+\t\t\treturn;\n \t\tshow_new_file(revs, idx, cached, match_missing);\n \t\treturn;\n \t}\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"257402","messageId":"1425910445-27383-3-git-send-email-pclouds@gmail.com","threadId":"38763","inReplyTo":"1425910445-27383-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/2] diff-files: mark i-t-a paths as \"new\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-09T14:14:05Z","receivedAt":"2015-03-09T14:14: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 <pclouds@gmail.com>\n---\n builtin/add.c | 1 +\n diff-lib.c    | 5 +++++\n 2 files changed, 6 insertions(+)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 3390933..ee370b0 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -63,6 +63,7 @@ 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 db0e6f8..5f1afa4 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -212,6 +212,11 @@ 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-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"257405","messageId":"40da6c2f97f836acbaa654f470a007e06a47a22e.1425915714.git.git@drmicha.warpmail.net","threadId":"38763","inReplyTo":"1425910445-27383-1-git-send-email-pclouds@gmail.com","subject":"[PATCH] t2203,t4011: adjust to changed intent-to-add treatment","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2015-03-09T15:45:25Z","receivedAt":"2015-03-09T15:45:25Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\nFor the record, the tests would need to change like this, and it makes\na lot of sense. After the change, \"i-t-a\" is not a \"change staged in\nthe index\" any more - and in fact in never was, as \"git commit\" shows.\n\n t/t2203-add-intent.sh   |  7 ++++---\n t/t4011-diff-symlink.sh | 10 ++++++----\n 2 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 2a4a749..1a9b3c4 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -43,7 +43,8 @@ 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) = 1\n+\ttest $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&\n+\ttest $(git diff --name-only -- nitfol | wc -l) = 1\n '\n \n test_expect_success 'can commit with an unrelated i-t-a entry in index' '\n@@ -71,13 +72,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '\n \n \t: >dir/bar &&\n \tgit add -N dir/bar &&\n-\tgit diff --cached --name-only >actual &&\n+\tgit diff --name-only >actual &&\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+\tgit diff --name-only >actual &&\n \techo dir/bar >expect &&\n \ttest_cmp expect actual\n '\ndiff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh\nindex 13e7f62..7452fce 100755\n--- a/t/t4011-diff-symlink.sh\n+++ b/t/t4011-diff-symlink.sh\n@@ -139,11 +139,13 @@ 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-\tindex e69de29..d95f3ad 100644\n-\tBinary files a/file.bin and b/file.bin differ\n+\tnew file mode 100644\n+\tindex 0000000..d95f3ad\n+\tBinary files /dev/null and b/file.bin differ\n \tdiff --git a/link.bin b/link.bin\n-\tindex e69de29..dce41ec 120000\n-\t--- a/link.bin\n+\tnew file mode 120000\n+\tindex 0000000..dce41ec\n+\t--- /dev/null\n \t+++ b/link.bin\n \t@@ -0,0 +1 @@\n \t+file.bin\n-- \n2.3.1.303.g5174db1\n"},{"id":"257707","messageId":"xmqqmw3evjg2.fsf@gitster.dls.corp.google.com","threadId":"38763","inReplyTo":"1425910445-27383-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/2] diff --cached: do not report i-t-a entries as \"new\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-15T06:55:09Z","receivedAt":"2015-03-15T06:55: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  <pclouds@gmail.com> writes:\n\n> Paths marked by \"git add -N\" are simply a reminder to the user that\n> these files should be staged. If the user does not stage any of them,\n> \"git commit\" will not record them.\n>\n> Align the behavior of \"diff --cached\" and \"git commit\". The most\n> prominent result of this patch is \"git status\" no longer reports i-t-a\n> paths as \"Changes to be committed\".\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  diff-lib.c | 2 ++\n>  1 file changed, 2 insertions(+)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index a85c497..db0e6f8 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -400,6 +400,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n>  \t * Something added to the tree?\n>  \t */\n>  \tif (!tree) {\n> +\t\tif (idx && (idx->ce_flags & CE_INTENT_TO_ADD))\n> +\t\t\treturn;\n>  \t\tshow_new_file(revs, idx, cached, match_missing);\n>  \t\treturn;\n>  \t}\n\nThis hunk by itself feels like it is going in the right direction.\nThe HEAD does not have it, and even though there is idx, it merely\nis an i-t-a entry.\n\nDon't you need to special case the call to show_modified() at the\nend of this function as well, though?  idx is i-t-a, and tree has a\nconcrete object.  Should it appear as if the path already exists in\nthe index, or should we pretend as if idx is not yet there?\n\nFor example, after this sequence:\n\n    $ git init\n    $ >void\n    $ git add void\n    $ git commit -m void\n    $ git rm --cached void\n    $ git add -N void\n\nHEAD has \"void\" with an empty blob, the index has i-t-a.  I am not\nsure if we should say something about path \"void\" when asked:\n\n    $ git diff --cached\n\nPerhaps something like this to cover that case?\n\n\t/*\n\t * Something removed from the tree?\n\t */\n-\tif (!idx) {\n+\tif (!idx || (ix->ce_flags & CE_INTENT_TO_ADD)) {\n\t\tdiff_index_show_file(revs, \"-\", tree, tree->sha1, 1, tree->ce_mode, 0);\n\t\treturn;\n\t}\n"},{"id":"257708","messageId":"xmqqioe2viye.fsf@gitster.dls.corp.google.com","threadId":"38763","inReplyTo":"1425910445-27383-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] diff-files: mark i-t-a paths as \"new\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-15T07:05:45Z","receivedAt":"2015-03-15T07:05:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  builtin/add.c | 1 +\n>  diff-lib.c    | 5 +++++\n>  2 files changed, 6 insertions(+)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index 3390933..ee370b0 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -63,6 +63,7 @@ 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)) {\n\nIs this related to making \"diff-files\" show an i-t-a as \"new\", or\nsomething else?\n\nAhh, \"added\" would have never appeared in diff-files output (because\nby definition comparing index and working tree for only paths known\nto the index would never produce \"add\"), and the point of this series\nis to use that status to signal that the path is marked as i-t-a.\n\nAnd an i-t-a path is \"not yet exist in the index, known to the\nsystem, and exists in the working tree\", so catching that new case\nhere and calling add_file_to_index() would cause such a path to be\ntruly added to the index (this is \"add -u\" codepath, right?).\n\nMakes sense.\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index db0e6f8..5f1afa4 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -212,6 +212,11 @@ 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\nAnd this makes sense, of course.\n\nThe way \"add -N\" entries appear in \"git status\" has been disturbing\nfor quite a while to me, too.  Thanks for starting to look into it.\n"},{"id":"257709","messageId":"xmqqegoqviv3.fsf@gitster.dls.corp.google.com","threadId":"38763","inReplyTo":"40da6c2f97f836acbaa654f470a007e06a47a22e.1425915714.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] t2203,t4011: adjust to changed intent-to-add treatment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-15T07:07:44Z","receivedAt":"2015-03-15T07:07:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n> ---\n> For the record, the tests would need to change like this, and it makes\n> a lot of sense. After the change, \"i-t-a\" is not a \"change staged in\n> the index\" any more - and in fact in never was, as \"git commit\" shows.\n\nPerhaps another follow-up patch can illustrate how these entries\nshould show \"git status\", both in the longform and -s output?\n\n\n\n>\n>  t/t2203-add-intent.sh   |  7 ++++---\n>  t/t4011-diff-symlink.sh | 10 ++++++----\n>  2 files changed, 10 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\n> index 2a4a749..1a9b3c4 100755\n> --- a/t/t2203-add-intent.sh\n> +++ b/t/t2203-add-intent.sh\n> @@ -43,7 +43,8 @@ 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) = 1\n> +\ttest $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&\n> +\ttest $(git diff --name-only -- nitfol | wc -l) = 1\n>  '\n>  \n>  test_expect_success 'can commit with an unrelated i-t-a entry in index' '\n> @@ -71,13 +72,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' '\n>  \n>  \t: >dir/bar &&\n>  \tgit add -N dir/bar &&\n> -\tgit diff --cached --name-only >actual &&\n> +\tgit diff --name-only >actual &&\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> +\tgit diff --name-only >actual &&\n>  \techo dir/bar >expect &&\n>  \ttest_cmp expect actual\n>  '\n> diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh\n> index 13e7f62..7452fce 100755\n> --- a/t/t4011-diff-symlink.sh\n> +++ b/t/t4011-diff-symlink.sh\n> @@ -139,11 +139,13 @@ 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> -\tindex e69de29..d95f3ad 100644\n> -\tBinary files a/file.bin and b/file.bin differ\n> +\tnew file mode 100644\n> +\tindex 0000000..d95f3ad\n> +\tBinary files /dev/null and b/file.bin differ\n>  \tdiff --git a/link.bin b/link.bin\n> -\tindex e69de29..dce41ec 120000\n> -\t--- a/link.bin\n> +\tnew file mode 120000\n> +\tindex 0000000..dce41ec\n> +\t--- /dev/null\n>  \t+++ b/link.bin\n>  \t@@ -0,0 +1 @@\n>  \t+file.bin\n"},{"id":"257782","messageId":"1426514206-30949-1-git-send-email-pclouds@gmail.com","threadId":"38763","inReplyTo":"1425910445-27383-2-git-send-email-pclouds@gmail.com","subject":"[PATCH v2] 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-03-16T13:56:46Z","receivedAt":"2015-03-16T13:56:46Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Entries added by \"git add -N\" are reminder for the user so that they\ndon't forget to add them before committing. These entries appear in\nthe index even though they are not real. Their presence in the index\nleads to a confusing \"git status\" like this:\n\n    On branch master\n    Changes to be committed:\n            new file:   foo\n\n    Changes not staged for commit:\n            modified:   foo\n\nIf you do a \"git commit\", \"foo\" will not be included even though\n\"status\" reports it as \"to be committed\". This patch changes the\noutput to become\n\n    On branch master\n    Changes not staged for commit:\n            new file:   foo\n\n    no changes added to commit\n\nThe two hunks in diff-lib.c adjust \"diff-index\" and \"diff-files\" so\nthat i-t-a entries appear as new files in diff-files and nothing in\ndiff-index.\n\nDue to this change, diff-files may start to report \"new files\" for the\nfirst time. \"add -u\" needs to be told about this or it will die in\ndenial, screaming \"new files can't exist! Reality is wrong.\" Luckily,\nit's the only one among run_diff_files() callers that needs fixing.\n\nThe test \"cache-tree invalidates i-t-a paths\" is marked failure\nbecause I don't think removing \"--cached\" from \"git diff\" is the right\nfix. This test relies on \"diff --cached\" behavior but the behavior now\nhas changed. We may need to revisit this test at some point in future.\n\nHelped-by: Michael J Gruber <git@drmicha.warpmail.net>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/add.c           |  1 +\n diff-lib.c              | 12 ++++++++++++\n t/t2203-add-intent.sh   | 21 ++++++++++++++++++---\n t/t4011-diff-symlink.sh | 10 ++++++----\n 4 files changed, 37 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 3390933..ee370b0 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -63,6 +63,7 @@ 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 a85c497..714501a 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -212,6 +212,11 @@ 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@@ -376,6 +381,13 @@ 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 2a4a749..42827b8 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -5,10 +5,24 @@ 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 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 '\n \n test_expect_success 'check result of \"add -N\"' '\n@@ -43,7 +57,8 @@ 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) = 1\n+\ttest $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&\n+\ttest $(git diff --name-only -- nitfol | wc -l) = 1\n '\n \n test_expect_success 'can commit with an unrelated i-t-a entry in index' '\n@@ -62,7 +77,7 @@ test_expect_success 'can \"commit -a\" with an i-t-a entry' '\n \tgit commit -a -m all\n '\n \n-test_expect_success 'cache-tree invalidates i-t-a paths' '\n+test_expect_failure 'cache-tree invalidates i-t-a paths' '\n \tgit reset --hard &&\n \tmkdir dir &&\n \t: >dir/foo &&\ndiff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh\nindex 13e7f62..7452fce 100755\n--- a/t/t4011-diff-symlink.sh\n+++ b/t/t4011-diff-symlink.sh\n@@ -139,11 +139,13 @@ 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-\tindex e69de29..d95f3ad 100644\n-\tBinary files a/file.bin and b/file.bin differ\n+\tnew file mode 100644\n+\tindex 0000000..d95f3ad\n+\tBinary files /dev/null and b/file.bin differ\n \tdiff --git a/link.bin b/link.bin\n-\tindex e69de29..dce41ec 120000\n-\t--- a/link.bin\n+\tnew file mode 120000\n+\tindex 0000000..dce41ec\n+\t--- /dev/null\n \t+++ b/link.bin\n \t@@ -0,0 +1 @@\n \t+file.bin\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"257789","messageId":"5506F3A9.1020704@drmicha.warpmail.net","threadId":"38763","inReplyTo":"1426514206-30949-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2015-03-16T15:15:53Z","receivedAt":"2015-03-16T15:15:53Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Nguyễn Thái Ngọc Duy venit, vidit, dixit 16.03.2015 14:56:\n> Entries added by \"git add -N\" are reminder for the user so that they\n> don't forget to add them before committing. These entries appear in\n> the index even though they are not real. Their presence in the index\n> leads to a confusing \"git status\" like this:\n> \n>     On branch master\n>     Changes to be committed:\n>             new file:   foo\n> \n>     Changes not staged for commit:\n>             modified:   foo\n> \n> If you do a \"git commit\", \"foo\" will not be included even though\n> \"status\" reports it as \"to be committed\". This patch changes the\n> output to become\n> \n>     On branch master\n>     Changes not staged for commit:\n>             new file:   foo\n> \n>     no changes added to commit\n> \n> The two hunks in diff-lib.c adjust \"diff-index\" and \"diff-files\" so\n> that i-t-a entries appear as new files in diff-files and nothing in\n> diff-index.\n> \n> Due to this change, diff-files may start to report \"new files\" for the\n> first time. \"add -u\" needs to be told about this or it will die in\n> denial, screaming \"new files can't exist! Reality is wrong.\" Luckily,\n> it's the only one among run_diff_files() callers that needs fixing.\n> \n> The test \"cache-tree invalidates i-t-a paths\" is marked failure\n> because I don't think removing \"--cached\" from \"git diff\" is the right\n> fix. This test relies on \"diff --cached\" behavior but the behavior now\n> has changed. We may need to revisit this test at some point in future.\n\nI can't quite follow that reasoning. If the test describes expected\nbehavior which the patch breaks, then we should not apply the patch. If\nthe patch changes behavior in an expected way, then we should change the\ntest to match.\n\n> Helped-by: Michael J Gruber <git@drmicha.warpmail.net>\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  builtin/add.c           |  1 +\n>  diff-lib.c              | 12 ++++++++++++\n>  t/t2203-add-intent.sh   | 21 ++++++++++++++++++---\n>  t/t4011-diff-symlink.sh | 10 ++++++----\n>  4 files changed, 37 insertions(+), 7 deletions(-)\n> \n> diff --git a/builtin/add.c b/builtin/add.c\n> index 3390933..ee370b0 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -63,6 +63,7 @@ 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)) {\n> diff --git a/diff-lib.c b/diff-lib.c\n> index a85c497..714501a 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -212,6 +212,11 @@ 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> @@ -376,6 +381,13 @@ 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)));\n> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\n> index 2a4a749..42827b8 100755\n> --- a/t/t2203-add-intent.sh\n> +++ b/t/t2203-add-intent.sh\n> @@ -5,10 +5,24 @@ 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 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>  '\n>  \n>  test_expect_success 'check result of \"add -N\"' '\n> @@ -43,7 +57,8 @@ 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) = 1\n> +\ttest $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&\n> +\ttest $(git diff --name-only -- nitfol | wc -l) = 1\n>  '\n>  \n>  test_expect_success 'can commit with an unrelated i-t-a entry in index' '\n> @@ -62,7 +77,7 @@ test_expect_success 'can \"commit -a\" with an i-t-a entry' '\n>  \tgit commit -a -m all\n>  '\n>  \n> -test_expect_success 'cache-tree invalidates i-t-a paths' '\n> +test_expect_failure 'cache-tree invalidates i-t-a paths' '\n>  \tgit reset --hard &&\n>  \tmkdir dir &&\n>  \t: >dir/foo &&\n> diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh\n> index 13e7f62..7452fce 100755\n> --- a/t/t4011-diff-symlink.sh\n> +++ b/t/t4011-diff-symlink.sh\n> @@ -139,11 +139,13 @@ 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> -\tindex e69de29..d95f3ad 100644\n> -\tBinary files a/file.bin and b/file.bin differ\n> +\tnew file mode 100644\n> +\tindex 0000000..d95f3ad\n> +\tBinary files /dev/null and b/file.bin differ\n>  \tdiff --git a/link.bin b/link.bin\n> -\tindex e69de29..dce41ec 120000\n> -\t--- a/link.bin\n> +\tnew file mode 120000\n> +\tindex 0000000..dce41ec\n> +\t--- /dev/null\n>  \t+++ b/link.bin\n>  \t@@ -0,0 +1 @@\n>  \t+file.bin\n> \n"},{"id":"257794","messageId":"xmqqa8zdrkpy.fsf@gitster.dls.corp.google.com","threadId":"38763","inReplyTo":"5506F3A9.1020704@drmicha.warpmail.net","subject":"Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-16T16:05:45Z","receivedAt":"2015-03-16T16:05:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> Nguyễn Thái Ngọc Duy venit, vidit, dixit 16.03.2015 14:56:\n>\n>> The test \"cache-tree invalidates i-t-a paths\" is marked failure\n>> because I don't think removing \"--cached\" from \"git diff\" is the right\n>> fix. This test relies on \"diff --cached\" behavior but the behavior now\n>> has changed. We may need to revisit this test at some point in future.\n>\n> I can't quite follow that reasoning. If the test describes expected\n> behavior which the patch breaks, then we should not apply the patch. If\n> the patch changes behavior in an expected way, then we should change the\n> test to match.\n\nThe offending one came from eec3e7e4 (cache-tree: invalidate i-t-a\npaths after generating trees, 2012-12-16), which was a fix to an\nearlier bug where a cache-tree written out of an index with i-t-a\nentries had incorrect information and still claimed it is fully\nvalid after write-tree rebuilt it.  The test probably should add\nanother path without i-t-a bit, run the same \"diff --cached\" with\nupdated expectation before write-tre, and run the \"diff --cached\"\nagain to make sure it produces a result that match the updated\nexpectation.\n"},{"id":"257854","messageId":"20150317140704.GA7248@lanh","threadId":"38763","inReplyTo":"xmqqa8zdrkpy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-17T14:07:04Z","receivedAt":"2015-03-17T14:07:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Mar 16, 2015 at 09:05:45AM -0700, Junio C Hamano wrote:\n> The offending one came from eec3e7e4 (cache-tree: invalidate i-t-a\n> paths after generating trees, 2012-12-16), which was a fix to an\n> earlier bug where a cache-tree written out of an index with i-t-a\n> entries had incorrect information and still claimed it is fully\n> valid after write-tree rebuilt it.  The test probably should add\n> another path without i-t-a bit, run the same \"diff --cached\" with\n> updated expectation before write-tre, and run the \"diff --cached\"\n> again to make sure it produces a result that match the updated\n> expectation.\n\nWould adding another non-i-t-a entry help? Before this patch\n\"diff --cached\" after write-tree shows the i-t-a entry only when\neec3e7e4 is applied. But with this patch we don't show i-t-a entry any\nmore, before or after write-tree, eec3e7e4 makes no visible difference.\n\nWe could even revert eec3e7e4 and the outcome of \"diff --cached\" would\nbe the same because we just sort of move the \"invalidation\" part from\ncache-tree to do_oneway_diff(). Not invalidating would speed up \"diff\n--cached\" when i-t-a entries are present. Still it may be a good idea\nto invalidate i-t-a paths to be on the safe side. Perhaps a patch like\nthis to resurrect the test?\n\n-- 8< --\nSubject: t2203: enable 'cache-tree invalidates i-t-a paths' test\n\nThis test is disabled in the previous patch because the \"diff\n--cached\" expectation is the same, with or without eec3e7e4 [1] where\nthis test is added.\n\nBut it still is a good idea to invalidate i-t-a paths because the\nindex does _not_ match the cached-tree exactly.  \"diff --cached\" may\nnot care, but other operations might. Update the test to catch this\ninvalidation instead.\n\n[1] cache-tree: invalidate i-t-a paths after generating trees, 2012-12-16\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 42827b8..1a6c814 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -77,7 +77,7 @@ test_expect_success 'can \"commit -a\" with an i-t-a entry' '\n \tgit commit -a -m all\n '\n \n-test_expect_failure 'cache-tree invalidates i-t-a paths' '\n+test_expect_success 'cache-tree invalidates i-t-a paths' '\n \tgit reset --hard &&\n \tmkdir dir &&\n \t: >dir/foo &&\n@@ -86,14 +86,14 @@ test_expect_failure 'cache-tree invalidates i-t-a paths' '\n \n \t: >dir/bar &&\n \tgit add -N dir/bar &&\n-\tgit diff --cached --name-only >actual &&\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-\techo dir/bar >expect &&\n+\ttest-dump-cache-tree >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tinvalid                                   (1 subtrees)\n+\tinvalid                                  #(ref)  (1 subtrees)\n+\tinvalid                                  dir/ (0 subtrees)\n+\tinvalid                                  #(ref) dir/ (0 subtrees)\n+\tEOF\n \ttest_cmp expect actual\n ' \n-- 8< --\n"},{"id":"257872","messageId":"xmqq1tknpkwd.fsf@gitster.dls.corp.google.com","threadId":"38763","inReplyTo":"20150317140704.GA7248@lanh","subject":"Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-17T17:57:06Z","receivedAt":"2015-03-17T17:57:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Mon, Mar 16, 2015 at 09:05:45AM -0700, Junio C Hamano wrote:\n>> The offending one came from eec3e7e4 (cache-tree: invalidate i-t-a\n>> paths after generating trees, 2012-12-16), which was a fix to an\n>> earlier bug where a cache-tree written out of an index with i-t-a\n>> entries had incorrect information and still claimed it is fully\n>> valid after write-tree rebuilt it.  The test probably should add\n>> another path without i-t-a bit, run the same \"diff --cached\" with\n>> updated expectation before write-tre, and run the \"diff --cached\"\n>> again to make sure it produces a result that match the updated\n>> expectation.\n>\n> Would adding another non-i-t-a entry help? Before this patch\n> \"diff --cached\" after write-tree shows the i-t-a entry only when\n> eec3e7e4 is applied. But with this patch we don't show i-t-a entry any\n> more, before or after write-tree, eec3e7e4 makes no visible difference.\n>\n> We could even revert eec3e7e4 and the outcome of \"diff --cached\" would\n> be the same because we just sort of move the \"invalidation\" part from\n> cache-tree to do_oneway_diff(). Not invalidating would speed up \"diff\n> --cached\" when i-t-a entries are present. Still it may be a good idea\n> to invalidate i-t-a paths to be on the safe side. Perhaps a patch like\n> this to resurrect the test?\n\nMy unerstanding of what eec3e7e4 (cache-tree: invalidate i-t-a paths\nafter generating trees, 2012-12-16) fixed was that in this sequence:\n\n    - You prepare an index.\n\n    - You write-tree out of the index, which involves:\n\n      - updating the cache-tree to match the shape of the resulting\n        from writing the index out.\n\n      - create tree objects matching all levels of the cache-tree as\n        needed on disk.\n\n      - report the top-level tree object name\n\n   - run \"diff-index --cached\", which can and will take advantage of\n     the fact that everything in a subtree below a known-to-be-valid\n     cache-tree entry does not have to be checked one-by-one.  If a\n     cache-tree says \"everything under D/ in the index would hash to\n     tree object T\" and the HEAD has tree object T at D/, then the\n     diff machinery will bypass the entire section in the index\n     under D/, which is a valid optimization.\n\n     However, when there is an i-t-a entry, we excluded that entry\n     from the tree object computation, its presence did not\n     contribute to the tree object name, but still marked the\n     cache-tree entries that contain it as valid by mistake.  This\n     old bug was what the commit fixed, so an invocation of \"diff\n     --cached\" after a write-tree, even if the index contains an\n     i-t-a entry, will not see cache-tree entries that are marked\n     valid when they are not.  Instead, \"diff --cached\" will bypass\n     the optimization and makes comparison one-by-one for the index\n     entries.\n\nSo reverting the fix obviously is not the right thing to do.  If the\ntests show different results from two invocations of \"diff --cached\"\nwith your patch applied, there is something that is broken by your\npatch, because the index and the HEAD does not change across\nwrite-tree in that test.\n\nIf on the other hand the tests show the same result from these two\n\"diff --cached\" and the result is different from what the test\nexpects, that means your patch changed the world order, i.e. an\ni-t-a entry used to be treated as if it were adding an empty blob to\nthe index but it is now treated as non-existent, then that is a good\nthing and the only thing we need to update is what the test expects.\nI am guessing that instead of expecting dir/bar to be shown, it now\nshould expect no output?\n\nDoes adding an non-i-t-a entry help?  It does not hurt, and it makes\nthe test uses a non-empty output, making its effect more visible,\nwhich may or may not count as helping.\n\n\n     \n"},{"id":"257950","messageId":"CACsJy8Beoz=qcHrOG=akCR+gOQRjBcsQHaXdL_=PW70BOf4q3g@mail.gmail.com","threadId":"38763","inReplyTo":"xmqq1tknpkwd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-18T12:47:11Z","receivedAt":"2015-03-18T12:47:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Mar 18, 2015 at 12:57 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> On Mon, Mar 16, 2015 at 09:05:45AM -0700, Junio C Hamano wrote:\n>>> The offending one came from eec3e7e4 (cache-tree: invalidate i-t-a\n>>> paths after generating trees, 2012-12-16), which was a fix to an\n>>> earlier bug where a cache-tree written out of an index with i-t-a\n>>> entries had incorrect information and still claimed it is fully\n>>> valid after write-tree rebuilt it.  The test probably should add\n>>> another path without i-t-a bit, run the same \"diff --cached\" with\n>>> updated expectation before write-tre, and run the \"diff --cached\"\n>>> again to make sure it produces a result that match the updated\n>>> expectation.\n>>\n>> Would adding another non-i-t-a entry help? Before this patch\n>> \"diff --cached\" after write-tree shows the i-t-a entry only when\n>> eec3e7e4 is applied. But with this patch we don't show i-t-a entry any\n>> more, before or after write-tree, eec3e7e4 makes no visible difference.\n>>\n>> We could even revert eec3e7e4 and the outcome of \"diff --cached\" would\n>> be the same because we just sort of move the \"invalidation\" part from\n>> cache-tree to do_oneway_diff(). Not invalidating would speed up \"diff\n>> --cached\" when i-t-a entries are present. Still it may be a good idea\n>> to invalidate i-t-a paths to be on the safe side. Perhaps a patch like\n>> this to resurrect the test?\n>\n> My unerstanding of what eec3e7e4 (cache-tree: invalidate i-t-a paths\n> after generating trees, 2012-12-16) fixed was that in this sequence:\n>\n>    ...\n>\n> So reverting the fix obviously is not the right thing to do.  If the\n> tests show different results from two invocations of \"diff --cached\"\n> with your patch applied, there is something that is broken by your\n> patch, because the index and the HEAD does not change across\n> write-tree in that test.\n\nRight. I missed this but I think this is a less important test because\nI added a new test to make sure \"diff --cached\" (\"git status\" to be\nexact) outputs the right thing when i-t-a entries are present.\n\n> If on the other hand the tests show the same result from these two\n> \"diff --cached\" and the result is different from what the test\n> expects, that means your patch changed the world order, i.e. an\n> i-t-a entry used to be treated as if it were adding an empty blob to\n> the index but it is now treated as non-existent, then that is a good\n> thing and the only thing we need to update is what the test expects.\n> I am guessing that instead of expecting dir/bar to be shown, it now\n> should expect no output?\n\nYes, no output.\n\n> Does adding an non-i-t-a entry help?  It does not hurt, and it makes\n> the test uses a non-empty output, making its effect more visible,\n> which may or may not count as helping.\n\nBut still, if I revert the change in cache-tree.c and force write-tree\nto produce valid cache-tree when i-t-a entries are present, this test\nstill passes incorrectly. This is why I changed the test.\n-- \nDuy\n"},{"id":"257995","messageId":"xmqqtwxikpz6.fsf@gitster.dls.corp.google.com","threadId":"38763","inReplyTo":"CACsJy8Beoz=qcHrOG=akCR+gOQRjBcsQHaXdL_=PW70BOf4q3g@mail.gmail.com","subject":"Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-18T20:30:53Z","receivedAt":"2015-03-18T20:30:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> Right. I missed this but I think this is a less important test\n> because I added a new test to make sure \"diff --cached\" (\"git\n> status\" to be exact) outputs the right thing when i-t-a entries\n> are present.\n\nOK.\n\n>> If on the other hand the tests show the same result from these two\n>> \"diff --cached\" and the result is different from what the test\n>> expects, that means your patch changed the world order, i.e. an\n>> i-t-a entry used to be treated as if it were adding an empty blob to\n>> the index but it is now treated as non-existent, then that is a good\n>> thing and the only thing we need to update is what the test expects.\n>> I am guessing that instead of expecting dir/bar to be shown, it now\n>> should expect no output?\n>\n> Yes, no output.\n\nGood, as that means your changes did not break things, right?\n\n> But still, if I revert the change in cache-tree.c and force write-tree\n> to produce valid cache-tree when i-t-a entries are present, this test\n> still passes incorrectly.\n\nThis is worrysome.\n\nDoesn't that suggest that the diff-cache optimization is disabled\nand cache-tree that is marked as valid (because you \"revert\" the\nfix) is not looked at?  That is the only explanation that makes\nsense to me---you write out a tree omitting i-t-a entries in the\nindex and update the index without your earlier fix eec3e7e4\n(cache-tree: invalidate i-t-a paths after generating trees,\n2012-12-16), i.e. marking the cache-tree entries valid.  A later\ninvocation of diff-cache *should* trust that validity and just say\n\"the tree and the index match\" without even digging into the\nindividual entries, at which point you should see a bogus result.\n\nIf that is the case, it would be a performance regression, which may\nbe already there before your patches or something your patches may\nhave introduced.\n\nAh, wait.\n\nI suspect that it all cancels out.\n\n * You \"add -N dir/bar\".  You write-tree.  The tree is written as if\n   dir/bar is not there.  cache-tree is updated and it is marked as\n   valid (because you deliberately broke the earlier fix you made at\n   eec3e7e4).\n\n * You run \"diff-cache\".  It walks the HEAD and the cache-tree and\n   finds that HEAD:dir and the cache-tree entry for \"dir\" records\n   the same tree object name, and the cache-tree entry is marked\n   \"valid\".  Hence you declare \"no differences\".\n\nBut for the updated definition of \"diff-cache\", there indeed is no\ndifference.  The HEAD and the index matches, because dir/bar does\nnot yet exist.\n\nOK, so I think I can see how the result could work without\ninvalidating the cache-tree entry that contains i-t-a entries.\n\nIt might be even the right thing to do in general.  We can view that\ninvalidation a workaround to achieve the old behaviour of diff-cache,\nwhich used to report that dir/ are different between HEAD and index.\n\nWe can even argue that it is the right thing to mark the cache-tree\nentry for dir/ as valid, no matter how many i-t-a entries are in\nthere, as long as writing out the tree for dir/ out of index results\nin the same tree as the tree that is stored as HEAD:dir/.  And from\nthat viewpoint, keeping the earlier fix (invalidation code) when\nthese patches are applied _is_ introducing a performance bug.\n\nNow, as you mentioned, there _may_ be codepaths that uses the same\ndefinition of \"what's in the index\" as what diff-cache used to take\nbefore your patches, and they may be broken by removing the\ninvalidation.  If we find such codepaths, we should treat their old\nbehaviour as buggy and fix them, instead of reintroducing the\ninvalidation and keep their current behaviour, as the new world\norder is \"i-t-a entries in the index does not yet exist.\"\n\nThanks for straightening my confusion out.\n"},{"id":"258009","messageId":"xmqqlhitle5w.fsf@gitster.dls.corp.google.com","threadId":"38763","inReplyTo":"xmqqtwxikpz6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T06:00:43Z","receivedAt":"2015-03-19T06:00:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Ah, wait.\n>\n> I suspect that it all cancels out.\n> ...\n> Now, as you mentioned, there _may_ be codepaths that uses the same\n> definition of \"what's in the index\" as what diff-cache used to take\n> before your patches, and they may be broken by removing the\n> invalidation.  If we find such codepaths, we should treat their old\n> behaviour as buggy and fix them, instead of reintroducing the\n> invalidation and keep their current behaviour, as the new world\n> order is \"i-t-a entries in the index does not yet exist.\"\n\nOne potential negative consequence of the new world order I can\nimmediately think of is this.  In many operations, we try to be\nlenient to changes in the working tree as long as the index is\nclean.  \"read-tree -m\" is the most visible one, where we require\nthat the index and HEAD matches while allowing changes to working\ntree paths as long as they do not interfere with the paths that are\ninvolved in the merge.  We need to make sure that the path dir/bar\nadded by \"add -N dir/bar\", which in the new world order does not\ncount as \"index is not clean and there is a difference from HEAD\",\n(1) does not interfere with the mergy operation that wants to touch\ndir (which _could_ even be expected to be a file) or dir/bar, and\n(2) is not lost because the mergy operation wants to touch dir or\ndir/bar, for example.\n"},{"id":"258384","messageId":"xmqqy4mn8miu.fsf@gitster.dls.corp.google.com","threadId":"38763","inReplyTo":"xmqqtwxikpz6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-23T20:52:09Z","receivedAt":"2015-03-23T20:52:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Ah, wait.\n>\n> I suspect that it all cancels out.\n> ...\n> OK, so I think I can see how the result could work without\n> invalidating the cache-tree entry that contains i-t-a entries.\n>\n> It might be even the right thing to do in general.  We can view that\n> invalidation a workaround to achieve the old behaviour of diff-cache,\n> which used to report that dir/ are different between HEAD and index.\n\nSo, we somehow left this hanging for a week without progress.  Here\nis the version I am going to queue.\n\nThanks.\n\n-- >8 --\nFrom: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nDate: Mon, 16 Mar 2015 20:56:46 +0700\nSubject: [PATCH] diff-lib.c: adjust position of i-t-a entries in diff\n\nEntries added by \"git add -N\" are reminder for the user so that they\ndon't forget to add them before committing. These entries appear in\nthe index even though they are not real. Their presence in the index\nleads to a confusing \"git status\" like this:\n\n    On branch master\n    Changes to be committed:\n            new file:   foo\n\n    Changes not staged for commit:\n            modified:   foo\n\nIf you do a \"git commit\", \"foo\" will not be included even though\n\"status\" reports it as \"to be committed\". This patch changes the\noutput to become\n\n    On branch master\n    Changes not staged for commit:\n            new file:   foo\n\n    no changes added to commit\n\nThe two hunks in diff-lib.c adjust \"diff-index\" and \"diff-files\" so\nthat i-t-a entries appear as new files in diff-files and nothing in\ndiff-index.\n\nDue to this change, diff-files may start to report \"new files\" for the\nfirst time. \"add -u\" needs to be told about this or it will die in\ndenial, screaming \"new files can't exist! Reality is wrong.\" Luckily,\nit's the only one among run_diff_files() callers that needs fixing.\n\nNow in the new world order, a hierarchy in the index that contain\ni-t-a paths is written out as a tree object as if these i-t-a\nentries do not exist, and comparing the index with such a tree\nobject that would result from writing out the hierarchy will result\nin no difference.  Update a test in t2203 that expected the i-t-a\nentries to appear as \"added to the index\" in the comparison to\ninstead expect no output.\n\nAn earlier change eec3e7e4 (cache-tree: invalidate i-t-a paths after\ngenerating trees, 2012-12-16) becomes an unnecessary pessimization\nin the new world order---a cache-tree in the index that corresponds\nto a hierarchy with i-t-a paths can now be marked as valid and\nrecord the object name of the tree that results from writing a tree\nobject out of that hierarchy, as it will compare equal to that tree.\n\nReverting the commit is left for the future, though, as it is purely\na performance issue and no longer affects correctness.\n\nHelped-by: Michael J Gruber <git@drmicha.warpmail.net>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\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, 38 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 3390933..ee370b0 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -63,6 +63,7 @@ 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 a85c497..714501a 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -212,6 +212,11 @@ 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@@ -376,6 +381,13 @@ 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 2a4a749..7c641bf 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -5,10 +5,24 @@ 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 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 '\n \n test_expect_success 'check result of \"add -N\"' '\n@@ -43,7 +57,8 @@ 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) = 1\n+\ttest $(git diff --name-only HEAD -- nitfol | wc -l) = 0 &&\n+\ttest $(git diff --name-only -- nitfol | wc -l) = 1\n '\n \n test_expect_success 'can commit with an unrelated i-t-a entry in index' '\n@@ -72,13 +87,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-\techo dir/bar >expect &&\n+\t>expect &&\n \ttest_cmp expect actual &&\n \n \tgit write-tree >/dev/null &&\n \n \tgit diff --cached --name-only >actual &&\n-\techo dir/bar >expect &&\n+\t>expect &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh\nindex 13e7f62..7452fce 100755\n--- a/t/t4011-diff-symlink.sh\n+++ b/t/t4011-diff-symlink.sh\n@@ -139,11 +139,13 @@ 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-\tindex e69de29..d95f3ad 100644\n-\tBinary files a/file.bin and b/file.bin differ\n+\tnew file mode 100644\n+\tindex 0000000..d95f3ad\n+\tBinary files /dev/null and b/file.bin differ\n \tdiff --git a/link.bin b/link.bin\n-\tindex e69de29..dce41ec 120000\n-\t--- a/link.bin\n+\tnew file mode 120000\n+\tindex 0000000..dce41ec\n+\t--- /dev/null\n \t+++ b/link.bin\n \t@@ -0,0 +1 @@\n \t+file.bin\n-- \n2.3.4-437-g9384a34\n"},{"id":"258393","messageId":"CACsJy8BczCNxm3WHK1gtkXiZCbDkFD-67oOMR+eK8uwOBfQXuw@mail.gmail.com","threadId":"38763","inReplyTo":"xmqqlhitle5w.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-24T01:15:50Z","receivedAt":"2015-03-24T01:15:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 19, 2015 at 1:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Ah, wait.\n>>\n>> I suspect that it all cancels out.\n>> ...\n>> Now, as you mentioned, there _may_ be codepaths that uses the same\n>> definition of \"what's in the index\" as what diff-cache used to take\n>> before your patches, and they may be broken by removing the\n>> invalidation.  If we find such codepaths, we should treat their old\n>> behaviour as buggy and fix them, instead of reintroducing the\n>> invalidation and keep their current behaviour, as the new world\n>> order is \"i-t-a entries in the index does not yet exist.\"\n>\n> One potential negative consequence of the new world order I can\n> immediately think of is this.  In many operations, we try to be\n> lenient to changes in the working tree as long as the index is\n> clean.  \"read-tree -m\" is the most visible one, where we require\n> that the index and HEAD matches while allowing changes to working\n> tree paths as long as they do not interfere with the paths that are\n> involved in the merge.  We need to make sure that the path dir/bar\n> added by \"add -N dir/bar\", which in the new world order does not\n> count as \"index is not clean and there is a difference from HEAD\",\n> (1) does not interfere with the mergy operation that wants to touch\n> dir (which _could_ even be expected to be a file) or dir/bar, and\n> (2) is not lost because the mergy operation wants to touch dir or\n> dir/bar, for example.\n\n\"read-tree -m\" does not invoke diff, does it? If I went with my\nprevious approach (modifying unpack-trees to ignore i-t-a entries)\nthen this could be a problem, but because unpack-trees is untouched,\nmerge operations should not be impacted by this patch. Even if some\nother command does \"diff --cached\" first to abort early, if \"diff\n--cached\" fails to report \"HEAD and index are different\" as you\ndescribed, I would expect unpack-trees to be able to deal with it\nanyway.\n\nPS. Sorry for the late response, busy fighting the evil last weekend.\nI blame Steam on Linux.\n-- \nDuy\n"},{"id":"258435","messageId":"xmqqtwxa72kk.fsf@gitster.dls.corp.google.com","threadId":"38763","inReplyTo":"CACsJy8BczCNxm3WHK1gtkXiZCbDkFD-67oOMR+eK8uwOBfQXuw@mail.gmail.com","subject":"Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-24T17:00:43Z","receivedAt":"2015-03-24T17:00:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> \"read-tree -m\" does not invoke diff, does it? If I went with my\n> previous approach (modifying unpack-trees to ignore i-t-a entries)\n> then this could be a problem, but because unpack-trees is untouched,\n> merge operations should not be impacted by this patch.\n\nTheoretically yes, but not quite.\n\nI wouldn't be surprised if an enterprising soul saw an optimization\nopportunity in the \"read-tree -m A B\" codepath.  When it finds that\na tree in A and a valid cache-tree entry that corresponds to the\ntree matches, it could blow away all index entries covered by the\ncache-tree entry and replace them with B, either\n\n (1) unconditionally when \"-u\" is not given; or\n\n (2) as long as the working tree matches the index inside that\n     directory when running with \"-u\".\n\nAnd such an optimization used to be a valid thing to do in the old\nworld; but (1) will break in the new world, if we drop that\ninvalidation---the i-t-a entries will be discarded from the index.\n\nAs i-t-a is not a norm but an abberration, I'd rather keep the\npessimizing invalidation to keep the door open for such an\noptimization for a more common case, and there may be other cases\nin which our correctness around i-t-a depends on.\n"}]}