git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Shourya Shukla <shouryashukla.oo@gmail.com>
To: git@vger.kernel.org
Cc: kaartic.sivaraam@gmail.com, christian.couder@gmail.com,
	gitster@pobox.com, Johannes.Schindelin@gmx.de,
	liu.denton@gmail.com, peff@peff.net,
	Shourya Shukla <shouryashukla.oo@gmail.com>
Subject: [GSoC][PATCH v2 0/3] submodule: fixup to summary-v3
Date: Thu, 27 Aug 2020 23:14:58 +0530	[thread overview]
Message-ID: <20200827174501.7103-1-shouryashukla.oo@gmail.com> (raw)

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


             reply	other threads:[~2020-08-27 17:45 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-27 17:44 Shourya Shukla [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200827174501.7103-1-shouryashukla.oo@gmail.com \
    --to=shouryashukla.oo@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kaartic.sivaraam@gmail.com \
    --cc=liu.denton@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).