{"thread":{"id":"53658","subject":"[PATCH 2/3] diff-files --raw: handle intent-to-add files correctly","startedAt":"2020-06-11T12:39:06Z","lastAt":"2020-07-01T21:20:47Z","messageCount":37,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Srinidhi Kaushik","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"399527","messageId":"6d423928512e62c91ef1ae9b469aa8cc361ed080.1591879139.git.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.git.1591879139.gitgitgadget@gmail.com","subject":"[PATCH 2/3] diff-files --raw: handle intent-to-add files correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-11T12:38:58Z","receivedAt":"2020-06-11T12:39:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn `run_diff_files()`, files that have been staged with the intention to\nadd are queued without a valid OID in the `diff_filepair`.\n\nWhen the output mode is, say, `DIFF_FORMAT_PATCH`, the\n`diff_fill_oid_info()` function, called from `run_diff()`, will remedy\nthat situation by reading the file contents from disk.\n\nHowever, when the output mode is `DIFF_FORMAT_RAW`, that does not hold\ntrue, and the output will contain a bogus OID (and the flag `M` for\n\"modified\" instead of the correct `A` for \"added\").\n\nAs a consequence, `git difftool -d` (which relies on `git diff-files\n--raw`'s output) does not work correctly.\n\nLet's fix this specifically by imitating `diff_fill_oid_info()`.\n\nNote: we can only do that for diff formats that do not actually need the\nfile contents, such as `DIFF_FORMAT_PATCH`: `run_diff()` would try to\nread the blob contents, but that blob might not have been written to\nGit's object database.\n\nThis fixes https://github.com/git-for-windows/git/issues/2677\n\nSigned-off-by: Johannes Schindelin \n---\n diff-lib.c | 14 ++++++++++++++\n t/t4000-diff-format.sh | 10 ++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 15bb45776e4..4af8f811ae8 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -223,6 +223,20 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\t\t the_hash_algo->empty_blob, 0,\n \t\t\t\t\t ce->name, 0);\n \t\t\t\tcontinue;\n+\t\t\t} else if (ce_intent_to_add(ce) &&\n+\t\t\t\t !(revs->diffopt.output_format &\n+\t\t\t\t ~(DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))) {\n+\t\t\t\tstruct object_id oid;\n+\t\t\t\tint ret = lstat(ce->name, &st);\n+\n+\t\t\t\tif (ret < 0)\n+\t\t\t\t\toidclr(&oid);\n+\t\t\t\telse\n+\t\t\t\t\tret = index_path(istate, &oid,\n+\t\t\t\t\t\t ce->name, &st, 0);\n+\t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n+\t\t\t\t\t &oid, ret >= 0, 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,\ndiff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh\nindex e5116a76a1c..48ff4e250b5 100755\n--- a/t/t4000-diff-format.sh\n+++ b/t/t4000-diff-format.sh\n@@ -89,4 +89,14 @@ test_expect_success 'git diff-files --patch --no-patch does not show the patch'\n \ttest_must_be_empty err\n '\n \n+test_expect_success 'git diff-files --raw handles intent-to-add files correctly' '\n+\techo 123 >ita &&\n+\tgit add -N ita &&\n+\tprintf \":000000 100644 %s %s A\\\\tita\\n\" \\\n+\t\t$ZERO_OID $(git hash-object --stdin expect &&\n+\tgit diff-files --raw ita >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"399528","messageId":"pull.654.git.1591879139.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":null,"subject":"[PATCH 0/3] Fix difftool problem with intent-to-add files","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-11T12:38:56Z","receivedAt":"2020-06-11T12:39:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This problem was reported in \nhttps://github.com/git-for-windows/git/issues/2677, but the problem actually\nlies with git diff --raw, and it seems that the bug has been with us ever\nsince--intent-to-add was introduced.\n\nJohannes Schindelin (3):\n diff-files: fix incorrect usage of an empty tree\n diff-files --raw: handle intent-to-add files correctly\n difftool -d: ensure that intent-to-add files are handled correctly\n\n diff-lib.c | 16 +++++++++++++++-\n t/t4000-diff-format.sh | 10 ++++++++++\n t/t7800-difftool.sh | 8 ++++++++\n 3 files changed, 33 insertions(+), 1 deletion(-)\n\n\nbase-commit: b3d7a52fac39193503a0b6728771d1bf6a161464\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-654%2Fdscho%2Fdifftool-ita-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-654/dscho/difftool-ita-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/654\n-- \ngitgitgadget\n"},{"id":"399529","messageId":"9c96c43f3d744a0f4f312bb466eaf54d372a56fb.1591879139.git.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.git.1591879139.gitgitgadget@gmail.com","subject":"[PATCH 1/3] diff-files: fix incorrect usage of an empty tree","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-11T12:38:57Z","receivedAt":"2020-06-11T12:39:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn c26022ea8f5 (diff: convert diff_addremove to struct object_id,\n2017-05-30), the OID to use for intent-to-add files was inadvertently\nchanged from the empty blob to the empty tree. Let's revert that.\n\nSigned-off-by: Johannes Schindelin \n---\n diff-lib.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 61812f48c27..15bb45776e4 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -220,7 +220,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n \t\t\t\t ce_intent_to_add(ce)) {\n \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n-\t\t\t\t\t the_hash_algo->empty_tree, 0,\n+\t\t\t\t\t the_hash_algo->empty_blob, 0,\n \t\t\t\t\t ce->name, 0);\n \t\t\t\tcontinue;\n \t\t\t}\n-- \ngitgitgadget\n\n"},{"id":"399530","messageId":"a1c889e2cd11f87f72bfd6f600db30ee59c46502.1591879139.git.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.git.1591879139.gitgitgadget@gmail.com","subject":"[PATCH 3/3] difftool -d: ensure that intent-to-add files are handled correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-11T12:38:59Z","receivedAt":"2020-06-11T12:39:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn https://github.com/git-for-windows/git/issues/2677, a `git difftool\n-d` problem was reported. The underlying cause was a bug in `git\ndiff-files --raw` that we just fixed.\n\nMake sure that the reported `difftool` problem stays fixed.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t7800-difftool.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 29b92907e2a..524f30f7dc7 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -720,6 +720,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'add -N and difftool -d' '\n+\ttest_when_finished git reset --hard &&\n+\n+\ttest_write_lines A B C >intent-to-add &&\n+\tgit add -N intent-to-add &&\n+\tgit difftool --dir-diff --extcmd ls\n+'\n+\n test_expect_success 'outside worktree' '\n \techo 1 >1 &&\n \techo 2 >2 &&\n-- \ngitgitgadget\n"},{"id":"400362","messageId":"pull.654.v2.git.1592916485.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.git.1591879139.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Fix difftool problem with intent-to-add files","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-23T12:48:02Z","receivedAt":"2020-06-23T12:48:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This problem was reported in \nhttps://github.com/git-for-windows/git/issues/2677, but the problem actually\nlies with git diff --raw, and it seems that the bug has been with us ever\nsince--intent-to-add was introduced.\n\nChanges since v1:\n\n * Rebased onto sk/diff-files-show-i-t-a-as-new.\n * Verified that sk/diff-files-show-i-t-a-as-new does not completely resolve\n the issue (the --raw output still claims the empty blob as the\n post-image, although the difftool symptom \"went away\").\n * Amended the central patch of this PR to include a fix for the regression\n test that was introduced in sk/diff-files-show-i-t-a-as-new: it expected\n the raw diff to contain the hash of the empty tree object (which is\n incorrect no matter how you turn it: any hash in any raw diff should\n refer to blob objects).\n * Reordered the patches so that the central patch comes first (otherwise,\n the \"empty tree\" fix would cause a test failure in t2203).\n\nJohannes Schindelin (3):\n diff-files --raw: handle intent-to-add files correctly\n diff-files: fix incorrect usage of an empty tree\n difftool -d: ensure that intent-to-add files are handled correctly\n\n diff-lib.c | 16 +++++++++++++++-\n t/t2203-add-intent.sh | 4 ++--\n t/t4000-diff-format.sh | 10 ++++++++++\n t/t7800-difftool.sh | 8 ++++++++\n 4 files changed, 35 insertions(+), 3 deletions(-)\n\n\nbase-commit: feea6946a5b746ff4ebf8ccdf959e303203a6011\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-654%2Fdscho%2Fdifftool-ita-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-654/dscho/difftool-ita-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/654\n\nRange-diff vs v1:\n\n 2: 6d423928512 ! 1: 640e2255508 diff-files --raw: handle intent-to-add files correctly\n @@ Commit message\n \n This fixes https://github.com/git-for-windows/git/issues/2677\n \n + This patch _also_ fixes the expectations set by the regression test\n + introduced in feea6946a5b (diff-files: treat \"i-t-a\" files as\n + \"not-in-index\", 2020-06-20).\n +\n Signed-off-by: Johannes Schindelin \n \n ## diff-lib.c ##\n @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)\n - \t\t\t\t\t the_hash_algo->empty_blob, 0,\n + \t\t\t\t\t !is_null_oid(&ce->oid),\n \t\t\t\t\t ce->name, 0);\n \t\t\t\tcontinue;\n +\t\t\t} else if (ce_intent_to_add(ce) &&\n @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)\n +\t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n +\t\t\t\t\t &oid, ret >= 0, ce->name, 0);\n +\t\t\t\tcontinue;\n - \t\t\t}\n + \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n + \t\t\t\t ce_intent_to_add(ce)) {\n + \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n +\n + ## t/t2203-add-intent.sh ##\n +@@ t/t2203-add-intent.sh: test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new\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 ++\t:000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf \"\\t\")empty\n ++\t:000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf \"\\t\")not-empty\n + \tEOF\n \n - \t\t\tchanged = match_stat_with_submodule(&revs->diffopt, ce, &st,\n + \tgit add -N empty not-empty &&\n \n ## t/t4000-diff-format.sh ##\n @@ t/t4000-diff-format.sh: test_expect_success 'git diff-files --patch --no-patch does not show the patch'\n 1: 9c96c43f3d7 ! 2: b9633315a2f diff-files: fix incorrect usage of an empty tree\n @@ Commit message\n \n In c26022ea8f5 (diff: convert diff_addremove to struct object_id,\n 2017-05-30), the OID to use for intent-to-add files was inadvertently\n - changed from the empty blob to the empty tree. Let's revert that.\n + changed from the empty blob to the empty tree.\n +\n + Let's revert that.\n +\n + To be able to do that, we just taught the regression test introduced in\n + feea6946a5b (diff-files: treat \"i-t-a\" files as \"not-in-index\",\n + 2020-06-20) to _not_ expect the raw diff to contain the hash of the\n + empty tree (we also had to fix the code to actually produce the expected\n + output, but for the sake of this here patch, that's beside the point).\n \n Signed-off-by: Johannes Schindelin \n \n 3: a1c889e2cd1 = 3: d2e9f704c9e difftool -d: ensure that intent-to-add files are handled correctly\n\n-- \ngitgitgadget\n"},{"id":"400363","messageId":"b9633315a2f09a56a46259a8540f458747250d6f.1592916485.git.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.v2.git.1592916485.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] diff-files: fix incorrect usage of an empty tree","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-23T12:48:04Z","receivedAt":"2020-06-23T12:48:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn c26022ea8f5 (diff: convert diff_addremove to struct object_id,\n2017-05-30), the OID to use for intent-to-add files was inadvertently\nchanged from the empty blob to the empty tree.\n\nLet's revert that.\n\nTo be able to do that, we just taught the regression test introduced in\nfeea6946a5b (diff-files: treat \"i-t-a\" files as \"not-in-index\",\n2020-06-20) to _not_ expect the raw diff to contain the hash of the\nempty tree (we also had to fix the code to actually produce the expected\noutput, but for the sake of this here patch, that's beside the point).\n\nSigned-off-by: Johannes Schindelin \n---\n diff-lib.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex ea23169afa2..7aafd7cc376 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -234,7 +234,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n \t\t\t\t ce_intent_to_add(ce)) {\n \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n-\t\t\t\t\t the_hash_algo->empty_tree, 0,\n+\t\t\t\t\t the_hash_algo->empty_blob, 0,\n \t\t\t\t\t ce->name, 0);\n \t\t\t\tcontinue;\n \t\t\t}\n-- \ngitgitgadget\n\n"},{"id":"400364","messageId":"d2e9f704c9e6466aa0ac826282fdf20e71235dde.1592916485.git.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.v2.git.1592916485.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] difftool -d: ensure that intent-to-add files are handled correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-23T12:48:05Z","receivedAt":"2020-06-23T12:48:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn https://github.com/git-for-windows/git/issues/2677, a `git difftool\n-d` problem was reported. The underlying cause was a bug in `git\ndiff-files --raw` that we just fixed.\n\nMake sure that the reported `difftool` problem stays fixed.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t7800-difftool.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 29b92907e2a..524f30f7dc7 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -720,6 +720,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'add -N and difftool -d' '\n+\ttest_when_finished git reset --hard &&\n+\n+\ttest_write_lines A B C >intent-to-add &&\n+\tgit add -N intent-to-add &&\n+\tgit difftool --dir-diff --extcmd ls\n+'\n+\n test_expect_success 'outside worktree' '\n \techo 1 >1 &&\n \techo 2 >2 &&\n-- \ngitgitgadget\n"},{"id":"400365","messageId":"640e225550886727594ca9dfaee6c9e6ea4b1014.1592916485.git.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.v2.git.1592916485.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-23T12:48:03Z","receivedAt":"2020-06-23T12:48:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn `run_diff_files()`, files that have been staged with the intention to\nadd are queued without a valid OID in the `diff_filepair`.\n\nWhen the output mode is, say, `DIFF_FORMAT_PATCH`, the\n`diff_fill_oid_info()` function, called from `run_diff()`, will remedy\nthat situation by reading the file contents from disk.\n\nHowever, when the output mode is `DIFF_FORMAT_RAW`, that does not hold\ntrue, and the output will contain a bogus OID (and the flag `M` for\n\"modified\" instead of the correct `A` for \"added\").\n\nAs a consequence, `git difftool -d` (which relies on `git diff-files\n--raw`'s output) does not work correctly.\n\nLet's fix this specifically by imitating `diff_fill_oid_info()`.\n\nNote: we can only do that for diff formats that do not actually need the\nfile contents, such as `DIFF_FORMAT_PATCH`: `run_diff()` would try to\nread the blob contents, but that blob might not have been written to\nGit's object database.\n\nThis fixes https://github.com/git-for-windows/git/issues/2677\n\nThis patch _also_ fixes the expectations set by the regression test\nintroduced in feea6946a5b (diff-files: treat \"i-t-a\" files as\n\"not-in-index\", 2020-06-20).\n\nSigned-off-by: Johannes Schindelin \n---\n diff-lib.c | 14 ++++++++++++++\n t/t2203-add-intent.sh | 4 ++--\n t/t4000-diff-format.sh | 10 ++++++++++\n 3 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 61812f48c27..ea23169afa2 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -217,6 +217,20 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\t\t !is_null_oid(&ce->oid),\n \t\t\t\t\t ce->name, 0);\n \t\t\t\tcontinue;\n+\t\t\t} else if (ce_intent_to_add(ce) &&\n+\t\t\t\t !(revs->diffopt.output_format &\n+\t\t\t\t ~(DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))) {\n+\t\t\t\tstruct object_id oid;\n+\t\t\t\tint ret = lstat(ce->name, &st);\n+\n+\t\t\t\tif (ret < 0)\n+\t\t\t\t\toidclr(&oid);\n+\t\t\t\telse\n+\t\t\t\t\tret = index_path(istate, &oid,\n+\t\t\t\t\t\t ce->name, &st, 0);\n+\t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n+\t\t\t\t\t &oid, ret >= 0, ce->name, 0);\n+\t\t\t\tcontinue;\n \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n \t\t\t\t ce_intent_to_add(ce)) {\n \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 8a5d55054f2..b000a2bdd1d 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -259,8 +259,8 @@ test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new\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+\t:000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf \"\\t\")empty\n+\t:000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf \"\\t\")not-empty\n \tEOF\n \n \tgit add -N empty not-empty &&\ndiff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh\nindex e5116a76a1c..48ff4e250b5 100755\n--- a/t/t4000-diff-format.sh\n+++ b/t/t4000-diff-format.sh\n@@ -89,4 +89,14 @@ test_expect_success 'git diff-files --patch --no-patch does not show the patch'\n \ttest_must_be_empty err\n '\n \n+test_expect_success 'git diff-files --raw handles intent-to-add files correctly' '\n+\techo 123 >ita &&\n+\tgit add -N ita &&\n+\tprintf \":000000 100644 %s %s A\\\\tita\\n\" \\\n+\t\t$ZERO_OID $(git hash-object --stdin expect &&\n+\tgit diff-files --raw ita >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"400473","messageId":"xmqqzh8t9vf5.fsf@gitster.c.googlers.com","threadId":"53658","inReplyTo":"640e225550886727594ca9dfaee6c9e6ea4b1014.1592916485.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-24T00:09:18Z","receivedAt":"2020-06-24T00:09:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In `run_diff_files()`, files that have been staged with the intention to\n> add are queued without a valid OID in the `diff_filepair`.\n>\n> When the output mode is, say, `DIFF_FORMAT_PATCH`, the\n> `diff_fill_oid_info()` function, called from `run_diff()`, will remedy\n> that situation by reading the file contents from disk.\n\nThe above is true. What do we do for a path that is actually added\nto the index but is stat-dirty or actually modified in the working\ntree when we are giving the raw output? Don't we give 0{40} to mean\n\"we dunno---you go look at the working tree\"? I think we should do\nthe same for i-t-a file wrt the object name. In both cases, the\nindex does not know what the actual object name is, and we do not\nwant to run the index_path() and write out a new object in the\nobject database.\n\nUsing the status letter 'A' would also be appropriate, as we would\nshow \"new file ...\" in the --patch output in this case, which would\nbe consistent.\n\n"},{"id":"400474","messageId":"xmqqv9jh9vcs.fsf@gitster.c.googlers.com","threadId":"53658","inReplyTo":"b9633315a2f09a56a46259a8540f458747250d6f.1592916485.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] diff-files: fix incorrect usage of an empty tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-24T00:10:43Z","receivedAt":"2020-06-24T00:10:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In c26022ea8f5 (diff: convert diff_addremove to struct object_id,\n> 2017-05-30), the OID to use for intent-to-add files was inadvertently\n> changed from the empty blob to the empty tree.\n\nWell spotted. Regardless of the other two patches, this is\nabsolutely the right thing to do. It is quite embarrassing\nthat nobody noticed the typo so far.\n\n"},{"id":"400489","messageId":"20200624071116.GA79097@mail.clickyotomy.dev","threadId":"53658","inReplyTo":"640e225550886727594ca9dfaee6c9e6ea4b1014.1592916485.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly","fromName":"Srinidhi Kaushik","fromEmail":"shrinidhi.kaushik@gmail.com","sentAt":"2020-06-24T07:11:16Z","receivedAt":"2020-06-24T07:11:25Z","isPatch":true,"sender":{"key":"shrinidhi.kaushik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3019537?v=4"},"body":"Hi Johannes,\n\n> The underlying problem is that some time ago, the (already incorrect)\n> empty blob constant was replaced by the empty tree constant, by mistake. I\n> contributed a patch series to fix that, and Cc:ed you you in v2 that I\n> sent out earlier today.\n\nThanks for CC-ing me!\n\n[...]\n\n> +\t\t\t} else if (ce_intent_to_add(ce) &&\n> +\t\t\t\t !(revs->diffopt.output_format &\n> +\t\t\t\t ~(DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))) {\n> +\t\t\t\tstruct object_id oid;\n> +\t\t\t\tint ret = lstat(ce->name, &st);\n> +\n> +\t\t\t\tif (ret < 0)\n> +\t\t\t\t\toidclr(&oid);\n> +\t\t\t\telse\n> +\t\t\t\t\tret = index_path(istate, &oid,\n> +\t\t\t\t\t\t ce->name, &st, 0);\n> +\t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n> +\t\t\t\t\t &oid, ret >= 0, ce->name, 0);\n> +\t\t\t\tcontinue;\n\nInstead of showing the hash for empty blobs for all entries previously,\nintroducing this shows the hash of non-empty \"i-t-a\" files correctly.\nNice.\n\n[...]\n\n> \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n> \t\t\t\t ce_intent_to_add(ce)) {\n> \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n> -\t\t\t\t\t the_hash_algo->empty_tree, 0,\n> +\t\t\t\t\t the_hash_algo->empty_blob, 0,\n> \t\t\t\t\t ce->name, 0);\n> \t\t\t\tcontinue;\n> \t\t\t}\n\nOh, I totally missed this in my patch; the change looks good!\n\n> - :000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf \"\\t\")empty\n> - :000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf \"\\t\")not-empty\n> + :000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf \"\\t\")empty\n> + :000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf \"\\t\")not-empty\n\nChanging the test-case to reflect to the hash of the blob also makes\nsense.\n\n\n[...]\n\n> > + hash_e=$(git hash-object empty) &&\n> > + hash_n=$(git hash-object not-empty) &&\n> > + hash_t=$(git hash-object -t tree /dev/null) &&\n>\n> > So this is the hash of the empty tree object, and...\n\nI guess we can get rid of the `hash_t' assignment here, because it\nwon't be used anywhere else in the test.\n\nThanks.\n"},{"id":"400501","messageId":"nycvar.QRO.7.76.6.2006241517320.54@tvgsbejvaqbjf.bet","threadId":"53658","inReplyTo":"xmqqzh8t9vf5.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-06-24T13:26:20Z","receivedAt":"2020-06-24T13:26:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 23 Jun 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > In `run_diff_files()`, files that have been staged with the intention to\n> > add are queued without a valid OID in the `diff_filepair`.\n> >\n> > When the output mode is, say, `DIFF_FORMAT_PATCH`, the\n> > `diff_fill_oid_info()` function, called from `run_diff()`, will remedy\n> > that situation by reading the file contents from disk.\n>\n> The above is true. What do we do for a path that is actually added\n> to the index but is stat-dirty or actually modified in the working\n> tree when we are giving the raw output? Don't we give 0{40} to mean\n> \"we dunno---you go look at the working tree\"? I think we should do\n> the same for i-t-a file wrt the object name. In both cases, the\n> index does not know what the actual object name is, and we do not\n> want to run the index_path() and write out a new object in the\n> object database.\n\nSure, but my intention was to synchronize the `--raw` vs the `--patch`\noutput: the latter _already_ shows the correct hash. This here patch makes\nthe hash in the former's output match the latter's.\n\nBesides, we're talking about the post-image of `diff-files`, i.e. the\nworktree version, here. I seem to remember that the pre-image already uses\nthe all-zero hash to indicate precisely what you mentioned above.\n\nCiao,\nDscho\n\n> Using the status letter 'A' would also be appropriate, as we would\n> show \"new file ...\" in the --patch output in this case, which would\n> be consistent.\n>\n>\n"},{"id":"400502","messageId":"nycvar.QRO.7.76.6.2006241528290.54@tvgsbejvaqbjf.bet","threadId":"53658","inReplyTo":"20200624071116.GA79097@mail.clickyotomy.dev","subject":"Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-06-24T13:34:19Z","receivedAt":"2020-06-24T13:34:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Srinidhi,\n\nOn Wed, 24 Jun 2020, Srinidhi Kaushik wrote:\n\n> > The underlying problem is that some time ago, the (already incorrect)\n> > empty blob constant was replaced by the empty tree constant, by mistake. I\n> > contributed a patch series to fix that, and Cc:ed you you in v2 that I\n> > sent out earlier today.\n>\n> Thanks for CC-ing me!\n\nOf course!\n\n> > - :000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf \"\\t\")empty\n> > - :000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf \"\\t\")not-empty\n> > + :000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf \"\\t\")empty\n> > + :000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf \"\\t\")not-empty\n>\n> Changing the test-case to reflect to the hash of the blob also makes\n> sense.\n\nYes, that's the post-image side. The pre-image side is marked with 0000000\n(which is Git's way to say \"dunno! no current information in the index\").\n\nAnd it is reflecting the `--patch` case that is unfortunately not visible\nin the diff context:\n\n cat >expect.diff_p <<-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\n(This comment is not so much directed at you as it really is a\ncontinuation of my reply to Junio, see\nhttps://lore.kernel.org/git/nycvar.QRO.7.76.6.2006241517320.54@tvgsbejvaqbjf.bet/)\n\n> [...]\n>\n> > > + hash_e=$(git hash-object empty) &&\n> > > + hash_n=$(git hash-object not-empty) &&\n> > > + hash_t=$(git hash-object -t tree /dev/null) &&\n> >\n> > > So this is the hash of the empty tree object, and...\n>\n> I guess we can get rid of the `hash_t' assignment here, because it\n> won't be used anywhere else in the test.\n\nRight! I _knew_ there was something I wanted to do, still, but then\nforgot all about it...\n\nThanks,\nDscho\n"},{"id":"400516","messageId":"pull.654.v3.git.1593010066.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.v2.git.1592916485.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] Fix difftool problem with intent-to-add files","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-24T14:47:43Z","receivedAt":"2020-06-24T14:47:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This problem was reported in \nhttps://github.com/git-for-windows/git/issues/2677, but the problem actually\nlies with git diff --raw, and it seems that the bug has been with us ever\nsince --intent-to-add was introduced.\n\nChanges since v2:\n\n * Now we also drop the no-longer-used definition of hash_t in t2203.\n\nChanges since v1:\n\n * Rebased onto sk/diff-files-show-i-t-a-as-new.\n * Verified that sk/diff-files-show-i-t-a-as-new does not completely resolve\n the issue (the --raw output still claims the empty blob as the\n post-image, although the difftool symptom \"went away\").\n * Amended the central patch of this PR to include a fix for the regression\n test that was introduced in sk/diff-files-show-i-t-a-as-new: it expected\n the raw diff to contain the hash of the empty tree object (which is\n incorrect no matter how you turn it: any hash in any raw diff should\n refer to blob objects).\n * Reordered the patches so that the central patch comes first (otherwise,\n the \"empty tree\" fix would cause a test failure in t2203).\n\nJohannes Schindelin (3):\n diff-files --raw: handle intent-to-add files correctly\n diff-files: fix incorrect usage of an empty tree\n difftool -d: ensure that intent-to-add files are handled correctly\n\n diff-lib.c | 16 +++++++++++++++-\n t/t2203-add-intent.sh | 5 ++---\n t/t4000-diff-format.sh | 10 ++++++++++\n t/t7800-difftool.sh | 8 ++++++++\n 4 files changed, 35 insertions(+), 4 deletions(-)\n\n\nbase-commit: feea6946a5b746ff4ebf8ccdf959e303203a6011\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-654%2Fdscho%2Fdifftool-ita-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-654/dscho/difftool-ita-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/654\n\nRange-diff vs v2:\n\n 1: 640e225550 ! 1: 8c27c78831 diff-files --raw: handle intent-to-add files correctly\n @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)\n \n ## t/t2203-add-intent.sh ##\n @@ t/t2203-add-intent.sh: test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new\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 +@@ t/t2203-add-intent.sh: test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new\n \t create mode 100644 not-empty\n \tEOF\n \tcat >expect.diff_a <<-EOF &&\n 2: b9633315a2 = 2: a9e06427ec diff-files: fix incorrect usage of an empty tree\n 3: d2e9f704c9 = 3: f51cbedd3f difftool -d: ensure that intent-to-add files are handled correctly\n\n-- \ngitgitgadget\n"},{"id":"400517","messageId":"8c27c78831be23fb560f3c012c014b11d32e75ca.1593010066.git.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.v3.git.1593010066.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] diff-files --raw: handle intent-to-add files correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-24T14:47:44Z","receivedAt":"2020-06-24T14:47:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn `run_diff_files()`, files that have been staged with the intention to\nadd are queued without a valid OID in the `diff_filepair`.\n\nWhen the output mode is, say, `DIFF_FORMAT_PATCH`, the\n`diff_fill_oid_info()` function, called from `run_diff()`, will remedy\nthat situation by reading the file contents from disk.\n\nHowever, when the output mode is `DIFF_FORMAT_RAW`, that does not hold\ntrue, and the output will contain a bogus OID (and the flag `M` for\n\"modified\" instead of the correct `A` for \"added\").\n\nAs a consequence, `git difftool -d` (which relies on `git diff-files\n--raw`'s output) does not work correctly.\n\nLet's fix this specifically by imitating `diff_fill_oid_info()`.\n\nNote: we can only do that for diff formats that do not actually need the\nfile contents, such as `DIFF_FORMAT_PATCH`: `run_diff()` would try to\nread the blob contents, but that blob might not have been written to\nGit's object database.\n\nThis fixes https://github.com/git-for-windows/git/issues/2677\n\nThis patch _also_ fixes the expectations set by the regression test\nintroduced in feea6946a5b (diff-files: treat \"i-t-a\" files as\n\"not-in-index\", 2020-06-20).\n\nSigned-off-by: Johannes Schindelin \n---\n diff-lib.c | 14 ++++++++++++++\n t/t2203-add-intent.sh | 5 ++---\n t/t4000-diff-format.sh | 10 ++++++++++\n 3 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 61812f48c2..ea23169afa 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -217,6 +217,20 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\t\t !is_null_oid(&ce->oid),\n \t\t\t\t\t ce->name, 0);\n \t\t\t\tcontinue;\n+\t\t\t} else if (ce_intent_to_add(ce) &&\n+\t\t\t\t !(revs->diffopt.output_format &\n+\t\t\t\t ~(DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))) {\n+\t\t\t\tstruct object_id oid;\n+\t\t\t\tint ret = lstat(ce->name, &st);\n+\n+\t\t\t\tif (ret < 0)\n+\t\t\t\t\toidclr(&oid);\n+\t\t\t\telse\n+\t\t\t\t\tret = index_path(istate, &oid,\n+\t\t\t\t\t\t ce->name, &st, 0);\n+\t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n+\t\t\t\t\t &oid, ret >= 0, ce->name, 0);\n+\t\t\t\tcontinue;\n \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n \t\t\t\t ce_intent_to_add(ce)) {\n \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 8a5d55054f..7d379bdbc6 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -240,7 +240,6 @@ test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new\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@@ -259,8 +258,8 @@ test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new\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+\t:000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf \"\\t\")empty\n+\t:000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf \"\\t\")not-empty\n \tEOF\n \n \tgit add -N empty not-empty &&\ndiff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh\nindex e5116a76a1..48ff4e250b 100755\n--- a/t/t4000-diff-format.sh\n+++ b/t/t4000-diff-format.sh\n@@ -89,4 +89,14 @@ test_expect_success 'git diff-files --patch --no-patch does not show the patch'\n \ttest_must_be_empty err\n '\n \n+test_expect_success 'git diff-files --raw handles intent-to-add files correctly' '\n+\techo 123 >ita &&\n+\tgit add -N ita &&\n+\tprintf \":000000 100644 %s %s A\\\\tita\\n\" \\\n+\t\t$ZERO_OID $(git hash-object --stdin expect &&\n+\tgit diff-files --raw ita >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"400518","messageId":"f51cbedd3f3517cc7d097501d2188709b11fc013.1593010066.git.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.v3.git.1593010066.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] difftool -d: ensure that intent-to-add files are handled correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-24T14:47:46Z","receivedAt":"2020-06-24T14:47:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn https://github.com/git-for-windows/git/issues/2677, a `git difftool\n-d` problem was reported. The underlying cause was a bug in `git\ndiff-files --raw` that we just fixed.\n\nMake sure that the reported `difftool` problem stays fixed.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t7800-difftool.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 29b92907e2..524f30f7dc 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -720,6 +720,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'add -N and difftool -d' '\n+\ttest_when_finished git reset --hard &&\n+\n+\ttest_write_lines A B C >intent-to-add &&\n+\tgit add -N intent-to-add &&\n+\tgit difftool --dir-diff --extcmd ls\n+'\n+\n test_expect_success 'outside worktree' '\n \techo 1 >1 &&\n \techo 2 >2 &&\n-- \ngitgitgadget\n"},{"id":"400519","messageId":"a9e06427ec4661b105f57ccf4ce5e842ba501a99.1593010066.git.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.v3.git.1593010066.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] diff-files: fix incorrect usage of an empty tree","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-24T14:47:45Z","receivedAt":"2020-06-24T14:47:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn c26022ea8f5 (diff: convert diff_addremove to struct object_id,\n2017-05-30), the OID to use for intent-to-add files was inadvertently\nchanged from the empty blob to the empty tree.\n\nLet's revert that.\n\nTo be able to do that, we just taught the regression test introduced in\nfeea6946a5b (diff-files: treat \"i-t-a\" files as \"not-in-index\",\n2020-06-20) to _not_ expect the raw diff to contain the hash of the\nempty tree (we also had to fix the code to actually produce the expected\noutput, but for the sake of this here patch, that's beside the point).\n\nSigned-off-by: Johannes Schindelin \n---\n diff-lib.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex ea23169afa..7aafd7cc37 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -234,7 +234,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n \t\t\t\t ce_intent_to_add(ce)) {\n \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n-\t\t\t\t\t the_hash_algo->empty_tree, 0,\n+\t\t\t\t\t the_hash_algo->empty_blob, 0,\n \t\t\t\t\t ce->name, 0);\n \t\t\t\tcontinue;\n \t\t\t}\n-- \ngitgitgadget\n\n"},{"id":"400526","messageId":"xmqqy2oc8oye.fsf@gitster.c.googlers.com","threadId":"53658","inReplyTo":"nycvar.QRO.7.76.6.2006241517320.54@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-24T15:26:33Z","receivedAt":"2020-06-24T15:26:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Sure, but my intention was to synchronize the `--raw` vs the `--patch`\n> output: the latter _already_ shows the correct hash. This here patch makes\n> the hash in the former's output match the latter's.\n\nThat is shooting for a wrong uniformity and breaking consistency\namong the `--raw` modes.\n\n $ git reset --hard\n $ echo \"/* end */\" >cache.h ;# taint\n $ git diff-files --raw\n ... this shows (M)odified with 0{40} on the postimage\n ... 0{40} for side that is known to have contents from low-level diff\n ... means \"object name unknown; figure it out yourself if you need it\"\n $ git update-index cache.h\n $ git diff-files --raw\n ... of course we see nothing here. Wait for a bit.\n $ touch cache.h ;# smudge\n $ git diff-files --raw\n ... this shows (M)odified with 0{40} on the postimage\n ... again, it says \"it is stat dirty so I do not bother to compute\"\n $ git update-index --refresh\n $ git diff-files --raw\n ... again we see nothing.\n\nAny tools that work on \"--raw\" output must be already prepared to\nsee 0{40} on the side that is known to have contents and must know\nto grab the contents from the working tree file if they need them,\nso showing the 0{40} for i-t-a entry (whose definition is \"the user\nsaid in the past that the final contents of the file will be added\nlater, but Git does not know what object it will be yet\") cannot\nbreak them. And the behaviour of giving 0{40} in such a case aligns\nwell with what is already done for paths already added to the index\nwhen Git does not have an already-computed object name handy.\n\n> Besides, we're talking about the post-image of `diff-files`, i.e. the\n> worktree version, here. I seem to remember that the pre-image already uses\n> the all-zero hash to indicate precisely what you mentioned above.\n\nThe 0{40} you see for pre-image for (A)dded paths means a completely\ndifferent thing from the 0{40} I have been explaining in the above,\nso that is not relevant here.\n\nBy definition, there is *no* contents for the pre-image side of\n(A)dded paths (that is why I stressed the \"side that must have\ncontents\" in the above description---it is determined by the type of\nthe change), but because the format requires us to place some\nhexadecimal there, we fill the space with 0{40}. \n\nWhen we do not know the object name for the side that is known to\nhave contents without performing extra computation (including \"stat\ndirty so we cannot tell without rehashing\"), we also use 0{40} as a\nsignal to say \"we do not know the actual contents\", but the consumer\nof \"--raw\" format is expected to know the difference between \"this\nside is known to have no data and 0{40} is here as filler\" and \"this\nside must have contents but we see 0{40} because Git does not have\nit handy in precomputed form\".\n\nThe above is the same for \"diff-index --raw\" without \"--cached\";\nwhen we have to hash before we can give the object name (e.g. the\npath is stat-dirty), we give 0{40} and let the consumer figure it\nout if it needs to.\n\n $ git reset --hard\n $ touch COPYING\n $ git diff-index --raw HEAD\n ... we see (M)odified with 0{40} on the right hand side.\n\nWhen the caller asks for \"--patch\" or any other output format that\nactually needs contents for output, however, these low-level tools\ndo read the contents, and as a side effect, they may hash to obtain\nthe object name and show it [*1*].\n\nBy the way, as I do not want to see you waste your time going in a\nwrong direction just to be \"different\", let me make it clear that as\nfar as the design of low level diff plumbing is concerned, what I\nsaid here is final. Please don't waste your time on arguing for\nchanging the design now after 15 years. I want to see your time\nused in a more productive way for the project.\n\nThanks.\n\n\n[Footnote]\n\n*1* This division of labor to free \"--raw\" mode of anything remotely\n unnecessary stems from the original diff plumbing design in May\n 2005 where the \"--raw\" mode was the only output mode, and there\n was a separate \"git-diff-helper\" (look for it in the mailing\n list archive if you want to learn more) that reads a \"--raw\"\n output and transforms it into the patch form. That \"once we\n have the raw diff, we can pipe it to post-processing and do more\n interesting things\" eventually led to the design of the diffcore\n pipeline where we match up (A)dded and (D)eleted entries to\n detect renames, etc.\n\n"},{"id":"400533","messageId":"xmqqh7v08nt9.fsf@gitster.c.googlers.com","threadId":"53658","inReplyTo":"nycvar.QRO.7.76.6.2006241528290.54@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-24T15:51:14Z","receivedAt":"2020-06-24T15:51:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > + :000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf \"\\t\")empty\n>> > + :000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf \"\\t\")not-empty\n>>\n>> Changing the test-case to reflect to the hash of the blob also makes\n>> sense.\n>\n> Yes, that's the post-image side. The pre-image side is marked with 0000000\n> (which is Git's way to say \"dunno! no current information in the index\").\n\nThe post-image side for (A)dded and (M)odified can have 0{40} when\nGit wants to say \"we don't have it in precomputed form. you can\ngrab the working tree files and figure it out, if you need it\", as\nyou said, but the pre-image side for (A)dded entries are \"we know\nthere is nothing because we are Adding, but we need to have\nsomething here, so we have 0+ as a filler\".\n\n"},{"id":"400551","messageId":"xmqq4kr071ch.fsf@gitster.c.googlers.com","threadId":"53658","inReplyTo":"xmqqy2oc8oye.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-24T18:41:50Z","receivedAt":"2020-06-24T18:41:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> Sure, but my intention was to synchronize the `--raw` vs the `--patch`\n>> output: the latter _already_ shows the correct hash. This here patch makes\n>> the hash in the former's output match the latter's.\n>\n> That is shooting for a wrong uniformity and breaking consistency\n> among the `--raw` modes.\n> ...\n>\n> [Footnote]\n>\n> *1* This division of labor to free \"--raw\" mode of anything remotely\n> unnecessary stems from the original diff plumbing design in May\n> 2005 where the \"--raw\" mode was the only output mode, and there\n> was a separate \"git-diff-helper\" (look for it in the mailing\n> list archive if you want to learn more) that reads a \"--raw\"\n> output and transforms it into the patch form. That \"once we\n> have the raw diff, we can pipe it to post-processing and do more\n> interesting things\" eventually led to the design of the diffcore\n> pipeline where we match up (A)dded and (D)eleted entries to\n> detect renames, etc.\n\nHaving said all that.\n\nIf somebody wants to shift the burden of computing object names from\nthe consumers of \"diff --raw\" output to generators like diff-files\nand diff-index, I do not mind if it is done under a new command line\noption and done consistently for not just I-T-A additions, but\n\"modified but not added yet\" and \"not modified but stat information\nis dirty\" paths. As I said, I would not recommend it, though.\n\n"},{"id":"400618","messageId":"nycvar.QRO.7.76.6.2006251618070.54@tvgsbejvaqbjf.bet","threadId":"53658","inReplyTo":"xmqqy2oc8oye.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/3] diff-files --raw: handle intent-to-add files correctly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-06-25T17:52:41Z","receivedAt":"2020-06-25T17:52:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 24 Jun 2020, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > Sure, but my intention was to synchronize the `--raw` vs the `--patch`\n> > output: the latter _already_ shows the correct hash. This here patch makes\n> > the hash in the former's output match the latter's.\n>\n> That is shooting for a wrong uniformity and breaking consistency\n> among the `--raw` modes.\n>\n> $ git reset --hard\n> $ echo \"/* end */\" >cache.h ;# taint\n> $ git diff-files --raw\n> ... this shows (M)odified with 0{40} on the postimage\n> ... 0{40} for side that is known to have contents from low-level diff\n> ... means \"object name unknown; figure it out yourself if you need it\"\n> $ git update-index cache.h\n> $ git diff-files --raw\n> ... of course we see nothing here. Wait for a bit.\n> $ touch cache.h ;# smudge\n> $ git diff-files --raw\n> ... this shows (M)odified with 0{40} on the postimage\n> ... again, it says \"it is stat dirty so I do not bother to compute\"\n> $ git update-index --refresh\n> $ git diff-files --raw\n> ... again we see nothing.\n>\n> Any tools that work on \"--raw\" output must be already prepared to\n> see 0{40} on the side that is known to have contents and must know\n> to grab the contents from the working tree file if they need them,\n> so showing the 0{40} for i-t-a entry (whose definition is \"the user\n> said in the past that the final contents of the file will be added\n> later, but Git does not know what object it will be yet\") cannot\n> break them. And the behaviour of giving 0{40} in such a case aligns\n> well with what is already done for paths already added to the index\n> when Git does not have an already-computed object name handy.\n\nWell, don't you know, I never realized that the hash shown by `git\ndiff-files --raw` for modified files was all-zero while `git diff-files\n-p` showed the computed one matching the current worktree version!\n\n> > Besides, we're talking about the post-image of `diff-files`, i.e. the\n> > worktree version, here. I seem to remember that the pre-image already uses\n> > the all-zero hash to indicate precisely what you mentioned above.\n>\n> The 0{40} you see for pre-image for (A)dded paths means a completely\n> different thing from the 0{40} I have been explaining in the above,\n> so that is not relevant here.\n>\n> By definition, there is *no* contents for the pre-image side of\n> (A)dded paths (that is why I stressed the \"side that must have\n> contents\" in the above description---it is determined by the type of\n> the change), but because the format requires us to place some\n> hexadecimal there, we fill the space with 0{40}.\n>\n> When we do not know the object name for the side that is known to\n> have contents without performing extra computation (including \"stat\n> dirty so we cannot tell without rehashing\"), we also use 0{40} as a\n> signal to say \"we do not know the actual contents\", but the consumer\n> of \"--raw\" format is expected to know the difference between \"this\n> side is known to have no data and 0{40} is here as filler\" and \"this\n> side must have contents but we see 0{40} because Git does not have\n> it handy in precomputed form\".\n>\n> The above is the same for \"diff-index --raw\" without \"--cached\";\n> when we have to hash before we can give the object name (e.g. the\n> path is stat-dirty), we give 0{40} and let the consumer figure it\n> out if it needs to.\n>\n> $ git reset --hard\n> $ touch COPYING\n> $ git diff-index --raw HEAD\n> ... we see (M)odified with 0{40} on the right hand side.\n>\n> When the caller asks for \"--patch\" or any other output format that\n> actually needs contents for output, however, these low-level tools\n> do read the contents, and as a side effect, they may hash to obtain\n> the object name and show it [*1*].\n>\n> By the way, as I do not want to see you waste your time going in a\n> wrong direction just to be \"different\", let me make it clear that as\n> far as the design of low level diff plumbing is concerned, what I\n> said here is final. Please don't waste your time on arguing for\n> changing the design now after 15 years. I want to see your time\n> used in a more productive way for the project.\n\nThank you for patienty explaining to me something I managed to miss for a\ndecade and a half.\n\nI'll send out v4 in a moment.\n\nCiao,\nDscho\n\n>\n> Thanks.\n>\n>\n> [Footnote]\n>\n> *1* This division of labor to free \"--raw\" mode of anything remotely\n> unnecessary stems from the original diff plumbing design in May\n> 2005 where the \"--raw\" mode was the only output mode, and there\n> was a separate \"git-diff-helper\" (look for it in the mailing\n> list archive if you want to learn more) that reads a \"--raw\"\n> output and transforms it into the patch form. That \"once we\n> have the raw diff, we can pipe it to post-processing and do more\n> interesting things\" eventually led to the design of the diffcore\n> pipeline where we match up (A)dded and (D)eleted entries to\n> detect renames, etc.\n>\n>\n"},{"id":"400619","messageId":"69256ab9107c3dba0dc007b69cc0ce98a9b91f9a.1593107621.git.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.v4.git.1593107621.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-25T17:53:40Z","receivedAt":"2020-06-25T17:53:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe documented behavior of `git diff-files --raw` is to display\n\n\t[...] 0{40} if creation, unmerged or \"look at work tree\".\n\nThis happens for example when showing modified, unstaged files.\n\nFor intent-to-add files, we used to show the empty blob's hash instead.\nIn c26022ea8f5 (diff: convert diff_addremove to struct object_id,\n2017-05-30), we made that worse by inadvertently changing that to the\nhash of the empty tree.\n\nLet's make the behavior consistent with modified files by showing\nall-zero values also for intent-to-add files.\n\nAccordingly, this patch adjusts the expectations set by the regression\ntest introduced in feea6946a5b (diff-files: treat \"i-t-a\" files as\n\"not-in-index\", 2020-06-20).\n\nSigned-off-by: Johannes Schindelin \n---\n diff-lib.c | 3 +--\n t/t2203-add-intent.sh | 5 ++---\n 2 files changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 61812f48c2..25fd2dee19 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -220,8 +220,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n \t\t\t\t ce_intent_to_add(ce)) {\n \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n-\t\t\t\t\t the_hash_algo->empty_tree, 0,\n-\t\t\t\t\t ce->name, 0);\n+\t\t\t\t\t &null_oid, 0, ce->name, 0);\n \t\t\t\tcontinue;\n \t\t\t}\n \ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 8a5d55054f..cf0175ad6e 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -240,7 +240,6 @@ test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new\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@@ -259,8 +258,8 @@ test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new\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+\t:000000 100644 0000000 0000000 A$(printf \"\\t\")empty\n+\t:000000 100644 0000000 0000000 A$(printf \"\\t\")not-empty\n \tEOF\n \n \tgit add -N empty not-empty &&\n-- \ngitgitgadget\n\n"},{"id":"400620","messageId":"9bb8d84ea956dcddefbe7b62baa3a5ff23b6b1e2.1593107621.git.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.v4.git.1593107621.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] difftool -d: ensure that intent-to-add files are handled correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-25T17:53:41Z","receivedAt":"2020-06-25T17:53:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn https://github.com/git-for-windows/git/issues/2677, a `git difftool\n-d` problem was reported. The underlying cause was a bug in `git\ndiff-files --raw` that we just fixed.\n\nMake sure that the reported `difftool` problem stays fixed.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t7800-difftool.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 29b92907e2..524f30f7dc 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -720,6 +720,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'add -N and difftool -d' '\n+\ttest_when_finished git reset --hard &&\n+\n+\ttest_write_lines A B C >intent-to-add &&\n+\tgit add -N intent-to-add &&\n+\tgit difftool --dir-diff --extcmd ls\n+'\n+\n test_expect_success 'outside worktree' '\n \techo 1 >1 &&\n \techo 2 >2 &&\n-- \ngitgitgadget\n"},{"id":"400621","messageId":"pull.654.v4.git.1593107621.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.v3.git.1593010066.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] Fix difftool problem with intent-to-add files","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-25T17:53:39Z","receivedAt":"2020-06-25T17:53:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This problem was reported in \nhttps://github.com/git-for-windows/git/issues/2677, but the problem actually\nlies with git diff --raw, and it seems that the bug has been with us ever\nsince --intent-to-add was introduced.\n\nChanges since v3:\n\n * Instead of showing the same OID in git diff-files --raw as in git\n diff-files -p for intent-to-add files, we now imitate the logic for\n modified (non-intent-to-add) files: we show the \"null\" OID (i.e.\n all-zero).\n \n \n * As a consequence, we no longer just undo the inadvertent change where\n intent-to-add files were reported with the empty tree instead of the\n empty blob, but we fix the bug that has been with us since \n run_diff_files() was taught about intent-to-add files, i.e. we fix the\n original bug of 425a28e0a4e (diff-lib: allow ita entries treated as \"not\n yet exist in index\", 2016-10-24), at long last.\n \n \n\nChanges since v2:\n\n * Now we also drop the no-longer-used definition of hash_t in t2203.\n\nChanges since v1:\n\n * Rebased onto sk/diff-files-show-i-t-a-as-new.\n * Verified that sk/diff-files-show-i-t-a-as-new does not completely resolve\n the issue (the --raw output still claims the empty blob as the\n post-image, although the difftool symptom \"went away\").\n * Amended the central patch of this PR to include a fix for the regression\n test that was introduced in sk/diff-files-show-i-t-a-as-new: it expected\n the raw diff to contain the hash of the empty tree object (which is\n incorrect no matter how you turn it: any hash in any raw diff should\n refer to blob objects).\n * Reordered the patches so that the central patch comes first (otherwise,\n the \"empty tree\" fix would cause a test failure in t2203).\n\nJohannes Schindelin (2):\n diff-files --raw: show correct post-image of intent-to-add files\n difftool -d: ensure that intent-to-add files are handled correctly\n\n diff-lib.c | 3 +--\n t/t2203-add-intent.sh | 5 ++---\n t/t7800-difftool.sh | 8 ++++++++\n 3 files changed, 11 insertions(+), 5 deletions(-)\n\n\nbase-commit: feea6946a5b746ff4ebf8ccdf959e303203a6011\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-654%2Fdscho%2Fdifftool-ita-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-654/dscho/difftool-ita-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/654\n\nRange-diff vs v3:\n\n 1: 8c27c78831 ! 1: 69256ab910 diff-files --raw: handle intent-to-add files correctly\n @@ Metadata\n Author: Johannes Schindelin \n \n ## Commit message ##\n - diff-files --raw: handle intent-to-add files correctly\n + diff-files --raw: show correct post-image of intent-to-add files\n \n - In `run_diff_files()`, files that have been staged with the intention to\n - add are queued without a valid OID in the `diff_filepair`.\n + The documented behavior of `git diff-files --raw` is to display\n \n - When the output mode is, say, `DIFF_FORMAT_PATCH`, the\n - `diff_fill_oid_info()` function, called from `run_diff()`, will remedy\n - that situation by reading the file contents from disk.\n + [...] 0{40} if creation, unmerged or \"look at work tree\".\n \n - However, when the output mode is `DIFF_FORMAT_RAW`, that does not hold\n - true, and the output will contain a bogus OID (and the flag `M` for\n - \"modified\" instead of the correct `A` for \"added\").\n + This happens for example when showing modified, unstaged files.\n \n - As a consequence, `git difftool -d` (which relies on `git diff-files\n - --raw`'s output) does not work correctly.\n + For intent-to-add files, we used to show the empty blob's hash instead.\n + In c26022ea8f5 (diff: convert diff_addremove to struct object_id,\n + 2017-05-30), we made that worse by inadvertently changing that to the\n + hash of the empty tree.\n \n - Let's fix this specifically by imitating `diff_fill_oid_info()`.\n + Let's make the behavior consistent with modified files by showing\n + all-zero values also for intent-to-add files.\n \n - Note: we can only do that for diff formats that do not actually need the\n - file contents, such as `DIFF_FORMAT_PATCH`: `run_diff()` would try to\n - read the blob contents, but that blob might not have been written to\n - Git's object database.\n -\n - This fixes https://github.com/git-for-windows/git/issues/2677\n -\n - This patch _also_ fixes the expectations set by the regression test\n - introduced in feea6946a5b (diff-files: treat \"i-t-a\" files as\n + Accordingly, this patch adjusts the expectations set by the regression\n + test introduced in feea6946a5b (diff-files: treat \"i-t-a\" files as\n \"not-in-index\", 2020-06-20).\n \n Signed-off-by: Johannes Schindelin \n \n ## diff-lib.c ##\n @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)\n - \t\t\t\t\t !is_null_oid(&ce->oid),\n - \t\t\t\t\t ce->name, 0);\n - \t\t\t\tcontinue;\n -+\t\t\t} else if (ce_intent_to_add(ce) &&\n -+\t\t\t\t !(revs->diffopt.output_format &\n -+\t\t\t\t ~(DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))) {\n -+\t\t\t\tstruct object_id oid;\n -+\t\t\t\tint ret = lstat(ce->name, &st);\n -+\n -+\t\t\t\tif (ret < 0)\n -+\t\t\t\t\toidclr(&oid);\n -+\t\t\t\telse\n -+\t\t\t\t\tret = index_path(istate, &oid,\n -+\t\t\t\t\t\t ce->name, &st, 0);\n -+\t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n -+\t\t\t\t\t &oid, ret >= 0, ce->name, 0);\n -+\t\t\t\tcontinue;\n \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n \t\t\t\t ce_intent_to_add(ce)) {\n \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n +-\t\t\t\t\t the_hash_algo->empty_tree, 0,\n +-\t\t\t\t\t ce->name, 0);\n ++\t\t\t\t\t &null_oid, 0, ce->name, 0);\n + \t\t\t\tcontinue;\n + \t\t\t}\n + \n \n ## t/t2203-add-intent.sh ##\n @@ t/t2203-add-intent.sh: test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new\n @@ t/t2203-add-intent.sh: test_expect_success 'i-t-a files shown as new for \"diff\",\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 -+\t:000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf \"\\t\")empty\n -+\t:000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf \"\\t\")not-empty\n ++\t:000000 100644 0000000 0000000 A$(printf \"\\t\")empty\n ++\t:000000 100644 0000000 0000000 A$(printf \"\\t\")not-empty\n \tEOF\n \n \tgit add -N empty not-empty &&\n -\n - ## t/t4000-diff-format.sh ##\n -@@ t/t4000-diff-format.sh: test_expect_success 'git diff-files --patch --no-patch does not show the patch'\n - \ttest_must_be_empty err\n - '\n - \n -+test_expect_success 'git diff-files --raw handles intent-to-add files correctly' '\n -+\techo 123 >ita &&\n -+\tgit add -N ita &&\n -+\tprintf \":000000 100644 %s %s A\\\\tita\\n\" \\\n -+\t\t$ZERO_OID $(git hash-object --stdin expect &&\n -+\tgit diff-files --raw ita >actual &&\n -+\ttest_cmp expect actual\n -+'\n -+\n -+\n - test_done\n 2: a9e06427ec < -: ---------- diff-files: fix incorrect usage of an empty tree\n 3: f51cbedd3f = 2: 9bb8d84ea9 difftool -d: ensure that intent-to-add files are handled correctly\n\n-- \ngitgitgadget\n"},{"id":"400623","messageId":"xmqqmu4r3to1.fsf@gitster.c.googlers.com","threadId":"53658","inReplyTo":"69256ab9107c3dba0dc007b69cc0ce98a9b91f9a.1593107621.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-25T18:08:14Z","receivedAt":"2020-06-25T18:09:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 61812f48c2..25fd2dee19 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -220,8 +220,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n> \t\t\t\t ce_intent_to_add(ce)) {\n> \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n> -\t\t\t\t\t the_hash_algo->empty_tree, 0,\n> -\t\t\t\t\t ce->name, 0);\n> +\t\t\t\t\t &null_oid, 0, ce->name, 0);\n\nThis (even if the preimage were correctly using empty_blob) is *so*\nsimple a change that it is very surprising that the new test in\n[2/2] passes without any other code change.\n\nIt means that difftool was written correctly in the first place,\nright?\n\nNicely done.\n"},{"id":"400624","messageId":"xmqqimff3tib.fsf@gitster.c.googlers.com","threadId":"53658","inReplyTo":"9bb8d84ea956dcddefbe7b62baa3a5ff23b6b1e2.1593107621.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/2] difftool -d: ensure that intent-to-add files are handled correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-25T18:11:40Z","receivedAt":"2020-06-25T18:11:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In https://github.com/git-for-windows/git/issues/2677, a `git difftool\n> -d` problem was reported. The underlying cause was a bug in `git\n> diff-files --raw` that we just fixed.\n\nThat leaves a gap between \"there is some unspecified problem\" and\n\"the problem was cased by such and such\" that forces readers to\neither know the problem at heart (may apply to you and me right now,\nbut I am not sure about me 3 months in the future) or go check the\nexternal website.\n\nCan we fill the gap by saying how seeing the object name of empty\nblob (or worse, tree) instead of 0{40} made \"difftool -d\" upset?\n\nThanks.\n\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> t/t7800-difftool.sh | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n>\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 29b92907e2..524f30f7dc 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -720,6 +720,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'add -N and difftool -d' '\n> +\ttest_when_finished git reset --hard &&\n> +\n> +\ttest_write_lines A B C >intent-to-add &&\n> +\tgit add -N intent-to-add &&\n> +\tgit difftool --dir-diff --extcmd ls\n> +'\n> +\n> test_expect_success 'outside worktree' '\n> \techo 1 >1 &&\n> \techo 2 >2 &&\n"},{"id":"400626","messageId":"xmqqeeq33t2q.fsf@gitster.c.googlers.com","threadId":"53658","inReplyTo":"69256ab9107c3dba0dc007b69cc0ce98a9b91f9a.1593107621.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-25T18:21:01Z","receivedAt":"2020-06-25T18:21:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> The documented behavior of `git diff-files --raw` is to display\n>\n> \t[...] 0{40} if creation, unmerged or \"look at work tree\".\n\n\"on the right hand (i.e. postimage) side\" is missing, which is\ncrucial in this description.\n\n> This happens for example when showing modified, unstaged files.\n\nModified I would understand. We notice that the contents on the\nwork tree is different from what is in the index, and we tell the\nconsumer \"look at work tree\". I do not think you meant \"unstaged\",\nthough. If truly removed from the index, diff-files won't even know\nabout the path. You probably meant \"removed from the working tree\",\nbut 0{40} in that case means totally different thing (i.e. it would\nbe a (D)eletion record, so 0{40} on the postimage side is a filler,\nnot even \"look at work tree\"). What would make more sense to\ndescribe might be\n\n\tThis happens for paths that are modified, or unmodified but\n\tstat-dirty.\n\nEither case, we tell the consumer to check the \"work tree\".\n\n> For intent-to-add files, we used to show the empty blob's hash instead.\n> In c26022ea8f5 (diff: convert diff_addremove to struct object_id,\n> 2017-05-30), we made that worse by inadvertently changing that to the\n> hash of the empty tree.\n>\n> Let's make the behavior consistent with modified files by showing\n> all-zero values also for intent-to-add files.\n\nWell described.\n\nThanks.\n"},{"id":"400710","messageId":"20200626174921.GA50735@mail.clickyotomy.dev","threadId":"53658","inReplyTo":"69256ab9107c3dba0dc007b69cc0ce98a9b91f9a.1593107621.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files","fromName":"Srinidhi Kaushik","fromEmail":"shrinidhi.kaushik@gmail.com","sentAt":"2020-06-26T17:49:21Z","receivedAt":"2020-06-26T17:49:31Z","isPatch":true,"sender":{"key":"shrinidhi.kaushik@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3019537?v=4"},"body":"Sorry for the late reply.\n\n> Let's make the behavior consistent with modified files by showing\n> all-zero values also for intent-to-add files.\n>\n-- >8 --\n>\n> diff_addremove(&revs->diffopt, '+', ce->ce_mode,\n> - the_hash_algo->empty_tree, 0,\n> - ce->name, 0);\n> + &null_oid, 0, ce->name, 0);\n> continue;\n> }\n>\n> diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\n> index 8a5d55054f..cf0175ad6e 100755\n> --- a/t/t2203-add-intent.sh\n> +++ b/t/t2203-add-intent.sh\n> @@ -240,7 +240,6 @@ test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new\n>\n> hash_e=$(git hash-object empty) &&\n> hash_n=$(git hash-object not-empty) &&\n> - hash_t=$(git hash-object -t tree /dev/null) &&\n>\n> cat >expect.diff_p <<-EOF &&\n> diff --git a/empty b/empty\n> @@ -259,8 +258,8 @@ test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new\n> create mode 100644 not-empty\n> EOF\n> cat >expect.diff_a <<-EOF &&\n> - :000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf \"\\t\")empty\n> - :000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf \"\\t\")not-empty\n> + :000000 100644 0000000 0000000 A$(printf \"\\t\")empty\n> + :000000 100644 0000000 0000000 A$(printf \"\\t\")not-empty\n\nThis change (and the new test in [PATCH v4 2/2]) looks good to me.\n\nI learnt quite a bit about what the string \"0{40}\" means in the context\nof `diff' post-image from the conversations around this patch. It was\nvery helpful.\n\nThank you.\n"},{"id":"400876","messageId":"nycvar.QRO.7.76.6.2007011144410.56@tvgsbejvaqbjf.bet","threadId":"53658","inReplyTo":"xmqqmu4r3to1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-01T09:46:42Z","receivedAt":"2020-07-01T09:46:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\n\nOn Thu, 25 Jun 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > diff --git a/diff-lib.c b/diff-lib.c\n> > index 61812f48c2..25fd2dee19 100644\n> > --- a/diff-lib.c\n> > +++ b/diff-lib.c\n> > @@ -220,8 +220,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> > \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n> > \t\t\t\t ce_intent_to_add(ce)) {\n> > \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n> > -\t\t\t\t\t the_hash_algo->empty_tree, 0,\n> > -\t\t\t\t\t ce->name, 0);\n> > +\t\t\t\t\t &null_oid, 0, ce->name, 0);\n>\n> This (even if the preimage were correctly using empty_blob) is *so*\n> simple a change that it is very surprising that the new test in\n> [2/2] passes without any other code change.\n>\n> It means that difftool was written correctly in the first place,\n> right?\n\nWell, it means that difftool does not even consume the OID. Sure, it\nparses it, but then it ignores it. All it _really_ is interested in is\nthat status flag (`A`), so technically, my fix in 1/2 is not even needed\nafter `sk/diff-files-show-i-t-a-as-new` to let 2/2 pass.\n\nCiao,\nDscho\n\n>\n> Nicely done.\n>\n"},{"id":"400878","messageId":"nycvar.QRO.7.76.6.2007011149000.56@tvgsbejvaqbjf.bet","threadId":"53658","inReplyTo":"xmqqeeq33t2q.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-01T09:52:12Z","receivedAt":"2020-07-01T09:52:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 25 Jun 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > The documented behavior of `git diff-files --raw` is to display\n> >\n> > \t[...] 0{40} if creation, unmerged or \"look at work tree\".\n>\n> \"on the right hand (i.e. postimage) side\" is missing, which is\n> crucial in this description.\n\nTrue. Sorry for omitting that.\n\n> > This happens for example when showing modified, unstaged files.\n>\n> Modified I would understand. We notice that the contents on the\n> work tree is different from what is in the index, and we tell the\n> consumer \"look at work tree\". I do not think you meant \"unstaged\",\n\nRight. I probably should have written \"[...] when showing files with\nunstaged modifications\".\n\n> though. If truly removed from the index, diff-files won't even know\n> about the path. You probably meant \"removed from the working tree\",\n> but 0{40} in that case means totally different thing (i.e. it would\n> be a (D)eletion record, so 0{40} on the postimage side is a filler,\n> not even \"look at work tree\"). What would make more sense to\n> describe might be\n>\n> \tThis happens for paths that are modified, or unmodified but\n> \tstat-dirty.\n\nYes, but that includes more than I wanted to describe because the\nstat-dirty does not matter for intent-to-add files, and I wanted to point\nout the analogy between unstaged modifications and intent-to-add-files.\n\nI noticed that you merged this commit into `next` already, so I am\nassuming that you do not want me to send a fifth iteration ;-)\n\nCiao,\nDscho\n\n> Either case, we tell the consumer to check the \"work tree\".\n>\n> > For intent-to-add files, we used to show the empty blob's hash instead.\n> > In c26022ea8f5 (diff: convert diff_addremove to struct object_id,\n> > 2017-05-30), we made that worse by inadvertently changing that to the\n> > hash of the empty tree.\n> >\n> > Let's make the behavior consistent with modified files by showing\n> > all-zero values also for intent-to-add files.\n>\n> Well described.\n>\n> Thanks.\n>\n"},{"id":"400882","messageId":"nycvar.QRO.7.76.6.2007011153040.56@tvgsbejvaqbjf.bet","threadId":"53658","inReplyTo":"xmqqimff3tib.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4 2/2] difftool -d: ensure that intent-to-add files are handled correctly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-01T10:01:42Z","receivedAt":"2020-07-01T10:01:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 25 Jun 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > In https://github.com/git-for-windows/git/issues/2677, a `git difftool\n> > -d` problem was reported. The underlying cause was a bug in `git\n> > diff-files --raw` that we just fixed.\n>\n> That leaves a gap between \"there is some unspecified problem\" and\n> \"the problem was cased by such and such\" that forces readers to\n> either know the problem at heart (may apply to you and me right now,\n> but I am not sure about me 3 months in the future) or go check the\n> external website.\n>\n> Can we fill the gap by saying how seeing the object name of empty\n> blob (or worse, tree) instead of 0{40} made \"difftool -d\" upset?\n\nSorry about catching this only now, after the commit hit `next`.\n\nFilling the gap is a slightly more complicated.\n\nAnd now that I looked at the code again, to make sure that I don't say\nanything stupid, I realize that I just provided incorrect information in\nmy reply elsewhere in this thread: Srinidhi's fix is _not_ enough to fix\nt7800 with this here patch.\n\nYour guess was almost spot on: the empty blob would have worked (as in:\nnot caused an error, but it would have shown incorrect information). The\nproblem really is the attempt trying to read the empty tree as if it was a\nblob. That results in something like this:\n\n\terror: unable to read sha1 file of /tmp/git-difftool.O8CoK9/right/intent-to-add (4b825dc642cb6eb9a060e54bf8d69288fbee4904)\n\terror: could not write 'intent-to-add'\n\nAnd yes, it would have been good to adjust the commit message as you\nsuggested. Sorry for not getting to it in time before it hit `next`.\n\nDo you want me to send out a v5 and drop v4 from `next` in favor of the\nnew iteration?\n\nCiao,\nDscho\n\n>\n> Thanks.\n>\n>\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > t/t7800-difftool.sh | 8 ++++++++\n> > 1 file changed, 8 insertions(+)\n> >\n> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> > index 29b92907e2..524f30f7dc 100755\n> > --- a/t/t7800-difftool.sh\n> > +++ b/t/t7800-difftool.sh\n> > @@ -720,6 +720,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n> > \ttest_cmp expect actual\n> > '\n> >\n> > +test_expect_success 'add -N and difftool -d' '\n> > +\ttest_when_finished git reset --hard &&\n> > +\n> > +\ttest_write_lines A B C >intent-to-add &&\n> > +\tgit add -N intent-to-add &&\n> > +\tgit difftool --dir-diff --extcmd ls\n> > +'\n> > +\n> > test_expect_success 'outside worktree' '\n> > \techo 1 >1 &&\n> > \techo 2 >2 &&\n>\n"},{"id":"400946","messageId":"xmqqeepvc5k5.fsf@gitster.c.googlers.com","threadId":"53658","inReplyTo":"nycvar.QRO.7.76.6.2007011144410.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 1/2] diff-files --raw: show correct post-image of intent-to-add files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-01T21:02:18Z","receivedAt":"2020-07-01T21:02:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Thu, 25 Jun 2020, Junio C Hamano wrote:\n>\n>> \"Johannes Schindelin via GitGitGadget\" \n>> writes:\n>>\n>> > diff --git a/diff-lib.c b/diff-lib.c\n>> > index 61812f48c2..25fd2dee19 100644\n>> > --- a/diff-lib.c\n>> > +++ b/diff-lib.c\n>> > @@ -220,8 +220,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n>> > \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n>> > \t\t\t\t ce_intent_to_add(ce)) {\n>> > \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n>> > -\t\t\t\t\t the_hash_algo->empty_tree, 0,\n>> > -\t\t\t\t\t ce->name, 0);\n>> > +\t\t\t\t\t &null_oid, 0, ce->name, 0);\n>>\n>> This (even if the preimage were correctly using empty_blob) is *so*\n>> simple a change that it is very surprising that the new test in\n>> [2/2] passes without any other code change.\n>>\n>> It means that difftool was written correctly in the first place,\n>> right?\n>\n> Well, it means that difftool does not even consume the OID. Sure, it\n> parses it, but then it ignores it. All it _really_ is interested in is\n> that status flag (`A`),\n\nAh, that's an ultimately defensive code. No matter what is on the\nright hand side of the raw output, as long as it knows that the\nright hand side is a file on the working tree, it can safely ignore\nthe object name and directly go to the working tree file.\n\nNice ;-)\n\n> so technically, my fix in 1/2 is not even needed\n> after `sk/diff-files-show-i-t-a-as-new` to let 2/2 pass.\n\nSure, but I think it is the right thing to do nevertheless. Giving\nthe object name for an empty blob would be wrong unless the working\ntree file is known to be empty (and empty tree? what were we even\nthinking, gee...).\n"},{"id":"400947","messageId":"xmqqa70jc5hx.fsf@gitster.c.googlers.com","threadId":"53658","inReplyTo":"nycvar.QRO.7.76.6.2007011153040.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 2/2] difftool -d: ensure that intent-to-add files are handled correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-01T21:03:38Z","receivedAt":"2020-07-01T21:03:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Can we fill the gap by saying how seeing the object name of empty\n>> blob (or worse, tree) instead of 0{40} made \"difftool -d\" upset?\n>\n> Sorry about catching this only now, after the commit hit `next`.\n>\n> Filling the gap is a slightly more complicated.\n>\n> And now that I looked at the code again, to make sure that I don't say\n> anything stupid, I realize that I just provided incorrect information in\n> my reply elsewhere in this thread: Srinidhi's fix is _not_ enough to fix\n> t7800 with this here patch.\n>\n> Your guess was almost spot on: the empty blob would have worked (as in:\n> not caused an error, but it would have shown incorrect information). The\n> problem really is the attempt trying to read the empty tree as if it was a\n> blob. That results in something like this:\n>\n> \terror: unable to read sha1 file of /tmp/git-difftool.O8CoK9/right/intent-to-add (4b825dc642cb6eb9a060e54bf8d69288fbee4904)\n> \terror: could not write 'intent-to-add'\n>\n> And yes, it would have been good to adjust the commit message as you\n> suggested. Sorry for not getting to it in time before it hit `next`.\n>\n> Do you want me to send out a v5 and drop v4 from `next` in favor of the\n> new iteration?\n\nThat would help the future \"us\" quite a lot. Thanks for carefully\nthinking it through.\n"},{"id":"400948","messageId":"pull.654.v5.git.1593638347.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.v4.git.1593107621.gitgitgadget@gmail.com","subject":"[PATCH v5 0/2] Fix difftool problem with intent-to-add files","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-07-01T21:19:05Z","receivedAt":"2020-07-01T21:19:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This problem was reported in \nhttps://github.com/git-for-windows/git/issues/2677, but the problem actually\nlies with git diff --raw, and it seems that the bug has been with us ever\nsince --intent-to-add was introduced.\n\nChanges since v4:\n\n * Improved both commit messages after feedback from Junio.\n\nChanges since v3:\n\n * Instead of showing the same OID in git diff-files --raw as in git\n diff-files -p for intent-to-add files, we now imitate the logic for\n modified (non-intent-to-add) files: we show the \"null\" OID (i.e.\n all-zero).\n \n \n * As a consequence, we no longer just undo the inadvertent change where\n intent-to-add files were reported with the empty tree instead of the\n empty blob, but we fix the bug that has been with us since \n run_diff_files() was taught about intent-to-add files, i.e. we fix the\n original bug of 425a28e0a4e (diff-lib: allow ita entries treated as \"not\n yet exist in index\", 2016-10-24), at long last.\n \n \n\nChanges since v2:\n\n * Now we also drop the no-longer-used definition of hash_t in t2203.\n\nChanges since v1:\n\n * Rebased onto sk/diff-files-show-i-t-a-as-new.\n * Verified that sk/diff-files-show-i-t-a-as-new does not completely resolve\n the issue (the --raw output still claims the empty blob as the\n post-image, although the difftool symptom \"went away\").\n * Amended the central patch of this PR to include a fix for the regression\n test that was introduced in sk/diff-files-show-i-t-a-as-new: it expected\n the raw diff to contain the hash of the empty tree object (which is\n incorrect no matter how you turn it: any hash in any raw diff should\n refer to blob objects).\n * Reordered the patches so that the central patch comes first (otherwise,\n the \"empty tree\" fix would cause a test failure in t2203).\n\nJohannes Schindelin (2):\n diff-files --raw: show correct post-image of intent-to-add files\n difftool -d: ensure that intent-to-add files are handled correctly\n\n diff-lib.c | 3 +--\n t/t2203-add-intent.sh | 5 ++---\n t/t7800-difftool.sh | 8 ++++++++\n 3 files changed, 11 insertions(+), 5 deletions(-)\n\n\nbase-commit: feea6946a5b746ff4ebf8ccdf959e303203a6011\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-654%2Fdscho%2Fdifftool-ita-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-654/dscho/difftool-ita-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/654\n\nRange-diff vs v4:\n\n 1: 69256ab910 ! 1: 4392ae4be6 diff-files --raw: show correct post-image of intent-to-add files\n @@ Commit message\n \n [...] 0{40} if creation, unmerged or \"look at work tree\".\n \n - This happens for example when showing modified, unstaged files.\n + on the right hand (i.e. postimage) side. This happens for files that\n + have unstaged modifications, and for files that are unmodified but\n + stat-dirty.\n \n For intent-to-add files, we used to show the empty blob's hash instead.\n In c26022ea8f5 (diff: convert diff_addremove to struct object_id,\n 2017-05-30), we made that worse by inadvertently changing that to the\n hash of the empty tree.\n \n - Let's make the behavior consistent with modified files by showing\n + Let's make the behavior consistent with files that have unstaged\n + modifications (which applies to intent-to-add files, too) by showing\n all-zero values also for intent-to-add files.\n \n Accordingly, this patch adjusts the expectations set by the regression\n 2: 9bb8d84ea9 ! 2: 5f933e852a difftool -d: ensure that intent-to-add files are handled correctly\n @@ Commit message\n \n In https://github.com/git-for-windows/git/issues/2677, a `git difftool\n -d` problem was reported. The underlying cause was a bug in `git\n - diff-files --raw` that we just fixed.\n + diff-files --raw` that we just fixed: it reported intent-to-add files\n + with the empty _tree_ as the post-image OID, when we need to show\n + an all-zero (or, \"null\") OID instead, to indicate to the caller that\n + they have to look at the worktree file.\n +\n + The symptom of that problem shown by `git difftool` was this:\n +\n + error: unable to read sha1 file of ()\n + error: could not write ''\n \n Make sure that the reported `difftool` problem stays fixed.\n \n\n-- \ngitgitgadget\n"},{"id":"400949","messageId":"4392ae4be6ff4f1baad4bba7d166a0aaf634ca6e.1593638347.git.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.v5.git.1593638347.gitgitgadget@gmail.com","subject":"[PATCH v5 1/2] diff-files --raw: show correct post-image of intent-to-add files","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-07-01T21:19:06Z","receivedAt":"2020-07-01T21:19:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe documented behavior of `git diff-files --raw` is to display\n\n\t[...] 0{40} if creation, unmerged or \"look at work tree\".\n\non the right hand (i.e. postimage) side. This happens for files that\nhave unstaged modifications, and for files that are unmodified but\nstat-dirty.\n\nFor intent-to-add files, we used to show the empty blob's hash instead.\nIn c26022ea8f5 (diff: convert diff_addremove to struct object_id,\n2017-05-30), we made that worse by inadvertently changing that to the\nhash of the empty tree.\n\nLet's make the behavior consistent with files that have unstaged\nmodifications (which applies to intent-to-add files, too) by showing\nall-zero values also for intent-to-add files.\n\nAccordingly, this patch adjusts the expectations set by the regression\ntest introduced in feea6946a5b (diff-files: treat \"i-t-a\" files as\n\"not-in-index\", 2020-06-20).\n\nSigned-off-by: Johannes Schindelin \n---\n diff-lib.c | 3 +--\n t/t2203-add-intent.sh | 5 ++---\n 2 files changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 61812f48c2..25fd2dee19 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -220,8 +220,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n \t\t\t\t ce_intent_to_add(ce)) {\n \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n-\t\t\t\t\t the_hash_algo->empty_tree, 0,\n-\t\t\t\t\t ce->name, 0);\n+\t\t\t\t\t &null_oid, 0, ce->name, 0);\n \t\t\t\tcontinue;\n \t\t\t}\n \ndiff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh\nindex 8a5d55054f..cf0175ad6e 100755\n--- a/t/t2203-add-intent.sh\n+++ b/t/t2203-add-intent.sh\n@@ -240,7 +240,6 @@ test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new\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@@ -259,8 +258,8 @@ test_expect_success 'i-t-a files shown as new for \"diff\", \"diff-files\"; not-new\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+\t:000000 100644 0000000 0000000 A$(printf \"\\t\")empty\n+\t:000000 100644 0000000 0000000 A$(printf \"\\t\")not-empty\n \tEOF\n \n \tgit add -N empty not-empty &&\n-- \ngitgitgadget\n\n"},{"id":"400950","messageId":"5f933e852a525d4721b6f3e38ec12abf9c0d1aea.1593638347.git.gitgitgadget@gmail.com","threadId":"53658","inReplyTo":"pull.654.v5.git.1593638347.gitgitgadget@gmail.com","subject":"[PATCH v5 2/2] difftool -d: ensure that intent-to-add files are handled correctly","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-07-01T21:19:07Z","receivedAt":"2020-07-01T21:19:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn https://github.com/git-for-windows/git/issues/2677, a `git difftool\n-d` problem was reported. The underlying cause was a bug in `git\ndiff-files --raw` that we just fixed: it reported intent-to-add files\nwith the empty _tree_ as the post-image OID, when we need to show\nan all-zero (or, \"null\") OID instead, to indicate to the caller that\nthey have to look at the worktree file.\n\nThe symptom of that problem shown by `git difftool` was this:\n\n\terror: unable to read sha1 file of ()\n\terror: could not write ''\n\nMake sure that the reported `difftool` problem stays fixed.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t7800-difftool.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 29b92907e2..524f30f7dc 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -720,6 +720,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'add -N and difftool -d' '\n+\ttest_when_finished git reset --hard &&\n+\n+\ttest_write_lines A B C >intent-to-add &&\n+\tgit add -N intent-to-add &&\n+\tgit difftool --dir-diff --extcmd ls\n+'\n+\n test_expect_success 'outside worktree' '\n \techo 1 >1 &&\n \techo 2 >2 &&\n-- \ngitgitgadget\n"},{"id":"400951","messageId":"nycvar.QRO.7.76.6.2007012319100.56@tvgsbejvaqbjf.bet","threadId":"53658","inReplyTo":"xmqqa70jc5hx.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4 2/2] difftool -d: ensure that intent-to-add files are handled correctly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-01T21:20:33Z","receivedAt":"2020-07-01T21:20:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 1 Jul 2020, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> >> Can we fill the gap by saying how seeing the object name of empty\n> >> blob (or worse, tree) instead of 0{40} made \"difftool -d\" upset?\n> >\n> > Sorry about catching this only now, after the commit hit `next`.\n> >\n> > Filling the gap is a slightly more complicated.\n> >\n> > And now that I looked at the code again, to make sure that I don't say\n> > anything stupid, I realize that I just provided incorrect information in\n> > my reply elsewhere in this thread: Srinidhi's fix is _not_ enough to fix\n> > t7800 with this here patch.\n> >\n> > Your guess was almost spot on: the empty blob would have worked (as in:\n> > not caused an error, but it would have shown incorrect information). The\n> > problem really is the attempt trying to read the empty tree as if it was a\n> > blob. That results in something like this:\n> >\n> > \terror: unable to read sha1 file of /tmp/git-difftool.O8CoK9/right/intent-to-add (4b825dc642cb6eb9a060e54bf8d69288fbee4904)\n> > \terror: could not write 'intent-to-add'\n> >\n> > And yes, it would have been good to adjust the commit message as you\n> > suggested. Sorry for not getting to it in time before it hit `next`.\n> >\n> > Do you want me to send out a v5 and drop v4 from `next` in favor of the\n> > new iteration?\n>\n> That would help the future \"us\" quite a lot. Thanks for carefully\n> thinking it through.\n\nYou're welcome. Here goes v5:\nhttps://lore.kernel.org/git/pull.654.v5.git.1593638347.gitgitgadget@gmail.com/\n\nCiao,\nDscho\n"}]}