| Message ID | cover.1683113177.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | fetch: introduce machine-parseable output | expand |
Patrick Steinhardt <ps@pks.im> writes: > this is the third version of my patch series to introduce a > machine-parseable output for git-fetch(1). Which base did you pick to build these 8 patches? It seems that 6/8 is not happy when the series is attempted to be applied on top of many recent tips of 'master' that I tried (including 667fcf4e15 that the previous iteration was queued on top of in my tree). Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Patrick Steinhardt <ps@pks.im> writes: > >> this is the third version of my patch series to introduce a >> machine-parseable output for git-fetch(1). > > Which base did you pick to build these 8 patches? It seems that 6/8 > is not happy when the series is attempted to be applied on top of > many recent tips of 'master' that I tried (including 667fcf4e15 that > the previous iteration was queued on top of in my tree). OK, I found e9dffbc7 (Merge branch 'ps/fetch-ref-update-reporting', 2023-04-06). I can manage from here. Thanks.
On Wed, May 03, 2023 at 09:53:04AM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Patrick Steinhardt <ps@pks.im> writes: > > > >> this is the third version of my patch series to introduce a > >> machine-parseable output for git-fetch(1). > > > > Which base did you pick to build these 8 patches? It seems that 6/8 > > is not happy when the series is attempted to be applied on top of > > many recent tips of 'master' that I tried (including 667fcf4e15 that > > the previous iteration was queued on top of in my tree). > > OK, I found e9dffbc7 (Merge branch 'ps/fetch-ref-update-reporting', > 2023-04-06). I can manage from here. > > Thanks. Yeah, that's where I based it off from. Sorry for not explicitly mentioning this fact. Patrick
Patrick Steinhardt <ps@pks.im> writes: > Hi, > > this is the third version of my patch series to introduce a > machine-parseable output for git-fetch(1). This version looks great! I only had minor comments this time, mainly on docs and tests. Thanks for your patience and receptiveness, and for the fruitful discussions :)
On Mon, May 08, 2023 at 05:06:28PM -0700, Glen Choo wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > Hi, > > > > this is the third version of my patch series to introduce a > > machine-parseable output for git-fetch(1). > > This version looks great! I only had minor comments this time, mainly on > docs and tests. > > Thanks for your patience and receptiveness, and for the fruitful > discussions :) Thank _you_ for the extensive feedback and constructive criticism. Patrick
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 +' +