{"thread":{"id":"56164","subject":"[PATCH 1/3] Remove unused var","startedAt":"2021-07-26T18:34:20Z","lastAt":"2021-08-31T17:16:43Z","messageCount":12,"participants":["David Turner","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"431214","messageId":"20210726183358.3255233-1-dturner@twosigma.com","threadId":"56164","inReplyTo":null,"subject":"[PATCH 1/3] Remove unused var","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2021-07-26T18:33:56Z","receivedAt":"2021-07-26T18:34:20Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Signed-off-by: David Turner <dturner@twosigma.com>\n---\n t/t4060-diff-submodule-option-diff-format.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\nindex dc7b242697..69b9946931 100755\n--- a/t/t4060-diff-submodule-option-diff-format.sh\n+++ b/t/t4060-diff-submodule-option-diff-format.sh\n@@ -361,7 +361,6 @@ test_expect_success 'typechanged submodule(submodule->blob)' '\n rm -f sm1 &&\n test_create_repo sm1 &&\n head6=$(add_file sm1 foo6 foo7)\n-fullhead6=$(cd sm1; git rev-parse --verify HEAD)\n test_expect_success 'nonexistent commit' '\n \tgit diff-index -p --submodule=diff HEAD >actual &&\n \tcat >expected <<-EOF &&\n-- \n2.11.GIT\n\n"},{"id":"431216","messageId":"20210726183358.3255233-2-dturner@twosigma.com","threadId":"56164","inReplyTo":"20210726183358.3255233-1-dturner@twosigma.com","subject":"[PATCH 2/3] diff --submodule=diff: do not fail on ever-initialied deleted submodules","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2021-07-26T18:33:57Z","receivedAt":"2021-07-26T18:41:57Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"If you have ever initialized a submodule, open_submodule will open it.\nIf you then delete the submodule's worktree directory (but don't\nremove it from .gitmodules), git diff --submodule=diff would crash as\nit attempted to chdir into the now-deleted working tree directory.\n\nSigned-off-by: David Turner <dturner@twosigma.com>\n---\n submodule.c                                  |  3 ++\n t/t4060-diff-submodule-option-diff-format.sh | 45 ++++++++++++++++++++++++----\n 2 files changed, 43 insertions(+), 5 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 0b1d9c1dde..9031527a16 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -673,6 +673,9 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path,\n \t    !(right || is_null_oid(two)))\n \t\tgoto done;\n \n+\tif (!is_directory(path))\n+\t\tgoto done;\n+\n \tif (left)\n \t\told_oid = one;\n \tif (right)\ndiff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\nindex 69b9946931..10e330c08a 100755\n--- a/t/t4060-diff-submodule-option-diff-format.sh\n+++ b/t/t4060-diff-submodule-option-diff-format.sh\n@@ -703,10 +703,26 @@ test_expect_success 'path filter' '\n \tdiff_cmp expected actual\n '\n \n-commit_file sm2\n+cat >.gitmodules <<-EOF\n+[submodule \"sm2\"]\n+\tpath = sm2\n+\turl = bogus_url\n+EOF\n+git add .gitmodules\n+commit_file sm2 .gitmodules\n+\n test_expect_success 'given commit' '\n \tgit diff-index -p --submodule=diff HEAD^ >actual &&\n \tcat >expected <<-EOF &&\n+\tdiff --git a/.gitmodules b/.gitmodules\n+\tnew file mode 100644\n+\tindex 1234567..89abcde\n+\t--- /dev/null\n+\t+++ b/.gitmodules\n+\t@@ -0,0 +1,3 @@\n+\t+[submodule \"sm2\"]\n+\t+path = sm2\n+\t+url = bogus_url\n \tSubmodule sm1 $head7...0000000 (submodule deleted)\n \tSubmodule sm2 0000000...$head9 (new submodule)\n \tdiff --git a/sm2/foo8 b/sm2/foo8\n@@ -728,15 +744,21 @@ test_expect_success 'given commit' '\n '\n \n test_expect_success 'setup .git file for sm2' '\n-\t(cd sm2 &&\n-\t REAL=\"$(pwd)/../.real\" &&\n-\t mv .git \"$REAL\" &&\n-\t echo \"gitdir: $REAL\" >.git)\n+\tgit submodule absorbgitdirs sm2\n '\n \n test_expect_success 'diff --submodule=diff with .git file' '\n \tgit diff --submodule=diff HEAD^ >actual &&\n \tcat >expected <<-EOF &&\n+\tdiff --git a/.gitmodules b/.gitmodules\n+\tnew file mode 100644\n+\tindex 1234567..89abcde\n+\t--- /dev/null\n+\t+++ b/.gitmodules\n+\t@@ -0,0 +1,3 @@\n+\t+[submodule \"sm2\"]\n+\t+path = sm2\n+\t+url = bogus_url\n \tSubmodule sm1 $head7...0000000 (submodule deleted)\n \tSubmodule sm2 0000000...$head9 (new submodule)\n \tdiff --git a/sm2/foo8 b/sm2/foo8\n@@ -757,6 +779,19 @@ test_expect_success 'diff --submodule=diff with .git file' '\n \tdiff_cmp expected actual\n '\n \n+mv sm2 sm2-bak\n+\n+test_expect_success 'deleted submodule with .git file' '\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 $head7...0000000 (submodule deleted)\n+\tSubmodule sm2 $head9...0000000 (submodule deleted)\n+\tEOF\n+\tdiff_cmp expected actual\n+'\n+\n+mv sm2-bak sm2\n+\n test_expect_success 'setup nested submodule' '\n \tgit submodule add -f ./sm2 &&\n \tgit commit -a -m \"add sm2\" &&\n-- \n2.11.GIT\n\n"},{"id":"431217","messageId":"20210726183358.3255233-3-dturner@twosigma.com","threadId":"56164","inReplyTo":"20210726183358.3255233-1-dturner@twosigma.com","subject":"[PATCH 3/3] diff --submodule=diff: Don't print failure message twice","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2021-07-26T18:33:58Z","receivedAt":"2021-07-26T18:43:16Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"When we fail to start a diff command inside a submodule, immediately\nexit the routine rather than trying to finish the command and printing\na second message.\n\nSigned-off-by: David Turner <dturner@twosigma.com>\n---\n submodule.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 9031527a16..0acfad3d4c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -713,8 +713,10 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path,\n \t\tstrvec_push(&cp.args, oid_to_hex(new_oid));\n \n \tprepare_submodule_repo_env(&cp.env_array);\n-\tif (start_command(&cp))\n+\tif (start_command(&cp)) {\n \t\tdiff_emit_submodule_error(o, \"(diff failed)\\n\");\n+\t\tgoto done;\n+\t}\n \n \twhile (strbuf_getwholeline_fd(&sb, cp.out, '\\n') != EOF)\n \t\tdiff_emit_submodule_pipethrough(o, sb.buf, sb.len);\n-- \n2.11.GIT\n\n"},{"id":"431244","messageId":"xmqq4kcghev1.fsf@gitster.g","threadId":"56164","inReplyTo":"20210726183358.3255233-1-dturner@twosigma.com","subject":"Re: [PATCH 1/3] Remove unused var","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-26T22:47:30Z","receivedAt":"2021-07-26T22:47:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twosigma.com> writes:\n\n> Signed-off-by: David Turner <dturner@twosigma.com>\n> ---\n>  t/t4060-diff-submodule-option-diff-format.sh | 1 -\n>  1 file changed, 1 deletion(-)\n>\n> diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\n> index dc7b242697..69b9946931 100755\n> --- a/t/t4060-diff-submodule-option-diff-format.sh\n> +++ b/t/t4060-diff-submodule-option-diff-format.sh\n> @@ -361,7 +361,6 @@ test_expect_success 'typechanged submodule(submodule->blob)' '\n>  rm -f sm1 &&\n>  test_create_repo sm1 &&\n>  head6=$(add_file sm1 foo6 foo7)\n> -fullhead6=$(cd sm1; git rev-parse --verify HEAD)\n>  test_expect_success 'nonexistent commit' '\n>  \tgit diff-index -p --submodule=diff HEAD >actual &&\n>  \tcat >expected <<-EOF &&\n\nMakes sense.  THanks.\n"},{"id":"431245","messageId":"xmqqzgu8g0ab.fsf@gitster.g","threadId":"56164","inReplyTo":"20210726183358.3255233-3-dturner@twosigma.com","subject":"Re: [PATCH 3/3] diff --submodule=diff: Don't print failure message twice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-26T22:47:40Z","receivedAt":"2021-07-26T22:47:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twosigma.com> writes:\n\n> When we fail to start a diff command inside a submodule, immediately\n> exit the routine rather than trying to finish the command and printing\n> a second message.\n>\n> Signed-off-by: David Turner <dturner@twosigma.com>\n> ---\n>  submodule.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 9031527a16..0acfad3d4c 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -713,8 +713,10 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path,\n>  \t\tstrvec_push(&cp.args, oid_to_hex(new_oid));\n>  \n>  \tprepare_submodule_repo_env(&cp.env_array);\n> -\tif (start_command(&cp))\n> +\tif (start_command(&cp)) {\n>  \t\tdiff_emit_submodule_error(o, \"(diff failed)\\n\");\n> +\t\tgoto done;\n> +\t}\n>  \n>  \twhile (strbuf_getwholeline_fd(&sb, cp.out, '\\n') != EOF)\n>  \t\tdiff_emit_submodule_pipethrough(o, sb.buf, sb.len);\n\nAgain, makes sense.  Thanks.\n"},{"id":"431247","messageId":"xmqqv94wfzu0.fsf@gitster.g","threadId":"56164","inReplyTo":"20210726183358.3255233-2-dturner@twosigma.com","subject":"Re: [PATCH 2/3] diff --submodule=diff: do not fail on ever-initialied deleted submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-26T22:57:27Z","receivedAt":"2021-07-26T22:57:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twosigma.com> writes:\n\n> If you have ever initialized a submodule, open_submodule will open it.\n> If you then delete the submodule's worktree directory (but don't\n> remove it from .gitmodules), git diff --submodule=diff would crash as\n> it attempted to chdir into the now-deleted working tree directory.\n\nHmph.  So what does the failure look like?  The child process inside\nstart_command() attempts chdir() and reports CHILD_ERR_CHDIR back,\nand we catch it as an error by reading from notify_pipe[0] and report\nfailure from start_command()?  Or are we talking about a more severe\n\"crash\" of an uncontrolled kind?\n\nBypassing the execution of diff in the submodule like this patch\ndoes may avoid such a failure, but is that all we need to \"fix\" this\nissue?  What does the user expect after removing a submodule that\nway and runs \"diff\" with the \"--submodule=diff\" option?  Shouldn't\nwe be giving \"all lines from all files have been removed\" patch?\n\nThanks.\n\n> Signed-off-by: David Turner <dturner@twosigma.com>\n> ---\n>  submodule.c                                  |  3 ++\n>  t/t4060-diff-submodule-option-diff-format.sh | 45 ++++++++++++++++++++++++----\n>  2 files changed, 43 insertions(+), 5 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 0b1d9c1dde..9031527a16 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -673,6 +673,9 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path,\n>  \t    !(right || is_null_oid(two)))\n>  \t\tgoto done;\n>  \n> +\tif (!is_directory(path))\n> +\t\tgoto done;\n> +\n>  \tif (left)\n>  \t\told_oid = one;\n>  \tif (right)\n> diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\n> index 69b9946931..10e330c08a 100755\n> --- a/t/t4060-diff-submodule-option-diff-format.sh\n> +++ b/t/t4060-diff-submodule-option-diff-format.sh\n> @@ -703,10 +703,26 @@ test_expect_success 'path filter' '\n>  \tdiff_cmp expected actual\n>  '\n>  \n> -commit_file sm2\n> +cat >.gitmodules <<-EOF\n> +[submodule \"sm2\"]\n> +\tpath = sm2\n> +\turl = bogus_url\n> +EOF\n> +git add .gitmodules\n> +commit_file sm2 .gitmodules\n> +\n>  test_expect_success 'given commit' '\n>  \tgit diff-index -p --submodule=diff HEAD^ >actual &&\n>  \tcat >expected <<-EOF &&\n> +\tdiff --git a/.gitmodules b/.gitmodules\n> +\tnew file mode 100644\n> +\tindex 1234567..89abcde\n> +\t--- /dev/null\n> +\t+++ b/.gitmodules\n> +\t@@ -0,0 +1,3 @@\n> +\t+[submodule \"sm2\"]\n> +\t+path = sm2\n> +\t+url = bogus_url\n>  \tSubmodule sm1 $head7...0000000 (submodule deleted)\n>  \tSubmodule sm2 0000000...$head9 (new submodule)\n>  \tdiff --git a/sm2/foo8 b/sm2/foo8\n> @@ -728,15 +744,21 @@ test_expect_success 'given commit' '\n>  '\n>  \n>  test_expect_success 'setup .git file for sm2' '\n> -\t(cd sm2 &&\n> -\t REAL=\"$(pwd)/../.real\" &&\n> -\t mv .git \"$REAL\" &&\n> -\t echo \"gitdir: $REAL\" >.git)\n> +\tgit submodule absorbgitdirs sm2\n>  '\n>  \n>  test_expect_success 'diff --submodule=diff with .git file' '\n>  \tgit diff --submodule=diff HEAD^ >actual &&\n>  \tcat >expected <<-EOF &&\n> +\tdiff --git a/.gitmodules b/.gitmodules\n> +\tnew file mode 100644\n> +\tindex 1234567..89abcde\n> +\t--- /dev/null\n> +\t+++ b/.gitmodules\n> +\t@@ -0,0 +1,3 @@\n> +\t+[submodule \"sm2\"]\n> +\t+path = sm2\n> +\t+url = bogus_url\n>  \tSubmodule sm1 $head7...0000000 (submodule deleted)\n>  \tSubmodule sm2 0000000...$head9 (new submodule)\n>  \tdiff --git a/sm2/foo8 b/sm2/foo8\n> @@ -757,6 +779,19 @@ test_expect_success 'diff --submodule=diff with .git file' '\n>  \tdiff_cmp expected actual\n>  '\n>  \n> +mv sm2 sm2-bak\n> +\n> +test_expect_success 'deleted submodule with .git file' '\n> +\tgit diff-index -p --submodule=diff HEAD >actual &&\n> +\tcat >expected <<-EOF &&\n> +\tSubmodule sm1 $head7...0000000 (submodule deleted)\n> +\tSubmodule sm2 $head9...0000000 (submodule deleted)\n> +\tEOF\n> +\tdiff_cmp expected actual\n> +'\n> +\n> +mv sm2-bak sm2\n> +\n>  test_expect_success 'setup nested submodule' '\n>  \tgit submodule add -f ./sm2 &&\n>  \tgit commit -a -m \"add sm2\" &&\n"},{"id":"431325","messageId":"91f6fc69470d4291a982cb9c4b3cd6c1@exmbdft7.ad.twosigma.com","threadId":"56164","inReplyTo":"xmqqv94wfzu0.fsf@gitster.g","subject":"RE: [PATCH 2/3] diff --submodule=diff: do not fail on ever-initialied deleted submodules","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2021-07-27T17:35:10Z","receivedAt":"2021-07-27T17:41:18Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> From: Junio C Hamano <gitster@pobox.com>\n> Sent: Monday, July 26, 2021 6:57 PM\n> To: David Turner <David.Turner@twosigma.com>\n> Cc: git@vger.kernel.org\n> Subject: Re: [PATCH 2/3] diff --submodule=diff: do not fail on \n> ever-initialied deleted submodules\n> \n> David Turner <dturner@twosigma.com> writes:\n> \n> > If you have ever initialized a submodule, open_submodule will open it.\n> > If you then delete the submodule's worktree directory (but don't \n> > remove it from .gitmodules), git diff --submodule=diff would crash \n> > as it attempted to chdir into the now-deleted working tree directory.\n> \n> Hmph.  So what does the failure look like?  The child process inside\n> start_command() attempts chdir() and reports CHILD_ERR_CHDIR back, and \n> we catch it as an error by reading from notify_pipe[0] and report \n> failure from start_command()?  Or are we talking about a more severe \n> \"crash\" of an uncontrolled kind?\n\nIt's the first kind.\n\n> Bypassing the execution of diff in the submodule like this patch does \n> may avoid such a failure, but is that all we need to \"fix\" this issue?  \n> What does the user expect after removing a submodule that way and runs \n> \"diff\" with the \"-- submodule=diff\" option?  Shouldn't we be giving \n> \"all lines from all files have been removed\" patch?\n\nJust a note: this only matters if the submodules git dir is\nabsorbed.  If not, then we no longer have anywhere to run the\ndiff.  But that case does not trigger this error, because in that\ncase, open_submodule fails, so we don't resolve a left commit, so\nwe exit early, which is the only thing we could do.\n\nIf absorbed, then we could, in theory, go into the submodule's\nabsorbed git dir (.git/modules/sm2) and run the diff there.  But\nin practice, that's a bit more complicated, because `git diff`\nexpects to be run from inside a working directory, not a git dir.\nSo it looks in the config for core.worktree, and does\nchdir(\"../../../sm2\"), which is the very dir that we're trying to\navoid visiting because it's been deleted.  We could work around\nthis by setting GIT_WORK_TREE (and GIT_DIR) to \".\".  This\nactually seems to work, but it's a little weird to set\nGIT_WORK_TREE to something that is not a working tree just to\navoid an unnecessary chdir.  To my mild surprise, it also seems\nto work correctly in the case of deleted nested (absorbed)\nsubmodules.  What do you think of this idea?\n\n(Side note: The same bit about chdir into the working tree is\ntrue for diff-tree even though it normally does not need anything\nfrom the working tree.  I say \"normally\", because in the case of\nnested submodules, it might need to look inside those submodules,\nwhich might themselves not be absorbed.  Of course, this case\ncannot obtain if the submodule in the worktree has been deleted.\nWe should consider fixing the docs for git diff-tree\n--submodule=diff to specify that it only works if -p is passed.)\n\n(Sorry if the formatting on this email ends up bad -- corporate\nemail is... corporate .  I'm going to CC my personal address so\nthat I can use a better mailer on future replies). \n"},{"id":"434236","messageId":"xmqqwno2xhmu.fsf@gitster.g","threadId":"56164","inReplyTo":"91f6fc69470d4291a982cb9c4b3cd6c1@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH 2/3] diff --submodule=diff: do not fail on ever-initialied deleted submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-31T06:17:29Z","receivedAt":"2021-08-31T06:17:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <David.Turner@twosigma.com> writes:\n\n>> From: Junio C Hamano <gitster@pobox.com>\n>> Sent: Monday, July 26, 2021 6:57 PM\n>> To: David Turner <David.Turner@twosigma.com>\n>> Cc: git@vger.kernel.org\n>> Subject: Re: [PATCH 2/3] diff --submodule=diff: do not fail on \n>> ever-initialied deleted submodules\n>> \n>> David Turner <dturner@twosigma.com> writes:\n>> \n>> > If you have ever initialized a submodule, open_submodule will open it.\n>> > If you then delete the submodule's worktree directory (but don't \n>> > remove it from .gitmodules), git diff --submodule=diff would crash \n>> > as it attempted to chdir into the now-deleted working tree directory.\n>> \n>> Hmph.  So what does the failure look like?  The child process inside\n>> start_command() attempts chdir() and reports CHILD_ERR_CHDIR back, and \n>> we catch it as an error by reading from notify_pipe[0] and report \n>> failure from start_command()?  Or are we talking about a more severe \n>> \"crash\" of an uncontrolled kind?\n>\n> It's the first kind.\n>\n>> Bypassing the execution of diff in the submodule like this patch does \n>> may avoid such a failure, but is that all we need to \"fix\" this issue?  \n>> What does the user expect after removing a submodule that way and runs \n>> \"diff\" with the \"-- submodule=diff\" option?  Shouldn't we be giving \n>> \"all lines from all files have been removed\" patch?\n>\n> Just a note: this only matters if the submodules git dir is\n> absorbed.  If not, then we no longer have anywhere to run the\n> diff.  But that case does not trigger this error, because in that\n> case, open_submodule fails, so we don't resolve a left commit, so\n> we exit early, which is the only thing we could do.\n>\n> If absorbed, then we could, in theory, go into the submodule's\n> absorbed git dir (.git/modules/sm2) and run the diff there.  But\n> in practice, that's a bit more complicated, because `git diff`\n> expects to be run from inside a working directory, not a git dir.\n> So it looks in the config for core.worktree, and does\n> chdir(\"../../../sm2\"), which is the very dir that we're trying to\n> avoid visiting because it's been deleted.  We could work around\n> this by setting GIT_WORK_TREE (and GIT_DIR) to \".\".  This\n> actually seems to work, but it's a little weird to set\n> GIT_WORK_TREE to something that is not a working tree just to\n> avoid an unnecessary chdir.  To my mild surprise, it also seems\n> to work correctly in the case of deleted nested (absorbed)\n> submodules.  What do you think of this idea?\n>\n> (Side note: The same bit about chdir into the working tree is\n> true for diff-tree even though it normally does not need anything\n> from the working tree.  I say \"normally\", because in the case of\n> nested submodules, it might need to look inside those submodules,\n> which might themselves not be absorbed.  Of course, this case\n> cannot obtain if the submodule in the worktree has been deleted.\n> We should consider fixing the docs for git diff-tree\n> --submodule=diff to specify that it only works if -p is passed.)\n>\n> (Sorry if the formatting on this email ends up bad -- corporate\n> email is... corporate .  I'm going to CC my personal address so\n> that I can use a better mailer on future replies). \n\nThat I had to ask questions based on the proposed log message and\nyou needed to answer to clarify means that the next person who\nencounters this commit in \"git log\" would likely have to ask the\nsame question, and worse, unlike I had you, there is nobody to whom\nthey ask for help understanding this commit.\n\nThanks.\n"},{"id":"434263","messageId":"20210831131257.1631316-1-dturner@twosigma.com","threadId":"56164","inReplyTo":"xmqqwno2xhmu.fsf@gitster.g","subject":"[PATCH v4 1/3] Remove unused var","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2021-08-31T13:12:55Z","receivedAt":"2021-08-31T13:13:22Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Signed-off-by: David Turner <dturner@twosigma.com>\n---\n t/t4060-diff-submodule-option-diff-format.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\nindex dc7b242697..69b9946931 100755\n--- a/t/t4060-diff-submodule-option-diff-format.sh\n+++ b/t/t4060-diff-submodule-option-diff-format.sh\n@@ -361,7 +361,6 @@ test_expect_success 'typechanged submodule(submodule->blob)' '\n rm -f sm1 &&\n test_create_repo sm1 &&\n head6=$(add_file sm1 foo6 foo7)\n-fullhead6=$(cd sm1; git rev-parse --verify HEAD)\n test_expect_success 'nonexistent commit' '\n \tgit diff-index -p --submodule=diff HEAD >actual &&\n \tcat >expected <<-EOF &&\n-- \n2.11.GIT\n\n"},{"id":"434264","messageId":"20210831131257.1631316-2-dturner@twosigma.com","threadId":"56164","inReplyTo":"20210831131257.1631316-1-dturner@twosigma.com","subject":"[PATCH v4 2/3] diff --submodule=diff: do not fail on ever-initialied deleted submodules","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2021-08-31T13:12:56Z","receivedAt":"2021-08-31T13:13:26Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"If you have ever initialized a submodule, open_submodule will open it.\nIf you then delete the submodule's worktree directory (but don't\nremove it from .gitmodules), git diff --submodule=diff would error out\nas it attempted to chdir into the now-deleted working tree directory.\n\nThis only matters if the submodules git dir is absorbed.  If not, then\nwe no longer have anywhere to run the diff.  But that case does not\ntrigger this error, because in that case, open_submodule fails, so we\ndon't resolve a left commit, so we exit early, which is the only thing\nwe could do.\n\nIf absorbed, then we can run the diff from the submodule's absorbed\ngit dir (.git/modules/sm2).  In practice, that's a bit more\ncomplicated, because `git diff` expects to be run from inside a\nworking directory, not a git dir.  So it looks in the config for\ncore.worktree, and does chdir(\"../../../sm2\"), which is the very dir\nthat we're trying to avoid visiting because it's been deleted.  We\nwork around this by setting GIT_WORK_TREE (and GIT_DIR) to \".\".  It is\nlittle weird to set GIT_WORK_TREE to something that is not a working\ntree just to avoid an unnecessary chdir, but it works.\n\nSigned-off-by: David Turner <dturner@twosigma.com\n---\n submodule.c                                  |  10 ++\n t/t4060-diff-submodule-option-diff-format.sh | 158 +++++++++++++++++++++++++--\n 2 files changed, 161 insertions(+), 7 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 0b1d9c1dde..8aeff95cfd 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -710,6 +710,16 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path,\n \t\tstrvec_push(&cp.args, oid_to_hex(new_oid));\n \n \tprepare_submodule_repo_env(&cp.env_array);\n+\n+\tif (!is_directory(path)) {\n+\t\t/* fall back to absorbed git dir, if any */\n+\t\tif (!sub)\n+\t\t\tgoto done;\n+\t\tcp.dir = sub->gitdir;\n+\t\tstrvec_push(&cp.env_array, GIT_DIR_ENVIRONMENT \"=.\");\n+\t\tstrvec_push(&cp.env_array, GIT_WORK_TREE_ENVIRONMENT \"=.\");\n+\t}\n+\n \tif (start_command(&cp))\n \t\tdiff_emit_submodule_error(o, \"(diff failed)\\n\");\n \ndiff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\nindex 69b9946931..d86e38abd8 100755\n--- a/t/t4060-diff-submodule-option-diff-format.sh\n+++ b/t/t4060-diff-submodule-option-diff-format.sh\n@@ -703,10 +703,26 @@ test_expect_success 'path filter' '\n \tdiff_cmp expected actual\n '\n \n-commit_file sm2\n+cat >.gitmodules <<-EOF\n+[submodule \"sm2\"]\n+\tpath = sm2\n+\turl = bogus_url\n+EOF\n+git add .gitmodules\n+commit_file sm2 .gitmodules\n+\n test_expect_success 'given commit' '\n \tgit diff-index -p --submodule=diff HEAD^ >actual &&\n \tcat >expected <<-EOF &&\n+\tdiff --git a/.gitmodules b/.gitmodules\n+\tnew file mode 100644\n+\tindex 1234567..89abcde\n+\t--- /dev/null\n+\t+++ b/.gitmodules\n+\t@@ -0,0 +1,3 @@\n+\t+[submodule \"sm2\"]\n+\t+path = sm2\n+\t+url = bogus_url\n \tSubmodule sm1 $head7...0000000 (submodule deleted)\n \tSubmodule sm2 0000000...$head9 (new submodule)\n \tdiff --git a/sm2/foo8 b/sm2/foo8\n@@ -728,15 +744,21 @@ test_expect_success 'given commit' '\n '\n \n test_expect_success 'setup .git file for sm2' '\n-\t(cd sm2 &&\n-\t REAL=\"$(pwd)/../.real\" &&\n-\t mv .git \"$REAL\" &&\n-\t echo \"gitdir: $REAL\" >.git)\n+\tgit submodule absorbgitdirs sm2\n '\n \n test_expect_success 'diff --submodule=diff with .git file' '\n \tgit diff --submodule=diff HEAD^ >actual &&\n \tcat >expected <<-EOF &&\n+\tdiff --git a/.gitmodules b/.gitmodules\n+\tnew file mode 100644\n+\tindex 1234567..89abcde\n+\t--- /dev/null\n+\t+++ b/.gitmodules\n+\t@@ -0,0 +1,3 @@\n+\t+[submodule \"sm2\"]\n+\t+path = sm2\n+\t+url = bogus_url\n \tSubmodule sm1 $head7...0000000 (submodule deleted)\n \tSubmodule sm2 0000000...$head9 (new submodule)\n \tdiff --git a/sm2/foo8 b/sm2/foo8\n@@ -757,9 +779,67 @@ test_expect_success 'diff --submodule=diff with .git file' '\n \tdiff_cmp expected actual\n '\n \n+mv sm2 sm2-bak\n+\n+test_expect_success 'deleted submodule with .git file' '\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 $head7...0000000 (submodule deleted)\n+\tSubmodule sm2 $head9...0000000 (submodule deleted)\n+\tdiff --git a/sm2/foo8 b/sm2/foo8\n+\tdeleted file mode 100644\n+\tindex 1234567..89abcde\n+\t--- a/sm2/foo8\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo8\n+\tdiff --git a/sm2/foo9 b/sm2/foo9\n+\tdeleted file mode 100644\n+\tindex 1234567..89abcde\n+\t--- a/sm2/foo9\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo9\n+\tEOF\n+\tdiff_cmp expected actual\n+'\n+\n+echo submodule-to-blob>sm2\n+\n+test_expect_success 'typechanged(submodule->blob) submodule with .git file' '\n+\tgit diff-index -p --submodule=diff HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 $head7...0000000 (submodule deleted)\n+\tSubmodule sm2 $head9...0000000 (submodule deleted)\n+\tdiff --git a/sm2/foo8 b/sm2/foo8\n+\tdeleted file mode 100644\n+\tindex 1234567..89abcde\n+\t--- a/sm2/foo8\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo8\n+\tdiff --git a/sm2/foo9 b/sm2/foo9\n+\tdeleted file mode 100644\n+\tindex 1234567..89abcde\n+\t--- a/sm2/foo9\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo9\n+\tdiff --git a/sm2 b/sm2\n+\tnew file mode 100644\n+\tindex 1234567..89abcde\n+\t--- /dev/null\n+\t+++ b/sm2\n+\t@@ -0,0 +1 @@\n+\t+submodule-to-blob\n+\tEOF\n+\tdiff_cmp expected actual\n+'\n+\n+rm sm2\n+mv sm2-bak sm2\n+\n test_expect_success 'setup nested submodule' '\n-\tgit submodule add -f ./sm2 &&\n-\tgit commit -a -m \"add sm2\" &&\n \tgit -C sm2 submodule add ../sm2 nested &&\n \tgit -C sm2 commit -a -m \"nested sub\" &&\n \thead10=$(git -C sm2 rev-parse --short --verify HEAD)\n@@ -790,6 +870,7 @@ test_expect_success 'diff --submodule=diff with moved nested submodule HEAD' '\n \n test_expect_success 'diff --submodule=diff recurses into nested submodules' '\n \tcat >expected <<-EOF &&\n+\tSubmodule sm1 $head7...0000000 (submodule deleted)\n \tSubmodule sm2 contains modified content\n \tSubmodule sm2 $head9..$head10:\n \tdiff --git a/sm2/.gitmodules b/sm2/.gitmodules\n@@ -829,4 +910,67 @@ test_expect_success 'diff --submodule=diff recurses into nested submodules' '\n \tdiff_cmp expected actual\n '\n \n+(cd sm2; commit_file nested)\n+commit_file sm2\n+head12=$(cd sm2; git rev-parse --short --verify HEAD)\n+\n+mv sm2 sm2-bak\n+\n+test_expect_success 'diff --submodule=diff recurses into deleted nested submodules' '\n+\tcat >expected <<-EOF &&\n+\tSubmodule sm1 $head7...0000000 (submodule deleted)\n+\tSubmodule sm2 $head12...0000000 (submodule deleted)\n+\tdiff --git a/sm2/.gitmodules b/sm2/.gitmodules\n+\tdeleted file mode 100644\n+\tindex 3a816b8..0000000\n+\t--- a/sm2/.gitmodules\n+\t+++ /dev/null\n+\t@@ -1,3 +0,0 @@\n+\t-[submodule \"nested\"]\n+\t-\tpath = nested\n+\t-\turl = ../sm2\n+\tdiff --git a/sm2/foo8 b/sm2/foo8\n+\tdeleted file mode 100644\n+\tindex db9916b..0000000\n+\t--- a/sm2/foo8\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo8\n+\tdiff --git a/sm2/foo9 b/sm2/foo9\n+\tdeleted file mode 100644\n+\tindex 9c3b4f6..0000000\n+\t--- a/sm2/foo9\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo9\n+\tSubmodule nested $head11...0000000 (submodule deleted)\n+\tdiff --git a/sm2/nested/file b/sm2/nested/file\n+\tdeleted file mode 100644\n+\tindex ca281f5..0000000\n+\t--- a/sm2/nested/file\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-nested content\n+\tdiff --git a/sm2/nested/foo8 b/sm2/nested/foo8\n+\tdeleted file mode 100644\n+\tindex db9916b..0000000\n+\t--- a/sm2/nested/foo8\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo8\n+\tdiff --git a/sm2/nested/foo9 b/sm2/nested/foo9\n+\tdeleted file mode 100644\n+\tindex 9c3b4f6..0000000\n+\t--- a/sm2/nested/foo9\n+\t+++ /dev/null\n+\t@@ -1 +0,0 @@\n+\t-foo9\n+\tEOF\n+\tgit diff --submodule=diff >actual 2>err &&\n+\ttest_must_be_empty err &&\n+\tdiff_cmp expected actual\n+'\n+\n+mv sm2-bak sm2\n+\n test_done\n-- \n2.11.GIT\n\n"},{"id":"434265","messageId":"20210831131257.1631316-3-dturner@twosigma.com","threadId":"56164","inReplyTo":"20210831131257.1631316-1-dturner@twosigma.com","subject":"[PATCH v4 3/3] diff --submodule=diff: Don't print failure message twice","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2021-08-31T13:12:57Z","receivedAt":"2021-08-31T13:13:30Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"When we fail to start a diff command inside a submodule, immediately\nexit the routine rather than trying to finish the command and printing\na second message.\n\nSigned-off-by: David Turner <dturner@twosigma.com>\n---\n submodule.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 8aeff95cfd..ab5f050f0e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -720,8 +720,10 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path,\n \t\tstrvec_push(&cp.env_array, GIT_WORK_TREE_ENVIRONMENT \"=.\");\n \t}\n \n-\tif (start_command(&cp))\n+\tif (start_command(&cp)) {\n \t\tdiff_emit_submodule_error(o, \"(diff failed)\\n\");\n+\t\tgoto done;\n+\t}\n \n \twhile (strbuf_getwholeline_fd(&sb, cp.out, '\\n') != EOF)\n \t\tdiff_emit_submodule_pipethrough(o, sb.buf, sb.len);\n-- \n2.11.GIT\n\n"},{"id":"434307","messageId":"xmqqtuj5v8jx.fsf@gitster.g","threadId":"56164","inReplyTo":"20210831131257.1631316-1-dturner@twosigma.com","subject":"Re: [PATCH v4 1/3] Remove unused var","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-31T17:16:34Z","receivedAt":"2021-08-31T17:16:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twosigma.com> writes:\n\n> Signed-off-by: David Turner <dturner@twosigma.com>\n> ---\n>  t/t4060-diff-submodule-option-diff-format.sh | 1 -\n>  1 file changed, 1 deletion(-)\n\nOf course this still makes sense ;-)\n\nLet me retitle it to \"t4060: remove unused variable\" to help readers\nof \"git shortlog --no-merges\", though, while queuing.\n\nOther two patches looked sensible to me, but I'd welcome comments by\nsubmodule users, of course ;-)\n\nThanks.\n\n> diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\n> index dc7b242697..69b9946931 100755\n> --- a/t/t4060-diff-submodule-option-diff-format.sh\n> +++ b/t/t4060-diff-submodule-option-diff-format.sh\n> @@ -361,7 +361,6 @@ test_expect_success 'typechanged submodule(submodule->blob)' '\n>  rm -f sm1 &&\n>  test_create_repo sm1 &&\n>  head6=$(add_file sm1 foo6 foo7)\n> -fullhead6=$(cd sm1; git rev-parse --verify HEAD)\n>  test_expect_success 'nonexistent commit' '\n>  \tgit diff-index -p --submodule=diff HEAD >actual &&\n>  \tcat >expected <<-EOF &&\n"}]}