{"thread":{"id":"52633","subject":"[PATCH] Fix submodule status of dirs initialized but empty","startedAt":"2020-01-14T14:02:24Z","lastAt":"2020-02-02T23:33:13Z","messageCount":16,"participants":["Peter Kästle","Peter Kaestle","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"389744","messageId":"23e3d410-5271-a919-aa03-9e776cf72f7c@nokia.com","threadId":"52633","inReplyTo":null,"subject":"[PATCH] Fix submodule status of dirs initialized but empty","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-01-14T14:02:16Z","receivedAt":"2020-01-14T14:02:24Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Fix submodule status of dirs initialized but empty\n\nOriginal bash helper for \"submodule status\" was doing a check for\ninitialized but not cloned submodules and prefixed them with \"-\" in case \n.git\nwas missing inside the submodule directory.\nThis check was skipped when originally porting the functionality\nfrom bash to C.\n\nSigned-off-by: Peter Kaestle \n---\n builtin/submodule--helper.c | 15 +++++++++++++--\n 1 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c72931e..50bb692 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -782,6 +782,9 @@ static void status_submodule(const char *path, const \nstruct object_id *ce_oid,\n \tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n \tstruct rev_info rev;\n \tint diff_files_result;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *git_dir;\n+\n \tif (!submodule_from_path(the_repository, &null_oid, path))\n \t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n@@ -793,11 +796,19 @@ static void status_submodule(const char *path, \nconst struct object_id *ce_oid,\n \t\tprint_status(flags, 'U', path, &null_oid, displaypath);\n \t\tgoto cleanup;\n \t}\n-\n-\tif (!is_submodule_active(the_repository, path)) {\n+\t\n+\tstrbuf_addf(&buf, \"%s/.git\", path);\n+\tgit_dir = read_gitfile(buf.buf);\n+\tif (!git_dir)\n+\t\tgit_dir = buf.buf;\n+\n+\tif (!is_submodule_active(the_repository, path) ||\n+\t\t\t!is_git_directory(git_dir)) {\n \t\tprint_status(flags, '-', path, ce_oid, displaypath);\n+\t\tstrbuf_release(&buf);\n \t\tgoto cleanup;\n \t}\n+\tstrbuf_release(&buf);\n \targv_array_pushl(&diff_files_args, \"diff-files\",\n \t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n-- \n1.7.1\n\n"},{"id":"390163","messageId":"1579601532-10694-1-git-send-email-peter.kaestle@nokia.com","threadId":"52633","inReplyTo":"23e3d410-5271-a919-aa03-9e776cf72f7c@nokia.com","subject":"[PATCH v2] Fix status of initialized but not cloned submodules","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-01-21T10:12:12Z","receivedAt":"2020-01-21T10:13:07Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Original bash helper for \"submodule status\" was doing a check for\ninitialized but not cloned submodules and prefixed the status with\na minus sign in case no .git file or folder was found inside the\nsubmodule directory.\nThis check was missed when the original port of the functionality\nfrom bash to C was done.\n\nSigned-off-by: Peter Kaestle \n---\n builtin/submodule--helper.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c72931e..c04241b 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -782,6 +782,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n \tstruct rev_info rev;\n \tint diff_files_result;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *git_dir;\n+\n \n \tif (!submodule_from_path(the_repository, &null_oid, path))\n \t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n@@ -794,10 +797,18 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \t\tgoto cleanup;\n \t}\n \n-\tif (!is_submodule_active(the_repository, path)) {\n+\tstrbuf_addf(&buf, \"%s/.git\", path);\n+\tgit_dir = read_gitfile(buf.buf);\n+\tif (!git_dir)\n+\t\tgit_dir = buf.buf;\n+\n+\tif (!is_submodule_active(the_repository, path) ||\n+\t\t\t!is_git_directory(git_dir)) {\n \t\tprint_status(flags, '-', path, ce_oid, displaypath);\n+\t\tstrbuf_release(&buf);\n \t\tgoto cleanup;\n \t}\n+\tstrbuf_release(&buf);\n \n \targv_array_pushl(&diff_files_args, \"diff-files\",\n \t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n-- \n2.6.2\n\n"},{"id":"390306","messageId":"xmqq4kwl512y.fsf@gitster-ct.c.googlers.com","threadId":"52633","inReplyTo":"1579601532-10694-1-git-send-email-peter.kaestle@nokia.com","subject":"Re: [PATCH v2] Fix status of initialized but not cloned submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-23T21:12:37Z","receivedAt":"2020-01-23T21:12:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Kaestle writes:\n\n> Original bash helper for \"submodule status\" was doing a check for\n> initialized but not cloned submodules and prefixed the status with\n> a minus sign in case no .git file or folder was found inside the\n> submodule directory.\n> This check was missed when the original port of the functionality\n> from bash to C was done.\n>\n> Signed-off-by: Peter Kaestle \n> ---\n> builtin/submodule--helper.c | 13 ++++++++++++-\n> 1 file changed, 12 insertions(+), 1 deletion(-)\n\nI wonder if this is easily protectable with an additional test.\n\nIn general, do we have a good coverage of \"git status\" output that\ninvolves submodules in various states?\n\nThanks.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index c72931e..c04241b 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -782,6 +782,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n> \tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n> \tstruct rev_info rev;\n> \tint diff_files_result;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tconst char *git_dir;\n> +\n> \n> \tif (!submodule_from_path(the_repository, &null_oid, path))\n> \t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n> @@ -794,10 +797,18 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n> \t\tgoto cleanup;\n> \t}\n> \n> -\tif (!is_submodule_active(the_repository, path)) {\n> +\tstrbuf_addf(&buf, \"%s/.git\", path);\n> +\tgit_dir = read_gitfile(buf.buf);\n> +\tif (!git_dir)\n> +\t\tgit_dir = buf.buf;\n> +\n> +\tif (!is_submodule_active(the_repository, path) ||\n> +\t\t\t!is_git_directory(git_dir)) {\n> \t\tprint_status(flags, '-', path, ce_oid, displaypath);\n> +\t\tstrbuf_release(&buf);\n> \t\tgoto cleanup;\n> \t}\n> +\tstrbuf_release(&buf);\n> \n> \targv_array_pushl(&diff_files_args, \"diff-files\",\n> \t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n"},{"id":"390354","messageId":"1579862044-29847-1-git-send-email-peter.kaestle@nokia.com","threadId":"52633","inReplyTo":"xmqq4kwl512y.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v3 1/2] Testcase for submodule status on empty dirs","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-01-24T10:34:03Z","receivedAt":"2020-01-24T10:34:44Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Test that submodule status reports initialized but not cloned\nsubmodules as missing.\n\nSigned-off-by: Peter Kaestle \n---\n t/t7400-submodule-basic.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 7f75bb1..2e84914 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -399,6 +399,14 @@ test_expect_success 'init should register submodule url in .git/config' '\n \ttest_cmp expect url\n '\n \n+test_expect_success 'status should still be \"missing\" after initializing' '\n+\trm -fr init &&\n+\tmkdir init &&\n+\tgit submodule status >lines &&\n+\trm -fr init &&\n+\tgrep \"^-$rev1\" lines\n+'\n+\n test_failure_with_unknown_submodule () {\n \ttest_must_fail git submodule $1 no-such-submodule 2>output.err &&\n \ttest_i18ngrep \"^error: .*no-such-submodule\" output.err\n-- \n2.6.2\n\n"},{"id":"390355","messageId":"1579862044-29847-2-git-send-email-peter.kaestle@nokia.com","threadId":"52633","inReplyTo":"1579862044-29847-1-git-send-email-peter.kaestle@nokia.com","subject":"[PATCH v3 2/2] Fix status of initialized but not cloned submodules","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-01-24T10:34:04Z","receivedAt":"2020-01-24T10:34:44Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Original bash helper for \"submodule status\" was doing a check for\ninitialized but not cloned submodules and prefixed the status with\na minus sign in case no .git file or folder was found inside the\nsubmodule directory.\nThis check was missed when the original port of the functionality\nfrom bash to C was done.\n\nSigned-off-by: Peter Kaestle \n---\n builtin/submodule--helper.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c72931e..c04241b 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -782,6 +782,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n \tstruct rev_info rev;\n \tint diff_files_result;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *git_dir;\n+\n \n \tif (!submodule_from_path(the_repository, &null_oid, path))\n \t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n@@ -794,10 +797,18 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \t\tgoto cleanup;\n \t}\n \n-\tif (!is_submodule_active(the_repository, path)) {\n+\tstrbuf_addf(&buf, \"%s/.git\", path);\n+\tgit_dir = read_gitfile(buf.buf);\n+\tif (!git_dir)\n+\t\tgit_dir = buf.buf;\n+\n+\tif (!is_submodule_active(the_repository, path) ||\n+\t\t\t!is_git_directory(git_dir)) {\n \t\tprint_status(flags, '-', path, ce_oid, displaypath);\n+\t\tstrbuf_release(&buf);\n \t\tgoto cleanup;\n \t}\n+\tstrbuf_release(&buf);\n \n \targv_array_pushl(&diff_files_args, \"diff-files\",\n \t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n-- \n2.6.2\n\n"},{"id":"390356","messageId":"9192695a-0b3f-d166-7efa-84f57e091c2c@nokia.com","threadId":"52633","inReplyTo":"xmqq4kwl512y.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] Fix status of initialized but not cloned submodules","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-01-24T11:55:25Z","receivedAt":"2020-01-24T11:55:33Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"On 23.01.20 22:12, Junio C Hamano wrote:\n> Peter Kaestle writes:\n> \n>> Original bash helper for \"submodule status\" was doing a check for\n>> initialized but not cloned submodules and prefixed the status with\n>> a minus sign in case no .git file or folder was found inside the\n>> submodule directory.\n>> This check was missed when the original port of the functionality\n>> from bash to C was done.\n>>\n>> Signed-off-by: Peter Kaestle \n>> ---\n>> builtin/submodule--helper.c | 13 ++++++++++++-\n>> 1 file changed, 12 insertions(+), 1 deletion(-)\n> \n> I wonder if this is easily protectable with an additional test.\n\nYes, please find it inside patch series v3.\n\n\n> In general, do we have a good coverage of \"git status\" output that\n> involves submodules in various states?\n\nOriginally, without applying the v3 series, I can see those testcases:\n\n1) not-init, not-cloned: 'status should initially be \"missing\"'\n2) init, not-cloned: MISSING\n3) not-init, cloned: MISSING\n4) init, cloned: 'status should be \"up-to-date\" after update'\n4.1) + modified: 'status should be \"modified\" after submodule commit'\n4.2) + modified, committed: 'status should be \"up-to-date\" after update'\n\nMy patch v3 series adds a testcase for 2).\nIs 3) even a possible use-case?\n\nBesides that, I think testcases for man git-submodule's description of \nstatus on merge conflicts are missing (...[prefix] U if the submodule \nhas merge conflicts).\nAt least, I didn't find any when searching the tests folder with:\n$> grep 'grep \"^U' *.sh\n\n> \n> Thanks.\n> \n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index c72931e..c04241b 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -782,6 +782,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n>> \tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n>> \tstruct rev_info rev;\n>> \tint diff_files_result;\n>> +\tstruct strbuf buf = STRBUF_INIT;\n>> +\tconst char *git_dir;\n>> +\n>> \n>> \tif (!submodule_from_path(the_repository, &null_oid, path))\n>> \t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n>> @@ -794,10 +797,18 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n>> \t\tgoto cleanup;\n>> \t}\n>> \n>> -\tif (!is_submodule_active(the_repository, path)) {\n>> +\tstrbuf_addf(&buf, \"%s/.git\", path);\n>> +\tgit_dir = read_gitfile(buf.buf);\n>> +\tif (!git_dir)\n>> +\t\tgit_dir = buf.buf;\n>> +\n>> +\tif (!is_submodule_active(the_repository, path) ||\n>> +\t\t\t!is_git_directory(git_dir)) {\n>> \t\tprint_status(flags, '-', path, ce_oid, displaypath);\n>> +\t\tstrbuf_release(&buf);\n>> \t\tgoto cleanup;\n>> \t}\n>> +\tstrbuf_release(&buf);\n>> \n>> \targv_array_pushl(&diff_files_args, \"diff-files\",\n>> \t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n"},{"id":"390369","messageId":"xmqqblqs3hbv.fsf@gitster-ct.c.googlers.com","threadId":"52633","inReplyTo":"9192695a-0b3f-d166-7efa-84f57e091c2c@nokia.com","subject":"Re: [PATCH v2] Fix status of initialized but not cloned submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-24T17:16:52Z","receivedAt":"2020-01-24T17:16:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Kästle writes:\n\n> Originally, without applying the v3 series, I can see those testcases:\n>\n> 1) not-init, not-cloned: 'status should initially be \"missing\"'\n> 2) init, not-cloned: MISSING\n> 3) not-init, cloned: MISSING\n> 4) init, cloned: 'status should be \"up-to-date\" after update'\n> 4.1) + modified: 'status should be \"modified\" after submodule commit'\n> 4.2) + modified, committed: 'status should be \"up-to-date\" after update'\n>\n> My patch v3 series adds a testcase for 2).\n\nThat's good.\n\n> Is 3) even a possible use-case?\n\nThat would be somebody who is about to add a submodule to an\nexisting project, right? You have a top-level project, you need\nsomebody else's project as its submodule or you need to use a\nlibrary in your top-level project and you develop the library\nyourself, but want to keep that library part separate from its first\nuser which is your top-level project. So in the working tree of a\ntop-level project you either \"git init lib/\" and start the submodule\nproject right there, or \"git clone $URL_of_upstream_of_lib lib/\",\nand then plan to do \"git submodule add lib\", but haven't done so\nyet. .gitmodules does not know about it, but the directory is\nalready populated and is a valid repository.\n\n> Besides that, I think testcases for man git-submodule's description of\n> status on merge conflicts are missing (...[prefix] U if the submodule\n> has merge conflicts).\n\nNice to know. Thanks.\n"},{"id":"390370","messageId":"xmqq7e1g3ggd.fsf@gitster-ct.c.googlers.com","threadId":"52633","inReplyTo":"1579862044-29847-1-git-send-email-peter.kaestle@nokia.com","subject":"Re: [PATCH v3 1/2] Testcase for submodule status on empty dirs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-24T17:35:46Z","receivedAt":"2020-01-24T17:35:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Kaestle writes:\n\n> Subject: Re: [PATCH v3 1/2] Testcase for submodule status on empty dirs\n>\n> Test that submodule status reports initialized but not cloned\n> submodules as missing.\n\nLet's not waste the findings you made and the description you wrote\nduring the review of the previous round. After all, the reason why\nreviewers ask questions and you write responses is not because we\nwant to satisify curiosity of the reviewers, but to polish the\nhistory we leave to the future readers, both in the patch text and\nits explanations.\n\nHow about explaining this step more like this?\n\n Subject: t7400: add a testcase for submodule status on empty dirs\n\n We have test coverage for \"git submodule status\" output in\n various cases, i.e.\n\n 1) not-init, not-cloned: status should initially be \"missing\"\n 2) init, not-cloned: status should be \"missing\"\n 3) not-init, cloned:\n 4) init, cloned:'status should be \"up-to-date\" after update\n 4.1) + modified: status should be \"modified\" after submodule commit\n 4.2) + modified, committed: status should be \"up-to-date\" after update\n\n but the cases 2) and 3) are not covered.\n\n Test that submodule status reports initialized but not cloned\n submodules as missing to fill the gap in test coverage.\n\nThanks.\n\n\n>\n> Signed-off-by: Peter Kaestle \n> ---\n> t/t7400-submodule-basic.sh | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n>\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 7f75bb1..2e84914 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -399,6 +399,14 @@ test_expect_success 'init should register submodule url in .git/config' '\n> \ttest_cmp expect url\n> '\n> \n> +test_expect_success 'status should still be \"missing\" after initializing' '\n> +\trm -fr init &&\n> +\tmkdir init &&\n> +\tgit submodule status >lines &&\n> +\trm -fr init &&\n> +\tgrep \"^-$rev1\" lines\n> +'\n> +\n> test_failure_with_unknown_submodule () {\n> \ttest_must_fail git submodule $1 no-such-submodule 2>output.err &&\n> \ttest_i18ngrep \"^error: .*no-such-submodule\" output.err\n"},{"id":"390545","messageId":"41a835a3-3809-eda8-fb3c-30fe9fac1a24@nokia.com","threadId":"52633","inReplyTo":"xmqqblqs3hbv.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] Fix status of initialized but not cloned submodules","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-01-27T08:52:21Z","receivedAt":"2020-01-27T08:52:29Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Hi,\n\nOn 24.01.20 18:16, Junio C Hamano wrote:\n> Peter Kästle writes:\n\n[...]\n\n>> Is 3) even a possible use-case?\n> \n> That would be somebody who is about to add a submodule to an\n> existing project, right? You have a top-level project, you need\n> somebody else's project as its submodule or you need to use a\n> library in your top-level project and you develop the library\n> yourself, but want to keep that library part separate from its first\n> user which is your top-level project. So in the working tree of a\n> top-level project you either \"git init lib/\" and start the submodule\n> project right there, or \"git clone $URL_of_upstream_of_lib lib/\",\n> and then plan to do \"git submodule add lib\", but haven't done so\n> yet. .gitmodules does not know about it, but the directory is\n> already populated and is a valid repository.\n\nYou are right, this is a possible scenario for 3). However the question \nabout how the outer git repo should behave is still open to me. I mean, \nthe outer git repo should treat the inner one as standard file structure \nand not as a submodule until the user explicitly calls the \"git \nsubmodule add\" command. And thus \"git submodule status\" should \ncompletely skip it. Calling \"git status\" in the outer repo should list \nthe inner one as \"untracked\" or?\nAt least this is, what I would expect.\nSo from my point of view the only test for 3) we could do is something \nlike this:\nmkdir innerfoo &&\n(cd innerfoo &&\ngit init) &&\ntest_must_fail git submodule status | grep \"innerfoo\"\n\n\n>> Besides that, I think testcases for man git-submodule's description of\n>> status on merge conflicts are missing (...[prefix] U if the submodule\n>> has merge conflicts).\n> \n> Nice to know. Thanks.\n\nMaybe I'll have spare time next weekend to create some. Not to promise \nanything, but sounds like fun.\n\n-- \nbest regards,\n--peter;\n"},{"id":"390546","messageId":"1580115989-32649-2-git-send-email-peter.kaestle@nokia.com","threadId":"52633","inReplyTo":"1580115989-32649-1-git-send-email-peter.kaestle@nokia.com","subject":"[PATCH v4 2/2] Fix status of initialized but not cloned submodules","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-01-27T09:06:29Z","receivedAt":"2020-01-27T09:06:58Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Original bash helper for \"submodule status\" was doing a check for\ninitialized but not cloned submodules and prefixed the status with\na minus sign in case no .git file or folder was found inside the\nsubmodule directory.\nThis check was missed when the original port of the functionality\nfrom bash to C was done.\n\nSigned-off-by: Peter Kaestle \n---\n builtin/submodule--helper.c | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c72931e..4031cf4 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -782,6 +782,8 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \tstruct argv_array diff_files_args = ARGV_ARRAY_INIT;\n \tstruct rev_info rev;\n \tint diff_files_result;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *git_dir;\n \n \tif (!submodule_from_path(the_repository, &null_oid, path))\n \t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n@@ -794,10 +796,18 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \t\tgoto cleanup;\n \t}\n \n-\tif (!is_submodule_active(the_repository, path)) {\n+\tstrbuf_addf(&buf, \"%s/.git\", path);\n+\tgit_dir = read_gitfile(buf.buf);\n+\tif (!git_dir)\n+\t\tgit_dir = buf.buf;\n+\n+\tif (!is_submodule_active(the_repository, path) ||\n+\t\t\t!is_git_directory(git_dir)) {\n \t\tprint_status(flags, '-', path, ce_oid, displaypath);\n+\t\tstrbuf_release(&buf);\n \t\tgoto cleanup;\n \t}\n+\tstrbuf_release(&buf);\n \n \targv_array_pushl(&diff_files_args, \"diff-files\",\n \t\t\t \"--ignore-submodules=dirty\", \"--quiet\", \"--\",\n-- \n2.6.2\n\n"},{"id":"390548","messageId":"1580115989-32649-1-git-send-email-peter.kaestle@nokia.com","threadId":"52633","inReplyTo":"xmqq7e1g3ggd.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v4 1/2] t7400: add a testcase for submodule status on empty dirs","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-01-27T09:06:28Z","receivedAt":"2020-01-27T09:25:59Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"We have test coverage for \"git submodule status\" output in\nvarious cases, i.e.\n\n 1) not-init, not-cloned: status should initially be \"missing\"\n 2) init, not-cloned: status should be \"missing\"\n 3) not-init, cloned:\n 4) init, cloned:'status should be \"up-to-date\" after update\n 4.1) + modified: status should be \"modified\" after submodule commit\n 4.2) + modified, committed: status should be \"up-to-date\" after update\n\nCase 2) will be covered by this patch. Case 3) remains uncovered.\n\nTest that submodule status reports initialized but not cloned\nsubmodules as missing to fill the gap in test coverage.\n\nSigned-off-by: Peter Kaestle \n---\n t/t7400-submodule-basic.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 7f75bb1..2e84914 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -399,6 +399,14 @@ test_expect_success 'init should register submodule url in .git/config' '\n \ttest_cmp expect url\n '\n \n+test_expect_success 'status should still be \"missing\" after initializing' '\n+\trm -fr init &&\n+\tmkdir init &&\n+\tgit submodule status >lines &&\n+\trm -fr init &&\n+\tgrep \"^-$rev1\" lines\n+'\n+\n test_failure_with_unknown_submodule () {\n \ttest_must_fail git submodule $1 no-such-submodule 2>output.err &&\n \ttest_i18ngrep \"^error: .*no-such-submodule\" output.err\n-- \n2.6.2\n\n"},{"id":"390569","messageId":"xmqqzhe8bwab.fsf@gitster-ct.c.googlers.com","threadId":"52633","inReplyTo":"1580115989-32649-1-git-send-email-peter.kaestle@nokia.com","subject":"Re: [PATCH v4 1/2] t7400: add a testcase for submodule status on empty dirs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-27T18:15:40Z","receivedAt":"2020-01-27T18:15:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Kaestle writes:\n\n> We have test coverage for \"git submodule status\" output in\n> various cases, i.e.\n>\n> 1) not-init, not-cloned: status should initially be \"missing\"\n> 2) init, not-cloned: status should be \"missing\"\n> 3) not-init, cloned:\n> 4) init, cloned:'status should be \"up-to-date\" after update\n> 4.1) + modified: status should be \"modified\" after submodule commit\n> 4.2) + modified, committed: status should be \"up-to-date\" after update\n>\n> Case 2) will be covered by this patch. Case 3) remains uncovered.\n>\n> Test that submodule status reports initialized but not cloned\n> submodules as missing to fill the gap in test coverage.\n\nThanks for an update.\n"},{"id":"390570","messageId":"xmqqv9owbw5o.fsf@gitster-ct.c.googlers.com","threadId":"52633","inReplyTo":"41a835a3-3809-eda8-fb3c-30fe9fac1a24@nokia.com","subject":"Re: [PATCH v2] Fix status of initialized but not cloned submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-27T18:18:27Z","receivedAt":"2020-01-27T18:18:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Kästle writes:\n\n> ... I mean, the outer git repo should treat the inner one as standard\n> file structure and not as a submodule until the user explicitly calls\n> the \"git submodule add\" command. And thus \"git submodule status\"\n> should completely skip it. Calling \"git status\" in the outer repo\n> should list the inner one as \"untracked\" or?\n\nI agree that is a sensible expectation.\n\n"},{"id":"390615","messageId":"2181f9d5-7556-90e8-14a5-0ca86f2c2c97@nokia.com","threadId":"52633","inReplyTo":"xmqqzhe8bwab.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4 1/2] t7400: add a testcase for submodule status on empty dirs","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-01-28T08:16:39Z","receivedAt":"2020-01-28T08:16:47Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Hi Junio,\n\nOn 27.01.20 19:15, Junio C Hamano wrote:\n> Peter Kaestle writes:\n> \n>> We have test coverage for \"git submodule status\" output in\n>> various cases, i.e.\n>>\n>> 1) not-init, not-cloned: status should initially be \"missing\"\n>> 2) init, not-cloned: status should be \"missing\"\n>> 3) not-init, cloned:\n>> 4) init, cloned:'status should be \"up-to-date\" after update\n>> 4.1) + modified: status should be \"modified\" after submodule commit\n>> 4.2) + modified, committed: status should be \"up-to-date\" after update\n>>\n>> Case 2) will be covered by this patch. Case 3) remains uncovered.\n>>\n>> Test that submodule status reports initialized but not cloned\n>> submodules as missing to fill the gap in test coverage.\n> \n> Thanks for an update.\n> \n\nSure, welcome. Will you include those patches, or do you need further \nrefinement?\n- Thanks a lot for your guidance and mentoring. It was a pleasure to \nlearn from you.\n\n-- \nkind regards,\n--peter;\n"},{"id":"391015","messageId":"0a541856-09c4-4549-713f-3d65b685129d@piie.net","threadId":"52633","inReplyTo":"xmqqblqs3hbv.fsf@gitster-ct.c.googlers.com","subject":"Re: Re: [PATCH v2] Fix status of initialized but not cloned submodules","fromName":"Peter Kästle","fromEmail":"peter@piie.net","sentAt":"2020-02-02T23:16:38Z","receivedAt":"2020-02-02T23:25:05Z","isPatch":true,"sender":{"key":"peter@piie.net","avatar":null},"body":"On 24.01.20 18:16, Junio C Hamano wrote:\n> Peter Kästle writes:\n>> Besides that, I think testcases for man git-submodule's description of\n>> status on merge conflicts are missing (...[prefix] U if the submodule\n>> has merge conflicts).\n> \n> Nice to know. Thanks.\n\nI was wrong. Testcases for \"submodule status\" to add prefix U in case \nof submodules with merge conflicts do exist in: t7405-submodule-merge.sh\nThe two tests are called:\ngit submodule status should display the merge conflict properly with[..]\n\n-- \n--peter;\n"},{"id":"391016","messageId":"20200202233244.199040-1-peter@piie.net","threadId":"52633","inReplyTo":"xmqqzhe8bwab.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] t7400: testcase for submodule status on unregistered inner git repos","fromName":"Peter Kaestle","fromEmail":"peter@piie.net","sentAt":"2020-02-02T23:32:44Z","receivedAt":"2020-02-02T23:33:13Z","isPatch":true,"sender":{"key":"peter@piie.net","avatar":null},"body":"We have test coverage for \"git submodule status\" output in\nvarious cases, i.e.\n\n 1) not-init, not-cloned: status should initially be \"missing\"\n 2) init, not-cloned: status should be \"missing\"\n 3) not-init, cloned: status should ignore the inner git-repo\n 4) init, cloned: status should be \"up-to-date\" after update\n 4.1) + modified: status should be \"modified\" after submodule commit\n 4.2) + modified, committed: status should be \"up-to-date\" after update\n\nthe case 3) is not covered yet.\n\nTest that submodule status reports an inner git repo as unknown, while\nit is not added to the superproject. This covers case (3).\n\nSigned-off-by: Peter Kaestle \n---\n t/t7400-submodule-basic.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 29dc6812f1..d926b8c772 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -55,6 +55,21 @@ test_expect_success 'add aborts on repository with no commits' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'status should ignore inner git repo when not added' '\n+\trm -fr inner &&\n+\tmkdir inner &&\n+\t(\n+\t\tcd inner &&\n+\t\tgit init &&\n+\t\t>t &&\n+\t\tgit add t &&\n+\t\tgit commit -m \"initial\"\n+\t) &&\n+\ttest_must_fail git submodule status inner 2>output.err &&\n+\trm -fr inner &&\n+\ttest_i18ngrep \"^error: .*did not match any file(s) known to git\" output.err\n+'\n+\n test_expect_success 'setup - repository in init subdirectory' '\n \tmkdir init &&\n \t(\n-- \n2.25.0\n\n"}]}