git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[GSoC][PATCH v2 0/3] submodule: fixup to summary-v3

From
Shourya Shukla <shouryashukla.oo@gmail.com>
Date
Aug 27, 2020, 17:44 UTC
Message-ID
<20200827174501.7103-1-shouryashukla.oo@gmail.com>
Greetings,

This is the v2 of the previous patch series with the same title. The v1 received some comments from Junio and Kaartic. The following changes were made:

    PATCH[3/3] received the comment that it had an unnecessary
    'char *dst_abbrev = NULL' which had to be reverted to 'char
    *dst_abbrev' since the assignment was pretty much useless.
    The commit message also needed some changes in the sense that it
    stated that the change of guarding the
    'verify_submodule_committish()' call was made for dst_abbrev as well
    which wasn't the case. Junio also suggested to clarify the reason
    for not having the guard in case of 'dst_abbrev'.

Another thing which came up was the cleanup of 'submodule--helper.c'. IT started with Junio commenting on PATCH[2/3] 'submodule: fix style in function definition'. He asked me to verify if there are any other similar faults regarding function or variable defintions which had a faulty asterisk placement. I did some digging and found a fault here in submodule--helper.c:

    static char *compute_rev_name(const char *sub_path, const char* object_id)

As yiou may notice, the correction should be 's/static char */static char* /. I also did some further digging and found that there we some other minor faults in the option descriptions of various subcommands. For instance in module_foreach:

	struct option module_foreach_options[] = {
		OPT__QUIET(&info.quiet, N_("Suppress output of entering each submodule command")),
		OPT_BOOL(0, "recursive", &info.recursive,
			 N_("Recurse into nested submodules")),
		OPT_END()
	};

The option descriptions should start in lowercase but they start with a capital letter. This convention is mentioned in L267-270 of 'api-parse-options.txt'. There are other such small violations such as die() messages starting with a captial letter.

I will do this cleanup after some time when I am a bit free since I have some personal engagements right now. Or something even better could be to add this as a 'good first issue' on gitgitgadget/git so that a newcomer can be something relatively easy and get familiar with the way work is done at Git. Please do tell what seems more fitting to you. Also, to be clear, I am not suggesting the latter out of laziness.

I am attaching a range-diff b/w v1 and v2 below for ease of review. Feedback and reviews are appreciated.

Regards, Shourya Shukla

----- range-diff:

1:  a22ffa950f = 1:  768f24de95 submodule: eliminate unused parameters from print_submodule_summary()
2:  32934998ee = 2:  35664360ac submodule: fix style in function definition
3:  82e0956cd2 ! 3:  f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
    @@ Commit message
             fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
    -    Tighten up the check to compute '{src,dst}_abbrev' by guarding the
    +    Tighten up the check to compute 'src_abbrev' by guarding the
         'verify_submodule_committish()' call using `p->status !='D'`, so that
         the former isn't called in case of non-existent submodule directory,
         consequently, there is no such error message on any execution
    -    environment.
    +    environment. The same need not be implemented for 'dst_abbrev' and is
    +    rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`
    +    already guards the `verify_submodule_committish` when we have a status
    +    of 'D'.
         Therefore, eliminate the 'grep' check in t7421. Instead, verify the
1:  a22ffa950f = 1:  768f24de95 submodule: eliminate unused parameters from print_submo
dule_summary()
2:  32934998ee = 2:  35664360ac submodule: fix style in function definition
3:  82e0956cd2 ! 3:  f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw comp
atibility
    @@ Commit message
             fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
    -    Tighten up the check to compute '{src,dst}_abbrev' by guarding the
:...skipping...
1:  a22ffa950f = 1:  768f24de95 submodule: eliminate unused parameters from print_submodule_summary()
2:  32934998ee = 2:  35664360ac submodule: fix style in function definition
3:  82e0956cd2 ! 3:  f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
    @@ Commit message
             fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
    -    Tighten up the check to compute '{src,dst}_abbrev' by guarding the
    +    Tighten up the check to compute 'src_abbrev' by guarding the
1:  a22ffa950f = 1:  768f24de95 submodule: eliminate unused parameters from print_submodule_summary()
2:  32934998ee = 2:  35664360ac submodule: fix style in function definition
3:  82e0956cd2 ! 3:  f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
    @@ Commit message
     
             fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
     
    -    Tighten up the check to compute '{src,dst}_abbrev' by guarding the
    +    Tighten up the check to compute 'src_abbrev' by guarding the
         'verify_submodule_committish()' call using `p->status !='D'`, so that
         the former isn't called in case of non-existent submodule directory,
         consequently, there is no such error message on any execution
    -    environment.
    +    environment. The same need not be implemented for 'dst_abbrev' and is
    +    rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`
    +    already guards the `verify_submodule_committish` when we have a status
    +    of 'D'.
     
         Therefore, eliminate the 'grep' check in t7421. Instead, verify the
1:  a22ffa950f = 1:  768f24de95 submodule: eliminate unused parameters from print_submo
dule_summary()
2:  32934998ee = 2:  35664360ac submodule: fix style in function definition
3:  82e0956cd2 ! 3:  f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw comp
atibility
    @@ Commit message
     
             fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
     
    -    Tighten up the check to compute '{src,dst}_abbrev' by guarding the
:...skipping...
1:  a22ffa950f = 1:  768f24de95 submodule: eliminate unused parameters from print_submodule_summary()
2:  32934998ee = 2:  35664360ac submodule: fix style in function definition
3:  82e0956cd2 ! 3:  f5ce59db84 t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
    @@ Commit message
     
             fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
     
    -    Tighten up the check to compute '{src,dst}_abbrev' by guarding the
    +    Tighten up the check to compute 'src_abbrev' by guarding the
         'verify_submodule_committish()' call using `p->status !='D'`, so that
         the former isn't called in case of non-existent submodule directory,
         consequently, there is no such error message on any execution
    -    environment.
    +    environment. The same need not be implemented for 'dst_abbrev' and is
    +    rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`
    +    already guards the `verify_submodule_committish` when we have a status
    +    of 'D'.
     
         Therefore, eliminate the 'grep' check in t7421. Instead, verify the
         absence of an error message by doing a 'test_must_be_empty' on the
    @@ builtin/submodule--helper.c: static void print_submodule_summary(struct summary_
                                       struct module_cb *p)
      {
     -  char *displaypath, *src_abbrev, *dst_abbrev;
    -+  char *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL;
    ++  char *displaypath, *src_abbrev = NULL, *dst_abbrev;
        int missing_src = 0, missing_dst = 0;
        char *errmsg = NULL;
        int total_commits = -1;
~
~
~
~
~
~
~
~
~         'verify_submodule_committish()' call using `p->status !='D'`, so that
         the former isn't called in case of non-existent submodule directory,
         consequently, there is no such error message on any execution
    -    environment.
    +    environment. The same need not be implemented for 'dst_abbrev' and is
    +    rather redundant since the conditional `if (S_ISGITLINK(p->mod_dst))`
    +    already guards the `verify_submodule_committish` when we have a status
    +    of 'D'.
         Therefore, eliminate the 'grep' check in t7421. Instead, verify the
         absence of an error message by doing a 'test_must_be_empty' on the
    @@ builtin/submodule--helper.c: static void print_submodule_summary(struct summary_
                                       struct module_cb *p)
      {
     -  char *displaypath, *src_abbrev, *dst_abbrev;
    -+  char *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL;
    ++  char *displaypath, *src_abbrev = NULL, *dst_abbrev;
        int missing_src = 0, missing_dst = 0;
        char *errmsg = NULL;
        int total_commits = -1;
~
~
~
~
~
~
~
~
~
-----
Shourya Shukla (3):
  submodule: eliminate unused parameters from print_submodule_summary()
  submodule: fix style in function definition
  t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
 builtin/submodule--helper.c      | 17 ++++++++---------
 t/t7421-submodule-summary-add.sh |  2 +-
 2 files changed, 9 insertions(+), 10 deletions(-)
-- 
2.28.0
Next: Shourya Shukla
Message 1 of 4 in “submodule: fixup to summary-v3”
  1. Shourya ShuklaAug 27, 2020
  2. 1/3 submodule: eliminate unused parameters from print_submodule_summary()Shourya Shukla, Aug 27, 2020
  3. 2/3 submodule: fix style in function definitionShourya Shukla, Aug 27, 2020
  4. 3/3 t7421: eliminate 'grep' check in t7421.4 for mingw compatibilityShourya Shukla, Aug 27, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.