From: Glen Choo <chooglen@google.com>
To: Patrick Steinhardt <ps@pks.im>, git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
Felipe Contreras <felipe.contreras@gmail.com>,
Jonathan Tan <jonathantanmy@google.com>,
Jacob Keller <jacob.e.keller@intel.com>
Subject: Re: [PATCH v3 8/8] fetch: introduce machine-parseable "porcelain" output format
Date: Mon, 08 May 2023 16:42:46 -0700 [thread overview]
Message-ID: <kl6lild21jbd.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <e132d9494ee7d8c4eb3a7a82b6420743c8c7a9b0.1683113177.git.ps@pks.im>
This version looks great! I only have minor comments.
Patrick Steinhardt <ps@pks.im> writes:
> A notable ommission here is that the output format does not include the
> remote from which a reference was fetched, which might be important
> information especially in the context of multi-remote fetches. But as
> such a format would require us to print the remote for every single
> reference update due to parallelizable fetches it feels wasteful for the
> most likely usecase, which is when fetching from a single remote.
>
> In a similar spirit, a second restriction is that this cannot be used
> with `--recurse-submodules`. This is because any reference updates would
> be ambiguous without also printing the repository in which the update
> happens.
>
> Considering that both multi-remote and submodule fetches are rather
> niche and likely not going to be useful for the majority of usecases
> these omissions 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.
As a point of clarification, I think these options aren't niche in
themselves, but they are more user-facing, so using them _in conjunction
with_ --porcelain is probably pretty niche, so I think this is okay for
now.
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -78,6 +78,12 @@ linkgit:git-config[1].
> --dry-run::
> Show what would be done, without making any changes.
>
> +--porcelain::
> + Print the output to standard output in an easy-to-parse format for
> + scripts. See section OUTPUT in linkgit:git-fetch[1] for details.
> ++
> +This is incompatible with `--recurse-submodules=[yes|on-demand]`.
> +
> ifndef::git-pull[]
> --[no-]write-fetch-head::
> Write the list of remote refs fetched in the `FETCH_HEAD`
> diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
> index fba66f1460..f123139c58 100644
> --- a/Documentation/git-fetch.txt
> +++ b/Documentation/git-fetch.txt
> @@ -204,6 +204,15 @@ representing the status of a single ref. Each line is of the form:
> <flag> <summary> <from> -> <to> [<reason>]
> -------------------------------
>
> +When using `--porcelain`, the output format is intended to be
> +machine-parseable. In contrast to the human-readable output formats it
> +thus prints to standard output instead of standard error. Each line is
> +of the form:
> +
> +-------------------------------
> +<flag> <old-object-id> <new-object-id> <local-reference>
> +-------------------------------
> +
> The status of up-to-date refs is shown only if the --verbose option is
> used.
I hadn't commented on the docs before, but they are very clear. Thanks!
> @@ -1830,6 +1857,7 @@ struct parallel_fetch_state {
> const char **argv;
> struct string_list *remotes;
> int next, result;
> + enum display_format format;
> };
>
> static int fetch_next_remote(struct child_process *cp,
> @@ -1849,7 +1877,7 @@ static int fetch_next_remote(struct child_process *cp,
> strvec_push(&cp->args, remote);
> cp->git_cmd = 1;
>
> - if (verbosity >= 0)
> + if (verbosity >= 0 && state->format != DISPLAY_FORMAT_PORCELAIN)
> printf(_("Fetching %s\n"), remote);
>
> return 1;
Here and elsewhere, I wonder if it's clearer to name the variable
"porcelain" and separate it from "enum display_format". Then we can
check "porcelain" directly instead of using "format ==
DISPLAY_FORMAT_PORCELAIN" as a proxy...
> +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;
> +}
> +
> int cmd_fetch(int argc, const char **argv, const char *prefix)
> {
> const char *bundle_uri;
> @@ -2104,6 +2140,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
> OPT_BOOL(0, "dry-run", &dry_run,
> N_("dry run")),
> + OPT_CALLBACK_F(0, "porcelain", &display_format, NULL, N_("machine-readable output"),
> + PARSE_OPT_NOARG|PARSE_OPT_NONEG, opt_parse_porcelain),
> OPT_BOOL(0, "write-fetch-head", &write_fetch_head,
> N_("write fetched references to the FETCH_HEAD file")),
> OPT_BOOL('k', "keep", &keep, N_("keep downloaded pack")),
e.g. since we are reusing the "display_format" variable, we need to make
sure we parse "--porcelain" after we read "fetch.output". I
double-checked to make sure we were doing the right thing, though it
would be nice to not have to worry about those sorts of things. This
shouldn't hold up the series though.
> +test_expect_success 'fetch porcelain with multiple remotes' '
> + test_when_finished "rm -rf porcelain" &&
> +
> + git clone . porcelain &&
> + git -C porcelain remote add second-remote "$PWD" &&
> + git -C porcelain fetch second-remote &&
> +
> + test_commit --no-tag multi-commit &&
> + old_commit=$(git rev-parse HEAD~) &&
> + new_commit=$(git rev-parse HEAD) &&
> +
> + cat >expect <<-EOF &&
> + $old_commit $new_commit refs/remotes/origin/force-updated
> + $old_commit $new_commit refs/remotes/second-remote/force-updated
> + EOF
The only thing in this test that relies on the previous test is that
HEAD is pointing to "force-updated", and it's hard to tell where HEAD is
since the previous test is so long. Could we create a new branch
instead?
next prev parent reply other threads:[~2023-05-08 23:42 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
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 [this message]
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=kl6lild21jbd.fsf@chooglen-macbookpro.roam.corp.google.com \
--to=chooglen@google.com \
--cc=felipe.contreras@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jacob.e.keller@intel.com \
--cc=jonathantanmy@google.com \
--cc=ps@pks.im \
/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).