On Tue, May 09, 2023 at 01:43:41PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> 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:
signature.asc
Description: PGP signature
