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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2463 bytes --]

On Tue, May 09, 2023 at 01:43:41PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 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.

It's a relict from previous iterations. I've fixed the preceding patch
to talk about `--porcelain` instead.

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

We don't have `--output-format` now though, but only the `--porcelain`
switch. The only way to configure "full"/"compact" output formats is via
the "fetch.output" config variable right now.

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

We pass `PARSE_OPT_NOARG|PARSE_OPT_NONEG`, so this isn't really much of
a concern. But yeah, it wouldn't allow `--no-porcelain`, and supporting
it is trivial enough to do. I'll change this to a `OPT_BOOL()`.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]