{"thread":{"id":"51472","subject":"[PATCH] reset: unstage empty deleted ita files","startedAt":"2019-07-12T15:03:49Z","lastAt":"2020-02-14T17:12:28Z","messageCount":11,"participants":["Varun Naik","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"378886","messageId":"20190712150235.12633-1-vcnaik94@gmail.com","threadId":"51472","inReplyTo":null,"subject":"[PATCH] reset: unstage empty deleted ita files","fromName":"Varun Naik","fromEmail":"vcnaik94@gmail.com","sentAt":"2019-07-12T15:02:35Z","receivedAt":"2019-07-12T15:03:49Z","isPatch":true,"sender":{"key":"vcnaik94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2382501?v=4"},"body":"It is possible to delete a committed file from the index and then add it\nas intent-to-add. After `git reset HEAD`, the file should be identical\nin the index and HEAD. This patch provides the desired behavior even\nwhen the file is empty in the index.\n\nSigned-off-by: Varun Naik \n---\nMy first patch!\n\nAccording to an old thread on the mailing list[0], this issue is known,\nbut it looks like people overlooked the fix that I'm making. Basically,\nthis one line causes diff-lib.c:do_oneway_diff() to call show_modified()\ninstead of diff_index_show_file() for ita files.\n\nAn alternative to the last line of the test code is running `git status\n--porcelain=v2` and then inspecting the output. I can adjust that if\nnecessary.\n\nUnfortunately, \"restore\" has a similar problem, but it follows a\ndifferent code path into read-cache.c rather than diff-lib.c, and there\ndoes not seem to be a similar one-line fix.\n\n[0]: https://public-inbox.org/git/CACsJy8Bov1asw+_J_fbhKqigM==xNPi8itDGkhibkYVch4pvmQ@mail.gmail.com/\n\n builtin/reset.c | 1 +\n t/t7102-reset.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 26ef9a7bd0..47a088f4b7 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -163,6 +163,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n \topt.format_callback_data = &intent_to_add;\n \topt.flags.override_submodule_config = 1;\n \topt.repo = the_repository;\n+\topt.ita_invisible_in_index = 1;\n \n \tif (do_diff_cache(tree_oid, &opt))\n \t\treturn 1;\ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex 97be0d968d..250fc1c890 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -566,4 +566,15 @@ test_expect_success 'reset --mixed sets up work tree' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'reset --mixed removes deleted intent-to-add file from index' '\n+\techo \"nonempty\" >nonempty &&\n+\t>empty &&\n+\tgit add nonempty empty &&\n+\tgit commit -m \"create files to be deleted\" &&\n+\tgit rm --cached nonempty empty &&\n+\tgit add -N nonempty empty &&\n+\tgit reset HEAD nonempty empty &&\n+\tgit diff --staged --exit-code\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"379325","messageId":"20190726044806.2216-1-vcnaik94@gmail.com","threadId":"51472","inReplyTo":"20190712150235.12633-1-vcnaik94@gmail.com","subject":"[PATCH v2] reset: unstage empty deleted ita files","fromName":"Varun Naik","fromEmail":"vcnaik94@gmail.com","sentAt":"2019-07-26T04:48:06Z","receivedAt":"2019-07-26T04:48:35Z","isPatch":true,"sender":{"key":"vcnaik94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2382501?v=4"},"body":"It is possible to delete a committed file from the index and then add it\nas intent-to-add. After `git reset HEAD`, the file should be identical\nin the index and HEAD. This patch provides the desired behavior even\nwhen the file is empty in the index.\n\nSigned-off-by: Varun Naik \n---\nCC Duy because you seem to be the one who is most involved in changes to\nita behavior.\n\nThe test description was incorrect, so I changed that now. I also\nfigured out how to fix the related problem in \"restore\"; separate patch\ncoming soon.\n\n builtin/reset.c | 1 +\n t/t7102-reset.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 26ef9a7bd0..47a088f4b7 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -163,6 +163,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n \topt.format_callback_data = &intent_to_add;\n \topt.flags.override_submodule_config = 1;\n \topt.repo = the_repository;\n+\topt.ita_invisible_in_index = 1;\n \n \tif (do_diff_cache(tree_oid, &opt))\n \t\treturn 1;\ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex 97be0d968d..9f3854e8f0 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -566,4 +566,15 @@ test_expect_success 'reset --mixed sets up work tree' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'reset --mixed adds deleted intent-to-add file back to index' '\n+\techo \"nonempty\" >nonempty &&\n+\t>empty &&\n+\tgit add nonempty empty &&\n+\tgit commit -m \"create files to be deleted\" &&\n+\tgit rm --cached nonempty empty &&\n+\tgit add -N nonempty empty &&\n+\tgit reset HEAD nonempty empty &&\n+\tgit diff --staged --exit-code\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"379380","messageId":"xmqqsgqsodyu.fsf@gitster-ct.c.googlers.com","threadId":"51472","inReplyTo":"20190726044806.2216-1-vcnaik94@gmail.com","subject":"Re: [PATCH v2] reset: unstage empty deleted ita files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-26T18:19:53Z","receivedAt":"2019-07-26T18:20:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Varun Naik writes:\n\n> It is possible to delete a committed file from the index and then add it\n> as intent-to-add. After `git reset HEAD`, the file should be identical\n> in the index and HEAD. This patch provides the desired behavior even\n> when the file is empty in the index.\n\nThe first and the second sentence describes what you want to achieve\nconcisely and sensibly. There is a vast leap between them and the\nlast sentence. What's missing is:\n\n - What goes wrong without this one-liner fix and how does the\n command make a wrong decision to leave the path 'empty' (in the\n new test) different from that of the tree of the HEAD?\n\n - How does the change help the machinery to make a right decision\n instead?\n\nI had to briefly wonder if this change interacts with \"reset -N\" in\nany way. In that mode, we want to make sure that diff sees the\nentries that are missing from the index that exist in the tree of\nthe HEAD, so that update_index_from_diff() can add them back to the\nindex as I-T-A entries.\n\nMaking I-T-A entries invisible in the index for the purpose of\nrunning that diff would mean that they appear as removed, but it is\nOK because we'll add them back as I-T-A entries anyway, so it all\nevens out, I think.\n\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 26ef9a7bd0..47a088f4b7 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -163,6 +163,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n> \topt.format_callback_data = &intent_to_add;\n> \topt.flags.override_submodule_config = 1;\n> \topt.repo = the_repository;\n> +\topt.ita_invisible_in_index = 1;\n> \n> \tif (do_diff_cache(tree_oid, &opt))\n> \t\treturn 1;\n> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\n> index 97be0d968d..9f3854e8f0 100755\n> --- a/t/t7102-reset.sh\n> +++ b/t/t7102-reset.sh\n> @@ -566,4 +566,15 @@ test_expect_success 'reset --mixed sets up work tree' '\n> \ttest_must_be_empty actual\n> '\n> \n> +test_expect_success 'reset --mixed adds deleted intent-to-add file back to index' '\n> +\techo \"nonempty\" >nonempty &&\n> +\t>empty &&\n> +\tgit add nonempty empty &&\n> +\tgit commit -m \"create files to be deleted\" &&\n> +\tgit rm --cached nonempty empty &&\n> +\tgit add -N nonempty empty &&\n> +\tgit reset HEAD nonempty empty &&\n> +\tgit diff --staged --exit-code\n\nWe are not testing if the \"diff --staged\" synonym (that is not even\nin \"git diff --help\") behaves identically to \"diff --cached\" here\n(if we wanted to do such a test, we should do so somewhere in t4xxx\nseries, not here), so let's spell it in the canonical form, like so:\n\n\tgit diff --cached --exit-code HEAD\n\nAt this point, we have three paths (empty, nonempty and secondfile)\nin the tree of the HEAD, and we just resetted the entries for the\nfirst two paths in the index to match. The 'secondfile' added, in\nprevious tests, is still there unchanged, and is not shown in the\ndiff output. The 'new-file', added as I-T-A in previous tests, is\nalso still there unchanged, and is not shown in the diff output.\n\nHow does the internal do_diff_cache() behave differently before and\nafter this patch on 'empty' and 'nonempty'? How does the difference\naffect the final outcome of \"git reset\" operation?\n\n - without ita-is-invisible bit set, we end up comparing the mode\n and blob object name; for 'nonempty', HEAD records a blob object\n name for the non-empty content, but the index has an empty blob\n object name (with I-T-A bit set, but that does not participate in\n the diff operation at the level of diff-lib.c::do_oneway_diff())\n and are deemed \"modified\". Even though we should say \"deleted\",\n the end result turns out to be the same---we restore from the\n tree of the HEAD.\n\n - however, for 'empty', we mistakenly end up saying \"both are empty\n blobs, so no difference; nothing to be done\", leaving the i-t-a\n entry in the index.\n\n - with ita-is-invisible bit set, both 'nonempty' and 'empty' are\n immediately marked as \"deleted\" in do_oneway_diff(). This causes\n both paths to be resurrected from the tree of the HEAD the same\n way.\n\nWith the above kind of analysis, a reader can fill in the leap in\nthe explanation between the second and the third sentence in the\nproposed log message. But we shouldn't force readers to make that\neffort to understand how the patch was meant to improve things.\n\nThanks.\n\n> +'\n> +\n> test_done\n"},{"id":"379473","messageId":"CAK_rgsFDpW3JhMAc=VBm=WD3drFnV0g=QN15+uZYdsnBTnX_Cg@mail.gmail.com","threadId":"51472","inReplyTo":"xmqqsgqsodyu.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] reset: unstage empty deleted ita files","fromName":"Varun Naik","fromEmail":"vcnaik94@gmail.com","sentAt":"2019-07-29T06:52:20Z","receivedAt":"2019-07-29T06:52:34Z","isPatch":true,"sender":{"key":"vcnaik94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2382501?v=4"},"body":"On Fri, Jul 26, 2019 at 11:20 AM Junio C Hamano wrote:\n>\n> Varun Naik writes:\n>\n> > It is possible to delete a committed file from the index and then add it\n> > as intent-to-add. After `git reset HEAD`, the file should be identical\n> > in the index and HEAD. This patch provides the desired behavior even\n> > when the file is empty in the index.\n>\n> The first and the second sentence describes what you want to achieve\n> concisely and sensibly. There is a vast leap between them and the\n> last sentence. What's missing is:\n>\n> - What goes wrong without this one-liner fix and how does the\n> command make a wrong decision to leave the path 'empty' (in the\n> new test) different from that of the tree of the HEAD?\n>\n> - How does the change help the machinery to make a right decision\n> instead?\n>\n> I had to briefly wonder if this change interacts with \"reset -N\" in\n> any way. In that mode, we want to make sure that diff sees the\n> entries that are missing from the index that exist in the tree of\n> the HEAD, so that update_index_from_diff() can add them back to the\n> index as I-T-A entries.\n>\n> Making I-T-A entries invisible in the index for the purpose of\n> running that diff would mean that they appear as removed, but it is\n> OK because we'll add them back as I-T-A entries anyway, so it all\n> evens out, I think.\n>\n> > diff --git a/builtin/reset.c b/builtin/reset.c\n> > index 26ef9a7bd0..47a088f4b7 100644\n> > --- a/builtin/reset.c\n> > +++ b/builtin/reset.c\n> > @@ -163,6 +163,7 @@ static int read_from_tree(const struct pathspec *pathspec,\n> > opt.format_callback_data = &intent_to_add;\n> > opt.flags.override_submodule_config = 1;\n> > opt.repo = the_repository;\n> > + opt.ita_invisible_in_index = 1;\n> >\n> > if (do_diff_cache(tree_oid, &opt))\n> > return 1;\n> > diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\n> > index 97be0d968d..9f3854e8f0 100755\n> > --- a/t/t7102-reset.sh\n> > +++ b/t/t7102-reset.sh\n> > @@ -566,4 +566,15 @@ test_expect_success 'reset --mixed sets up work tree' '\n> > test_must_be_empty actual\n> > '\n> >\n> > +test_expect_success 'reset --mixed adds deleted intent-to-add file back to index' '\n> > + echo \"nonempty\" >nonempty &&\n> > + >empty &&\n> > + git add nonempty empty &&\n> > + git commit -m \"create files to be deleted\" &&\n> > + git rm --cached nonempty empty &&\n> > + git add -N nonempty empty &&\n> > + git reset HEAD nonempty empty &&\n> > + git diff --staged --exit-code\n>\n> We are not testing if the \"diff --staged\" synonym (that is not even\n> in \"git diff --help\") behaves identically to \"diff --cached\" here\n> (if we wanted to do such a test, we should do so somewhere in t4xxx\n> series, not here), so let's spell it in the canonical form, like so:\n>\n> git diff --cached --exit-code HEAD\n>\n> At this point, we have three paths (empty, nonempty and secondfile)\n> in the tree of the HEAD, and we just resetted the entries for the\n> first two paths in the index to match. The 'secondfile' added, in\n> previous tests, is still there unchanged, and is not shown in the\n> diff output. The 'new-file', added as I-T-A in previous tests, is\n> also still there unchanged, and is not shown in the diff output.\n>\n\nTo guard against changes to the test cases in the future, would it be\nbetter if I write something like the following instead?\n git diff --cached --exit-code HEAD nonempty empty\n\n> How does the internal do_diff_cache() behave differently before and\n> after this patch on 'empty' and 'nonempty'? How does the difference\n> affect the final outcome of \"git reset\" operation?\n>\n> - without ita-is-invisible bit set, we end up comparing the mode\n> and blob object name; for 'nonempty', HEAD records a blob object\n> name for the non-empty content, but the index has an empty blob\n> object name (with I-T-A bit set, but that does not participate in\n> the diff operation at the level of diff-lib.c::do_oneway_diff())\n> and are deemed \"modified\". Even though we should say \"deleted\",\n> the end result turns out to be the same---we restore from the\n> tree of the HEAD.\n>\n> - however, for 'empty', we mistakenly end up saying \"both are empty\n> blobs, so no difference; nothing to be done\", leaving the i-t-a\n> entry in the index.\n>\n> - with ita-is-invisible bit set, both 'nonempty' and 'empty' are\n> immediately marked as \"deleted\" in do_oneway_diff(). This causes\n> both paths to be resurrected from the tree of the HEAD the same\n> way.\n>\n> With the above kind of analysis, a reader can fill in the leap in\n> the explanation between the second and the third sentence in the\n> proposed log message. But we shouldn't force readers to make that\n> effort to understand how the patch was meant to improve things.\n>\n\nThank you for the detailed explanation, this really helped me\nunderstand the internals of do_diff_cache(). While adjusting my commit\nmessage, I realized that my change actually breaks `git reset HEAD`\nfor ita files (i.e. after `git add -N nonempty` and `git reset HEAD\nnonempty`, the file is still marked as intent-to-add). So, instead of\nsetting the flag ita_invisible_in_index to 1 before calling\ndo_diff_cache(), we want to handle a specific edge case deep inside\nthe diff machinery. It looks like I fixed another bug in the process,\nso I will write a test case for that and then send out v3.\n\n> Thanks.\n>\n> > +'\n> > +\n> > test_done\n"},{"id":"379508","messageId":"xmqqtvb4vn73.fsf@gitster-ct.c.googlers.com","threadId":"51472","inReplyTo":"CAK_rgsFDpW3JhMAc=VBm=WD3drFnV0g=QN15+uZYdsnBTnX_Cg@mail.gmail.com","subject":"Re: [PATCH v2] reset: unstage empty deleted ita files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-29T16:07:44Z","receivedAt":"2019-07-29T16:07:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Varun Naik writes:\n\n> To guard against changes to the test cases in the future, would it be\n> better if I write something like the following instead?\n> git diff --cached --exit-code HEAD nonempty empty\n\nHmph, that is probably a good idea, as it matches the kind of\n\"reset\" we just did, which is what we are testing.\n\n"},{"id":"379758","messageId":"20190801161558.12838-1-vcnaik94@gmail.com","threadId":"51472","inReplyTo":"20190712150235.12633-1-vcnaik94@gmail.com","subject":"[PATCH v3] diff-lib.c: handle empty deleted ita files","fromName":"Varun Naik","fromEmail":"vcnaik94@gmail.com","sentAt":"2019-08-01T16:15:58Z","receivedAt":"2019-08-01T16:16:23Z","isPatch":true,"sender":{"key":"vcnaik94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2382501?v=4"},"body":"It is possible to delete a committed file from the index and then add it\nas intent-to-add. Certain forms of `git diff` should show the file.\nAfter `git reset HEAD`, the file should be identical in the index and\nHEAD. The commands already work correctly if the file has contents in\nHEAD. This patch provides the desired behavior even when the file is\nempty in HEAD.\n\nThe affected \"diff\" commands and the \"reset\" command call\ndiff-lib.c:do_oneway_diff() with a cache entry in the index and a cache\nentry in HEAD. An ita file is represented in the index by a cache entry\nwith the same hash as an empty file. For a nonempty deleted ita file,\ndo_oneway_diff() calls show_modified(), which detects a diff between the\ncache entry in the index and the cache entry in HEAD and therefore deems\nthe file \"modified\". However, for an empty deleted ita file,\ndo_oneway_diff() previously detected no such diff between the two cache\nentries and therefore deemed the file \"not modified\". After this fix,\nfor any deleted ita file, do_oneway_diff() calls diff_index_show_file()\nand deems the file \"deleted\".\n\n`git diff-index --cached HEAD` prints a row of output for both a\n\"modified\" and a \"deleted\" file, although the output differs slightly.\n`git reset HEAD` treats a \"modified\" and a \"deleted\" file similarly,\nresurrecting the file in the index from HEAD.\n\nThis change should not affect newly added ita files. For those, the\n\"tree\" cache entry is NULL, so the changed code is not executed.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Varun Naik \n---\nI tried to limit the \"blast radius\" of affected commands as much as\npossible. To find commands that were affected, I ran the following:\n git diff\n git diff HEAD\n git diff --cached HEAD\n git diff-index HEAD\n git diff-index --cached HEAD\n git diff-files\n\nI also ran each command with the option \"--ita-visible-in-index\" and the\noption \"--ita-invisible-in-index\". Of these 18 commands, the following\nthree showed a diff for nonempty deleted ita files, but no diff for\nempty deleted ita files. All three commands now work correctly, but I\nchose the first one for the test case because the option\n\"--ita-visible-in-index\" is still marked as experimental.\n git diff-index --cached HEAD\n git diff-index --cached --ita-visible-in-index HEAD\n git diff --cached --ita-visible-in-index HEAD\n\nThe `git add` at the end of the \"diff-index\" test case is necessary\nbecause `git reset --hard HEAD` at the beginning of the next test case\n_also_ breaks for empty deleted ita files. That command goes into\nunpack-trees.c:oneway_merge() rather than diff-lib.c:do_oneway_diff().\nI plan to create a separate patch to fix that, after I figure out which\ncommands are part of its blast radius.\n\n diff-lib.c | 5 ++++-\n t/t2203-add-intent.sh | 13 +++++++++++++\n t/t7102-reset.sh | 11 +++++++++++\n 3 files changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 61812f48c2..29dba467d5 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -433,8 +433,11 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \n \t/*\n \t * Something removed from the tree?\n+\t * Consider a file deleted from the index and added as ita to be \"deleted\",\n+\t * even though it should arguably be \"modified\", because we want empty\n+\t * deleted ita files to appear in the diff.\n \t */\n-\tif (!idx) {\n+\tif (!idx || (cached && ce_intent_to_add(idx))) {\n \t\tdiff_index_show_file(revs, \"-\", tree, &tree->oid, 1,\n \t\t\t\t tree->ce_mode, 0);\n \t\treturn;\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 68e54d5c44..4e4a972921 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -261,6 +261,19 @@ test_expect_success '\"diff HEAD\" includes ita as new files' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success '\"diff-index --cached HEAD\" detects diff for deleted intent-to-add file' '\n+\tgit reset --hard &&\n+\techo \"nonempty\" >nonempty &&\n+\t>empty &&\n+\tgit add nonempty empty &&\n+\tgit commit -m \"create files to be deleted\" &&\n+\tgit rm --cached nonempty empty &&\n+\tgit add -N nonempty empty &&\n+\ttest_expect_code 1 git diff-index --cached --exit-code HEAD nonempty &&\n+\ttest_expect_code 1 git diff-index --cached --exit-code HEAD empty &&\n+\tgit add nonempty empty\n+'\n+\n test_expect_success 'apply --intent-to-add' '\n \tgit reset --hard &&\n \techo new >new-ita &&\ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex 97be0d968d..7b79502f7d 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -566,4 +566,15 @@ test_expect_success 'reset --mixed sets up work tree' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'reset --mixed removes deleted intent-to-add file from index' '\n+\techo \"nonempty\" >nonempty &&\n+\t>empty &&\n+\tgit add nonempty empty &&\n+\tgit commit -m \"create files to be deleted\" &&\n+\tgit rm --cached nonempty empty &&\n+\tgit add -N nonempty empty &&\n+\tgit reset HEAD nonempty empty &&\n+\tgit diff --cached --exit-code nonempty empty\n+'\n+\n test_done\n-- \n2.22.0\n\n"},{"id":"380478","messageId":"CAK_rgsGYa6LoGDHz=sGDXY26Lvg3dxV-poB5FQHf5G6BuOKVPg@mail.gmail.com","threadId":"51472","inReplyTo":"20190801161558.12838-1-vcnaik94@gmail.com","subject":"Re: [PATCH v3] diff-lib.c: handle empty deleted ita files","fromName":"Varun Naik","fromEmail":"vcnaik94@gmail.com","sentAt":"2019-08-15T16:26:21Z","receivedAt":"2019-08-15T16:26:35Z","isPatch":true,"sender":{"key":"vcnaik94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2382501?v=4"},"body":"On Thu, Aug 1, 2019 at 9:16 AM Varun Naik wrote:\n>\n> It is possible to delete a committed file from the index and then add it\n> as intent-to-add. Certain forms of `git diff` should show the file.\n> After `git reset HEAD`, the file should be identical in the index and\n> HEAD. The commands already work correctly if the file has contents in\n> HEAD. This patch provides the desired behavior even when the file is\n> empty in HEAD.\n>\n> The affected \"diff\" commands and the \"reset\" command call\n> diff-lib.c:do_oneway_diff() with a cache entry in the index and a cache\n> entry in HEAD. An ita file is represented in the index by a cache entry\n> with the same hash as an empty file. For a nonempty deleted ita file,\n> do_oneway_diff() calls show_modified(), which detects a diff between the\n> cache entry in the index and the cache entry in HEAD and therefore deems\n> the file \"modified\". However, for an empty deleted ita file,\n> do_oneway_diff() previously detected no such diff between the two cache\n> entries and therefore deemed the file \"not modified\". After this fix,\n> for any deleted ita file, do_oneway_diff() calls diff_index_show_file()\n> and deems the file \"deleted\".\n>\n> `git diff-index --cached HEAD` prints a row of output for both a\n> \"modified\" and a \"deleted\" file, although the output differs slightly.\n> `git reset HEAD` treats a \"modified\" and a \"deleted\" file similarly,\n> resurrecting the file in the index from HEAD.\n>\n> This change should not affect newly added ita files. For those, the\n> \"tree\" cache entry is NULL, so the changed code is not executed.\n>\n> Helped-by: Junio C Hamano \n> Signed-off-by: Varun Naik \n> ---\n\nBumping this email, since I noticed v2 is still in pu.\n\nVarun\n"},{"id":"380487","messageId":"xmqqv9uy2qpj.fsf@gitster-ct.c.googlers.com","threadId":"51472","inReplyTo":"20190801161558.12838-1-vcnaik94@gmail.com","subject":"Re: [PATCH v3] diff-lib.c: handle empty deleted ita files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-15T19:06:48Z","receivedAt":"2019-08-15T19:06:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Varun Naik writes:\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 61812f48c2..29dba467d5 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -433,8 +433,11 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n> \n> \t/*\n> \t * Something removed from the tree?\n> +\t * Consider a file deleted from the index and added as ita to be \"deleted\",\n> +\t * even though it should arguably be \"modified\", because we want empty\n> +\t * deleted ita files to appear in the diff.\n> \t */\n> -\tif (!idx) {\n> +\tif (!idx || (cached && ce_intent_to_add(idx))) {\n> \t\tdiff_index_show_file(revs, \"-\", tree, &tree->oid, 1,\n> \t\t\t\t tree->ce_mode, 0);\n> \t\treturn;\n\nThere is already half of the same logic near the beginning of this\nfunction, no?\n\n\t/*\n\t * i-t-a entries do not actually exist in the index (if we're\n\t * looking at its content)\n\t */\n\tif (o->index_only &&\n\t revs->diffopt.ita_invisible_in_index &&\n\t idx && ce_intent_to_add(idx)) {\n\t\tidx = NULL;\n\t\tif (!tree)\n\t\t\treturn;\t/* nothing to diff.. */\n\t}\n\nIOW, when ita-invisible-in-index flag is set, idx is made NULL and\nall the rest of the function behaves as if there is no such entry in\nthe index (e.g. relative to HEAD it looks as if the entry is removed\nin the index).\n\nSo for example, when ita-invisible-in-index is not set, this piece,\njust above the part you touched, kicks in:\n\n\t/*\n\t * Something added to the tree?\n\t */\n\tif (!tree) {\n\t\tshow_new_file(revs, idx, cached, match_missing);\n\t\treturn;\n\t}\n\nand says \"no such entry in the tree, but you have an I-T-A entry\nthere in the index\".\n\nIt is unclear why we can unconditionally declare \"I-T-A entry does\nnot exist, the entry was in the tree but not in the index\" in the\ncode you touched, without consulting ita-invisible-in-index flag.\nIt feels awfully inconsistent to me.\n\nOf course, consistency could go the other way around, and the right\nfix to achieve consistency might turn out to be to drop the check\nfor ita-invisible-in-index flag (and perhaps the flag itself) from\nthe early part of this function. I dunno.\n\nThanks.\n"},{"id":"380711","messageId":"CAK_rgsH0Yb=CAsCgD20xx==RaDzchtEua=q6K=7R-cfURFGHuQ@mail.gmail.com","threadId":"51472","inReplyTo":"xmqqv9uy2qpj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] diff-lib.c: handle empty deleted ita files","fromName":"Varun Naik","fromEmail":"vcnaik94@gmail.com","sentAt":"2019-08-19T15:42:08Z","receivedAt":"2019-08-19T15:42:23Z","isPatch":true,"sender":{"key":"vcnaik94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2382501?v=4"},"body":"On Thu, Aug 15, 2019 at 12:06 PM Junio C Hamano wrote:\n>\n> /*\n> * i-t-a entries do not actually exist in the index (if we're\n> * looking at its content)\n> */\n> if (o->index_only &&\n> revs->diffopt.ita_invisible_in_index &&\n> idx && ce_intent_to_add(idx)) {\n> idx = NULL;\n> if (!tree)\n> return; /* nothing to diff.. */\n> }\n>\n> IOW, when ita-invisible-in-index flag is set, idx is made NULL and\n> all the rest of the function behaves as if there is no such entry in\n> the index (e.g. relative to HEAD it looks as if the entry is removed\n> in the index).\n>\n\nThat's right. I was hoping to avoid simply setting the ita-invisible-in-\nindex flag because then an ita file (i.e. a file marked as ita in the\nindex with no contents in HEAD) would be considered nonexistent in both\nthe tree and the index, and so the function would hit the return\nstatement above because it believes there is \"nothing to diff\".\n\n> So for example, when ita-invisible-in-index is not set, this piece,\n> just above the part you touched, kicks in:\n>\n> /*\n> * Something added to the tree?\n> */\n> if (!tree) {\n> show_new_file(revs, idx, cached, match_missing);\n> return;\n> }\n>\n> and says \"no such entry in the tree, but you have an I-T-A entry\n> there in the index\".\n>\n\nThat sounds right, an ita file is considered to be a \"new file\".\n\n> It is unclear why we can unconditionally declare \"I-T-A entry does\n> not exist, the entry was in the tree but not in the index\" in the\n> code you touched, without consulting ita-invisible-in-index flag.\n> It feels awfully inconsistent to me.\n>\n> Of course, consistency could go the other way around, and the right\n> fix to achieve consistency might turn out to be to drop the check\n> for ita-invisible-in-index flag (and perhaps the flag itself) from\n> the early part of this function. I dunno.\n\nActually, I agree that it seems strange to consider a deleted ita file\nas a \"deleted file\", when an ita file that did not previously exist in\nthe tree is considered a \"new file\". It felt like a dirty hack when I\nwas originally writing it. And the longer I look at the logic that I\nadded towards the end of the function, the more I worry that it\nsacrifices readability and maintainability to achieve minimal gain.\n\nDropping the check at the beginning is probably not right either - this\nwould break a lot of other places that call into do_oneway_diff().\n\nThis function is probably not the place where we want to make changes.\nIt would be better to change diff-lib.c:show_modified() and\ndiff.c:diff_change() to consider the intent-to-add bit when performing a\ndiff. A small change to show_modified() prevents the function from\nreturning prematurely when \"new_entry\" has the intent-to-add bit set.\nBut now, the call to diff_change() hits the error \"feeding unmodified %s\nto diffcore\". We can avoid this by adding a boolean \"int ita\" field to\n\"struct diff_filespec\" and adjusting the following code:\n\n@@ -5847,7 +5847,7 @@ static void diff_resolve_rename_copy(void)\n else\n p->status = DIFF_STATUS_RENAMED;\n }\n- else if (!oideq(&p->one->oid, &p->two->oid) ||\n+ else if (!oideq(&p->one->oid, &p->two->oid) || p->two->ita ||\n p->one->mode != p->two->mode ||\n p->one->dirty_submodule ||\n p->two->dirty_submodule ||\n\nNext, we need to pass something like \"int new_ita\" to diff_change() and\nset \"two->ita\" accordingly. This is where I'm stuck right now. It's easy\nenough to adjust the call to diff_change() inside show_modified(), but\nthe adjustments to other calls to diff_change(), especially the one\ninside diff-lib.c:run_diff_files(), might need some other accompanying\nchanges.\n\nDoes changing the diffcore code seem like a reasonable approach? If not,\nthen I can't think of a better one, and it might be best to leave this\npatch unmerged.\n\nVarun\n"},{"id":"380727","messageId":"xmqqwof8x66a.fsf@gitster-ct.c.googlers.com","threadId":"51472","inReplyTo":"CAK_rgsH0Yb=CAsCgD20xx==RaDzchtEua=q6K=7R-cfURFGHuQ@mail.gmail.com","subject":"Re: [PATCH v3] diff-lib.c: handle empty deleted ita files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-19T20:15:57Z","receivedAt":"2019-08-19T20:16:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Varun Naik writes:\n\n> This function is probably not the place where we want to make changes.\n> It would be better to change diff-lib.c:show_modified() and\n> diff.c:diff_change() to consider the intent-to-add bit when performing a\n> diff.\n\nI do agree that diff-lib.c::show_modified(), which is about \"git\ndiff-cache\" (and hence \"git diff \", i.e. comparison\nbetween a tree-ish and the index), is the right layer for this\nchange.\n\nI am not offhand sure about the diff.c::diff_change(), though, and\ncannot say much without first thinking about it a bit more.\n\nThanks.\n"},{"id":"391770","messageId":"CAK_rgsEV9t3okUDrG5mHX3DkyZFCX7yGoJEsSeCGTK7zBHJPgA@mail.gmail.com","threadId":"51472","inReplyTo":"xmqqwof8x66a.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] diff-lib.c: handle empty deleted ita files","fromName":"Varun Naik","fromEmail":"vcnaik94@gmail.com","sentAt":"2020-02-14T17:12:13Z","receivedAt":"2020-02-14T17:12:28Z","isPatch":true,"sender":{"key":"vcnaik94@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2382501?v=4"},"body":"On Mon, Aug 19, 2019 at 1:15 PM Junio C Hamano wrote:\n>\n> I do agree that diff-lib.c::show_modified(), which is about \"git\n> diff-cache\" (and hence \"git diff \", i.e. comparison\n> between a tree-ish and the index), is the right layer for this\n> change.\n>\n> I am not offhand sure about the diff.c::diff_change(), though, and\n> cannot say much without first thinking about it a bit more.\n\nI'm back!\n\nI made the changes to \"struct diff_filespec\" and diff_resolve_rename_copy()\nthat I mentioned in my previous email on this thread, and it seems to work.\nBut the patch seems very clunky. Even though it has been in the back of my\nmind for six months, I can't think of a more elegant approach. Furthermore,\nthe few test cases that I could create are extremely unlikely in practice. For\nall of these reasons, I would rather avoid this patch altogether.\n\nv2 is still sitting in pu. Could we discard it? I'll send an update to the\nrelated patch [0] in a few minutes.\n\n[0]: https://public-inbox.org/git/20190821032100.73917-1-vcnaik94@gmail.com/\n\nBest,\nVarun\n"}]}