{"thread":{"id":"52321","subject":"[PATCH 0/1] submodule: Fix 'submodule status' when called from a subdirectory","startedAt":"2019-11-23T06:28:58Z","lastAt":"2019-11-25T06:32:42Z","messageCount":11,"participants":["Manish Goregaokar via GitGitGadget","Junio C Hamano","Manish Goregaokar"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"386898","messageId":"pull.472.git.1574490521.gitgitgadget@gmail.com","threadId":"52321","inReplyTo":null,"subject":"[PATCH 0/1] submodule: Fix 'submodule status' when called from a subdirectory","fromName":"Manish Goregaokar via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-23T06:28:40Z","receivedAt":"2019-11-23T06:28:58Z","isPatch":true,"sender":{"key":"manishearth@gmail.com","avatar":"https://gravatar.com/avatar/023027eb83ba0eeb1774e43360d71f411f5cd9aff19ce1c954bc0459b766217a?d=mp&s=160"},"body":"Previously, when calling git submodule status while in a subdirectory, it\nwas incorrectly not detecting modified submodules and thus reporting that\nall of the submodules were unchanged.\n\nThis was because the submodule helper was calling diff-index with the\nsubmodule path assuming the path was relative to the current prefix\ndirectory, however the submodule path used is actually relative to the root.\n\nThis fixes the bug by setting the prefix to NULL when runningdiff-index from\nthe helper, so that it correctly interprets the path as relative to the\nrepository root.\n\nSigned-off-by: Manish Goregaokar manishsmail@gmail.com\n[manishsmail@gmail.com]\n\nManish Goregaokar (1):\n submodule: Fix 'submodule status' when called from a subdirectory\n\n builtin/submodule--helper.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\n\nbase-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-472%2FManishearth%2Fsubdir-status-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-472/Manishearth/subdir-status-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/472\n-- \ngitgitgadget\n"},{"id":"386899","messageId":"2004f4aaa99e5d936a3f3d141b5612df9b013f14.1574490521.git.gitgitgadget@gmail.com","threadId":"52321","inReplyTo":"pull.472.git.1574490521.gitgitgadget@gmail.com","subject":"[PATCH 1/1] submodule: Fix 'submodule status' when called from a subdirectory","fromName":"Manish Goregaokar via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-23T06:28:41Z","receivedAt":"2019-11-23T06:28:59Z","isPatch":true,"sender":{"key":"manishearth@gmail.com","avatar":"https://gravatar.com/avatar/023027eb83ba0eeb1774e43360d71f411f5cd9aff19ce1c954bc0459b766217a?d=mp&s=160"},"body":"From: Manish Goregaokar \n\nPreviously, when calling `git submodule status` while in a\nsubdirectory, it was incorrectly not detecting modified submodules and\nthus reporting that all of the submodules were unchanged.\n\nThis was because the submodule helper was calling `diff-index` with the\nsubmodule path assuming the path was relative to the current prefix\ndirectory, however the submodule path used is actually relative to the root.\n\nThis fixes the bug by setting the `prefix` to NULL when running\n`diff-index` from the helper, so that it correctly interprets the path\nas relative to the repository root.\n\nSigned-off-by: Manish Goregaokar \n---\n builtin/submodule--helper.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 909e77e802..abc5b46d46 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -802,7 +802,10 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \t\t\t path, NULL);\n \n \tgit_config(git_diff_basic_config, NULL);\n-\trepo_init_revisions(the_repository, &rev, prefix);\n+\t/*\n+\t * prefix is NULL since path is an absolute path\n+\t */\n+\trepo_init_revisions(the_repository, &rev, NULL);\n \trev.abbrev = 0;\n \tdiff_files_args.argc = setup_revisions(diff_files_args.argc,\n \t\t\t\t\t diff_files_args.argv,\n-- \ngitgitgadget\n"},{"id":"386929","messageId":"xmqqd0dh26q3.fsf@gitster-ct.c.googlers.com","threadId":"52321","inReplyTo":"2004f4aaa99e5d936a3f3d141b5612df9b013f14.1574490521.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] submodule: Fix 'submodule status' when called from a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-24T05:17:40Z","receivedAt":"2019-11-24T05:17:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Manish Goregaokar via GitGitGadget\" \nwrites:\n\n> From: Manish Goregaokar \n> Subject: [PATCH 1/1] submodule: Fix 'submodule status' when called from a subdirectory\n\nAt least, downcase 'fix' to match the project convention on commit\ntitles (you can see \"git shortlog -n 100 --no-merges\", for example).\n\n> Previously, when calling `git submodule status` while in a\n> subdirectory, it was incorrectly not detecting modified submodules and\n> thus reporting that all of the submodules were unchanged.\n\nWe do not have to \"Previously, \", as the norm for our log messages\nis to talk about the status quo (the state before applying your\npatch) in the current tense. The word used like this is fine,\nthough: \"Previously commit X changed the behaviour of feature A, but\nit was wrong for such and such reasons; let's address the issue\ncommit X wanted to change by doing this\"\n\n> This was because the submodule helper was calling `diff-index` with the\n> submodule path assuming the path was relative to the current prefix\n> directory, however the submodule path used is actually relative to the root.\n\nOK; modulo tense, nicely explained.\n\n> This fixes the bug by setting the `prefix` to NULL when running\n> `diff-index` from the helper, so that it correctly interprets the path\n> as relative to the repository root.\n\nAnd then we outline the fix as if we are giving an order to the\ncodebase \"to be like so\", in imperative mood. I.e.\n\n Always pass NULL as the `prefix` when running the diff-index\n command inside the submodule, to make sure that we see changes\n to submodules anywhere in the superproject's tree, not limited\n to the current directory.\n\nor something like that. By the way, aren't we running diff-files,\nnot diff-index, in this context?\n\n> Signed-off-by: Manish Goregaokar \n> ---\n> builtin/submodule--helper.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n\nThis fix should be protected by an additional test or two to make\nsure others won't break it again.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 909e77e802..abc5b46d46 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -802,7 +802,10 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n> \t\t\t path, NULL);\n> \n> \tgit_config(git_diff_basic_config, NULL);\n> -\trepo_init_revisions(the_repository, &rev, prefix);\n> +\t/*\n> +\t * prefix is NULL since path is an absolute path\n> +\t */\n> +\trepo_init_revisions(the_repository, &rev, NULL);\n\nAbsolute??? Isn't 'path' relative to the top-level of the\nsuperproject's working tree?\n\nIn any case, I do not think this does not deserve a multi-line\nin-code comment, once we get the code right. Explaining why this\nmust be NULL instead of prefix is better done in the commit log\nmessage.\n\nThanks.\n"},{"id":"386933","messageId":"e4c932bd0907daa53d1d721f9c9400bdad17fb62.1574582473.git.gitgitgadget@gmail.com","threadId":"52321","inReplyTo":"pull.472.v2.git.1574582473.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] submodule: fix 'submodule status' when called from a subdirectory","fromName":"Manish Goregaokar via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-24T08:01:13Z","receivedAt":"2019-11-24T08:01:19Z","isPatch":true,"sender":{"key":"manishearth@gmail.com","avatar":"https://gravatar.com/avatar/023027eb83ba0eeb1774e43360d71f411f5cd9aff19ce1c954bc0459b766217a?d=mp&s=160"},"body":"From: Manish Goregaokar \n\nWhen calling `git submodule status` while in a subdirectory, we are\nincorrectly not detecting modified submodules and\nthus reporting that all of the submodules are unchanged.\n\nThis is because the submodule helper is calling `diff-index` with the\nsubmodule path assuming the path is relative to the current prefix\ndirectory, however the submodule path used is actually relative to the root.\n\nAlways pass NULL as the `prefix` when running diff-files on the\nsubmodule, to make sure the submodule's path is interpreted as relative\nto the superproject's repository root.\n\nSigned-off-by: Manish Goregaokar \n---\n builtin/submodule--helper.c | 3 ++-\n t/t7400-submodule-basic.sh | 19 +++++++++++++++++++\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 909e77e802..eeea8dfa97 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -802,7 +802,8 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \t\t\t path, NULL);\n \n \tgit_config(git_diff_basic_config, NULL);\n-\trepo_init_revisions(the_repository, &rev, prefix);\n+\n+\trepo_init_revisions(the_repository, &rev, NULL);\n \trev.abbrev = 0;\n \tdiff_files_args.argc = setup_revisions(diff_files_args.argc,\n \t\t\t\t\t diff_files_args.argv,\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex a208cb26e1..4545b47ca4 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -356,6 +356,25 @@ test_expect_success 'status should only print one line' '\n \ttest_line_count = 1 lines\n '\n \n+test_expect_success 'status from subdirectory should have the same SHA1' '\n+\ttest_when_finished \"rmdir addtest/subdir\" &&\n+\t(\n+\t\tcd addtest &&\n+\t\tgit status > /tmp/foo &&\n+\t\tgit submodule status | awk \"{print \\$1}\" >expected &&\n+\t\tmkdir subdir &&\n+\t\tcd subdir &&\n+\t\tgit submodule status | awk \"{print \\$1}\" >../actual &&\n+\t\ttest_cmp ../expected ../actual &&\n+\t\tgit -C ../submod checkout @^ &&\n+\t\tgit submodule status | awk \"{print \\$1}\" >../actual2 &&\n+\t\tcd .. &&\n+\t\tgit submodule status | awk \"{print \\$1}\" >expected2 &&\n+\t\ttest_cmp actual2 expected2 &&\n+\t\ttest_must_fail test_cmp actual actual2\n+\t)\n+'\n+\n test_expect_success 'setup - fetch commit name from submodule' '\n \trev1=$(cd .subrepo && git rev-parse HEAD) &&\n \tprintf \"rev1: %s\\n\" \"$rev1\" &&\n-- \ngitgitgadget\n"},{"id":"386934","messageId":"pull.472.v2.git.1574582473.gitgitgadget@gmail.com","threadId":"52321","inReplyTo":"pull.472.git.1574490521.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] submodule: Fix 'submodule status' when called from a subdirectory","fromName":"Manish Goregaokar via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-24T08:01:12Z","receivedAt":"2019-11-24T08:01:19Z","isPatch":true,"sender":{"key":"manishearth@gmail.com","avatar":"https://gravatar.com/avatar/023027eb83ba0eeb1774e43360d71f411f5cd9aff19ce1c954bc0459b766217a?d=mp&s=160"},"body":"Updated to include a test and a more standard commit message.\n\nManish Goregaokar (1):\n submodule: fix 'submodule status' when called from a subdirectory\n\n builtin/submodule--helper.c | 3 ++-\n t/t7400-submodule-basic.sh | 19 +++++++++++++++++++\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\n\nbase-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-472%2FManishearth%2Fsubdir-status-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-472/Manishearth/subdir-status-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/472\n\nRange-diff vs v1:\n\n 1: 2004f4aaa9 ! 1: e4c932bd09 submodule: Fix 'submodule status' when called from a subdirectory\n @@ -1,18 +1,18 @@\n Author: Manish Goregaokar \n \n - submodule: Fix 'submodule status' when called from a subdirectory\n + submodule: fix 'submodule status' when called from a subdirectory\n \n - Previously, when calling `git submodule status` while in a\n - subdirectory, it was incorrectly not detecting modified submodules and\n - thus reporting that all of the submodules were unchanged.\n + When calling `git submodule status` while in a subdirectory, we are\n + incorrectly not detecting modified submodules and\n + thus reporting that all of the submodules are unchanged.\n \n - This was because the submodule helper was calling `diff-index` with the\n - submodule path assuming the path was relative to the current prefix\n + This is because the submodule helper is calling `diff-index` with the\n + submodule path assuming the path is relative to the current prefix\n directory, however the submodule path used is actually relative to the root.\n \n - This fixes the bug by setting the `prefix` to NULL when running\n - `diff-index` from the helper, so that it correctly interprets the path\n - as relative to the repository root.\n + Always pass NULL as the `prefix` when running diff-files on the\n + submodule, to make sure the submodule's path is interpreted as relative\n + to the superproject's repository root.\n \n Signed-off-by: Manish Goregaokar \n \n @@ -24,10 +24,38 @@\n \n \tgit_config(git_diff_basic_config, NULL);\n -\trepo_init_revisions(the_repository, &rev, prefix);\n -+\t/*\n -+\t * prefix is NULL since path is an absolute path\n -+\t */\n ++\n +\trepo_init_revisions(the_repository, &rev, NULL);\n \trev.abbrev = 0;\n \tdiff_files_args.argc = setup_revisions(diff_files_args.argc,\n \t\t\t\t\t diff_files_args.argv,\n +\n + diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n + --- a/t/t7400-submodule-basic.sh\n + +++ b/t/t7400-submodule-basic.sh\n +@@\n + \ttest_line_count = 1 lines\n + '\n + \n ++test_expect_success 'status from subdirectory should have the same SHA1' '\n ++\ttest_when_finished \"rmdir addtest/subdir\" &&\n ++\t(\n ++\t\tcd addtest &&\n ++\t\tgit status > /tmp/foo &&\n ++\t\tgit submodule status | awk \"{print \\$1}\" >expected &&\n ++\t\tmkdir subdir &&\n ++\t\tcd subdir &&\n ++\t\tgit submodule status | awk \"{print \\$1}\" >../actual &&\n ++\t\ttest_cmp ../expected ../actual &&\n ++\t\tgit -C ../submod checkout @^ &&\n ++\t\tgit submodule status | awk \"{print \\$1}\" >../actual2 &&\n ++\t\tcd .. &&\n ++\t\tgit submodule status | awk \"{print \\$1}\" >expected2 &&\n ++\t\ttest_cmp actual2 expected2 &&\n ++\t\ttest_must_fail test_cmp actual actual2\n ++\t)\n ++'\n ++\n + test_expect_success 'setup - fetch commit name from submodule' '\n + \trev1=$(cd .subrepo && git rev-parse HEAD) &&\n + \tprintf \"rev1: %s\\n\" \"$rev1\" &&\n\n-- \ngitgitgadget\n"},{"id":"386962","messageId":"xmqqfticzpke.fsf@gitster-ct.c.googlers.com","threadId":"52321","inReplyTo":"e4c932bd0907daa53d1d721f9c9400bdad17fb62.1574582473.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] submodule: fix 'submodule status' when called from a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-25T01:56:33Z","receivedAt":"2019-11-25T02:02:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Manish Goregaokar via GitGitGadget\" \nwrites:\n\n> From: Manish Goregaokar \n>\n> When calling `git submodule status` while in a subdirectory, we are\n> incorrectly not detecting modified submodules and\n> thus reporting that all of the submodules are unchanged.\n>\n> This is because the submodule helper is calling `diff-index` with the\n> submodule path assuming the path is relative to the current prefix\n> directory, however the submodule path used is actually relative to the root.\n>\n> Always pass NULL as the `prefix` when running diff-files on the\n> submodule, to make sure the submodule's path is interpreted as relative\n> to the superproject's repository root.\n>\n> Signed-off-by: Manish Goregaokar \n> ---\n> builtin/submodule--helper.c | 3 ++-\n> t/t7400-submodule-basic.sh | 19 +++++++++++++++++++\n> 2 files changed, 21 insertions(+), 1 deletion(-)\n\nThanks.\n\nWill queue as-is for now, but others may have comments on the patch\n(and certainly the test part would see a few issues pointed out),\nwhich we may want to address before this hits the 'next' and lower\nbranches.\n\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index a208cb26e1..4545b47ca4 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -356,6 +356,25 @@ test_expect_success 'status should only print one line' '\n> \ttest_line_count = 1 lines\n> '\n> \n> +test_expect_success 'status from subdirectory should have the same SHA1' '\n> +\ttest_when_finished \"rmdir addtest/subdir\" &&\n> +\t(\n> +\t\tcd addtest &&\n\n> +\t\tgit status > /tmp/foo &&\n\nI think that you added this line for debugging the test; because\nwhat it does has no effect on anything in the test, let's remove it.\n\n> +\t\tgit submodule status | awk \"{print \\$1}\" >expected &&\n\nThis construct to have \"git submodule status\" on the left hand side\nof a pipe hides its exit status. We wouldn't notice even if it\ncrashes with a segfault, which is bad especially if it does so after\nshowing the output we expect. This instance is doubly bad because\nthe output is not even compared against a known-good copy. In fact,\nthis is to create a known-good copy, so if \"git submodule status\"\ngets broken so badly that it crashes even before emitting anything,\nwe would get an empty \"expected\" file (by the way, we tend to\ncompare 'expect' and 'actual', not 'expected' and 'actual',\nespecially in our newer tests) here, which would be compared with\noutputs from other invocations of \"git submodule status\" later in\nthe test. If \"git ubmodule status\" is so broken that it crashes\nimmediately, these later invocations would die without showing any\noutput, so all the actual* files would also be empty and out\ntest_cmp would be very happy to report that all tests are good.\n\nNot so good.\n\n\tgit submodule status >output &&\n\tsed -e \"s/ .*// >expect &&\n\nperhaps?\n\n> +\t\tmkdir subdir &&\n\nIf the test fails before reaching this line for whatever reason,\naddtest/subdir directory won't exist, and test-when-finished would\nnot be so happy.\n\n> +\t\tcd subdir &&\n> +\t\tgit submodule status | awk \"{print \\$1}\" >../actual &&\n> +\t\ttest_cmp ../expected ../actual &&\n> +\t\tgit -C ../submod checkout @^ &&\n\nGahh. Please stick to human language HEAD^ and avoid line noise @^.\n\n> +\t\tgit submodule status | awk \"{print \\$1}\" >../actual2 &&\n> +\t\tcd .. &&\n> +\t\tgit submodule status | awk \"{print \\$1}\" >expected2 &&\n> +\t\ttest_cmp actual2 expected2 &&\n> +\t\ttest_must_fail test_cmp actual actual2\n\nPlease do not apply test_must_fail to anything but \"git subcmd\".\nFor now,\n\n\t! test_cmp actual actual2\n\nis a safer alternative. Right now we are cooking a topic to allow\nus to write it as\n\n\ttest_cmp ! actual actual2\n\nbut it hasn't been merged to 'master' yet. \n\n> +\t)\n> +'\n> +\n> test_expect_success 'setup - fetch commit name from submodule' '\n> \trev1=$(cd .subrepo && git rev-parse HEAD) &&\n> \tprintf \"rev1: %s\\n\" \"$rev1\" &&\n"},{"id":"386970","messageId":"CACpkpxm3=6KjTjk3WAnAXg8_SibtPPo5B08d4mnOUCsMGwv9ag@mail.gmail.com","threadId":"52321","inReplyTo":"xmqqfticzpke.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] submodule: fix 'submodule status' when called from a subdirectory","fromName":"Manish Goregaokar","fromEmail":"manishsmail@gmail.com","sentAt":"2019-11-25T04:08:41Z","receivedAt":"2019-11-25T04:11:24Z","isPatch":true,"sender":{"key":"manishsmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1617736?v=4"},"body":"I'll make the changes and submit, thanks.\n\n> sed -e \"s/ .*// >expect &&\n\nThis won't work since the output may start with a space, but I'll do\nthis as a two-step thing with awk\n\n-Manish Goregaokar\n\nOn Sun, Nov 24, 2019 at 5:56 PM Junio C Hamano wrote:\n>\n> \"Manish Goregaokar via GitGitGadget\" \n> writes:\n>\n> > From: Manish Goregaokar \n> >\n> > When calling `git submodule status` while in a subdirectory, we are\n> > incorrectly not detecting modified submodules and\n> > thus reporting that all of the submodules are unchanged.\n> >\n> > This is because the submodule helper is calling `diff-index` with the\n> > submodule path assuming the path is relative to the current prefix\n> > directory, however the submodule path used is actually relative to the root.\n> >\n> > Always pass NULL as the `prefix` when running diff-files on the\n> > submodule, to make sure the submodule's path is interpreted as relative\n> > to the superproject's repository root.\n> >\n> > Signed-off-by: Manish Goregaokar \n> > ---\n> > builtin/submodule--helper.c | 3 ++-\n> > t/t7400-submodule-basic.sh | 19 +++++++++++++++++++\n> > 2 files changed, 21 insertions(+), 1 deletion(-)\n>\n> Thanks.\n>\n> Will queue as-is for now, but others may have comments on the patch\n> (and certainly the test part would see a few issues pointed out),\n> which we may want to address before this hits the 'next' and lower\n> branches.\n>\n> > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> > index a208cb26e1..4545b47ca4 100755\n> > --- a/t/t7400-submodule-basic.sh\n> > +++ b/t/t7400-submodule-basic.sh\n> > @@ -356,6 +356,25 @@ test_expect_success 'status should only print one line' '\n> > test_line_count = 1 lines\n> > '\n> >\n> > +test_expect_success 'status from subdirectory should have the same SHA1' '\n> > + test_when_finished \"rmdir addtest/subdir\" &&\n> > + (\n> > + cd addtest &&\n>\n> > + git status > /tmp/foo &&\n>\n> I think that you added this line for debugging the test; because\n> what it does has no effect on anything in the test, let's remove it.\n>\n> > + git submodule status | awk \"{print \\$1}\" >expected &&\n>\n> This construct to have \"git submodule status\" on the left hand side\n> of a pipe hides its exit status. We wouldn't notice even if it\n> crashes with a segfault, which is bad especially if it does so after\n> showing the output we expect. This instance is doubly bad because\n> the output is not even compared against a known-good copy. In fact,\n> this is to create a known-good copy, so if \"git submodule status\"\n> gets broken so badly that it crashes even before emitting anything,\n> we would get an empty \"expected\" file (by the way, we tend to\n> compare 'expect' and 'actual', not 'expected' and 'actual',\n> especially in our newer tests) here, which would be compared with\n> outputs from other invocations of \"git submodule status\" later in\n> the test. If \"git ubmodule status\" is so broken that it crashes\n> immediately, these later invocations would die without showing any\n> output, so all the actual* files would also be empty and out\n> test_cmp would be very happy to report that all tests are good.\n>\n> Not so good.\n>\n> git submodule status >output &&\n> sed -e \"s/ .*// >expect &&\n>\n> perhaps?\n>\n> > + mkdir subdir &&\n>\n> If the test fails before reaching this line for whatever reason,\n> addtest/subdir directory won't exist, and test-when-finished would\n> not be so happy.\n>\n> > + cd subdir &&\n> > + git submodule status | awk \"{print \\$1}\" >../actual &&\n> > + test_cmp ../expected ../actual &&\n> > + git -C ../submod checkout @^ &&\n>\n> Gahh. Please stick to human language HEAD^ and avoid line noise @^.\n>\n> > + git submodule status | awk \"{print \\$1}\" >../actual2 &&\n> > + cd .. &&\n> > + git submodule status | awk \"{print \\$1}\" >expected2 &&\n> > + test_cmp actual2 expected2 &&\n> > + test_must_fail test_cmp actual actual2\n>\n> Please do not apply test_must_fail to anything but \"git subcmd\".\n> For now,\n>\n> ! test_cmp actual actual2\n>\n> is a safer alternative. Right now we are cooking a topic to allow\n> us to write it as\n>\n> test_cmp ! actual actual2\n>\n> but it hasn't been merged to 'master' yet.\n>\n> > + )\n> > +'\n> > +\n> > test_expect_success 'setup - fetch commit name from submodule' '\n> > rev1=$(cd .subrepo && git rev-parse HEAD) &&\n> > printf \"rev1: %s\\n\" \"$rev1\" &&\n"},{"id":"386971","messageId":"pull.472.v3.git.1574655344.gitgitgadget@gmail.com","threadId":"52321","inReplyTo":"pull.472.v2.git.1574582473.gitgitgadget@gmail.com","subject":"[PATCH v3 0/1] submodule: Fix 'submodule status' when called from a subdirectory","fromName":"Manish Goregaokar via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-25T04:15:43Z","receivedAt":"2019-11-25T04:15:52Z","isPatch":true,"sender":{"key":"manishearth@gmail.com","avatar":"https://gravatar.com/avatar/023027eb83ba0eeb1774e43360d71f411f5cd9aff19ce1c954bc0459b766217a?d=mp&s=160"},"body":"Updated with an improved test.\n\nManish Goregaokar (1):\n submodule: fix 'submodule status' when called from a subdirectory\n\n builtin/submodule--helper.c | 3 ++-\n t/t7400-submodule-basic.sh | 22 ++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\n\nbase-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-472%2FManishearth%2Fsubdir-status-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-472/Manishearth/subdir-status-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/472\n\nRange-diff vs v2:\n\n 1: e4c932bd09 ! 1: 8dcea6b399 submodule: fix 'submodule status' when called from a subdirectory\n @@ -41,18 +41,21 @@\n +\ttest_when_finished \"rmdir addtest/subdir\" &&\n +\t(\n +\t\tcd addtest &&\n -+\t\tgit status > /tmp/foo &&\n -+\t\tgit submodule status | awk \"{print \\$1}\" >expected &&\n +\t\tmkdir subdir &&\n ++\t\tgit submodule status >output &&\n ++\t\tawk \"{print \\$1}\" expect &&\n +\t\tcd subdir &&\n -+\t\tgit submodule status | awk \"{print \\$1}\" >../actual &&\n -+\t\ttest_cmp ../expected ../actual &&\n -+\t\tgit -C ../submod checkout @^ &&\n -+\t\tgit submodule status | awk \"{print \\$1}\" >../actual2 &&\n ++\t\tgit submodule status >../output &&\n ++\t\tawk \"{print \\$1}\" <../output >../actual &&\n ++\t\ttest_cmp ../expect ../actual &&\n ++\t\tgit -C ../submod checkout HEAD^ &&\n ++\t\tgit submodule status >../output &&\n ++\t\tawk \"{print \\$1}\" <../output >../actual2 &&\n +\t\tcd .. &&\n -+\t\tgit submodule status | awk \"{print \\$1}\" >expected2 &&\n -+\t\ttest_cmp actual2 expected2 &&\n -+\t\ttest_must_fail test_cmp actual actual2\n ++\t\tgit submodule status >output &&\n ++\t\tawk \"{print \\$1}\" expect2 &&\n ++\t\ttest_cmp actual2 expect2 &&\n ++\t\t! test_cmp actual actual2\n +\t)\n +'\n +\n\n-- \ngitgitgadget\n"},{"id":"386972","messageId":"8dcea6b399923e931625ef09d66661e93975b93a.1574655344.git.gitgitgadget@gmail.com","threadId":"52321","inReplyTo":"pull.472.v3.git.1574655344.gitgitgadget@gmail.com","subject":"[PATCH v3 1/1] submodule: fix 'submodule status' when called from a subdirectory","fromName":"Manish Goregaokar via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-25T04:15:44Z","receivedAt":"2019-11-25T04:15:53Z","isPatch":true,"sender":{"key":"manishearth@gmail.com","avatar":"https://gravatar.com/avatar/023027eb83ba0eeb1774e43360d71f411f5cd9aff19ce1c954bc0459b766217a?d=mp&s=160"},"body":"From: Manish Goregaokar \n\nWhen calling `git submodule status` while in a subdirectory, we are\nincorrectly not detecting modified submodules and\nthus reporting that all of the submodules are unchanged.\n\nThis is because the submodule helper is calling `diff-index` with the\nsubmodule path assuming the path is relative to the current prefix\ndirectory, however the submodule path used is actually relative to the root.\n\nAlways pass NULL as the `prefix` when running diff-files on the\nsubmodule, to make sure the submodule's path is interpreted as relative\nto the superproject's repository root.\n\nSigned-off-by: Manish Goregaokar \n---\n builtin/submodule--helper.c | 3 ++-\n t/t7400-submodule-basic.sh | 22 ++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 909e77e802..eeea8dfa97 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -802,7 +802,8 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \t\t\t path, NULL);\n \n \tgit_config(git_diff_basic_config, NULL);\n-\trepo_init_revisions(the_repository, &rev, prefix);\n+\n+\trepo_init_revisions(the_repository, &rev, NULL);\n \trev.abbrev = 0;\n \tdiff_files_args.argc = setup_revisions(diff_files_args.argc,\n \t\t\t\t\t diff_files_args.argv,\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex a208cb26e1..09d8408f88 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -356,6 +356,28 @@ test_expect_success 'status should only print one line' '\n \ttest_line_count = 1 lines\n '\n \n+test_expect_success 'status from subdirectory should have the same SHA1' '\n+\ttest_when_finished \"rmdir addtest/subdir\" &&\n+\t(\n+\t\tcd addtest &&\n+\t\tmkdir subdir &&\n+\t\tgit submodule status >output &&\n+\t\tawk \"{print \\$1}\" expect &&\n+\t\tcd subdir &&\n+\t\tgit submodule status >../output &&\n+\t\tawk \"{print \\$1}\" <../output >../actual &&\n+\t\ttest_cmp ../expect ../actual &&\n+\t\tgit -C ../submod checkout HEAD^ &&\n+\t\tgit submodule status >../output &&\n+\t\tawk \"{print \\$1}\" <../output >../actual2 &&\n+\t\tcd .. &&\n+\t\tgit submodule status >output &&\n+\t\tawk \"{print \\$1}\" expect2 &&\n+\t\ttest_cmp actual2 expect2 &&\n+\t\t! test_cmp actual actual2\n+\t)\n+'\n+\n test_expect_success 'setup - fetch commit name from submodule' '\n \trev1=$(cd .subrepo && git rev-parse HEAD) &&\n \tprintf \"rev1: %s\\n\" \"$rev1\" &&\n-- \ngitgitgadget\n"},{"id":"386973","messageId":"xmqqmucky2wj.fsf@gitster-ct.c.googlers.com","threadId":"52321","inReplyTo":"8dcea6b399923e931625ef09d66661e93975b93a.1574655344.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/1] submodule: fix 'submodule status' when called from a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-25T04:51:24Z","receivedAt":"2019-11-25T04:51:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Manish Goregaokar via GitGitGadget\" \nwrites:\n\n> +\t\ttest_cmp ../expect ../actual &&\n\nNote that \"test_cmp expect actual\" highlights a breakage, when it\nfinds one, by showing a diff that shows \"what we saw is different\nfrom what we expected to see in this way\". So this invocation is\ngood.\n\n> +\t\tgit -C ../submod checkout HEAD^ &&\n> +\t\tgit submodule status >../output &&\n> +\t\tawk \"{print \\$1}\" <../output >../actual2 &&\n> +\t\tcd .. &&\n> +\t\tgit submodule status >output &&\n> +\t\tawk \"{print \\$1}\" expect2 &&\n> +\t\ttest_cmp actual2 expect2 &&\n\nBut this one is probably the other way around. I'll fix this up\nwhile queuing, so if there is no other changes necessary, there is\nno need to resend the patch.\n\n> +\t\t! test_cmp actual actual2\n\nAnd the order of this one actually does not matter ;-)\n\n> +\t)\n> +'\n\nThanks.\n"},{"id":"386976","messageId":"CACpkpxmFK61CgOS0M_2RV7BMT1Pk6Q2XdWYzdf5LVbAvd1rnXQ@mail.gmail.com","threadId":"52321","inReplyTo":"xmqqmucky2wj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/1] submodule: fix 'submodule status' when called from a subdirectory","fromName":"Manish Goregaokar","fromEmail":"manishsmail@gmail.com","sentAt":"2019-11-25T06:32:20Z","receivedAt":"2019-11-25T06:32:42Z","isPatch":true,"sender":{"key":"manishsmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1617736?v=4"},"body":"Thank you!\n-Manish Goregaokar\n\nOn Sun, Nov 24, 2019 at 8:51 PM Junio C Hamano wrote:\n>\n> \"Manish Goregaokar via GitGitGadget\" \n> writes:\n>\n> > + test_cmp ../expect ../actual &&\n>\n> Note that \"test_cmp expect actual\" highlights a breakage, when it\n> finds one, by showing a diff that shows \"what we saw is different\n> from what we expected to see in this way\". So this invocation is\n> good.\n>\n> > + git -C ../submod checkout HEAD^ &&\n> > + git submodule status >../output &&\n> > + awk \"{print \\$1}\" <../output >../actual2 &&\n> > + cd .. &&\n> > + git submodule status >output &&\n> > + awk \"{print \\$1}\" expect2 &&\n> > + test_cmp actual2 expect2 &&\n>\n> But this one is probably the other way around. I'll fix this up\n> while queuing, so if there is no other changes necessary, there is\n> no need to resend the patch.\n>\n> > + ! test_cmp actual actual2\n>\n> And the order of this one actually does not matter ;-)\n>\n> > + )\n> > +'\n>\n> Thanks.\n"}]}