Re: [PATCH v2 2/4] submodule: move status parsing into function

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Oct 11 2022, Calvin Wan wrote:

> A future patch requires the ability to parse the output of git
> status --porcelain=2. Move parsing code from is_submodule_modified to
> parse_status_porcelain.
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>  submodule.c | 71 +++++++++++++++++++++++++++++------------------------
>  1 file changed, 39 insertions(+), 32 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 72b295b87b..a3410ed8f0 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1864,6 +1864,43 @@ int fetch_submodules(struct repository *r,
>  	return spf.result;
>  }
>  
> +static int parse_status_porcelain(char *buf, unsigned *dirty_submodule, int ignore_untracked)
> +{
> +	/* regular untracked files */
> +	if (buf[0] == '?')
> +		*dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
> +
> +	if (buf[0] == 'u' ||
> +		buf[0] == '1' ||
> +		buf[0] == '2') {
> +		/* T = line type, XY = status, SSSS = submodule state */
> +		if (strlen(buf) < strlen("T XY SSSS"))
> +			BUG("invalid status --porcelain=2 line %s",
> +				buf);
> +
> +		if (buf[5] == 'S' && buf[8] == 'U')
> +			/* nested untracked file */
> +			*dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
> +
> +		if (buf[0] == 'u' ||
> +			buf[0] == '2' ||
> +			memcmp(buf + 5, "S..U", 4))
> +			/* other change */
> +			*dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
> +	}
> +
> +	if ((*dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
> +		((*dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
> +		ignore_untracked)) {
> +		/*
> +		* We're not interested in any further information from
> +		* the child any more, neither output nor its exit code.
> +		*/
> +		return 1;
> +	}
> +	return 0;
> +}
> +
>  unsigned is_submodule_modified(const char *path, int ignore_untracked)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
> @@ -1900,39 +1937,9 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
>  
>  	fp = xfdopen(cp.out, "r");
>  	while (strbuf_getwholeline(&buf, fp, '\n') != EOF) {
> -		/* regular untracked files */
> -		if (buf.buf[0] == '?')
> -			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
> -
> -		if (buf.buf[0] == 'u' ||
> -		    buf.buf[0] == '1' ||
> -		    buf.buf[0] == '2') {
> -			/* T = line type, XY = status, SSSS = submodule state */
> -			if (buf.len < strlen("T XY SSSS"))
> -				BUG("invalid status --porcelain=2 line %s",
> -				    buf.buf);
> -
> -			if (buf.buf[5] == 'S' && buf.buf[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))
> -				/* other change */
> -				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
> -		}
> -
> -		if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
> -		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
> -		     ignore_untracked)) {
> -			/*
> -			 * We're not interested in any further information from
> -			 * the child any more, neither output nor its exit code.
> -			 */
> -			ignore_cp_exit_code = 1;
> +		ignore_cp_exit_code = parse_status_porcelain(buf.buf, &dirty_submodule, ignore_untracked);
> +		if (ignore_cp_exit_code)
>  			break;
> -		}
>  	}
>  	fclose(fp);

This isn't just a code move, but a rewrite of much of the code to accept
a "char *buf" rather than a "struct strbuf buf".

I can see in a later commit that you'd like to change this to accept a
.string from a string_list.


Without changing anything else, if you lead with a commit that does (in
the initial loop):

	char *str = buf.buf;
        const size_t len = buf.len;

And then make it use "buf" and "len" instead you could follow with the
move commit, which at that ponit would benefit from the rename detection
more more than it does now.

Also: If we have a strbuf in this caller let's pass a "len", and not
here make it need to do a strlen() on every line when we've computed it
already, the other caller could just pass another strlen([...].string).