git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [GSoC][PATCH v2 0/3] submodule: fixup to summary-v3
@ 2020-08-27 17:44 Shourya Shukla
  2020-08-27 17:44 ` [PATCH v2 1/3] submodule: eliminate unused parameters from print_submodule_summary() Shourya Shukla
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Shourya Shukla @ 2020-08-27 17:44 UTC (permalink / raw)
  To: git
  Cc: kaartic.sivaraam, christian.couder, gitster, Johannes.Schindelin,
	liu.denton, peff, Shourya Shukla

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


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-27 17:45 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-27 17:44 [GSoC][PATCH v2 0/3] submodule: fixup to summary-v3 Shourya Shukla
2020-08-27 17:44 ` [PATCH v2 1/3] submodule: eliminate unused parameters from print_submodule_summary() Shourya Shukla
2020-08-27 17:45 ` [PATCH v2 2/3] submodule: fix style in function definition Shourya Shukla
2020-08-27 17:45 ` [PATCH v2 3/3] t7421: eliminate 'grep' check in t7421.4 for mingw compatibility Shourya Shukla

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).