patch, 3 partssubmodule: fixup to summary-v3

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

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

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

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

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