[PATCH v3 0/8] fetch: introduce machine-parseable output

From: Patrick Steinhardt <ps@pks.im>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
	Felipe Contreras <felipe.contreras@gmail.com>,
	Glen Choo <chooglen@google.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	Jacob Keller <jacob.e.keller@intel.com>
Subject: [PATCH v3 0/8] fetch: introduce machine-parseable output
Date: Wed, 3 May 2023 13:34:14 +0200	[thread overview]
Message-ID: <cover.1683113177.git.ps@pks.im> (raw)
In-Reply-To: <cover.1681906948.git.ps@pks.im>

[-- Attachment #1: Type: text/plain, Size: 22706 bytes --]

Hi,

this is the third version of my patch series to introduce a
machine-parseable output for git-fetch(1).

Changes compared to v2:

    - A new commit that fixes `--no-recurse-submodules` not being
      honored for multi-remote fetches was added. This fixes issues that
      would otherwise be hit later in the patch series.

    - Tests were adjusted to explicitly verify stdout and stderr as
      standalone units instead of redirecting 2>&1.

    - Instead of introducing `--output-format` and a new "porcelain"
      format I've changed this to instead introduce `--porcelain`. What
      finally convinced me to go this way is Glen's list of preexisting
      commands that have `--porcelain`. So I decided to not be the only
      outlier here.

    - The DISPLAY_FORMAT_MAX macro was dropped in favor of Junio's
      proposal that uses designated initializers for the array.

    - Similar to `--negotiate-only`, the porcelain output is now
      incompatible with `--recurse-submodules=[yes|on-demand]`. This is
      done to avoid ambiguity when references in both the superproject
      and any of its submodules is updated.

Thanks again for all the feedback!

Patrick

Patrick Steinhardt (8):
  fetch: fix `--no-recurse-submodules` with multi-remote fetches
  fetch: split out tests for output format
  fetch: add a test to exercise invalid output formats
  fetch: fix missing from-reference when fetching HEAD:foo
  fetch: introduce `display_format` enum
  fetch: move display format parsing into main function
  fetch: move option related variables into main function
  fetch: introduce machine-parseable "porcelain" output format

 Documentation/fetch-options.txt |   6 +
 Documentation/git-fetch.txt     |   9 +
 builtin/fetch.c                 | 437 +++++++++++++++++++-------------
 t/t5510-fetch.sh                |  53 ----
 t/t5526-fetch-submodules.sh     |  31 +++
 t/t5574-fetch-output.sh         | 231 +++++++++++++++++
 6 files changed, 543 insertions(+), 224 deletions(-)
 create mode 100755 t/t5574-fetch-output.sh

Range-diff against v2:
-:  ---------- > 1:  4b2b0cfe15 fetch: fix `--no-recurse-submodules` with multi-remote fetches
1:  0d0d50d14c = 2:  6ebc7450ba fetch: split out tests for output format
2:  29d2c58914 ! 3:  78479922ac fetch: add a test to exercise invalid output formats
    @@ t/t5574-fetch-output.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
     +	test_when_finished "rm -rf clone" &&
     +	git clone . clone &&
     +
    -+	test_must_fail git -C clone -c fetch.output= fetch origin >actual 2>&1 &&
    ++	test_must_fail git -C clone -c fetch.output= fetch origin >actual.out 2>actual.err &&
     +	cat >expect <<-EOF &&
     +	fatal: invalid value for ${SQ}fetch.output${SQ}: ${SQ}${SQ}
     +	EOF
    -+	test_cmp expect actual &&
    ++	test_must_be_empty actual.out &&
    ++	test_cmp expect actual.err &&
     +
    -+	test_must_fail git -C clone -c fetch.output=garbage fetch origin >actual 2>&1 &&
    ++	test_must_fail git -C clone -c fetch.output=garbage fetch origin >actual.out 2>actual.err &&
     +	cat >expect <<-EOF &&
     +	fatal: invalid value for ${SQ}fetch.output${SQ}: ${SQ}garbage${SQ}
     +	EOF
    -+	test_cmp expect actual
    ++	test_must_be_empty actual.out &&
    ++	test_cmp expect actual.err
     +'
     +
      test_expect_success 'fetch aligned output' '
3:  d1fb6eeae7 ! 4:  46e1266ab0 fetch: fix missing from-reference when fetching HEAD:foo
    @@ t/t5574-fetch-output.sh: test_expect_success 'fetch compact output' '
      	test_cmp expect actual
      '

    -+test_expect_success 'fetch output with HEAD and --dry-run' '
    ++test_expect_success 'fetch output with HEAD' '
     +	test_when_finished "rm -rf head" &&
     +	git clone . head &&
     +
    -+	git -C head fetch --dry-run origin HEAD >actual 2>&1 &&
    ++	git -C head fetch --dry-run origin HEAD >actual.out 2>actual.err &&
     +	cat >expect <<-EOF &&
     +	From $(test-tool path-utils real_path .)/.
     +	 * branch            HEAD       -> FETCH_HEAD
     +	EOF
    -+	test_cmp expect actual &&
    ++	test_must_be_empty actual.out &&
    ++	test_cmp expect actual.err &&
     +
    -+	git -C head fetch origin HEAD >actual 2>&1 &&
    -+	test_cmp expect actual &&
    ++	git -C head fetch origin HEAD >actual.out 2>actual.err &&
    ++	test_must_be_empty actual.out &&
    ++	test_cmp expect actual.err &&
     +
    -+	git -C head fetch --dry-run origin HEAD:foo >actual 2>&1 &&
    ++	git -C head fetch --dry-run origin HEAD:foo >actual.out 2>actual.err &&
     +	cat >expect <<-EOF &&
     +	From $(test-tool path-utils real_path .)/.
     +	 * [new ref]         HEAD       -> foo
     +	EOF
    -+	test_cmp expect actual &&
    ++	test_must_be_empty actual.out &&
    ++	test_cmp expect actual.err &&
     +
    -+	git -C head fetch origin HEAD:foo >actual 2>&1 &&
    -+	test_cmp expect actual
    ++	git -C head fetch origin HEAD:foo >actual.out 2>actual.err &&
    ++	test_must_be_empty actual.out &&
    ++	test_cmp expect actual.err
     +'
     +
      test_expect_success '--no-show-forced-updates' '
4:  b545bf8bb9 ! 5:  acc0f7f520 fetch: introduce `display_format` enum
    @@ builtin/fetch.c: static void display_state_init(struct display_state *display_st
     +		break;
     +	}
     +	default:
    -+		BUG("unexpected display foramt %d", display_state->format);
    ++		BUG("unexpected display format %d", display_state->format);
      	}
      }

5:  4990d35998 = 6:  cd23440128 fetch: move display format parsing into main function
6:  cfe84129ab = 7:  edbc31013f fetch: move option related variables into main function
7:  0335e5eeb4 < -:  ---------- fetch: introduce new `--output-format` option
8:  d7c1bc1a80 ! 8:  e132d9494e fetch: introduce machine-parseable "porcelain" output format
    @@ Commit message
         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. If
    -    usecases come up for this in the future though it is easy enough to add
    -    a new "porcelain-v2" format that adds this information.
    +    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.

         Signed-off-by: Patrick Steinhardt <ps@pks.im>

    - ## Documentation/config/fetch.txt ##
    -@@ Documentation/config/fetch.txt: fetch.pruneTags::
    - 
    - fetch.output::
    - 	Control how ref update status is printed. Valid values are
    --	`full` and `compact`. Default value is `full`. See section
    --	OUTPUT in linkgit:git-fetch[1] for detail.
    -+	`full`, `compact` and `porcelain`. Default value is `full`.
    -+	See section OUTPUT in linkgit:git-fetch[1] for detail.
    - 
    - fetch.negotiationAlgorithm::
    - 	Control how information about the commits in the local repository
    -
      ## Documentation/fetch-options.txt ##
     @@ Documentation/fetch-options.txt: linkgit:git-config[1].
    + --dry-run::
    + 	Show what would be done, without making any changes.

    - --output-format::
    - 	Control how ref update status is printed. Valid values are
    --	`full` and `compact`. Default value is `full`. See section
    --	OUTPUT in linkgit:git-fetch[1] for detail.
    -+	`full`, `compact` and `porcelain`. Default value is `full`.
    -+	See section OUTPUT in linkgit:git-fetch[1] for detail.
    - 
    ++--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`

      ## Documentation/git-fetch.txt ##
    -@@ Documentation/git-fetch.txt: The output of "git fetch" depends on the transport method used; this
    - section describes the output when fetching over the Git protocol
    - (either locally or via ssh) and Smart HTTP protocol.
    - 
    --The status of the fetch is output in tabular form, with each line
    --representing the status of a single ref. Each line is of the form:
    -+The output format can be chosen either via the `fetch.output` config
    -+(see linkgit:git-config[1]), or via the `--output-format` switch.
    -+Supported values include:
    -+
    -+For the `full` and `compact` output formats, the status of the fetch is
    -+output in tabular, with each line representing the status of a single
    -+ref. Each line is of the form:
    - 
    - -------------------------------
    +@@ Documentation/git-fetch.txt: representing the status of a single ref. Each line is of the form:
       <flag> <summary> <from> -> <to> [<reason>]
      -------------------------------

    -+The `porcelain` 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:
    ++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>
    @@ builtin/fetch.c: enum display_format {
      	DISPLAY_FORMAT_FULL,
      	DISPLAY_FORMAT_COMPACT,
     +	DISPLAY_FORMAT_PORCELAIN,
    - 	DISPLAY_FORMAT_MAX,
    - };
    - 
    -@@ builtin/fetch.c: static const char * const display_formats[DISPLAY_FORMAT_MAX] = {
    - 	NULL,
    - 	"full",
    - 	"compact",
    -+	"porcelain",
      };

      struct display_state {
    @@ builtin/fetch.c: static void display_state_init(struct display_state *display_st
     +		/* We don't need to precompute anything here. */
     +		break;
      	default:
    --		BUG("unexpected display foramt %d", display_state->format);
    -+		BUG("unexpected display format %d", display_state->format);
    + 		BUG("unexpected display format %d", display_state->format);
      	}
    - }
    - 
     @@ builtin/fetch.c: static void print_compact(struct display_state *display_state,
      static void display_ref_update(struct display_state *display_state, char code,
      			       const char *summary, const char *error,
    @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state,
      					   summary_width);
      			warn_dangling_symref(stderr, dangling_msg, ref->name);
      		}
    +@@ builtin/fetch.c: 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");
    +@@ builtin/fetch.c: 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");
    + }
    + 
    + /* Fetch multiple remotes in parallel */
    +@@ builtin/fetch.c: 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,
    +@@ builtin/fetch.c: 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;
    +@@ builtin/fetch.c: static int fetch_finished(int result, struct strbuf *out,
    + 	return 0;
    + }
    + 
    +-static int fetch_multiple(struct string_list *list, int max_children)
    ++static int fetch_multiple(struct string_list *list, int max_children,
    ++			  enum display_format format)
    + {
    + 	int i, result = 0;
    + 	struct strvec argv = STRVEC_INIT;
    +@@ builtin/fetch.c: static int fetch_multiple(struct string_list *list, int max_children)
    + 
    + 	strvec_pushl(&argv, "fetch", "--append", "--no-auto-gc",
    + 		     "--no-write-commit-graph", NULL);
    +-	add_options_to_argv(&argv);
    ++	add_options_to_argv(&argv, format);
    + 
    + 	if (max_children != 1 && list->nr != 1) {
    +-		struct parallel_fetch_state state = { argv.v, list, 0, 0 };
    ++		struct parallel_fetch_state state = { argv.v, list, 0, 0, format };
    + 		const struct run_process_parallel_opts opts = {
    + 			.tr2_category = "fetch",
    + 			.tr2_label = "parallel/fetch",
    +@@ builtin/fetch.c: static int fetch_multiple(struct string_list *list, int max_children)
    + 
    + 			strvec_pushv(&cmd.args, argv.v);
    + 			strvec_push(&cmd.args, name);
    +-			if (verbosity >= 0)
    ++			if (verbosity >= 0 && format != DISPLAY_FORMAT_PORCELAIN)
    + 				printf(_("Fetching %s\n"), name);
    + 			cmd.git_cmd = 1;
    + 			if (run_command(&cmd)) {
    +@@ builtin/fetch.c: static int fetch_one(struct remote *remote, int argc, const char **argv,
    + 	return exit_code;
    + }
    + 
    ++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;
    +@@ builtin/fetch.c: 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")),
    +@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
    + 		fetch_config_from_gitmodules(sfjc, rs);
    + 	}
    + 
    ++	if (display_format == DISPLAY_FORMAT_PORCELAIN) {
    ++		switch (recurse_submodules_cli) {
    ++		case RECURSE_SUBMODULES_OFF:
    ++		case RECURSE_SUBMODULES_DEFAULT:
    ++			/*
    ++			 * Reference updates in submodules would be ambiguous
    ++			 * in porcelain mode, so we reject this combination.
    ++			 */
    ++			recurse_submodules = RECURSE_SUBMODULES_OFF;
    ++			break;
    ++
    ++		default:
    ++			die(_("options '%s' and '%s' cannot be used together"),
    ++			    "--porcelain", "--recurse-submodules");
    ++		}
    ++	}
    ++
    + 	if (negotiate_only && !negotiation_tip.nr)
    + 		die(_("--negotiate-only needs one or more --negotiation-tip=*"));
    + 
    +@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
    + 			max_children = fetch_parallel_config;
    + 
    + 		/* TODO should this also die if we have a previous partial-clone? */
    +-		result = fetch_multiple(&list, max_children);
    ++		result = fetch_multiple(&list, max_children, display_format);
    + 	}
    + 
    + 
    +@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
    + 		if (max_children < 0)
    + 			max_children = fetch_parallel_config;
    + 
    +-		add_options_to_argv(&options);
    ++		add_options_to_argv(&options, display_format);
    + 		result = fetch_submodules(the_repository,
    + 					  &options,
    + 					  submodule_prefix,

      ## t/t5574-fetch-output.sh ##
    -@@ t/t5574-fetch-output.sh: test_expect_success 'fetch compact output with multiple remotes' '
    +@@ t/t5574-fetch-output.sh: test_expect_success 'fetch compact output' '
      	test_cmp expect actual
      '

     +test_expect_success 'fetch porcelain output' '
    -+	test_when_finished "rm -rf porcelain-cfg porcelain-cli" &&
    ++	test_when_finished "rm -rf porcelain" &&
     +
     +	# Set up a bunch of references that we can use to demonstrate different
     +	# kinds of flag symbols in the output format.
    @@ t/t5574-fetch-output.sh: test_expect_success 'fetch compact output with multiple
     +	# namespaces so that we can test that rejected and force-updated
     +	# references are reported properly.
     +	refspecs="refs/heads/*:refs/unforced/* +refs/heads/*:refs/forced/*" &&
    -+	git clone . porcelain-cli &&
    -+	git clone . porcelain-cfg &&
    -+	git -C porcelain-cfg fetch origin $refspecs &&
    -+	git -C porcelain-cli fetch origin $refspecs &&
    ++	git clone . porcelain &&
    ++	git -C porcelain fetch origin $refspecs &&
     +
     +	# Now that we have set up the client repositories we can change our
     +	# local references.
    @@ t/t5574-fetch-output.sh: test_expect_success 'fetch compact output with multiple
     +	# Execute a dry-run fetch first. We do this to assert that the dry-run
     +	# and non-dry-run fetches produces the same output. Execution of the
     +	# fetch is expected to fail as we have a rejected reference update.
    -+	test_must_fail git -C porcelain-cfg -c fetch.output=porcelain fetch --dry-run --prune origin $refspecs >actual-dry-run-cfg &&
    -+	test_must_fail git -C porcelain-cli fetch --output-format=porcelain --dry-run --prune origin $refspecs >actual-dry-run-cli &&
    -+	test_cmp actual-dry-run-cfg actual-dry-run-cli &&
    -+	test_cmp expect actual-dry-run-cfg &&
    ++	test_must_fail git -C porcelain fetch \
    ++		--porcelain --dry-run --prune origin $refspecs >actual &&
    ++	test_cmp expect actual &&
     +
     +	# And now we perform a non-dry-run fetch.
    -+	test_must_fail git -C porcelain-cfg -c fetch.output=porcelain fetch --prune origin $refspecs >actual-cfg &&
    -+	test_must_fail git -C porcelain-cli fetch --output-format=porcelain --prune origin $refspecs >actual-cli &&
    -+	test_cmp actual-cfg actual-cli &&
    -+	test_cmp expect actual-cfg &&
    -+
    -+	# Ensure that the dry-run and non-dry-run output matches.
    -+	test_cmp actual-dry-run-cfg actual-cfg
    ++	test_must_fail git -C porcelain fetch \
    ++		--porcelain --prune origin $refspecs >actual 2>stderr &&
    ++	test_cmp expect actual &&
    ++	test_must_be_empty stderr
     +'
     +
    - test_expect_success 'fetch output with HEAD and --dry-run' '
    ++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
    ++
    ++	git -C porcelain fetch --porcelain --all >actual 2>stderr &&
    ++	test_cmp expect actual &&
    ++	test_must_be_empty stderr
    ++'
    ++
    ++test_expect_success 'fetch porcelain refuses to work with submodules' '
    ++	test_when_finished "rm -rf porcelain" &&
    ++
    ++	cat >expect <<-EOF &&
    ++	fatal: options ${SQ}--porcelain${SQ} and ${SQ}--recurse-submodules${SQ} cannot be used together
    ++	EOF
    ++
    ++	git init porcelain &&
    ++	test_must_fail git -C porcelain fetch --porcelain --recurse-submodules=yes 2>stderr &&
    ++	test_cmp expect stderr &&
    ++
    ++	test_must_fail git -C porcelain fetch --porcelain --recurse-submodules=on-demand 2>stderr &&
    ++	test_cmp expect stderr
    ++'
    ++
    + test_expect_success 'fetch output with HEAD' '
      	test_when_finished "rm -rf head" &&
      	git clone . head &&
    -@@ t/t5574-fetch-output.sh: test_expect_success 'fetch output with HEAD and --dry-run' '
    - 	test_cmp expect actual
    +@@ t/t5574-fetch-output.sh: test_expect_success 'fetch output with HEAD' '
    + 	test_cmp expect actual.err
      '

    -+test_expect_success 'fetch porcelain output with HEAD and --dry-run' '
    ++test_expect_success 'fetch porcelain output with HEAD' '
     +	test_when_finished "rm -rf head" &&
     +	git clone . head &&
     +	COMMIT_ID=$(git rev-parse HEAD) &&
     +
    -+	git -C head fetch --output-format=porcelain --dry-run origin HEAD >actual &&
    ++	git -C head fetch --porcelain --dry-run origin HEAD >actual &&
     +	cat >expect <<-EOF &&
     +	* $ZERO_OID $COMMIT_ID FETCH_HEAD
     +	EOF
     +	test_cmp expect actual &&
     +
    -+	git -C head fetch --output-format=porcelain --dry-run origin HEAD:foo >actual &&
    ++	git -C head fetch --porcelain origin HEAD >actual &&
    ++	test_cmp expect actual &&
    ++
    ++	git -C head fetch --porcelain --dry-run origin HEAD:foo >actual &&
     +	cat >expect <<-EOF &&
     +	* $ZERO_OID $COMMIT_ID refs/heads/foo
     +	EOF
    ++	test_cmp expect actual &&
    ++
    ++	git -C head fetch --porcelain origin HEAD:foo >actual &&
     +	test_cmp expect actual
     +'
     +
-- 
2.40.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]