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

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

[Shourya Shukla]

Greetings,

The v3 of 'git submodule summary' port to C is currently in 'next'
branch of git/git. Recently, the patch recieved some comments from
Peff, Dscho and Kaartic:

    1. The definition of 'print_submodule_summary()' contained two
       unused parameters namely 'missing_src' and 'missing_dst'. Hence,
       I had to eliminate them as covered in the commit a22ffa950f
       a22ffa950f (submodule: eliminate unused parameters from
       print_submodule_summary(), 2020-08-21). Reported by Peff.
       Junio also advised to make the output in case of an unexpected
       file mode a bit more user friendly by outputting an octal instead
       of a decimal.

    2. The function definitions of 'verify_submodule_committish()' and
       'print_submodule_summary()' had wrong styling in terms of the
       asterisk placement. Hence it was fixed in 32934998ee (submodule:
       fix style in function definition, 2020-08-22). Reported by
       Kaartic.

    2. The test script 't7421-submodule-summary-add.sh' failed in
       Windows due to failure of t7421.4. Precisely, the 'test_i18ngrep'
       check failed on Windows since the error message which was being
       grepped was different on Windows; it was designed to work on
       Linux. Therefore, we had to eliminate the grep check in t7421.4
       and replace it with a check to see if there is any error message
       or not using 'test_must_be_empty'. Also, to support this change,
       we had to make some small changes in 'print_submodule_summary()'
       function. The call to verify_submodule_committish()' had to be
       guarded using 'p->status !=D' so that it isn't called when the SM
       directory does not exist, therefore, the error message is not
       displayed. This resulted in 82e0956cd2 (t7421: eliminate 'grep'
       check in t7421.4 for mingw compatibility, 2020-08-22). Reported
       by Dscho.

summary-v3: https://lore.kernel.org/git/20200812194404.17028-1-shouryashukla.oo@gmail.com/
Peff's comment: https://lore.kernel.org/git/20200818020838.GA1872632@coredump.intra.peff.net/
Dscho' comment: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2008211708280.56@tvgsbejvaqbjf.bet/
Kaartic's comment: https://lore.kernel.org/git/377b1a2ad60c5ca30864f48c5921ff89b5aca65b.camel@gmail.com/
Junio's comment regarding unexpected file mode: https://lore.kernel.org/git/xmqqo8n053r6.fsf@gitster.c.googlers.com/

Feedback and reviews are appreciated.

Regards,
Shourya Shukla

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 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 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

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

[Junio C Hamano]

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

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

I pointed out only these two, but that does not necessarily mean
they are the only ones.  Have you checked all the new code added by
the series?

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

I actually did check this side ;-) and am reasonably sure that there
aren't any other irrational choice of format specifiers.

Thanks.

Re: [PATCH 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.

> I pointed out only these two, but that does not necessarily mean
> they are the only ones.  Have you checked all the new code added by
> the series?

There is one more. It is not related to my patch series though. Here it
is:
----
static char *compute_rev_name(const char *sub_path, const char* object_id)
----
Would you like me to correct this one too?

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

>I actually did check this side ;-) and am reasonably sure that there
>aren't any other irrational choice of format specifiers.

Sure! No worries!

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

[Junio C Hamano]

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

>>> The definitions of 'verify_submodule_committish()' and
>>> 'print_submodule_summary()' had wrong styling in terms of the asterisk
>>> placement. Amend them.
>
>> I pointed out only these two, but that does not necessarily mean
>> they are the only ones.  Have you checked all the new code added by
>> the series?
>
> There is one more. It is not related to my patch series though.

Cleaning up the existing breakage is outside the scope your series,
but of course fixes as an independent patch is welcomed.

Thanks for checking.

[PATCH 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,dst}_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.

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..f1951680f7 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 = NULL;
 	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

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

[Kaartic Sivaraam]

On 25-08-2020 17:00, Shourya Shukla wrote:
> 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,dst}_abbrev' by guarding the

The change only affects `src_abbrev`. So, it's misleading to mention
`dst_abbrev` here.

> '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.
> 
> 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..f1951680f7 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 = NULL;

Unlike `src_abbrev`, I don't think we need to initilialize `dst_abbrev`
to NULL here as it would be assigned in all code paths.

>  	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;
>  			/*

-- 
Sivaraam

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

[Junio C Hamano]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

>> @@ -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;
>>  			/*

Interesting.  There is a mirroring if-else cascade that begins with
"if (S_ISGITLINK(p->mod_dst))" immediately after the if-else cascade
started here, and in there, the same verify_submodule_committish()
is called for oid_dst unconditionally.  Should the asymmetry bother
readers of the code, or is the source side somehow special and needs
extra care?

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

[Shourya Shukla]

> Interesting.  There is a mirroring if-else cascade that begins with
> "if (S_ISGITLINK(p->mod_dst))" immediately after the if-else cascade
> started here, and in there, the same verify_submodule_committish()
> is called for oid_dst unconditionally.  Should the asymmetry bother
> readers of the code, or is the source side somehow special and needs
> extra care?

I understand what you are trying to say. The thing is that the
conditional `if (S_ISGITLINK(p->mod_dst))` already guards the
`verify_submodule_committish` when we have a status of 'D'. So, we do
not need another similar if-statement for that. It does seem a bit weird
to someone who is reading this thing for the first time, hence, I will
mention this in the commit message.

Apologies for the late reply, I was a little busy with something.

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

[Shourya Shukla]

On 25/08 08:03, Kaartic Sivaraam wrote:
> On 25-08-2020 17:00, Shourya Shukla wrote:
> > 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,dst}_abbrev' by guarding the
> 
> The change only affects `src_abbrev`. So, it's misleading to mention
> `dst_abbrev` here.

I forgot to change that. Thank you for pointing this out.

> > '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.
> > 
> > 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..f1951680f7 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 = NULL;
> 
> Unlike `src_abbrev`, I don't think we need to initilialize `dst_abbrev`
> to NULL here as it would be assigned in all code paths.

Alright. Changed!

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

[Kaartic Sivaraam]

On 25-08-2020 17:00, Shourya Shukla wrote:
> Greetings,
> 
> The v3 of 'git submodule summary' port to C is currently in 'next'
> branch of git/git. Recently, the patch recieved some comments from
> Peff, Dscho and Kaartic:
> 
>     1. The definition of 'print_submodule_summary()' contained two
>        unused parameters namely 'missing_src' and 'missing_dst'. Hence,
>        I had to eliminate them as covered in the commit a22ffa950f
>        a22ffa950f (submodule: eliminate unused parameters from
>        print_submodule_summary(), 2020-08-21). Reported by Peff.
>        Junio also advised to make the output in case of an unexpected
>        file mode a bit more user friendly by outputting an octal instead
>        of a decimal.
> 
>     2. The function definitions of 'verify_submodule_committish()' and
>        'print_submodule_summary()' had wrong styling in terms of the
>        asterisk placement. Hence it was fixed in 32934998ee (submodule:
>        fix style in function definition, 2020-08-22). Reported by
>        Kaartic.
> 
>     2. The test script 't7421-submodule-summary-add.sh' failed in
>        Windows due to failure of t7421.4. Precisely, the 'test_i18ngrep'
>        check failed on Windows since the error message which was being
>        grepped was different on Windows; it was designed to work on
>        Linux. Therefore, we had to eliminate the grep check in t7421.4
>        and replace it with a check to see if there is any error message
>        or not using 'test_must_be_empty'. Also, to support this change,
>        we had to make some small changes in 'print_submodule_summary()'
>        function. The call to verify_submodule_committish()' had to be
>        guarded using 'p->status !=D' so that it isn't called when the SM
>        directory does not exist, therefore, the error message is not
>        displayed. This resulted in 82e0956cd2 (t7421: eliminate 'grep'
>        check in t7421.4 for mingw compatibility, 2020-08-22). Reported
>        by Dscho.
> 

While the cover letter is nice, it doesn't make much sense to refer to
patches that are part of the series using the commit hashes of their
"local" commits. It's more common to refer to them as using the position
of the patch such as [1/3] etc.

-- 
Sivaraam