Hi,
this is the fourth version of my patch series to introduce a
machine-parseable output format for git-fetch(1). It applies on top of
e9dffbc7f1 (Merge branch 'ps/fetch-ref-update-reporting', 2023-04-06).
Changes compared to v4:
- Patch 1/9: Simplified the test as proposed by Junio and Glen.
- Patch 3/9: Added a test to verify that `git fetch -c fetch.output`
without a value set fails as expected. Also dropped the tests that
checked whether stdout was empty.
- Patch 4/9: Reformulated the commit message to treat the missing
left-hand side of displayed references as an inconsistency instead
of a bug. I've also added a testcase to verify that direct OID
fetches continue to work as expected.
- Patch 5/9: New patch that makes calculation of the table width for
displayed reference updates self-contained in `refcol_width()`.
This is a preparatory refactoring that should make patch 6/9
easier to review.
- Patch 7/9: Refactored the code to parse the "fetch.output" config
variable inside of `git_fetch_config()` before we parse command
line options. Also fixed that the commit message was still
referring to `--output-format=porcelain` instead of the new
`--porcelain` switch.
- Patch 9/9: The `--porcelain` option is now a simple `OPT_BOOL()`
that can be negated. Added a test that `--no-porcelain` works as
expected.
Thanks for your feedback, Junio and Glen!
Patrick
Patrick Steinhardt (9):
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: print left-hand side when fetching HEAD:foo
fetch: refactor calculation of the display table width
fetch: introduce `display_format` enum
fetch: lift up parsing of "fetch.output" config variable
fetch: move option related variables into main function
fetch: introduce machine-parseable "porcelain" output format
Documentation/fetch-options.txt | 7 +
Documentation/git-fetch.txt | 9 +
builtin/fetch.c | 490 +++++++++++++++++++-------------
t/t5510-fetch.sh | 53 ----
t/t5526-fetch-submodules.sh | 13 +
t/t5574-fetch-output.sh | 293 +++++++++++++++++++
6 files changed, 611 insertions(+), 254 deletions(-)
create mode 100755 t/t5574-fetch-output.sh
Range-diff against v4:
1: d82b42ed34 ! 1: 02ee4fab7e fetch: fix `--no-recurse-submodules` with multi-remote fetches
@@ t/t5526-fetch-submodules.sh: test_expect_success 'fetch --all with --recurse-sub
+ cd src_clone &&
+ git remote add secondary ../src &&
+ git config submodule.recurse true &&
-+ git config fetch.parallel 0 &&
-+ git fetch --all --no-recurse-submodules 2>../actual
++ git fetch --all --no-recurse-submodules 2>../fetch-log
+ ) &&
-+
-+ cat >expect <<-EOF &&
-+ From ../src
-+ * [new branch] master -> secondary/master
-+ EOF
-+ test_cmp expect actual
++ ! grep "Fetching submodule" fetch-log
+'
+
test_done
2: 33112dc51a = 2: e459d8a1ec fetch: split out tests for output format
3: 006ea93afb ! 3: d503c425fe 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.out 2>actual.err &&
++ test_must_fail git -C clone -c fetch.output fetch origin 2>actual.err &&
++ cat >expect <<-EOF &&
++ error: missing value for ${SQ}fetch.output${SQ}
++ fatal: unable to parse ${SQ}fetch.output${SQ} from command-line config
++ EOF
++ test_cmp expect actual.err &&
++
++ test_must_fail git -C clone -c fetch.output= fetch origin 2>actual.err &&
+ cat >expect <<-EOF &&
+ fatal: invalid value for ${SQ}fetch.output${SQ}: ${SQ}${SQ}
+ EOF
-+ test_must_be_empty actual.out &&
+ test_cmp expect actual.err &&
+
-+ test_must_fail git -C clone -c fetch.output=garbage fetch origin >actual.out 2>actual.err &&
++ test_must_fail git -C clone -c fetch.output=garbage fetch origin 2>actual.err &&
+ cat >expect <<-EOF &&
+ fatal: invalid value for ${SQ}fetch.output${SQ}: ${SQ}garbage${SQ}
+ EOF
-+ test_must_be_empty actual.out &&
+ test_cmp expect actual.err
+'
+
4: e599ea6d33 ! 4: 2cc7318697 fetch: fix missing from-reference when fetching HEAD:foo
@@ Metadata
Author: Patrick Steinhardt <ps@xxxxxx>
## Commit message ##
- fetch: fix missing from-reference when fetching HEAD:foo
+ fetch: print left-hand side when fetching HEAD:foo
`store_updated_refs()` parses the remote reference for two purposes:
@@ Commit message
name and can thus be used for both cases. But if the remote reference is
HEAD, the parsed remote reference becomes empty. This is intended when
we write the FETCH_HEAD, where we skip writing the note in that case.
- But it is not intended when displaying the updated references and would
- cause us to miss the left-hand side of the displayed reference update:
+ But when displaying the updated references this leads to inconsistent
+ output where the left-hand side of reference updates is missing in some
+ cases:
```
- $ git fetch origin HEAD:foo
- From https://github.com/git/git
- * [new ref] -> foo
- ```
-
- The HEAD string is clearly missing from the left-hand side of the arrow,
- which is further stressed by the point that the following commands show
- the left-hand side as expected:
-
- ```
- $ git fetch origin HEAD
+ $ git fetch origin HEAD HEAD:explicit-head :implicit-head main
From https://github.com/git/git
* branch HEAD -> FETCH_HEAD
-
- $ git fetch origin master
- From https://github.com/git/git
- * branch master -> FETCH_HEAD
- * branch master -> origin/master
+ * [new ref] -> explicit-head
+ * [new ref] -> implicit-head
+ * branch main -> FETCH_HEAD
```
+ This behaviour has existed ever since the table-based output has been
+ introduced for git-fetch(1) via 165f390250 (git-fetch: more terse fetch
+ output, 2007-11-03) and was never explicitly documented either in the
+ commit message or in any of our tests. So while it may not be a bug per
+ se, it feels like a weird inconsistency and not like it was a concious
+ design decision.
+
The logic of how we compute the remote reference name that we ultimately
pass to `display_ref_update()` is not easy to follow. There are three
different cases here:
- When the remote reference name is "HEAD" we set the remote
reference name to the empty string. This is the case that causes
- the bug to occur, where we would indeed want to print "HEAD"
- instead of the empty string. This is what `prettify_refname()`
- would return.
+ the left-hand side to go missing, where we would indeed want to
+ print "HEAD" instead of the empty string. This is what
+ `prettify_refname()` would return.
- When the remote reference name has a well-known prefix then we
strip this prefix. This matches what `prettify_refname()` does.
@@ Commit message
matches what `prettify_refname()` does.
As the return value of `prettify_refname()` would do the correct thing
- for us in all three cases, we can fix the bug by passing through the
- full remote reference name to `display_ref_update()`, which learns to
- call `prettify_refname()`. At the same time, this also simplifies the
- code a bit.
+ for us in all three cases, we can thus fix the inconsistency by passing
+ through the full remote reference name to `display_ref_update()`, which
+ learns to call `prettify_refname()`. At the same time, this also
+ simplifies the code a bit.
Note that this patch also changes formatting of the block that computes
- the "kind" and "what" variables. This is done on purpose so that it is
- part of the diff, hopefully making the change easier to comprehend.
+ the "kind" (which is the category like "branch" or "tag") and "what"
+ (which is the prettified reference name like "master" or "v1.0")
+ variables. This is done on purpose so that it is part of the diff,
+ hopefully making the change easier to comprehend.
Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
@@ t/t5574-fetch-output.sh: test_expect_success 'fetch compact output' '
+ test_must_be_empty actual.out &&
+ test_cmp expect actual.err
+'
++
++test_expect_success 'fetch output with object ID' '
++ test_when_finished "rm -rf object-id" &&
++ git clone . object-id &&
++ commit=$(git rev-parse HEAD) &&
++
++ git -C object-id fetch --dry-run origin $commit:object-id >actual.out 2>actual.err &&
++ cat >expect <<-EOF &&
++ From $(test-tool path-utils real_path .)/.
++ * [new ref] $commit -> object-id
++ EOF
++ test_must_be_empty actual.out &&
++ test_cmp expect actual.err &&
++
++ git -C object-id fetch origin $commit:object-id >actual.out 2>actual.err &&
++ test_must_be_empty actual.out &&
++ test_cmp expect actual.err
++'
+
test_expect_success '--no-show-forced-updates' '
mkdir forced-updates &&
-: ---------- > 5: bb1a591c2f fetch: refactor calculation of the display table width
5: 80ac00b0c4 ! 6: 3cac552f5f fetch: introduce `display_format` enum
@@ builtin/fetch.c: enum {
char *url;
int url_len, shown_url;
-@@ builtin/fetch.c: static int refcol_width(const struct ref *ref, int compact_format)
- static void display_state_init(struct display_state *display_state, struct ref *ref_map,
- const char *raw_url)
- {
-- struct ref *rm;
- const char *format = "full";
- int i;
-
@@ builtin/fetch.c: static void display_state_init(struct display_state *display_state, struct ref *
git_config_get_string_tmp("fetch.output", &format);
@@ builtin/fetch.c: static void display_state_init(struct display_state *display_st
die(_("invalid value for '%s': '%s'"),
"fetch.output", format);
-- display_state->refcol_width = 10;
-- for (rm = ref_map; rm; rm = rm->next) {
-- int width;
+- display_state->refcol_width = refcol_width(ref_map, display_state->compact_format);
+ switch (display_state->format) {
+ case DISPLAY_FORMAT_FULL:
-+ case DISPLAY_FORMAT_COMPACT: {
-+ struct ref *rm;
-
-- if (rm->status == REF_STATUS_REJECT_SHALLOW ||
-- !rm->peer_ref ||
-- !strcmp(rm->name, "HEAD"))
-- continue;
-+ display_state->refcol_width = 10;
-+ for (rm = ref_map; rm; rm = rm->next) {
-+ int width;
-
-- width = refcol_width(rm, display_state->compact_format);
-+ if (rm->status == REF_STATUS_REJECT_SHALLOW ||
-+ !rm->peer_ref ||
-+ !strcmp(rm->name, "HEAD"))
-+ continue;
-
-- /*
-- * Not precise calculation for compact mode because '*' can
-- * appear on the left hand side of '->' and shrink the column
-- * back.
-- */
-- if (display_state->refcol_width < width)
-- display_state->refcol_width = width;
-+ width = refcol_width(rm, display_state->format == DISPLAY_FORMAT_COMPACT);
-+
-+ /*
-+ * Not precise calculation for compact mode because '*' can
-+ * appear on the left hand side of '->' and shrink the column
-+ * back.
-+ */
-+ if (display_state->refcol_width < width)
-+ display_state->refcol_width = width;
-+ }
-+
++ case DISPLAY_FORMAT_COMPACT:
++ display_state->refcol_width = refcol_width(ref_map,
++ display_state->format == DISPLAY_FORMAT_COMPACT);
+ break;
-+ }
+ default:
+ BUG("unexpected display format %d", display_state->format);
- }
++ }
}
+ static void display_state_release(struct display_state *display_state)
@@ builtin/fetch.c: static void display_ref_update(struct display_state *display_state, char code,
const char *remote, const char *local,
int summary_width)
6: 826b8b7bc0 ! 7: 0c3dbcd09f fetch: move display format parsing into main function
@@ Metadata
Author: Patrick Steinhardt <ps@xxxxxx>
## Commit message ##
- fetch: move display format parsing into main function
+ fetch: lift up parsing of "fetch.output" config variable
Parsing the display format happens inside of `display_state_init()`. As
we only need to check for a simple config entry, this is a natural
location to put this code as it means that display-state logic is neatly
contained in a single location.
- We're about to introduce a output format though that is intended to be
- parseable by machines, for example inside of a script. In that case it
- becomes a bit awkward of an interface if you have to call git-fetch(1)
- with the `fetch.output` config key set. We're thus going to introduce a
- new `--output-format` switch for git-fetch(1) so that the output format
- can be configured more directly.
-
- This means we'll have to hook parsing of the display format into the
- command line options parser. Having the code to determine the actual
- output format scattered across two different sites is hard to reason
- about though.
+ We're about to introduce a new "porcelain" output format though that is
+ intended to be parseable by machines, for example inside of a script.
+ This format can be enabled by passing the `--porcelain` switch to
+ git-fetch(1). As a consequence, we'll have to add a second callsite that
+ influences the output format, which will become awkward to handle.
Refactor the code such that callers are expected to pass the display
format that is to be used into `display_state_init()`. This allows us to
@@ Commit message
Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
## builtin/fetch.c ##
-@@ builtin/fetch.c: static int refcol_width(const struct ref *ref, int compact_format)
+@@ builtin/fetch.c: static int fetch_write_commit_graph = -1;
+ static int stdin_refspecs = 0;
+ static int negotiate_only;
+
++struct fetch_config {
++ enum display_format display_format;
++};
++
+ static int git_fetch_config(const char *k, const char *v, void *cb)
+ {
++ struct fetch_config *fetch_config = cb;
++
+ if (!strcmp(k, "fetch.prune")) {
+ fetch_prune_config = git_config_bool(k, v);
+ return 0;
+@@ builtin/fetch.c: static int git_fetch_config(const char *k, const char *v, void *cb)
+ return 0;
+ }
+
++ if (!strcmp(k, "fetch.output")) {
++ if (!v)
++ return config_error_nonbool(k);
++ else if (!strcasecmp(v, "full"))
++ fetch_config->display_format = DISPLAY_FORMAT_FULL;
++ else if (!strcasecmp(v, "compact"))
++ fetch_config->display_format = DISPLAY_FORMAT_COMPACT;
++ else
++ die(_("invalid value for '%s': '%s'"),
++ "fetch.output", v);
++ }
++
+ return git_default_config(k, v, cb);
+ }
+
+@@ builtin/fetch.c: static int refcol_width(const struct ref *ref_map, int compact_format)
}
static void display_state_init(struct display_state *display_state, struct ref *ref_map,
@@ builtin/fetch.c: static void display_state_init(struct display_state *display_st
-
switch (display_state->format) {
case DISPLAY_FORMAT_FULL:
- case DISPLAY_FORMAT_COMPACT: {
+ case DISPLAY_FORMAT_COMPACT:
@@ builtin/fetch.c: static int backfill_tags(struct display_state *display_state,
}
@@ builtin/fetch.c: static int fetch_one(struct remote *remote, int argc, const cha
sigchain_pop(SIGPIPE);
refspec_clear(&rs);
transport_disconnect(gtransport);
-@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
+@@ builtin/fetch.c: static int fetch_one(struct remote *remote, int argc, const char **argv,
+
+ int cmd_fetch(int argc, const char **argv, const char *prefix)
{
++ struct fetch_config config = {
++ .display_format = DISPLAY_FORMAT_FULL,
++ };
int i;
const char *bundle_uri;
-+ enum display_format display_format = DISPLAY_FORMAT_UNKNOWN;
struct string_list list = STRING_LIST_INIT_DUP;
- struct remote *remote = NULL;
- int result = 0;
@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
- argc = parse_options(argc, argv, prefix,
- builtin_fetch_options, builtin_fetch_usage, 0);
-
-+ if (display_format == DISPLAY_FORMAT_UNKNOWN) {
-+ const char *format = "full";
-+
-+ git_config_get_string_tmp("fetch.output", &format);
-+ if (!strcasecmp(format, "full"))
-+ display_format = DISPLAY_FORMAT_FULL;
-+ else if (!strcasecmp(format, "compact"))
-+ display_format = DISPLAY_FORMAT_COMPACT;
-+ else
-+ die(_("invalid value for '%s': '%s'"),
-+ "fetch.output", format);
-+ }
-+
- if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
- recurse_submodules = recurse_submodules_cli;
+ free(anon);
+ }
+- git_config(git_fetch_config, NULL);
++ git_config(git_fetch_config, &config);
+ if (the_repository->gitdir) {
+ prepare_repo_settings(the_repository);
+ the_repository->settings.command_requires_full_index = 0;
@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
} else if (remote) {
if (filter_options.choice || has_promisor_remote())
fetch_one_setup_partial(remote);
- result = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs);
+ result = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs,
-+ display_format);
++ config.display_format);
} else {
int max_children = max_jobs;
7: 20f2e061d6 ! 8: 8e33a08c35 fetch: move option related variables into main function
@@ builtin/fetch.c: static struct string_list deepen_not = STRING_LIST_INIT_NODUP;
-static int stdin_refspecs = 0;
-static int negotiate_only;
- static int git_fetch_config(const char *k, const char *v, void *cb)
- {
+ struct fetch_config {
+ enum display_format display_format;
@@ builtin/fetch.c: static int parse_refmap_arg(const struct option *opt, const char *arg, int unset
return 0;
}
@@ builtin/fetch.c: static int parse_refmap_arg(const struct option *opt, const cha
static void unlock_pack(unsigned int flags)
{
if (gtransport)
-@@ builtin/fetch.c: static int fetch_one(struct remote *remote, int argc, const char **argv,
-
- int cmd_fetch(int argc, const char **argv, const char *prefix)
- {
+@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
+ struct fetch_config config = {
+ .display_format = DISPLAY_FORMAT_FULL,
+ };
- int i;
- const char *bundle_uri;
+ const char *submodule_prefix = "";
- enum display_format display_format = DISPLAY_FORMAT_UNKNOWN;
+ const char *bundle_uri;
struct string_list list = STRING_LIST_INIT_DUP;
struct remote *remote = NULL;
+ int all = 0, multiple = 0;
8: 24ae381950 ! 9: d49152c220 fetch: introduce machine-parseable "porcelain" output format
@@ builtin/fetch.c: enum display_format {
struct display_state {
@@ builtin/fetch.c: static void display_state_init(struct display_state *display_state, struct ref *
-
+ display_state->refcol_width = refcol_width(ref_map,
+ display_state->format == DISPLAY_FORMAT_COMPACT);
break;
- }
+ case DISPLAY_FORMAT_PORCELAIN:
+ /* We don't need to precompute anything here. */
+ break;
@@ builtin/fetch.c: static int fetch_multiple(struct string_list *list, int max_chi
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;
- }
+@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
+ int fetch_write_commit_graph = -1;
+ int stdin_refspecs = 0;
+ int negotiate_only = 0;
++ int porcelain = 0;
+ int i;
-+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;
+ struct option builtin_fetch_options[] = {
@@ 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, "porcelain", &porcelain, N_("machine-readable output")),
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) {
++
++ if (porcelain) {
+ switch (recurse_submodules_cli) {
+ case RECURSE_SUBMODULES_OFF:
+ case RECURSE_SUBMODULES_DEFAULT:
@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
+ die(_("options '%s' and '%s' cannot be used together"),
+ "--porcelain", "--recurse-submodules");
+ }
++
++ config.display_format = DISPLAY_FORMAT_PORCELAIN;
+ }
+
if (negotiate_only && !negotiation_tip.nr)
@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
/* 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);
++ result = fetch_multiple(&list, max_children, config.display_format);
}
-
+-
+ /*
+ * This is only needed after fetch_one(), which does not fetch
+ * submodules by itself.
@@ 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);
++ add_options_to_argv(&options, config.display_format);
result = fetch_submodules(the_repository,
&options,
submodule_prefix,
@@ t/t5574-fetch-output.sh: test_expect_success 'fetch compact output' '
+ test_must_be_empty stderr &&
+ test_cmp expect stdout
+'
++
++test_expect_success 'fetch --no-porcelain overrides previous --porcelain' '
++ test_when_finished "rm -rf no-porcelain" &&
++
++ git switch --create no-porcelain &&
++ git clone . no-porcelain &&
++ test_commit --no-tag no-porcelain &&
++ old_commit=$(git rev-parse --short HEAD~) &&
++ new_commit=$(git rev-parse --short HEAD) &&
++
++ cat >expect <<-EOF &&
++ From $(test-tool path-utils real_path .)/.
++ $old_commit..$new_commit no-porcelain -> origin/no-porcelain
++ EOF
++
++ git -C no-porcelain fetch --porcelain --no-porcelain >stdout 2>stderr &&
++ test_cmp expect stderr &&
++ test_must_be_empty stdout
++'
+
test_expect_success 'fetch output with HEAD' '
test_when_finished "rm -rf head" &&
@@ t/t5574-fetch-output.sh: test_expect_success 'fetch output with HEAD' '
+ test_cmp expect actual
+'
+
- test_expect_success '--no-show-forced-updates' '
- mkdir forced-updates &&
- (
+ test_expect_success 'fetch output with object ID' '
+ test_when_finished "rm -rf object-id" &&
+ git clone . object-id &&
--
2.40.1
Attachment:
signature.asc
Description: PGP signature
