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

From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Aug 25, 2020, 14:33 UTC
Message-ID: <2a1ea501-4974-4d74-fe3c-d173bbe76855@gmail.com>
In-Reply-To: <20200825113020.71801-4-shouryashukla.oo@gmail.com>

On 25-08-2020 17:00, Shourya Shukla wrote:

Show 10 quoted lines

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

Show 29 quoted lines

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

Show 15 quoted lines

>  	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