Re: [PATCH v4 8/8] fetch: introduce machine-parseable "porcelain" output format

From: Junio C Hamano <gitster@pobox.com>
Date: May 9, 2023, 20:43 UTC
Message-ID: <xmqqjzxhw802.fsf@gitster.g>
In-Reply-To: <24ae381950249fd5c1cd799b75dda1407495d9d4.1683636885.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 6 quoted lines

> Considering that both multi-remote and submodule fetches are user-facing
> features, using them in conjunction with `--porcelain` that is intended
> for scripting purposes is likely not going to be useful in the majority
> of cases. With that in mind these restrictions feel acceptable. If
> usecases for either of these come up in the future though it is easy
> enough to add a new "porcelain-v2" format that adds this information.

Two steps ago, the proposed log message still mentioned "--output-format", which may want to be proofread again and revised.

Show 17 quoted lines

> @@ -1786,7 +1810,8 @@ static int add_remote_or_group(const char *name, struct string_list *list)
>  	return 1;
>  }
>  
> -static void add_options_to_argv(struct strvec *argv)
> +static void add_options_to_argv(struct strvec *argv,
> +				enum display_format format)
>  {
>  	if (dry_run)
>  		strvec_push(argv, "--dry-run");
> @@ -1822,6 +1847,8 @@ static void add_options_to_argv(struct strvec *argv)
>  		strvec_push(argv, "--ipv6");
>  	if (!write_fetch_head)
>  		strvec_push(argv, "--no-write-fetch-head");
> +	if (format == DISPLAY_FORMAT_PORCELAIN)
> +		strvec_pushf(argv, "--porcelain");
>  }

Hmph.

[PATCH 9/8] may want to also introduce and pass down the "--output-format=full/compact" option, but that is clearly outside of the scope of this step.

Show 6 quoted lines

> +static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset)
> +{
> +	enum display_format *format = opt->value;
> +	*format = DISPLAY_FORMAT_PORCELAIN;
> +	return 0;
> +}

Lack of "if (unset) ..." worries me.  Shouldn't the code allow

	git fetch --porcelain --no-porcelain

to defeat an earlier one and revert back to the default?

Other than that the changes in this step were mostly expected and I didn't spot anything glaringly wrong, but I didn't spend as much time as I did for the other steps.

Thanks.