Re: [PATCH v9 2/6] submodule: rename strbuf variable

From: Junio C Hamano <gitster@pobox.com>
To: Calvin Wan <calvinwan@google.com>
Cc: git@vger.kernel.org, avarab@gmail.com, chooglen@google.com,
	newren@gmail.com, jonathantanmy@google.com,
	phillip.wood123@gmail.com
Subject: Re: [PATCH v9 2/6] submodule: rename strbuf variable
Date: Thu, 02 Mar 2023 16:25:03 -0800	[thread overview]
Message-ID: <xmqqv8jiheao.fsf@gitster.g> (raw)
In-Reply-To: 20230302220251.1474923-2-calvinwan@google.com

Calvin Wan <calvinwan@google.com> writes:

> A prepatory change for a future patch that moves the status parsing
> logic to a separate function.
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>  submodule.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)

> Subject: Re: [PATCH v9 2/6] submodule: rename strbuf variable

What strbuf variable renamed to what?

I have a feeling that squashing this and 3/6 into a single patch,
and pass buf.buf and buf.len to the new helper function without
introducing an intermediate variables in the caller, would make the
resulting code easier to follow.

In any case, nice factoring out of a useful helper function.

> diff --git a/submodule.c b/submodule.c
> index fae24ef34a..faf37c1101 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1906,25 +1906,28 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
>  
>  	fp = xfdopen(cp.out, "r");
>  	while (strbuf_getwholeline(&buf, fp, '\n') != EOF) {
> +		char *str = buf.buf;
> +		const size_t len = buf.len;
> +
>  		/* regular untracked files */
> -		if (buf.buf[0] == '?')
> +		if (str[0] == '?')
>  			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
>  
> -		if (buf.buf[0] == 'u' ||
> -		    buf.buf[0] == '1' ||
> -		    buf.buf[0] == '2') {
> +		if (str[0] == 'u' ||
> +		    str[0] == '1' ||
> +		    str[0] == '2') {
>  			/* T = line type, XY = status, SSSS = submodule state */
> -			if (buf.len < strlen("T XY SSSS"))
> +			if (len < strlen("T XY SSSS"))
>  				BUG("invalid status --porcelain=2 line %s",
> -				    buf.buf);
> +				    str);
>  
> -			if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
> +			if (str[5] == 'S' && str[8] == 'U')
>  				/* nested untracked file */
>  				dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
>  
> -			if (buf.buf[0] == 'u' ||
> -			    buf.buf[0] == '2' ||
> -			    memcmp(buf.buf + 5, "S..U", 4))
> +			if (str[0] == 'u' ||
> +			    str[0] == '2' ||
> +			    memcmp(str + 5, "S..U", 4))
>  				/* other change */
>  				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
>  		}