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

From: Patrick Steinhardt <ps@pks.im>
Date: May 10, 2023, 12:35 UTC
Message-ID: <ZFuPgh2ad-Cw1r3P@ncase>
In-Reply-To: <xmqqjzxhw802.fsf@gitster.g>

On Tue, May 09, 2023 at 01:43:41PM -0700, Junio C Hamano wrote:

Show 11 quoted lines

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

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

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.

Show 12 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?

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