| Message ID | cover.1683721293.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | fetch: introduce machine-parseable output | expand |
Patrick Steinhardt <ps@pks.im> writes: > - 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. Thank you for the attention to this small detail of valueless configuration variable definition. > - 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. Again, the direct OID fetch is a good thing to test here. I noticed that the test added here insists that the standard output stream is empty when the command errors out, which is not consistent with [3/9] above. > - 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. It is an excellent idea to avoid calls to refcol_width() for each ref that gets shown and make the helper responsible for computing the required maxwidth. The result indeed has become easier to follow as you mentioned in your response to my review on the previous round. > - 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. As a standalone step, it leaves an impression that the step makes the way we handle the output-format configuration variable inconsistent with the way we handle the other configuration variables, but I think it is a good place to stop for the purpose of this topic. It lays a good foundation for future clean-up after the dust settles from this topic---we may want to move global variables assigned in the git_fetch_config() into the fetch_config structure. > - 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. OK, this time the familiar "prepare a variable to its default, let config callback to overwrite it by reading configuration variables, and then let the command line option override it" is used and the result easy to follow. I do not think .display_format is never assigned DISPLAY_FORMAT_UNKNOWN with this change, so [6/9] could lose the value from the enum, I think. The defensive switch statement that has BUG() to notice an erroneous caller that pass values other than DISPLAY_FORMAT_{FULL,COMPACT} is still a good idea. Looking good. Thanks.
On Wed, May 10, 2023 at 11:05:19AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: [snip] > > - 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. > > Again, the direct OID fetch is a good thing to test here. I noticed > that the test added here insists that the standard output stream is > empty when the command errors out, which is not consistent with [3/9] > above. I think you misread the test: yes, we do test stdout and stderr separately and in many cases assert that stdout is in fact empty. But none of the added tests are about failing commands. So given that: - The added tests explicitly are about verifying the output format. - The distinction between stdout and stderr matters. - The distinction matters even more with the addition of `--porcelain`. I think that explicitly verifing both output streams is the correct thing to do. > > - 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. > > OK, this time the familiar "prepare a variable to its default, let > config callback to overwrite it by reading configuration variables, > and then let the command line option override it" is used and the > result easy to follow. I do not think .display_format is never > assigned DISPLAY_FORMAT_UNKNOWN with this change, so [6/9] could > lose the value from the enum, I think. The defensive switch > statement that has BUG() to notice an erroneous caller that pass > values other than DISPLAY_FORMAT_{FULL,COMPACT} is still a good > idea. Ah, right, `DISPLAY_FORMAT_UNKNOWN` isn't really needed anymore. I think it's still good to have valid values of the enum start with `1` so that it becomes easier to detect cases where we accidentally use a default initialized variable. But that can be achieved without giving the default value an explicit name. I'll refrain from sending a new version just to remove this constant as it doesn't really feel worth it, though. But please, let me know in case you disagree and I'll send an updated version. Thanks Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Wed, May 10, 2023 at 11:05:19AM -0700, Junio C Hamano wrote: >> Patrick Steinhardt <ps@pks.im> writes: > [snip] >> > - 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. >> >> Again, the direct OID fetch is a good thing to test here. I noticed >> that the test added here insists that the standard output stream is >> empty when the command errors out, which is not consistent with [3/9] >> above. > > I think you misread the test: yes, we do test stdout and stderr > separately and in many cases assert that stdout is in fact empty. But > none of the added tests are about failing commands. Ah, you are absolutely right. Sorry for the noise---I do not have an objection to ensure that program output in the successful case is predictable. My main concern is to avoid giving unneeded assurance in the failing case. >> assigned DISPLAY_FORMAT_UNKNOWN with this change, so [6/9] could >> lose the value from the enum, I think. The defensive switch >> statement that has BUG() to notice an erroneous caller that pass >> values other than DISPLAY_FORMAT_{FULL,COMPACT} is still a good >> idea. > > Ah, right, `DISPLAY_FORMAT_UNKNOWN` isn't really needed anymore. I think > it's still good to have valid values of the enum start with `1` so that > it becomes easier to detect cases where we accidentally use a default > initialized variable. But that can be achieved without giving the > default value an explicit name. Unless we explicitly take advantage of UNKNOWN being 0 and perform clever defaulting, the result is far safer if you get rid of it, and instead give 0 to a real choice that is used as the default, for two reasons: (1) naturally 0 initialization work fine, (2) we have one less enum constant people (or editor's auto-completer) assign to a variable by mistake instead of a real one they intended to. I agree with the last sentence of your paragraph above.
Patrick Steinhardt wrote: > On Wed, May 10, 2023 at 11:05:19AM -0700, Junio C Hamano wrote: > > OK, this time the familiar "prepare a variable to its default, let > > config callback to overwrite it by reading configuration variables, > > and then let the command line option override it" is used and the > > result easy to follow. I do not think .display_format is never > > assigned DISPLAY_FORMAT_UNKNOWN with this change, so [6/9] could > > lose the value from the enum, I think. The defensive switch > > statement that has BUG() to notice an erroneous caller that pass > > values other than DISPLAY_FORMAT_{FULL,COMPACT} is still a good > > idea. > > Ah, right, `DISPLAY_FORMAT_UNKNOWN` isn't really needed anymore. I think > it's still good to have valid values of the enum start with `1` so that > it becomes easier to detect cases where we accidentally use a default > initialized variable. But that can be achieved without giving the > default value an explicit name. I think it's standard to have an element like that. It could be UNKNOWN, NULL, INVALID, or even DEFAULT. For example, you could have code like this: if (format == DISPLAY_FORMAT_DEFAULT) format = DISPLAY_FORMAT_FULL; This would distinguish cases in which the user did not specify a display format and we choose a default, from those where the user explicitely chose the format that happens to be the default.
Patrick Steinhardt <ps@pks.im> writes: > 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. I didn't spot any blocking issues in this version, and the various improvements (especially 6-7/9) are really welcome. I also read through Junio's comments, but I didn't spot anything that I thought should block the series, so I'm happy to leave Reviewed-by: Glen Choo <chooglen@google.com>
On Thu, May 11, 2023 at 06:09:03PM -0700, Glen Choo wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > 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. > > I didn't spot any blocking issues in this version, and the various > improvements (especially 6-7/9) are really welcome. I also read through > Junio's comments, but I didn't spot anything that I thought should block > the series, so I'm happy to leave > > Reviewed-by: Glen Choo <chooglen@google.com> Thanks a lot for your reviews! Patrick
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@pks.im> ## 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@pks.im> @@ 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@pks.im> ## 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@pks.im> ## 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 &&