{"thread":{"id":"54130","subject":"[GSoC][PATCH 0/3] submodule: fixup to summary-v3","startedAt":"2020-08-25T11:30:59Z","lastAt":"2020-08-27T09:15:14Z","messageCount":12,"participants":["Shourya Shukla","Kaartic Sivaraam","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"404401","messageId":"20200825113020.71801-1-shouryashukla.oo@gmail.com","threadId":"54130","inReplyTo":null,"subject":"[GSoC][PATCH 0/3] submodule: fixup to summary-v3","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-25T11:30:17Z","receivedAt":"2020-08-25T11:30:59Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Greetings,\n\nThe v3 of 'git submodule summary' port to C is currently in 'next'\nbranch of git/git. Recently, the patch recieved some comments from\nPeff, Dscho and Kaartic:\n\n 1. The definition of 'print_submodule_summary()' contained two\n unused parameters namely 'missing_src' and 'missing_dst'. Hence,\n I had to eliminate them as covered in the commit a22ffa950f\n a22ffa950f (submodule: eliminate unused parameters from\n print_submodule_summary(), 2020-08-21). Reported by Peff.\n Junio also advised to make the output in case of an unexpected\n file mode a bit more user friendly by outputting an octal instead\n of a decimal.\n\n 2. The function definitions of 'verify_submodule_committish()' and\n 'print_submodule_summary()' had wrong styling in terms of the\n asterisk placement. Hence it was fixed in 32934998ee (submodule:\n fix style in function definition, 2020-08-22). Reported by\n Kaartic.\n\n 2. The test script 't7421-submodule-summary-add.sh' failed in\n Windows due to failure of t7421.4. Precisely, the 'test_i18ngrep'\n check failed on Windows since the error message which was being\n grepped was different on Windows; it was designed to work on\n Linux. Therefore, we had to eliminate the grep check in t7421.4\n and replace it with a check to see if there is any error message\n or not using 'test_must_be_empty'. Also, to support this change,\n we had to make some small changes in 'print_submodule_summary()'\n function. The call to verify_submodule_committish()' had to be\n guarded using 'p->status !=D' so that it isn't called when the SM\n directory does not exist, therefore, the error message is not\n displayed. This resulted in 82e0956cd2 (t7421: eliminate 'grep'\n check in t7421.4 for mingw compatibility, 2020-08-22). Reported\n by Dscho.\n\nsummary-v3: https://lore.kernel.org/git/20200812194404.17028-1-shouryashukla.oo@gmail.com/\nPeff's comment: https://lore.kernel.org/git/20200818020838.GA1872632@coredump.intra.peff.net/\nDscho' comment: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2008211708280.56@tvgsbejvaqbjf.bet/\nKaartic's comment: https://lore.kernel.org/git/377b1a2ad60c5ca30864f48c5921ff89b5aca65b.camel@gmail.com/\nJunio's comment regarding unexpected file mode: https://lore.kernel.org/git/xmqqo8n053r6.fsf@gitster.c.googlers.com/\n\nFeedback and reviews are appreciated.\n\nRegards,\nShourya Shukla\n\nShourya Shukla (3):\n submodule: eliminate unused parameters from print_submodule_summary()\n submodule: fix style in function definition\n t7421: eliminate 'grep' check in t7421.4 for mingw compatibility\n\n builtin/submodule--helper.c | 17 ++++++++---------\n t/t7421-submodule-summary-add.sh | 2 +-\n 2 files changed, 9 insertions(+), 10 deletions(-)\n\n-- \n2.28.0\n\n"},{"id":"404402","messageId":"20200825113020.71801-3-shouryashukla.oo@gmail.com","threadId":"54130","inReplyTo":"20200825113020.71801-1-shouryashukla.oo@gmail.com","subject":"[PATCH 2/3] submodule: fix style in function definition","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-25T11:30:19Z","receivedAt":"2020-08-25T11:31:11Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"The definitions of 'verify_submodule_committish()' and\n'print_submodule_summary()' had wrong styling in terms of the asterisk\nplacement. Amend them.\n\nAlso, the warning printed in case of an unexpected file mode printed the\nmode in decimal. Print it in octal for enhanced readability.\n\nReported-by: Kaartic Sivaraam \nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nHelped-by: Junio C Hamano \nSigned-off-by: Shourya Shukla \n---\n builtin/submodule--helper.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex b83f840251..93d0700891 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -959,7 +959,7 @@ enum diff_cmd {\n \tDIFF_FILES\n };\n \n-static char* verify_submodule_committish(const char *sm_path,\n+static char *verify_submodule_committish(const char *sm_path,\n \t\t\t\t\t const char *committish)\n {\n \tstruct child_process cp_rev_parse = CHILD_PROCESS_INIT;\n@@ -979,7 +979,7 @@ static char* verify_submodule_committish(const char *sm_path,\n \treturn strbuf_detach(&result, NULL);\n }\n \n-static void print_submodule_summary(struct summary_cb *info, char* errmsg,\n+static void print_submodule_summary(struct summary_cb *info, char *errmsg,\n \t\t\t\t int total_commits, const char *displaypath,\n \t\t\t\t const char *src_abbrev, const char *dst_abbrev,\n \t\t\t\t struct module_cb *p)\n@@ -1056,7 +1056,7 @@ static void generate_submodule_summary(struct summary_cb *info,\n \t\t} else {\n \t\t\t/* for a submodule removal (mode:0000000), don't warn */\n \t\t\tif (p->mod_dst)\n-\t\t\t\twarning(_(\"unexpected mode %d\\n\"), p->mod_dst);\n+\t\t\t\twarning(_(\"unexpected mode %o\\n\"), p->mod_dst);\n \t\t}\n \t}\n \n-- \n2.28.0\n\n"},{"id":"404403","messageId":"20200825113020.71801-2-shouryashukla.oo@gmail.com","threadId":"54130","inReplyTo":"20200825113020.71801-1-shouryashukla.oo@gmail.com","subject":"[PATCH 1/3] submodule: eliminate unused parameters from print_submodule_summary()","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-25T11:30:18Z","receivedAt":"2020-08-25T11:31:29Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Eliminate the parameters 'missing_{src,dst}' from the\n'print_submodule_summary()' function call since they are not used\nanywhere in the function.\n\nReported-by: Jeff King \nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Shourya Shukla \n---\n builtin/submodule--helper.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 63ea39025d..b83f840251 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -982,7 +982,6 @@ static char* verify_submodule_committish(const char *sm_path,\n static void print_submodule_summary(struct summary_cb *info, char* errmsg,\n \t\t\t\t int total_commits, const char *displaypath,\n \t\t\t\t const char *src_abbrev, const char *dst_abbrev,\n-\t\t\t\t int missing_src, int missing_dst,\n \t\t\t\t struct module_cb *p)\n {\n \tif (p->status == 'T') {\n@@ -1154,8 +1153,7 @@ static void generate_submodule_summary(struct summary_cb *info,\n \n \tprint_submodule_summary(info, errmsg, total_commits,\n \t\t\t\tdisplaypath, src_abbrev,\n-\t\t\t\tdst_abbrev, missing_src,\n-\t\t\t\tmissing_dst, p);\n+\t\t\t\tdst_abbrev, p);\n \n \tfree(displaypath);\n \tfree(src_abbrev);\n-- \n2.28.0\n\n"},{"id":"404404","messageId":"20200825113020.71801-4-shouryashukla.oo@gmail.com","threadId":"54130","inReplyTo":"20200825113020.71801-1-shouryashukla.oo@gmail.com","subject":"[PATCH 3/3] t7421: eliminate 'grep' check in t7421.4 for mingw compatibility","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-25T11:30:20Z","receivedAt":"2020-08-25T11:31:36Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"The 'grep' check in test 4 of t7421 resulted in the failure of t7421 on\nWindows due to a different error message\n\n error: cannot spawn git: No such file or directory\n\ninstead of\n\n fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory\n\nTighten up the check to compute '{src,dst}_abbrev' by guarding the\n'verify_submodule_committish()' call using `p->status !='D'`, so that\nthe former isn't called in case of non-existent submodule directory,\nconsequently, there is no such error message on any execution\nenvironment.\n\nTherefore, eliminate the 'grep' check in t7421. Instead, verify the\nabsence of an error message by doing a 'test_must_be_empty' on the\nfile containing the error.\n\nReported-by: Johannes Schindelin \nHelped-by: Kaartic Sivaraam \nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Shourya Shukla \n---\n builtin/submodule--helper.c | 7 ++++---\n t/t7421-submodule-summary-add.sh | 2 +-\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 93d0700891..f1951680f7 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1035,7 +1035,7 @@ static void print_submodule_summary(struct summary_cb *info, char *errmsg,\n static void generate_submodule_summary(struct summary_cb *info,\n \t\t\t\t struct module_cb *p)\n {\n-\tchar *displaypath, *src_abbrev, *dst_abbrev;\n+\tchar *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL;\n \tint missing_src = 0, missing_dst = 0;\n \tchar *errmsg = NULL;\n \tint total_commits = -1;\n@@ -1061,8 +1061,9 @@ static void generate_submodule_summary(struct summary_cb *info,\n \t}\n \n \tif (S_ISGITLINK(p->mod_src)) {\n-\t\tsrc_abbrev = verify_submodule_committish(p->sm_path,\n-\t\t\t\t\t\t\t oid_to_hex(&p->oid_src));\n+\t\tif (p->status != 'D')\n+\t\t\tsrc_abbrev = verify_submodule_committish(p->sm_path,\n+\t\t\t\t\t\t\t\t oid_to_hex(&p->oid_src));\n \t\tif (!src_abbrev) {\n \t\t\tmissing_src = 1;\n \t\t\t/*\ndiff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh\nindex 59a9b00467..b070f13714 100755\n--- a/t/t7421-submodule-summary-add.sh\n+++ b/t/t7421-submodule-summary-add.sh\n@@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'\n \tgit commit -m \"change submodule path\" &&\n \trev=$(git -C sm rev-parse --short HEAD^) &&\n \tgit submodule summary HEAD^^ -- my-subm >actual 2>err &&\n-\tgrep \"fatal:.*my-subm\" err &&\n+\ttest_must_be_empty err &&\n \tcat >expected <<-EOF &&\n \t* my-subm ${rev}...0000000:\n \n-- \n2.28.0\n\n"},{"id":"404412","messageId":"2a1ea501-4974-4d74-fe3c-d173bbe76855@gmail.com","threadId":"54130","inReplyTo":"20200825113020.71801-4-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 3/3] t7421: eliminate 'grep' check in t7421.4 for mingw compatibility","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-08-25T14:33:24Z","receivedAt":"2020-08-25T14:33:35Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On 25-08-2020 17:00, Shourya Shukla wrote:\n> The 'grep' check in test 4 of t7421 resulted in the failure of t7421 on\n> Windows due to a different error message\n> \n> error: cannot spawn git: No such file or directory\n> \n> instead of\n> \n> fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory\n> \n> Tighten up the check to compute '{src,dst}_abbrev' by guarding the\n\nThe change only affects `src_abbrev`. So, it's misleading to mention\n`dst_abbrev` here.\n\n> 'verify_submodule_committish()' call using `p->status !='D'`, so that\n> the former isn't called in case of non-existent submodule directory,\n> consequently, there is no such error message on any execution\n> environment.\n> \n> Therefore, eliminate the 'grep' check in t7421. Instead, verify the\n> absence of an error message by doing a 'test_must_be_empty' on the\n> file containing the error.\n> \n> Reported-by: Johannes Schindelin \n> Helped-by: Kaartic Sivaraam \n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Signed-off-by: Shourya Shukla \n> ---\n> builtin/submodule--helper.c | 7 ++++---\n> t/t7421-submodule-summary-add.sh | 2 +-\n> 2 files changed, 5 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 93d0700891..f1951680f7 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1035,7 +1035,7 @@ static void print_submodule_summary(struct summary_cb *info, char *errmsg,\n> static void generate_submodule_summary(struct summary_cb *info,\n> \t\t\t\t struct module_cb *p)\n> {\n> -\tchar *displaypath, *src_abbrev, *dst_abbrev;\n> +\tchar *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL;\n\nUnlike `src_abbrev`, I don't think we need to initilialize `dst_abbrev`\nto NULL here as it would be assigned in all code paths.\n\n> \tint missing_src = 0, missing_dst = 0;\n> \tchar *errmsg = NULL;\n> \tint total_commits = -1;\n> @@ -1061,8 +1061,9 @@ static void generate_submodule_summary(struct summary_cb *info,\n> \t}\n> \n> \tif (S_ISGITLINK(p->mod_src)) {\n> -\t\tsrc_abbrev = verify_submodule_committish(p->sm_path,\n> -\t\t\t\t\t\t\t oid_to_hex(&p->oid_src));\n> +\t\tif (p->status != 'D')\n> +\t\t\tsrc_abbrev = verify_submodule_committish(p->sm_path,\n> +\t\t\t\t\t\t\t\t oid_to_hex(&p->oid_src));\n> \t\tif (!src_abbrev) {\n> \t\t\tmissing_src = 1;\n> \t\t\t/*\n\n-- \nSivaraam\n"},{"id":"404413","messageId":"93f9eec6-dc77-01f2-c2fe-2f02b97f853b@gmail.com","threadId":"54130","inReplyTo":"20200825113020.71801-1-shouryashukla.oo@gmail.com","subject":"Re: [GSoC][PATCH 0/3] submodule: fixup to summary-v3","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2020-08-25T14:38:16Z","receivedAt":"2020-08-25T14:38:28Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On 25-08-2020 17:00, Shourya Shukla wrote:\n> Greetings,\n> \n> The v3 of 'git submodule summary' port to C is currently in 'next'\n> branch of git/git. Recently, the patch recieved some comments from\n> Peff, Dscho and Kaartic:\n> \n> 1. The definition of 'print_submodule_summary()' contained two\n> unused parameters namely 'missing_src' and 'missing_dst'. Hence,\n> I had to eliminate them as covered in the commit a22ffa950f\n> a22ffa950f (submodule: eliminate unused parameters from\n> print_submodule_summary(), 2020-08-21). Reported by Peff.\n> Junio also advised to make the output in case of an unexpected\n> file mode a bit more user friendly by outputting an octal instead\n> of a decimal.\n> \n> 2. The function definitions of 'verify_submodule_committish()' and\n> 'print_submodule_summary()' had wrong styling in terms of the\n> asterisk placement. Hence it was fixed in 32934998ee (submodule:\n> fix style in function definition, 2020-08-22). Reported by\n> Kaartic.\n> \n> 2. The test script 't7421-submodule-summary-add.sh' failed in\n> Windows due to failure of t7421.4. Precisely, the 'test_i18ngrep'\n> check failed on Windows since the error message which was being\n> grepped was different on Windows; it was designed to work on\n> Linux. Therefore, we had to eliminate the grep check in t7421.4\n> and replace it with a check to see if there is any error message\n> or not using 'test_must_be_empty'. Also, to support this change,\n> we had to make some small changes in 'print_submodule_summary()'\n> function. The call to verify_submodule_committish()' had to be\n> guarded using 'p->status !=D' so that it isn't called when the SM\n> directory does not exist, therefore, the error message is not\n> displayed. This resulted in 82e0956cd2 (t7421: eliminate 'grep'\n> check in t7421.4 for mingw compatibility, 2020-08-22). Reported\n> by Dscho.\n> \n\nWhile the cover letter is nice, it doesn't make much sense to refer to\npatches that are part of the series using the commit hashes of their\n\"local\" commits. It's more common to refer to them as using the position\nof the patch such as [1/3] etc.\n\n-- \nSivaraam\n"},{"id":"404425","messageId":"xmqqlfi21zb8.fsf@gitster.c.googlers.com","threadId":"54130","inReplyTo":"2a1ea501-4974-4d74-fe3c-d173bbe76855@gmail.com","subject":"Re: [PATCH 3/3] t7421: eliminate 'grep' check in t7421.4 for mingw compatibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-25T16:10:35Z","receivedAt":"2020-08-25T16:10:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n>> @@ -1061,8 +1061,9 @@ static void generate_submodule_summary(struct summary_cb *info,\n>> \t}\n>> \n>> \tif (S_ISGITLINK(p->mod_src)) {\n>> -\t\tsrc_abbrev = verify_submodule_committish(p->sm_path,\n>> -\t\t\t\t\t\t\t oid_to_hex(&p->oid_src));\n>> +\t\tif (p->status != 'D')\n>> +\t\t\tsrc_abbrev = verify_submodule_committish(p->sm_path,\n>> +\t\t\t\t\t\t\t\t oid_to_hex(&p->oid_src));\n>> \t\tif (!src_abbrev) {\n>> \t\t\tmissing_src = 1;\n>> \t\t\t/*\n\nInteresting. There is a mirroring if-else cascade that begins with\n\"if (S_ISGITLINK(p->mod_dst))\" immediately after the if-else cascade\nstarted here, and in there, the same verify_submodule_committish()\nis called for oid_dst unconditionally. Should the asymmetry bother\nreaders of the code, or is the source side somehow special and needs\nextra care?\n\n\n"},{"id":"404478","messageId":"xmqqwo1mzc6y.fsf@gitster.c.googlers.com","threadId":"54130","inReplyTo":"20200825113020.71801-3-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 2/3] submodule: fix style in function definition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-25T20:45:57Z","receivedAt":"2020-08-25T20:46:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n> The definitions of 'verify_submodule_committish()' and\n> 'print_submodule_summary()' had wrong styling in terms of the asterisk\n> placement. Amend them.\n\nI pointed out only these two, but that does not necessarily mean\nthey are the only ones. Have you checked all the new code added by\nthe series?\n\n> Also, the warning printed in case of an unexpected file mode printed the\n> mode in decimal. Print it in octal for enhanced readability.\n\nI actually did check this side ;-) and am reasonably sure that there\naren't any other irrational choice of format specifiers.\n\nThanks.\n"},{"id":"404505","messageId":"20200826094506.GA311769@konoha","threadId":"54130","inReplyTo":"xmqqwo1mzc6y.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/3] submodule: fix style in function definition","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-26T09:45:27Z","receivedAt":"2020-08-26T09:46:13Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":">> The definitions of 'verify_submodule_committish()' and\n>> 'print_submodule_summary()' had wrong styling in terms of the asterisk\n>> placement. Amend them.\n\n> I pointed out only these two, but that does not necessarily mean\n> they are the only ones. Have you checked all the new code added by\n> the series?\n\nThere is one more. It is not related to my patch series though. Here it\nis:\n----\nstatic char *compute_rev_name(const char *sub_path, const char* object_id)\n----\nWould you like me to correct this one too?\n\n>> Also, the warning printed in case of an unexpected file mode printed the\n>> mode in decimal. Print it in octal for enhanced readability.\n\n>I actually did check this side ;-) and am reasonably sure that there\n>aren't any other irrational choice of format specifiers.\n\nSure! No worries!\n\n"},{"id":"404507","messageId":"20200826104047.GA315563@konoha","threadId":"54130","inReplyTo":"2a1ea501-4974-4d74-fe3c-d173bbe76855@gmail.com","subject":"Re: [PATCH 3/3] t7421: eliminate 'grep' check in t7421.4 for mingw compatibility","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-26T10:40:47Z","receivedAt":"2020-08-26T10:41:21Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 25/08 08:03, Kaartic Sivaraam wrote:\n> On 25-08-2020 17:00, Shourya Shukla wrote:\n> > The 'grep' check in test 4 of t7421 resulted in the failure of t7421 on\n> > Windows due to a different error message\n> > \n> > error: cannot spawn git: No such file or directory\n> > \n> > instead of\n> > \n> > fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory\n> > \n> > Tighten up the check to compute '{src,dst}_abbrev' by guarding the\n> \n> The change only affects `src_abbrev`. So, it's misleading to mention\n> `dst_abbrev` here.\n\nI forgot to change that. Thank you for pointing this out.\n\n> > 'verify_submodule_committish()' call using `p->status !='D'`, so that\n> > the former isn't called in case of non-existent submodule directory,\n> > consequently, there is no such error message on any execution\n> > environment.\n> > \n> > Therefore, eliminate the 'grep' check in t7421. Instead, verify the\n> > absence of an error message by doing a 'test_must_be_empty' on the\n> > file containing the error.\n> > \n> > Reported-by: Johannes Schindelin \n> > Helped-by: Kaartic Sivaraam \n> > Mentored-by: Christian Couder \n> > Mentored-by: Kaartic Sivaraam \n> > Signed-off-by: Shourya Shukla \n> > ---\n> > builtin/submodule--helper.c | 7 ++++---\n> > t/t7421-submodule-summary-add.sh | 2 +-\n> > 2 files changed, 5 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> > index 93d0700891..f1951680f7 100644\n> > --- a/builtin/submodule--helper.c\n> > +++ b/builtin/submodule--helper.c\n> > @@ -1035,7 +1035,7 @@ static void print_submodule_summary(struct summary_cb *info, char *errmsg,\n> > static void generate_submodule_summary(struct summary_cb *info,\n> > \t\t\t\t struct module_cb *p)\n> > {\n> > -\tchar *displaypath, *src_abbrev, *dst_abbrev;\n> > +\tchar *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL;\n> \n> Unlike `src_abbrev`, I don't think we need to initilialize `dst_abbrev`\n> to NULL here as it would be assigned in all code paths.\n\nAlright. Changed!\n\n"},{"id":"404537","messageId":"xmqq8se1we02.fsf@gitster.c.googlers.com","threadId":"54130","inReplyTo":"20200826094506.GA311769@konoha","subject":"Re: [PATCH 2/3] submodule: fix style in function definition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T16:47:25Z","receivedAt":"2020-08-26T16:47:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n>>> The definitions of 'verify_submodule_committish()' and\n>>> 'print_submodule_summary()' had wrong styling in terms of the asterisk\n>>> placement. Amend them.\n>\n>> I pointed out only these two, but that does not necessarily mean\n>> they are the only ones. Have you checked all the new code added by\n>> the series?\n>\n> There is one more. It is not related to my patch series though.\n\nCleaning up the existing breakage is outside the scope your series,\nbut of course fixes as an independent patch is welcomed.\n\nThanks for checking.\n"},{"id":"404597","messageId":"20200827091441.GA6656@konoha","threadId":"54130","inReplyTo":"xmqqlfi21zb8.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/3] t7421: eliminate 'grep' check in t7421.4 for mingw compatibility","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-27T09:14:41Z","receivedAt":"2020-08-27T09:15:14Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"> Interesting. There is a mirroring if-else cascade that begins with\n> \"if (S_ISGITLINK(p->mod_dst))\" immediately after the if-else cascade\n> started here, and in there, the same verify_submodule_committish()\n> is called for oid_dst unconditionally. Should the asymmetry bother\n> readers of the code, or is the source side somehow special and needs\n> extra care?\n\nI understand what you are trying to say. The thing is that the\nconditional `if (S_ISGITLINK(p->mod_dst))` already guards the\n`verify_submodule_committish` when we have a status of 'D'. So, we do\nnot need another similar if-statement for that. It does seem a bit weird\nto someone who is reading this thing for the first time, hence, I will\nmention this in the commit message.\n\nApologies for the late reply, I was a little busy with something.\n\n"}]}