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

From: Glen Choo <chooglen@google.com>
Date: Apr 28, 2023, 17:20 UTC
Message-ID: <kl6ledo33ovx.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <ZEuJFOBCdnCLdGc5@ncase>

Patrick Steinhardt <ps@pks.im> writes:

Show 7 quoted lines

>> I'm happy with either approach as long as we don't have to bikeshed
>> about the "perfect" porcelain output :)
>
> Agreed, and that's why I'm currently defending the "good enough" format.
> It should likely work for most usecases that exist out there. The target
> audience is going to be quite small here as this is not a user-directed
> feature.

Okay, I agree that this is "good enough" for most cases and we can extend it if needed.

Show 16 quoted lines

>> My full thoughts on this are in
>> 
>>   https://lore.kernel.org/git/kl6lildhlz3i.fsf@chooglen-macbookpro.roam.corp.google.com
>> 
>> but the short version is that I'm not sure if I expect something as
>> innocuous-looking as --output-format would imply other, machine-friendly
>> things (like stdout instead of stderr), and using --porcelain might make
>> option precedence clearer in some situtations (like if -z is given).
>
> I'm not even sure that `-z` makes sense in this context. If we see cases
> where not using `-z` can cause the machine-readable interface to become
> unparseable then this is a bug in the output format, if you ask me.
> Mostly because the whole intent of it is to be machine-parseable. So if
> we output data that can e.g. contain newlines, then we must not use
> newlines as part of the output format or alternatively escape them. Why
> let the author of the script shoot themselves into the foot?

Agreed. I think the current output format is resilient enough, just that "-z" is something we choose to support for plumbing anyway.

Show 7 quoted lines

> Anyway, I'm digressing. It's hard for me to decide what to do right now.
> The thread with Junio and Jacob points into the direction of keeping the
> `--output-format=` interface, while this thread points into the other
> direction. I'm naturally more inclined to keep `--output-format=`,
> mostly because I personally feel like it's the more obvious interface.
> But I also see your point, so it's not really a choice of right-or-wrong
> here, but rather of style.

Yes. All sides seem to understand the tradeoffs, but we value them differently.

I'd personally prefer to err on the side of consistency, because even if the existing behavior is less-than-ideal since:

1) This makes it possible for us to fix it in a consistent, well-thought
   out way, so we don't have to decide the future for the whole project
   in a git-fetch series.
2) At least it still behaves how _some_ users have come to expect it.

As for which is more consistent, a grep for --porcelain over Documentation/* shows:

- git-push
- git-status
- git-worktree
- git-blame
- git-commit

Most of them already output to stdout. The lone exception is git-push, which does exactly this "use stdout for the main output, but keep using stderr for debugging output" behavior you added, so using --porcelain seems somewhat consistent.

On the other hand, I couldn't find any other option that switches from stderr to stdout (we sometimes say --stdout to say "use stdout instead of a file", but that's different), so if we added this behavior to "git fetch --[output-]format", this might be the first.

(Sidenote: most instances of --format are for templated-style format, but "git-replace" does accept an enum for it. I don't think this should weigh heavily in our decision though.)

For the other, non git-push commands, --porcelain is treated like an output format and respects "last one wins" regular option precedence, e.g. in the case of "git status --porcelain --short". I find this behavior somewhat confusing (I'd expect --porcelain to win, or at least be incompatible), but at least we can consistently change this in the future [*].

That said, I'm willing to accept that I'm biased toward my own ideas. If others don't find "--format=porcelain implies stdout" confusing, I can accept that decision.

[*] In all likelihood, we probably can't make the change to --porcelain for backwards-compatibility reasons, but we could introduce a synonym with the behavior we want ("--machine"?) and have OPT_PARSE_MACHINE handle both --porcelain and --machine.