{"thread":{"id":"54144","subject":"[GSoC][PATCH v2 0/3] submodule: fixup to summary-v3","startedAt":"2020-08-27T17:45:13Z","lastAt":"2020-08-27T17:45:25Z","messageCount":4,"participants":["Shourya Shukla"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"404633","messageId":"20200827174501.7103-1-shouryashukla.oo@gmail.com","threadId":"54144","inReplyTo":null,"subject":"[GSoC][PATCH v2 0/3] submodule: fixup to summary-v3","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-27T17:44:58Z","receivedAt":"2020-08-27T17:45:13Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Greetings,\n\nThis is the v2 of the previous patch series with the same title. The v1\nreceived some comments from Junio and Kaartic. The following changes\nwere made:\n\n    PATCH[3/3] received the comment that it had an unnecessary\n    'char *dst_abbrev = NULL' which had to be reverted to 'char\n    *dst_abbrev' since the assignment was pretty much useless.\n    The commit message also needed some changes in the sense that it\n    stated that the change of guarding the\n    'verify_submodule_committish()' call was made for dst_abbrev as well\n    which wasn't the case. Junio also suggested to clarify the reason\n    for not having the guard in case of 'dst_abbrev'.\n\nAnother thing which came up was the cleanup of 'submodule--helper.c'. IT\nstarted with Junio commenting on PATCH[2/3] 'submodule: fix style in\nfunction definition'. He asked me to verify if there are any other\nsimilar faults regarding function or variable defintions which had a\nfaulty asterisk placement. I did some digging and found a fault here in\nsubmodule--helper.c:\n\n    static char *compute_rev_name(const char *sub_path, const char* object_id)\n\nAs yiou may notice, the correction should be 's/static char */static\nchar* /. I also did some further digging and found that there we some\nother minor faults in the option descriptions of various subcommands.\nFor instance in module_foreach:\n\n\tstruct option module_foreach_options[] = {\n\t\tOPT__QUIET(&info.quiet, N_(\"Suppress output of entering each submodule command\")),\n\t\tOPT_BOOL(0, \"recursive\", &info.recursive,\n\t\t\t N_(\"Recurse into nested submodules\")),\n\t\tOPT_END()\n\t};\n\nThe option descriptions should start in lowercase but they start with a\ncapital letter. This convention is mentioned in L267-270 of\n'api-parse-options.txt'. There are other such small violations such as\ndie() messages starting with a captial letter.\n\nI will do this cleanup after some time when I am a bit free since I have\nsome personal engagements right now. Or something even better could be\nto add this as a 'good first issue' on gitgitgadget/git so that a\nnewcomer can be something relatively easy and get familiar with the way\nwork is done at Git. Please do tell what seems more fitting to you.\nAlso, to be clear, I am not suggesting the latter out of laziness.\n\nI am attaching a range-diff b/w v1 and v2 below for ease of review.\nFeedback and reviews are appreciated.\n\nRegards,\nShourya Shukla\n\n-----\nrange-diff:\n\n1:  a22ffa950f = 1:  768f24de95 submodule: eliminate unused parameters from print_submodule_summary()\n2:  32934998ee = 2:  35664360ac submodule: fix style in function definition\n3:  82e0956cd2 ! 3:  f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility\n    @@ Commit message\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    +    Tighten up the check to compute 'src_abbrev' by guarding the\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    +    environment. The same need not be implemented for 'dst_abbrev' and is\n    +    rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`\n    +    already guards the `verify_submodule_committish` when we have a status\n    +    of 'D'.\n\n         Therefore, eliminate the 'grep' check in t7421. Instead, verify the\n1:  a22ffa950f = 1:  768f24de95 submodule: eliminate unused parameters from print_submo\ndule_summary()\n2:  32934998ee = 2:  35664360ac submodule: fix style in function definition\n3:  82e0956cd2 ! 3:  f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw comp\natibility\n    @@ Commit message\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:...skipping...\n1:  a22ffa950f = 1:  768f24de95 submodule: eliminate unused parameters from print_submodule_summary()\n2:  32934998ee = 2:  35664360ac submodule: fix style in function definition\n3:  82e0956cd2 ! 3:  f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility\n    @@ Commit message\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    +    Tighten up the check to compute 'src_abbrev' by guarding the\n1:  a22ffa950f = 1:  768f24de95 submodule: eliminate unused parameters from print_submodule_summary()\n2:  32934998ee = 2:  35664360ac submodule: fix style in function definition\n3:  82e0956cd2 ! 3:  f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility\n    @@ Commit message\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    +    Tighten up the check to compute 'src_abbrev' by guarding the\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    +    environment. The same need not be implemented for 'dst_abbrev' and is\n    +    rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`\n    +    already guards the `verify_submodule_committish` when we have a status\n    +    of 'D'.\n     \n         Therefore, eliminate the 'grep' check in t7421. Instead, verify the\n1:  a22ffa950f = 1:  768f24de95 submodule: eliminate unused parameters from print_submo\ndule_summary()\n2:  32934998ee = 2:  35664360ac submodule: fix style in function definition\n3:  82e0956cd2 ! 3:  f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw comp\natibility\n    @@ Commit message\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:...skipping...\n1:  a22ffa950f = 1:  768f24de95 submodule: eliminate unused parameters from print_submodule_summary()\n2:  32934998ee = 2:  35664360ac submodule: fix style in function definition\n3:  82e0956cd2 ! 3:  f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility\n    @@ Commit message\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    +    Tighten up the check to compute 'src_abbrev' by guarding the\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    +    environment. The same need not be implemented for 'dst_abbrev' and is\n    +    rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`\n    +    already guards the `verify_submodule_committish` when we have a status\n    +    of 'D'.\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    @@ builtin/submodule--helper.c: static void print_submodule_summary(struct summary_\n                                       struct module_cb *p)\n      {\n     -  char *displaypath, *src_abbrev, *dst_abbrev;\n    -+  char *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL;\n    ++  char *displaypath, *src_abbrev = NULL, *dst_abbrev;\n        int missing_src = 0, missing_dst = 0;\n        char *errmsg = NULL;\n        int total_commits = -1;\n~\n~\n~\n~\n~\n~\n~\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    +    environment. The same need not be implemented for 'dst_abbrev' and is\n    +    rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`\n    +    already guards the `verify_submodule_committish` when we have a status\n    +    of 'D'.\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    @@ builtin/submodule--helper.c: static void print_submodule_summary(struct summary_\n                                       struct module_cb *p)\n      {\n     -  char *displaypath, *src_abbrev, *dst_abbrev;\n    -+  char *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL;\n    ++  char *displaypath, *src_abbrev = NULL, *dst_abbrev;\n        int missing_src = 0, missing_dst = 0;\n        char *errmsg = NULL;\n        int total_commits = -1;\n~\n~\n~\n~\n~\n~\n~\n~\n~\n-----\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":"404634","messageId":"20200827174501.7103-2-shouryashukla.oo@gmail.com","threadId":"54144","inReplyTo":"20200827174501.7103-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 1/3] submodule: eliminate unused parameters from print_submodule_summary()","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-27T17:44:59Z","receivedAt":"2020-08-27T17:45:17Z","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 <peff@peff.net>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\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":"404635","messageId":"20200827174501.7103-3-shouryashukla.oo@gmail.com","threadId":"54144","inReplyTo":"20200827174501.7103-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 2/3] submodule: fix style in function definition","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-27T17:45:00Z","receivedAt":"2020-08-27T17:45:20Z","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 <kaartic.sivaraam@gmail.com>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\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":"404636","messageId":"20200827174501.7103-4-shouryashukla.oo@gmail.com","threadId":"54144","inReplyTo":"20200827174501.7103-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 3/3] t7421: eliminate 'grep' check in t7421.4 for mingw compatibility","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-27T17:45:01Z","receivedAt":"2020-08-27T17:45:25Z","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_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. The same need not be implemented for 'dst_abbrev' and is\nrather redundant since the conditional 'if (S_ISGITLINK(p->mod_dst))'\nalready guards the 'verify_submodule_committish()' when we have a\nstatus of 'D'.\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 <Johannes.Schindelin@gmx.de>\nHelped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>\nSigned-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>\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..1db1176e48 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;\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"}]}