{"thread":{"id":"53661","subject":"[PATCH] diff-files: treat \"i-t-a\" files as \"not-in-index\"","startedAt":"2020-06-11T16:16:48Z","lastAt":"2020-06-23T15:17:17Z","messageCount":12,"participants":["Srinidhi Kaushik","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"399550","messageId":"20200611161640.52156-1-shrinidhi.kaushik@gmail.com","threadId":"53661","inReplyTo":null,"subject":"[PATCH] diff-files: treat \"i-t-a\" files as \"not-in-index\"","fromName":"Srinidhi Kaushik","fromEmail":"shrinidhi.kaushik@gmail.com","sentAt":"2020-06-11T16:16:40Z","receivedAt":"2020-06-11T16:16:48Z","isPatch":true,"sender":{"key":"shrinidhi.kaushik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3019537?v=4"},"body":"The `diff-files' command and related commands which call `cmd_diff_files()',\nconsider the \"intent-to-add\" files as a part of the index when comparing the\nwork-tree against it. This was previously addressed in [1] and [2] by turning\nthe option `--ita-invisible-in-index' (introduced in [3]) on by default.\n\nFor `diff-files' (and `add -p' as a consequence) to show the i-t-a files as\nas new, `ita_invisible_in_index' will be enabled by default here as well.\n\n[1] 0231ae71d3 (diff: turn --ita-invisible-in-index on by default, 2018-05-26)\n[2] 425a28e0a4 (diff-lib: allow ita entries treated as \"not yet exist in\n                index\", 2016-10-24)\n[3] b42b451919 (diff: add --ita-[in]visible-in-index, 2016-10-24)\n\nSigned-off-by: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>\n---\n\nHello! This is my first patch in this project.\nThis issue was mentioned in #leftoverbits on GitHub: [1], and this\npatch implements the change proposed in [2].\n\n[1] https://github.com/gitgitgadget/git/issues/647\n[2] https://lore.kernel.org/git/20200527230357.GB546534@coredump.intra.peff.net\n\n\n builtin/diff-files.c  |  7 +++++++\n t/t2203-add-intent.sh | 25 ++++++++++++++++++++++++-\n 2 files changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 86ae474fbf..1e352dd8f7 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -28,6 +28,13 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n+\n+\t/*\n+\t * Consider \"intent-to-add\" files as new by default, unless\n+\t * explicitly specified in the command line or anywhere else.\n+\t */\n+\trev.diffopt.ita_invisible_in_index = 1;\n+\n \tprecompose_argv(argc, argv);\n \n \targc = setup_revisions(argc, argv, &rev, NULL);\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 5bbe8dcce4..742f27a935 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -232,7 +232,7 @@ test_expect_success 'double rename detection in status' '\n \t)\n '\n \n-test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '\n+test_expect_success 'diff/diff-cached shows ita as new/not-new files' '\n \tgit reset --hard &&\n \techo new >new-ita &&\n \tgit add -N new-ita &&\n@@ -243,6 +243,29 @@ test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '\n \ttest_must_be_empty actual2\n '\n \n+test_expect_success 'diff-files shows i-t-a files as new files' '\n+\tgit reset --hard &&\n+\ttouch empty &&\n+\tcontent=\"foo\" &&\n+\techo $content >not-empty &&\n+\tgit add -N empty not-empty &&\n+\tgit diff-files -p >actual &&\n+\thash_e=$(git hash-object empty) &&\n+\thash_n=$(git hash-object not-empty) &&\n+\tcat >expect <<-EOF &&\n+\tdiff --git a/empty b/empty\n+\tnew file mode 100644\n+\tindex 0000000..$(git rev-parse --short $hash_e)\n+\tdiff --git a/not-empty b/not-empty\n+\tnew file mode 100644\n+\tindex 0000000..$(git rev-parse --short $hash_n)\n+\t--- /dev/null\n+\t+++ b/not-empty\n+\t@@ -0,0 +1 @@\n+\t+$content\n+\tEOF\n+\ttest_cmp expect actual\n+'\n \n test_expect_success '\"diff HEAD\" includes ita as new files' '\n \tgit reset --hard &&\n-- \n2.27.0\n\n"},{"id":"399565","messageId":"xmqq4krhjql1.fsf@gitster.c.googlers.com","threadId":"53661","inReplyTo":"20200611161640.52156-1-shrinidhi.kaushik@gmail.com","subject":"Re: [PATCH] diff-files: treat \"i-t-a\" files as \"not-in-index\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-11T20:27:22Z","receivedAt":"2020-06-11T20:27:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:\n\n> The `diff-files' command and related commands which call `cmd_diff_files()',\n> consider the \"intent-to-add\" files as a part of the index when comparing the\n> work-tree against it. This was previously addressed in [1] and [2] by turning\n> the option `--ita-invisible-in-index' (introduced in [3]) on by default.\n>\n> For `diff-files' (and `add -p' as a consequence) to show the i-t-a files as\n> as new, `ita_invisible_in_index' will be enabled by default here as well.\n>\n> [1] 0231ae71d3 (diff: turn --ita-invisible-in-index on by default, 2018-05-26)\n> [2] 425a28e0a4 (diff-lib: allow ita entries treated as \"not yet exist in\n>                 index\", 2016-10-24)\n> [3] b42b451919 (diff: add --ita-[in]visible-in-index, 2016-10-24)\n\nIs there any place where we still run the internal diff machinery to\ncompare the index and the working tree without setting the\nita_invisible_in_index bit on with this patch applied, and if so,\nwhy?  Does the justification why that other place needs to leave\nthe bit off apply to this codepath as well?\n\nWhat I am trying to get at is if this is helping only one usecase\nfor \"diff-files\" while breaking other usecases.\n\nOn the other hand, if there is no longer anybody who wants\nita_invisible_in_index off, perhaps we can get rid of the bit and\nlose many conditionals.\n"},{"id":"399570","messageId":"20200611232851.GA55419@death-star","threadId":"53661","inReplyTo":"xmqq4krhjql1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] diff-files: treat \"i-t-a\" files as \"not-in-index\"","fromName":"Srinidhi Kaushik","fromEmail":"shrinidhi.kaushik@gmail.com","sentAt":"2020-06-11T23:28:51Z","receivedAt":"2020-06-11T23:28:58Z","isPatch":true,"sender":{"key":"shrinidhi.kaushik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3019537?v=4"},"body":"Thanks for replying!\n\nOn Thu, Jun 11, 2020 at 01:27:22PM -0700, Junio C Hamano wrote:\n> Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:\n> \n> > The `diff-files' command and related commands which call `cmd_diff_files()',\n> > consider the \"intent-to-add\" files as a part of the index when comparing the\n> > work-tree against it. This was previously addressed in [1] and [2] by turning\n> > the option `--ita-invisible-in-index' (introduced in [3]) on by default.\n> >\n> > For `diff-files' (and `add -p' as a consequence) to show the i-t-a files as\n> > as new, `ita_invisible_in_index' will be enabled by default here as well.\n> >\n> > [1] 0231ae71d3 (diff: turn --ita-invisible-in-index on by default, 2018-05-26)\n> > [2] 425a28e0a4 (diff-lib: allow ita entries treated as \"not yet exist in\n> >                 index\", 2016-10-24)\n> > [3] b42b451919 (diff: add --ita-[in]visible-in-index, 2016-10-24)\n> \n> Is there any place where we still run the internal diff machinery to\n> compare the index and the working tree without setting the\n> ita_invisible_in_index bit on with this patch applied, and if so,\n> why?  Does the justification why that other place needs to leave\n> the bit off apply to this codepath as well?\n\nYes, I believe that there exist some use cases for `ita_invisible_in_index'\nto be unset. For instance, `index_differs_from' which is used in a quite a\nfew places -- like \"commit\", \"revert\", and \"rebase\" -- which require a\n\"no change\" to be returned.\n\nThis commit: [1] addressed the issue where the cache-tree was producing\nthe same tree as the current commit when it involved \"intent-to-add\"\nentries, instead of aborting.\n\n[1] 018ec3c820 (commit: fix empty commit creation when there's no changes\n                but ita entries, 2016-10-24) \n\n> What I am trying to get at is if this is helping only one usecase\n> for \"diff-files\" while breaking other usecases.\n\nCurrently, `run_add_p' (for \"add\"; which this patch addresses\nthe fix), and `push_to_deploy' (in \"receive-pack\"; where this\nis the intended behavior), call \"diff-files\" as a subprocess,\nin which case the `ita_invisible_in_index' bit is explicitly\nset. For all other cases, calls are made directly\nto `run_diff_files' and will be unaffected by this change.\n \n> On the other hand, if there is no longer anybody who wants\n> ita_invisible_in_index off, perhaps we can get rid of the bit and\n> lose many conditionals.\n"},{"id":"400069","messageId":"20200618175808.GA36733@mail.clickyotomy.dev","threadId":"53661","inReplyTo":"20200611232851.GA55419@death-star","subject":"Re: [PATCH] diff-files: treat \"i-t-a\" files as \"not-in-index\"","fromName":"Srinidhi Kaushik","fromEmail":"shrinidhi.kaushik@gmail.com","sentAt":"2020-06-18T17:58:08Z","receivedAt":"2020-06-18T17:58:18Z","isPatch":true,"sender":{"key":"shrinidhi.kaushik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3019537?v=4"},"body":"Hello,\nIs there any update on this patch?\nPlease let me know if I missed anything.\n\nThanks!\n\nOn 06/12/2020 04:58, Srinidhi Kaushik wrote:\n> Thanks for replying!\n> \n> On Thu, Jun 11, 2020 at 01:27:22PM -0700, Junio C Hamano wrote:\n> > Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:\n> > \n> > > The `diff-files' command and related commands which call `cmd_diff_files()',\n> > > consider the \"intent-to-add\" files as a part of the index when comparing the\n> > > work-tree against it. This was previously addressed in [1] and [2] by turning\n> > > the option `--ita-invisible-in-index' (introduced in [3]) on by default.\n> > >\n> > > For `diff-files' (and `add -p' as a consequence) to show the i-t-a files as\n> > > as new, `ita_invisible_in_index' will be enabled by default here as well.\n> > >\n> > > [1] 0231ae71d3 (diff: turn --ita-invisible-in-index on by default, 2018-05-26)\n> > > [2] 425a28e0a4 (diff-lib: allow ita entries treated as \"not yet exist in\n> > >                 index\", 2016-10-24)\n> > > [3] b42b451919 (diff: add --ita-[in]visible-in-index, 2016-10-24)\n> > \n> > Is there any place where we still run the internal diff machinery to\n> > compare the index and the working tree without setting the\n> > ita_invisible_in_index bit on with this patch applied, and if so,\n> > why?  Does the justification why that other place needs to leave\n> > the bit off apply to this codepath as well?\n> \n> Yes, I believe that there exist some use cases for `ita_invisible_in_index'\n> to be unset. For instance, `index_differs_from' which is used in a quite a\n> few places -- like \"commit\", \"revert\", and \"rebase\" -- which require a\n> \"no change\" to be returned.\n> \n> This commit: [1] addressed the issue where the cache-tree was producing\n> the same tree as the current commit when it involved \"intent-to-add\"\n> entries, instead of aborting.\n> \n> [1] 018ec3c820 (commit: fix empty commit creation when there's no changes\n>                 but ita entries, 2016-10-24) \n> \n> > What I am trying to get at is if this is helping only one usecase\n> > for \"diff-files\" while breaking other usecases.\n> \n> Currently, `run_add_p' (for \"add\"; which this patch addresses\n> the fix), and `push_to_deploy' (in \"receive-pack\"; where this\n> is the intended behavior), call \"diff-files\" as a subprocess,\n> in which case the `ita_invisible_in_index' bit is explicitly\n> set. For all other cases, calls are made directly\n> to `run_diff_files' and will be unaffected by this change.\n>  \n> > On the other hand, if there is no longer anybody who wants\n> > ita_invisible_in_index off, perhaps we can get rid of the bit and\n> > lose many conditionals.\n"},{"id":"400095","messageId":"xmqqk104knrs.fsf@gitster.c.googlers.com","threadId":"53661","inReplyTo":"20200611161640.52156-1-shrinidhi.kaushik@gmail.com","subject":"Re: [PATCH] diff-files: treat \"i-t-a\" files as \"not-in-index\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-18T22:33:11Z","receivedAt":"2020-06-18T22:33:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:\n\n> -test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '\n> +test_expect_success 'diff/diff-cached shows ita as new/not-new files' '\n>  \tgit reset --hard &&\n>  \techo new >new-ita &&\n>  \tgit add -N new-ita &&\n\nInteresting.  \n\nI thought that the test originally tested \"diff-files\" and\n\"diff-index --cached\" and a modernization patch forgot to update the\ntitle when the test body was changed to use \"diff\" and \"diff\n--cached\", but that is not the case here.  When 0231ae71 (diff: turn\n--ita-invisible-in-index on by default, 2018-05-26) added this test,\nit gave a wrong title from the beginning.\n\nNice catch.\n\n> @@ -243,6 +243,29 @@ test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '\n>  \ttest_must_be_empty actual2\n>  '\n>  \n> +test_expect_success 'diff-files shows i-t-a files as new files' '\n> +\tgit reset --hard &&\n> +\ttouch empty &&\n\nUse of \"touch\" gives a wrong impression that you care about the file\ntimestamp; use something like \": >empty &&\" instead when you care\nabout the presence of the file and do not care about its timestamp.\n\n> +\tcontent=\"foo\" &&\n> +\techo $content >not-empty &&\n\nThe quoting decision is backwards in these two lines.  It is OK not\nto quote when the right hand side literal is clearly a single word\nwithout $IFS.  On the other hand, it is a good practice to always\nquote when using what is in a \"$variable\".\n\n> +\tgit add -N empty not-empty &&\n> +\tgit diff-files -p >actual &&\n> +\thash_e=$(git hash-object empty) &&\n> +\thash_n=$(git hash-object not-empty) &&\n> +\tcat >expect <<-EOF &&\n> +\tdiff --git a/empty b/empty\n> +\tnew file mode 100644\n> +\tindex 0000000..$(git rev-parse --short $hash_e)\n> +\tdiff --git a/not-empty b/not-empty\n> +\tnew file mode 100644\n> +\tindex 0000000..$(git rev-parse --short $hash_n)\n> +\t--- /dev/null\n> +\t+++ b/not-empty\n> +\t@@ -0,0 +1 @@\n> +\t+$content\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n\nOK.  Do we want to show what happens when \"diff\" and \"diff --cached\"\nare run with these two \"added but not quite added yet\" paths to\ncontrast with this new case?\n\nThanks.\n"},{"id":"400096","messageId":"xmqqftasknqj.fsf@gitster.c.googlers.com","threadId":"53661","inReplyTo":"20200618175808.GA36733@mail.clickyotomy.dev","subject":"Re: [PATCH] diff-files: treat \"i-t-a\" files as \"not-in-index\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-18T22:33:56Z","receivedAt":"2020-06-18T22:34:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:\n\n> Hello,\n> Is there any update on this patch?\n> Please let me know if I missed anything.\n\nSorry, the patch totally fell through the cracks.\n\nI just sent out a brief review on the part I didn't review during\nthe first round.\n\nThanks.\n"},{"id":"400097","messageId":"xmqqbllgknfd.fsf@gitster.c.googlers.com","threadId":"53661","inReplyTo":"xmqqk104knrs.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] diff-files: treat \"i-t-a\" files as \"not-in-index\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-18T22:40:38Z","receivedAt":"2020-06-18T22:40:46Z","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>> +\ttouch empty &&\n>\n> Use of \"touch\" gives a wrong impression that you care about the file\n> timestamp; use something like \": >empty &&\" instead when you care\n> about the presence of the file and do not care about its timestamp.\n\nI just realized that this is even more important in this case not to\nuse \"touch\".\n\nThe test that uses this file cares not just the presence, but it\ndeeply cares that its contents is empty.  The thing it least cares\nabout is its timestamp.\n\nThe purpose of using \"touch\" is to update the timestamp, to keep the\ncurrent contents if it exists, and to ensure it exists (as a side\neffect), in the decreasing order of importance.  Use of the command\nhere misleads the readers.\n\nThanks.\n"},{"id":"400107","messageId":"20200619093127.GA47528@mail.clickyotomy.dev","threadId":"53661","inReplyTo":"xmqqbllgknfd.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] diff-files: treat \"i-t-a\" files as \"not-in-index\"","fromName":"Srinidhi Kaushik","fromEmail":"shrinidhi.kaushik@gmail.com","sentAt":"2020-06-19T09:31:27Z","receivedAt":"2020-06-19T09:34:16Z","isPatch":true,"sender":{"key":"shrinidhi.kaushik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3019537?v=4"},"body":"Thank you for reviewing this; I appreciate it!\n\n> > +     content=\"foo\" &&\n> > +     echo $content >not-empty &&\n>\n> The quoting decision is backwards in these two lines.  It is OK not\n> to quote when the right hand side literal is clearly a single word\n> without $IFS.  On the other hand, it is a good practice to always\n> quote when using what is in a \"$variable\".\n\nYes, that doesn't look right, I will make changes in v2.\n\n[...]\n> > > +  touch empty &&\n> >\n> > Use of \"touch\" gives a wrong impression that you care about the file\n> > timestamp; use something like \": >empty &&\" instead when you care\n> > about the presence of the file and do not care about its timestamp.\n>\n> I just realized that this is even more important in this case not to\n> use \"touch\".\n>\n> The test that uses this file cares not just the presence, but it\n> deeply cares that its contents is empty.  The thing it least cares\n> about is its timestamp.\n>\n> The purpose of using \"touch\" is to update the timestamp, to keep the\n> current contents if it exists, and to ensure it exists (as a side\n> effect), in the decreasing order of importance.  Use of the command\n> here misleads the readers.\n\nOops, you are right. That makes sense. Will update to \": >empty\".\n\n[...]\n> > +     git add -N empty not-empty &&\n> > +     git diff-files -p >actual &&\n> > +     hash_e=$(git hash-object empty) &&\n> > +     hash_n=$(git hash-object not-empty) &&\n> > +     cat >expect <<-EOF &&\n> > +     diff --git a/empty b/empty\n> > +     new file mode 100644\n> > +     index 0000000..$(git rev-parse --short $hash_e)\n> > +     diff --git a/not-empty b/not-empty\n> > +     new file mode 100644\n> > +     index 0000000..$(git rev-parse --short $hash_n)\n> > +     --- /dev/null\n> > +     +++ b/not-empty\n> > +     @@ -0,0 +1 @@\n> > +     +$content\n> > +     EOF\n> > +     test_cmp expect actual\n> > +'\n>\n> OK.  Do we want to show what happens when \"diff\" and \"diff --cached\"\n> are run with these two \"added but not quite added yet\" paths to\n> contrast with this new case?\n\nI'm not sure if we want to repeat an older test. The test (which was\nrenamed in this patch) in t2203-add-intent.sh: \"diff/diff-cached shows\nita as new/not-new files\" is already doing that. Should  the \"diff\" and\n\"diff --cached\" steps be appended here again?\n\nThanks.\n"},{"id":"400225","messageId":"xmqqftaqg299.fsf@gitster.c.googlers.com","threadId":"53661","inReplyTo":"20200619093127.GA47528@mail.clickyotomy.dev","subject":"Re: [PATCH] diff-files: treat \"i-t-a\" files as \"not-in-index\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-19T21:43:46Z","receivedAt":"2020-06-19T21:43:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:\n\n>> > +     git add -N empty not-empty &&\n>> > +     git diff-files -p >actual &&\n>> > +     hash_e=$(git hash-object empty) &&\n>> > +     hash_n=$(git hash-object not-empty) &&\n>> > +     cat >expect <<-EOF &&\n>> > +     diff --git a/empty b/empty\n>> > +     new file mode 100644\n>> > +     index 0000000..$(git rev-parse --short $hash_e)\n>> > +     diff --git a/not-empty b/not-empty\n>> > +     new file mode 100644\n>> > +     index 0000000..$(git rev-parse --short $hash_n)\n>> > +     --- /dev/null\n>> > +     +++ b/not-empty\n>> > +     @@ -0,0 +1 @@\n>> > +     +$content\n>> > +     EOF\n>> > +     test_cmp expect actual\n>> > +'\n>>\n>> OK.  Do we want to show what happens when \"diff\" and \"diff --cached\"\n>> are run with these two \"added but not quite added yet\" paths to\n>> contrast with this new case?\n>\n> I'm not sure if we want to repeat an older test. The test (which was\n> renamed in this patch) in t2203-add-intent.sh: \"diff/diff-cached shows\n> ita as new/not-new files\" is already doing that. Should  the \"diff\" and\n> \"diff --cached\" steps be appended here again?\n\nNo, there is no need to repeat essentially the same test that exists\nelsewhere.  I wonder if it reduces duplication even further if we\nextend that existing test that checks \"diff\" and \"diff --cached\" so\nthat it also checks \"diff-files\" as well?, instead of adding this\nnew one?  The existing one checks diff and diff-cached only with a\nnew non-empty path, and it can use tests with a new empty path at\nthe same time, with a unified \"set up\" code that is in the early\npart of the test, e.g.\n\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 5bbe8dcce4..cfde790ac7 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -235,7 +235,8 @@ test_expect_success 'double rename detection in status' '\n test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '\n \tgit reset --hard &&\n \techo new >new-ita &&\n-\tgit add -N new-ita &&\n+\t: >new-ita-empty &&\n+\tgit add -N new-ita new-ita-empty &&\n \tgit diff --summary >actual &&\n \t...\n\nThen the existing tests can be updated to see not just --summary but\nalso for the contents like you did in the new test---and another test\nthat examines what \"git diff-files\" sees (which is what you added)\ncan happen in the same test (that way, the same set-up can be reused\nfor three tests).\n\nThanks.\n\n\n\n\n\n\n\n\n\n"},{"id":"400256","messageId":"20200620163845.871-1-shrinidhi.kaushik@gmail.com","threadId":"53661","inReplyTo":"20200611161640.52156-1-shrinidhi.kaushik@gmail.com","subject":"[PATCH v2] diff-files: treat \"i-t-a\" files as \"not-in-index\"","fromName":"Srinidhi Kaushik","fromEmail":"shrinidhi.kaushik@gmail.com","sentAt":"2020-06-20T16:38:45Z","receivedAt":"2020-06-20T16:39:12Z","isPatch":true,"sender":{"key":"shrinidhi.kaushik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3019537?v=4"},"body":"The `diff-files' command and related commands which call the function\n`cmd_diff_files()', consider the \"intent-to-add\" files as a part of the\nindex when comparing the work-tree against it. This was previously\naddressed in commits [1] and [2] by turning the option\n`--ita-invisible-in-index' (introduced in [3]) on by default.\n\nFor `diff-files' (and `add -p' as a consequence) to show the i-t-a\nfiles as as new, `ita_invisible_in_index' will be enabled by default\nhere as well.\n\n[1] 0231ae71d3 (diff: turn --ita-invisible-in-index on by default,\n                2018-05-26)\n[2] 425a28e0a4 (diff-lib: allow ita entries treated as \"not yet exist\n                in index\", 2016-10-24)\n[3] b42b451919 (diff: add --ita-[in]visible-in-index, 2016-10-24)\n\nSigned-off-by: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>\n---\n\nSummary of changes since v1:\n  - Combine the existing \"diff\", \"diff --cached\" test with the new one.\n  - Fix quoting, empty file creation in tests.\n  - Add an additional test for \"diff-files --abbrev\".\n\n builtin/diff-files.c  |  7 ++++++\n t/t2203-add-intent.sh | 53 ++++++++++++++++++++++++++++++++++++-------\n 2 files changed, 52 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 86ae474fbf..1e352dd8f7 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -28,6 +28,13 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n+\n+\t/*\n+\t * Consider \"intent-to-add\" files as new by default, unless\n+\t * explicitly specified in the command line or anywhere else.\n+\t */\n+\trev.diffopt.ita_invisible_in_index = 1;\n+\n \tprecompose_argv(argc, argv);\n\n \targc = setup_revisions(argc, argv, &rev, NULL);\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 5bbe8dcce4..8a5d55054f 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -232,17 +232,54 @@ test_expect_success 'double rename detection in status' '\n \t)\n '\n\n-test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '\n+test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new for \"diff --cached\"' '\n \tgit reset --hard &&\n-\techo new >new-ita &&\n-\tgit add -N new-ita &&\n+\t: >empty &&\n+\tcontent=\"foo\" &&\n+\techo \"$content\" >not-empty &&\n+\n+\thash_e=$(git hash-object empty) &&\n+\thash_n=$(git hash-object not-empty) &&\n+\thash_t=$(git hash-object -t tree /dev/null) &&\n+\n+\tcat >expect.diff_p <<-EOF &&\n+\tdiff --git a/empty b/empty\n+\tnew file mode 100644\n+\tindex 0000000..$(git rev-parse --short $hash_e)\n+\tdiff --git a/not-empty b/not-empty\n+\tnew file mode 100644\n+\tindex 0000000..$(git rev-parse --short $hash_n)\n+\t--- /dev/null\n+\t+++ b/not-empty\n+\t@@ -0,0 +1 @@\n+\t+$content\n+\tEOF\n+\tcat >expect.diff_s <<-EOF &&\n+\t create mode 100644 empty\n+\t create mode 100644 not-empty\n+\tEOF\n+\tcat >expect.diff_a <<-EOF &&\n+\t:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf \"\\t\")empty\n+\t:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf \"\\t\")not-empty\n+\tEOF\n+\n+\tgit add -N empty not-empty &&\n+\n+\tgit diff >actual &&\n+\ttest_cmp expect.diff_p actual &&\n+\n \tgit diff --summary >actual &&\n-\techo \" create mode 100644 new-ita\" >expected &&\n-\ttest_cmp expected actual &&\n-\tgit diff --cached --summary >actual2 &&\n-\ttest_must_be_empty actual2\n-'\n+\ttest_cmp expect.diff_s actual &&\n+\n+\tgit diff-files -p >actual &&\n+\ttest_cmp expect.diff_p actual &&\n\n+\tgit diff-files --abbrev >actual &&\n+\ttest_cmp expect.diff_a actual &&\n+\n+\tgit diff --cached >actual &&\n+\ttest_must_be_empty actual\n+'\n\n test_expect_success '\"diff HEAD\" includes ita as new files' '\n \tgit reset --hard &&\n--\n2.27.0\n"},{"id":"400257","messageId":"xmqqsgepekzr.fsf@gitster.c.googlers.com","threadId":"53661","inReplyTo":"20200620163845.871-1-shrinidhi.kaushik@gmail.com","subject":"Re: [PATCH v2] diff-files: treat \"i-t-a\" files as \"not-in-index\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-20T16:54:16Z","receivedAt":"2020-06-20T16:54:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:\n\n> The `diff-files' command and related commands which call the function\n> `cmd_diff_files()', consider the \"intent-to-add\" files as a part of the\n> index when comparing the work-tree against it. This was previously\n> addressed in commits [1] and [2] by turning the option\n> `--ita-invisible-in-index' (introduced in [3]) on by default.\n>\n> For `diff-files' (and `add -p' as a consequence) to show the i-t-a\n> files as as new, `ita_invisible_in_index' will be enabled by default\n> here as well.\n>\n> [1] 0231ae71d3 (diff: turn --ita-invisible-in-index on by default,\n>                 2018-05-26)\n> [2] 425a28e0a4 (diff-lib: allow ita entries treated as \"not yet exist\n>                 in index\", 2016-10-24)\n> [3] b42b451919 (diff: add --ita-[in]visible-in-index, 2016-10-24)\n>\n> Signed-off-by: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>\n> ---\n\nThanks.\n\n> -test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '\n> +test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new for \"diff --cached\"' '\n>  \tgit reset --hard &&\n> +\t: >empty &&\n> +\tcontent=\"foo\" &&\n> +\techo \"$content\" >not-empty &&\n> +\n> +\thash_e=$(git hash-object empty) &&\n> +\thash_n=$(git hash-object not-empty) &&\n> +\thash_t=$(git hash-object -t tree /dev/null) &&\n> +\n> +\tcat >expect.diff_p <<-EOF &&\n> +\tdiff --git a/empty b/empty\n> +\tnew file mode 100644\n> +\tindex 0000000..$(git rev-parse --short $hash_e)\n> +\tdiff --git a/not-empty b/not-empty\n> +\tnew file mode 100644\n> +\tindex 0000000..$(git rev-parse --short $hash_n)\n> +\t--- /dev/null\n> +\t+++ b/not-empty\n> +\t@@ -0,0 +1 @@\n> +\t+$content\n> +\tEOF\n> +\tcat >expect.diff_s <<-EOF &&\n> +\t create mode 100644 empty\n> +\t create mode 100644 not-empty\n> +\tEOF\n> +\tcat >expect.diff_a <<-EOF &&\n> +\t:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf \"\\t\")empty\n> +\t:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf \"\\t\")not-empty\n\nThis is good, but just FYI you did not have to use printf --- \"<<-\"\ntreats only leading tab specially, and not in the middle of a line.\n\n> +\tEOF\n> +\n> +\tgit add -N empty not-empty &&\n\nOK.  So with two paths...\n\n> +\n> +\tgit diff >actual &&\n> +\ttest_cmp expect.diff_p actual &&\n\n... the patch output of \"git diff\", and\n\n>  \tgit diff --summary >actual &&\n\n... the summary part, and\n\n> -\techo \" create mode 100644 new-ita\" >expected &&\n> -\ttest_cmp expected actual &&\n> -\tgit diff --cached --summary >actual2 &&\n> -\ttest_must_be_empty actual2\n> -'\n> +\ttest_cmp expect.diff_s actual &&\n> +\n> +\tgit diff-files -p >actual &&\n> +\ttest_cmp expect.diff_p actual &&\n>\n> +\tgit diff-files --abbrev >actual &&\n> +\ttest_cmp expect.diff_a actual &&\n\n... the same for \"diff-files\" and\n\n> +\tgit diff --cached >actual &&\n\n... \"diff -cached\" are all checked.\n\nLooking good.\n\n> +\ttest_must_be_empty actual\n> +'\n>\n>  test_expect_success '\"diff HEAD\" includes ita as new files' '\n>  \tgit reset --hard &&\n> --\n> 2.27.0\n"},{"id":"400375","messageId":"nycvar.QRO.7.76.6.2006231714450.54@tvgsbejvaqbjf.bet","threadId":"53661","inReplyTo":"20200620163845.871-1-shrinidhi.kaushik@gmail.com","subject":"Re: [PATCH v2] diff-files: treat \"i-t-a\" files as \"not-in-index\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-06-23T15:17:12Z","receivedAt":"2020-06-23T15:17:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Srinidhi,\n\nOn Sat, 20 Jun 2020, Srinidhi Kaushik wrote:\n\n> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\n> index 5bbe8dcce4..8a5d55054f 100755\n> --- a/t/t2203-add-intent.sh\n> +++ b/t/t2203-add-intent.sh\n> @@ -232,17 +232,54 @@ test_expect_success 'double rename detection in status' '\n>  \t)\n>  '\n>\n> -test_expect_success 'diff-files/diff-cached shows ita as new/not-new files' '\n> +test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new for \"diff --cached\"' '\n>  \tgit reset --hard &&\n> -\techo new >new-ita &&\n> -\tgit add -N new-ita &&\n> +\t: >empty &&\n> +\tcontent=\"foo\" &&\n> +\techo \"$content\" >not-empty &&\n> +\n> +\thash_e=$(git hash-object empty) &&\n> +\thash_n=$(git hash-object not-empty) &&\n> +\thash_t=$(git hash-object -t tree /dev/null) &&\n\nSo this is the hash of the empty tree object, and...\n\n> +\n> +\tcat >expect.diff_p <<-EOF &&\n> +\tdiff --git a/empty b/empty\n> +\tnew file mode 100644\n> +\tindex 0000000..$(git rev-parse --short $hash_e)\n> +\tdiff --git a/not-empty b/not-empty\n> +\tnew file mode 100644\n> +\tindex 0000000..$(git rev-parse --short $hash_n)\n> +\t--- /dev/null\n> +\t+++ b/not-empty\n> +\t@@ -0,0 +1 @@\n> +\t+$content\n> +\tEOF\n> +\tcat >expect.diff_s <<-EOF &&\n> +\t create mode 100644 empty\n> +\t create mode 100644 not-empty\n> +\tEOF\n> +\tcat >expect.diff_a <<-EOF &&\n> +\t:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf \"\\t\")empty\n> +\t:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf \"\\t\")not-empty\n\n... here we expect `git diff --raw` to claim that the contents of `empty`\nand of `non-empty` are actually the empty tree.\n\nThe underlying problem is that some time ago, the (already incorrect)\nempty blob constant was replaced by the empty tree constant, by mistake. I\ncontributed a patch series to fix that, and Cc:ed you you in v2 that I\nsent out earlier today.\n\nIt would be helpful if you reviewed it carefully, as you are already\nfamiliar with the code in question.\n\nThank you,\nJohannes\n\n> +\tEOF\n> +\n> +\tgit add -N empty not-empty &&\n> +\n> +\tgit diff >actual &&\n> +\ttest_cmp expect.diff_p actual &&\n> +\n>  \tgit diff --summary >actual &&\n> -\techo \" create mode 100644 new-ita\" >expected &&\n> -\ttest_cmp expected actual &&\n> -\tgit diff --cached --summary >actual2 &&\n> -\ttest_must_be_empty actual2\n> -'\n> +\ttest_cmp expect.diff_s actual &&\n> +\n> +\tgit diff-files -p >actual &&\n> +\ttest_cmp expect.diff_p actual &&\n>\n> +\tgit diff-files --abbrev >actual &&\n> +\ttest_cmp expect.diff_a actual &&\n> +\n> +\tgit diff --cached >actual &&\n> +\ttest_must_be_empty actual\n> +'\n>\n>  test_expect_success '\"diff HEAD\" includes ita as new files' '\n>  \tgit reset --hard &&\n> --\n> 2.27.0\n>\n>\n"}]}