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

From: Glen Choo <chooglen@google.com>
Date: Apr 26, 2023, 19:52 UTC
Message-ID: <kl6lzg6umne9.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <301138da039451519c6e60d6126e7756a54d346f.1681906949.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 5 quoted lines

> The output format is quite simple:
>
> ```
> <flag> <old-object-id> <new-object-id> <local-reference>\n
> ```

This format doesn't show the remote name or url that was fetched. That seems okay when fetching with a single remote, but it seems necessary with "--all". Perhaps you were planning to add that in a later series? If so, I think it's okay to call the "porcelain" format experimental, and forbid porcelain + --all until then.

Show 7 quoted lines

>
> We assume two conditions which are generally true:
>
>     - The old and new object IDs have fixed known widths and cannot
>       contain spaces.
>
>     - References cannot contain newlines.

This seems like a non-issue if we add a -z CLI option to indicate that entries should be NUL terminated instead of newline terminated, but that can be done as a followup.

Show 6 quoted lines

> 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.

The .c changes look good to me.

Show 17 quoted lines

> +test_expect_success 'fetch porcelain output with HEAD and --dry-run' '
> +	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 &&
> +	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 &&
> +	cat >expect <<-EOF &&
> +	* $ZERO_OID $COMMIT_ID refs/heads/foo
> +	EOF
> +	test_cmp expect actual
> +'

As mentioned upthread, I think this test isn't needed because "porcelain" wouldn't run into the bug we are checking for anyway.