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

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

[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

[PATCH v2 1/3] submodule: eliminate unused parameters from print_submodule_summary()

[Shourya Shukla]

Eliminate the parameters 'missing_{src,dst}' from the
'print_submodule_summary()' function call since they are not used
anywhere in the function.

Reported-by: Jeff King <peff@peff.net>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 builtin/submodule--helper.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 63ea39025d..b83f840251 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -982,7 +982,6 @@ static char* verify_submodule_committish(const char *sm_path,
 static void print_submodule_summary(struct summary_cb *info, char* errmsg,
 				    int total_commits, const char *displaypath,
 				    const char *src_abbrev, const char *dst_abbrev,
-				    int missing_src, int missing_dst,
 				    struct module_cb *p)
 {
 	if (p->status == 'T') {
@@ -1154,8 +1153,7 @@ static void generate_submodule_summary(struct summary_cb *info,
 
 	print_submodule_summary(info, errmsg, total_commits,
 				displaypath, src_abbrev,
-				dst_abbrev, missing_src,
-				missing_dst, p);
+				dst_abbrev, p);
 
 	free(displaypath);
 	free(src_abbrev);
-- 
2.28.0

[PATCH v2 2/3] submodule: fix style in function definition

[Shourya Shukla]

The definitions of 'verify_submodule_committish()' and
'print_submodule_summary()' had wrong styling in terms of the asterisk
placement. Amend them.

Also, the warning printed in case of an unexpected file mode printed the
mode in decimal. Print it in octal for enhanced readability.

Reported-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 builtin/submodule--helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b83f840251..93d0700891 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -959,7 +959,7 @@ enum diff_cmd {
 	DIFF_FILES
 };
 
-static char* verify_submodule_committish(const char *sm_path,
+static char *verify_submodule_committish(const char *sm_path,
 					 const char *committish)
 {
 	struct child_process cp_rev_parse = CHILD_PROCESS_INIT;
@@ -979,7 +979,7 @@ static char* verify_submodule_committish(const char *sm_path,
 	return strbuf_detach(&result, NULL);
 }
 
-static void print_submodule_summary(struct summary_cb *info, char* errmsg,
+static void print_submodule_summary(struct summary_cb *info, char *errmsg,
 				    int total_commits, const char *displaypath,
 				    const char *src_abbrev, const char *dst_abbrev,
 				    struct module_cb *p)
@@ -1056,7 +1056,7 @@ static void generate_submodule_summary(struct summary_cb *info,
 		} else {
 			/* for a submodule removal (mode:0000000), don't warn */
 			if (p->mod_dst)
-				warning(_("unexpected mode %d\n"), p->mod_dst);
+				warning(_("unexpected mode %o\n"), p->mod_dst);
 		}
 	}
 
-- 
2.28.0

[PATCH v2 3/3] t7421: eliminate 'grep' check in t7421.4 for mingw compatibility

[Shourya Shukla]

The 'grep' check in test 4 of t7421 resulted in the failure of t7421 on
Windows due to a different error message

    error: cannot spawn git: No such file or directory

instead of

    fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory

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. 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
file containing the error.

Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 builtin/submodule--helper.c      | 7 ++++---
 t/t7421-submodule-summary-add.sh | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 93d0700891..1db1176e48 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1035,7 +1035,7 @@ static void print_submodule_summary(struct summary_cb *info, char *errmsg,
 static void generate_submodule_summary(struct summary_cb *info,
 				       struct module_cb *p)
 {
-	char *displaypath, *src_abbrev, *dst_abbrev;
+	char *displaypath, *src_abbrev = NULL, *dst_abbrev;
 	int missing_src = 0, missing_dst = 0;
 	char *errmsg = NULL;
 	int total_commits = -1;
@@ -1061,8 +1061,9 @@ static void generate_submodule_summary(struct summary_cb *info,
 	}
 
 	if (S_ISGITLINK(p->mod_src)) {
-		src_abbrev = verify_submodule_committish(p->sm_path,
-							 oid_to_hex(&p->oid_src));
+		if (p->status != 'D')
+			src_abbrev = verify_submodule_committish(p->sm_path,
+								 oid_to_hex(&p->oid_src));
 		if (!src_abbrev) {
 			missing_src = 1;
 			/*
diff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh
index 59a9b00467..b070f13714 100755
--- a/t/t7421-submodule-summary-add.sh
+++ b/t/t7421-submodule-summary-add.sh
@@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'
 	git commit -m "change submodule path" &&
 	rev=$(git -C sm rev-parse --short HEAD^) &&
 	git submodule summary HEAD^^ -- my-subm >actual 2>err &&
-	grep "fatal:.*my-subm" err &&
+	test_must_be_empty err &&
 	cat >expected <<-EOF &&
 	* my-subm ${rev}...0000000:
 
-- 
2.28.0