From: Patrick Steinhardt <ps@pks.im>
To: Glen Choo <chooglen@google.com>
Cc: git@vger.kernel.org, Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH 8/8] fetch: introduce machine-parseable "porcelain" output format
Date: Fri, 28 Apr 2023 10:51:32 +0200 [thread overview]
Message-ID: <ZEuJFOBCdnCLdGc5@ncase> (raw)
In-Reply-To: <kl6lfs8knc9f.fsf@chooglen-macbookpro.roam.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 6461 bytes --]
On Thu, Apr 27, 2023 at 04:20:12PM -0700, Glen Choo wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > On Wed, Apr 26, 2023 at 12:52:46PM -0700, Glen Choo wrote:
> >> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > So with that in mind, I'd like to continue ignoring this issue for now
> > and just not report the remote that the ref came from. But I'd also
> > argue that we don't have to restrict porcelain mode to single-remote
> > fetches: it can still be useful to do multi-remote fetches even without
> > the information where a certain reference update comes from. So any kind
> > of restriction would feel artificial to me here.
> >
> > Furthermore, I'd argue that it is not necessary to label the format as
> > experimental only because of this limitation. With the refactorings done
> > in this and the preceding patch series it is easy to add a new format in
> > case there indeed is somebody that would have a usecase for this. The
> > "porcelain" format should stay stable, and if we decide that we want to
> > also report the remote for each reference in a follow-up we can easily
> > add a "porcelain-v2" or "porcelain-with-remote" format.
>
> I agree that the current form is useful, but it seems quite likely that
> users will want the remote name/url at some point. In the majority of
> cases, the user can parse the remote name from the updated local ref and
> if they want, they look up the url by asking "git config", but that
> breaks down quite quickly if they have a custom refspec.
>
> My suggestion to label it experimental was meant to give us the freedom
> to change the "first version" in backwards-incompatible ways that we
> otherwise couldn't. We could say "here's porcelain-v2, use this and
> ignore porcelain-v1", but it would be even nicer if the user didn't need
> to think about porcelain-v1. The idea is that we could call it
> experimental, see what users need in the wild, then make changes without
> having to create a new "porcelain-v2" mode.
>
> (I realize in hindsight that calling it experimental sounds a bit
> insulting. I thought your proposal was quite sensible, actually. i
> apologize if it came across that way)
No offense taken, so no need to apologize.
> Perhaps part of the reason why you'd prefer not to make it experimental
> is that users who might want this feature are too afraid of
> backwards-incompatible changes. I think that's fair.
Exactly. I'm implementing this whole feature with a concrete usecase at
GitLab in mind, and if we declare this as experimental then it will
become a whole lot more risky for us to actually adopt it. Because if
the output format may change at any point in time, then the code that
builds on top of it is likely to be broken.
> A different way of
> achieving the same flexibility would be to make the "porcelain" format
> extensible (like git status --porcelain=v2). A future, multivalued
> "--porcelain-options=show-remote-url,show-remote-ref" would cover
> our bases.
Yeah, that would indeed be quite flexible. Theoretically, there is no
reason why we couldn't have `--output-format=porcelain,show-remote-url`
though.
> 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. Furthermore, I assume that the overlap of machines that want to
parse all reference updates done in a multi-remote fetch while also
being able to exactly tell which remote brought in what updates is going
to be tiny.
> >> > With these assumptions, the output format becomes unambiguously
> >> > parseable. Furthermore, given that this output is designed to be
> >> > consumed by scripts, the machine-readable data is printed to stdout
> >> > instead of stderr like the human-readable output is. This is mostly done
> >> > so that other data printed to stderr, like error messages or progress
> >> > meters, don't interfere with the parseable data.
> >>
> >> Sending the 'main output' to stdout makes sense to me, but this (and
> >> possibly respecting -z) sounds like a different mode of operation, not
> >> just a matter of formats. It seems different enough that I'd prefer not
> >> to piggyback on "fetch.output" for this (even though this adds more
> >> surface to the interface...).
> >>
> >> We could add --porcelain and say that "fetch.output" is ignored if
> >> --porcelain is also given. That also eliminates the need for
> >> --output-format, I think.
> >
> > I was thinking about this initially, as well. But ultimately I decided
> > against this especially because of your second paragraph: we'd now need
> > to think about precedence of options and mutual exclusion, and that to
> > me feels like an interface that is less obvious than a single knob that
> > works as you'd expect.
>
> 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?
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.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2023-04-28 8:52 UTC|newest]
Thread overview: 120+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-19 12:31 [PATCH 0/8] fetch: introduce machine-parseable output Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 1/8] fetch: split out tests for output format Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 2/8] fetch: add a test to exercise invalid output formats Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 3/8] fetch: fix missing from-reference when fetching HEAD:foo Patrick Steinhardt
2023-04-26 19:20 ` Jacob Keller
2023-04-27 10:58 ` Patrick Steinhardt
2023-04-26 19:21 ` Jacob Keller
2023-04-27 10:58 ` Patrick Steinhardt
2023-04-26 19:25 ` Glen Choo
2023-04-27 10:58 ` Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 4/8] fetch: introduce `display_format` enum Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 5/8] fetch: move display format parsing into main function Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 6/8] fetch: move option related variables " Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 7/8] fetch: introduce new `--output-format` option Patrick Steinhardt
2023-04-26 19:40 ` Glen Choo
2023-04-27 10:58 ` Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 8/8] fetch: introduce machine-parseable "porcelain" output format Patrick Steinhardt
2023-04-26 19:52 ` Glen Choo
2023-04-27 10:58 ` Patrick Steinhardt
2023-04-27 23:20 ` Glen Choo
2023-04-28 8:51 ` Patrick Steinhardt [this message]
2023-04-28 17:20 ` Glen Choo
2023-05-02 20:55 ` Felipe Contreras
2023-04-24 20:17 ` [PATCH 0/8] fetch: introduce machine-parseable output Felipe Contreras
2023-04-25 9:58 ` Patrick Steinhardt
2023-04-26 19:14 ` Jacob Keller
2023-04-26 20:23 ` Junio C Hamano
2023-04-26 20:30 ` Jacob Keller
2023-04-27 10:58 ` Patrick Steinhardt
2023-04-27 19:46 ` Jacob Keller
2023-04-27 22:49 ` Glen Choo
2023-04-26 20:24 ` Junio C Hamano
2023-04-26 18:54 ` Glen Choo
2023-04-26 21:14 ` Glen Choo
2023-04-26 19:17 ` Jacob Keller
2023-04-27 11:13 ` [PATCH v2 " Patrick Steinhardt
2023-04-27 11:13 ` [PATCH v2 1/8] fetch: split out tests for output format Patrick Steinhardt
2023-04-29 17:34 ` SZEDER Gábor
2023-05-03 11:21 ` Patrick Steinhardt
2023-04-27 11:13 ` [PATCH v2 2/8] fetch: add a test to exercise invalid output formats Patrick Steinhardt
2023-04-27 11:13 ` [PATCH v2 3/8] fetch: fix missing from-reference when fetching HEAD:foo Patrick Steinhardt
2023-04-27 17:26 ` Glen Choo
2023-04-27 19:49 ` Jacob Keller
2023-04-27 11:13 ` [PATCH v2 4/8] fetch: introduce `display_format` enum Patrick Steinhardt
2023-04-27 11:13 ` [PATCH v2 5/8] fetch: move display format parsing into main function Patrick Steinhardt
2023-04-27 11:13 ` [PATCH v2 6/8] fetch: move option related variables " Patrick Steinhardt
2023-04-27 21:52 ` Junio C Hamano
2023-04-27 11:13 ` [PATCH v2 7/8] fetch: introduce new `--output-format` option Patrick Steinhardt
2023-04-27 22:01 ` Junio C Hamano
2023-04-28 22:03 ` Glen Choo
2023-05-03 9:12 ` Patrick Steinhardt
2023-04-28 22:31 ` Glen Choo
2023-05-03 9:43 ` Patrick Steinhardt
2023-05-03 11:36 ` Patrick Steinhardt
2023-04-27 11:13 ` [PATCH v2 8/8] fetch: introduce machine-parseable "porcelain" output format Patrick Steinhardt
2023-04-27 19:52 ` Jacob Keller
2023-04-28 22:42 ` Glen Choo
2023-05-03 11:34 ` [PATCH v3 0/8] fetch: introduce machine-parseable output Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 1/8] fetch: fix `--no-recurse-submodules` with multi-remote fetches Patrick Steinhardt
2023-05-08 22:51 ` Glen Choo
2023-05-09 12:41 ` Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 2/8] fetch: split out tests for output format Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 3/8] fetch: add a test to exercise invalid output formats Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 4/8] fetch: fix missing from-reference when fetching HEAD:foo Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 5/8] fetch: introduce `display_format` enum Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 6/8] fetch: move display format parsing into main function Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 7/8] fetch: move option related variables " Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 8/8] fetch: introduce machine-parseable "porcelain" output format Patrick Steinhardt
2023-05-08 23:42 ` Glen Choo
2023-05-09 12:41 ` Patrick Steinhardt
2023-05-09 0:03 ` Glen Choo
2023-05-03 16:48 ` [PATCH v3 0/8] fetch: introduce machine-parseable output Junio C Hamano
2023-05-03 16:53 ` Junio C Hamano
2023-05-04 7:57 ` Patrick Steinhardt
2023-05-09 0:06 ` Glen Choo
2023-05-09 12:42 ` Patrick Steinhardt
2023-05-09 13:01 ` [PATCH v4 " Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 1/8] fetch: fix `--no-recurse-submodules` with multi-remote fetches Patrick Steinhardt
2023-05-09 17:49 ` Junio C Hamano
2023-05-09 18:27 ` Glen Choo
2023-05-10 12:34 ` Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 2/8] fetch: split out tests for output format Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 3/8] fetch: add a test to exercise invalid output formats Patrick Steinhardt
2023-05-09 17:58 ` Junio C Hamano
2023-05-10 12:34 ` Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 4/8] fetch: fix missing from-reference when fetching HEAD:foo Patrick Steinhardt
2023-05-09 19:28 ` Junio C Hamano
2023-05-10 12:34 ` Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 5/8] fetch: introduce `display_format` enum Patrick Steinhardt
2023-05-09 20:19 ` Junio C Hamano
2023-05-10 12:35 ` Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 6/8] fetch: move display format parsing into main function Patrick Steinhardt
2023-05-09 20:35 ` Junio C Hamano
2023-05-09 22:30 ` Glen Choo
2023-05-10 12:35 ` Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 7/8] fetch: move option related variables " Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 8/8] fetch: introduce machine-parseable "porcelain" output format Patrick Steinhardt
2023-05-09 20:43 ` Junio C Hamano
2023-05-10 12:35 ` Patrick Steinhardt
2023-05-10 12:33 ` [PATCH v5 0/9] fetch: introduce machine-parseable output Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 1/9] fetch: fix `--no-recurse-submodules` with multi-remote fetches Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 2/9] fetch: split out tests for output format Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 3/9] fetch: add a test to exercise invalid output formats Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 4/9] fetch: print left-hand side when fetching HEAD:foo Patrick Steinhardt
2023-05-12 0:16 ` Glen Choo
2023-05-13 16:59 ` Jeff King
2023-05-15 5:15 ` Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 5/9] fetch: refactor calculation of the display table width Patrick Steinhardt
2023-05-12 0:49 ` Glen Choo
2023-05-10 12:34 ` [PATCH v5 6/9] fetch: introduce `display_format` enum Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 7/9] fetch: lift up parsing of "fetch.output" config variable Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 8/9] fetch: move option related variables into main function Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 9/9] fetch: introduce machine-parseable "porcelain" output format Patrick Steinhardt
2023-05-12 1:02 ` Glen Choo
2023-05-10 18:05 ` [PATCH v5 0/9] fetch: introduce machine-parseable output Junio C Hamano
2023-05-11 11:05 ` Patrick Steinhardt
2023-05-11 16:53 ` Junio C Hamano
2023-05-11 17:24 ` Felipe Contreras
2023-05-12 1:09 ` Glen Choo
2023-05-12 7:16 ` Patrick Steinhardt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZEuJFOBCdnCLdGc5@ncase \
--to=ps@pks.im \
--cc=chooglen@google.com \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).