From bec587d4c11e851c1e7b5ed7890d627c8346d1cb Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Tue, 18 Jan 2022 16:00:54 -0800 Subject: [PATCH 1/4] fetch: use goto cleanup in cmd_fetch() Replace an early return with 'goto cleanup' in cmd_fetch() so that the string_list is always cleared (the string_list_clear() call is purely cleanup; the string_list is not reused). This makes cleanup consistent so that a subsequent commit can use 'goto cleanup' to bail out early. Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/fetch.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index f7abbc31ff..eab73d7417 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -2076,7 +2076,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) gtransport->smart_options->acked_commits = &acked_commits; } else { warning(_("Protocol does not support --negotiate-only, exiting.")); - return 1; + result = 1; + goto cleanup; } if (server_options.nr) gtransport->server_options = &server_options; @@ -2132,8 +2133,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) strvec_clear(&options); } - string_list_clear(&list, 0); - prepare_repo_settings(the_repository); if (fetch_write_commit_graph > 0 || (fetch_write_commit_graph < 0 && @@ -2151,5 +2150,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) if (enable_auto_gc) run_auto_maintenance(verbosity < 0); + cleanup: + string_list_clear(&list, 0); return result; } From 135a12bc1472290ca6b9a4c2f06c838a1495a612 Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Tue, 18 Jan 2022 16:00:55 -0800 Subject: [PATCH 2/4] fetch: skip tasks related to fetching objects cmd_fetch() does the following with the assumption that objects are fetched: * Run gc * Write commit graphs (if enabled by fetch.writeCommitGraph=true) However, neither of these tasks makes sense if objects are not fetched e.g. `git fetch --negotiate-only` never fetches objects. Speed up cmd_fetch() by bailing out early if we know for certain that objects will not be fetched. cmd_fetch() can bail out early whenever objects are not fetched, but for now this only considers --negotiate-only. The same optimization does not apply to `git fetch --dry-run` because that actually fetches objects; the dry run refers to not updating refs. Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/fetch.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/builtin/fetch.c b/builtin/fetch.c index eab73d7417..06e514b07e 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -2133,6 +2133,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) strvec_clear(&options); } + /* + * Skip irrelevant tasks because we know objects were not + * fetched. + * + * NEEDSWORK: as a future optimization, we can return early + * whenever objects were not fetched e.g. if we already have all + * of them. + */ + if (negotiate_only) + goto cleanup; + prepare_repo_settings(the_repository); if (fetch_write_commit_graph > 0 || (fetch_write_commit_graph < 0 && From 386c076a863cfafd733b71564245be973e3d1bda Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Tue, 18 Jan 2022 16:00:56 -0800 Subject: [PATCH 3/4] fetch --negotiate-only: do not update submodules `git fetch --negotiate-only` is an implementation detail of push negotiation and, unlike most `git fetch` invocations, does not actually update the main repository. Thus it should not update submodules even if submodule recursion is enabled. This is not just slow, it is wrong e.g. push negotiation with "submodule.recurse=true" will cause submodules to be updated because it invokes `git fetch --negotiate-only`. Fix this by disabling submodule recursion if --negotiate-only was given. Since this makes --negotiate-only and --recurse-submodules incompatible, check for this invalid combination and die. This does not use the "goto cleanup" introduced in the previous commit because we want to recurse through submodules whenever a ref is fetched, and this can happen without introducing new objects. Signed-off-by: Glen Choo Signed-off-by: Junio C Hamano --- Documentation/fetch-options.txt | 1 + builtin/fetch.c | 24 +++++++++++++++++++++++- t/t5516-fetch-push.sh | 12 ++++++++++++ t/t5702-protocol-v2.sh | 12 ++++++++++++ 4 files changed, 48 insertions(+), 1 deletion(-) diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt index e967ff1874..f903683189 100644 --- a/Documentation/fetch-options.txt +++ b/Documentation/fetch-options.txt @@ -71,6 +71,7 @@ configuration variables documented in linkgit:git-config[1], and the ancestors of the provided `--negotiation-tip=*` arguments, which we have in common with the server. + +This is incompatible with `--recurse-submodules=[yes|on-demand]`. Internally this is used to implement the `push.negotiate` option, see linkgit:git-config[1]. diff --git a/builtin/fetch.c b/builtin/fetch.c index 06e514b07e..dc6e637428 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -75,6 +75,7 @@ static struct transport *gtransport; static struct transport *gsecondary; static const char *submodule_prefix = ""; static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT; +static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT; static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND; static int shown_url = 0; static struct refspec refmap = REFSPEC_INIT_FETCH; @@ -166,7 +167,7 @@ static struct option builtin_fetch_options[] = { N_("prune remote-tracking branches no longer on remote")), OPT_BOOL('P', "prune-tags", &prune_tags, N_("prune local tags no longer on remote and clobber changed tags")), - OPT_CALLBACK_F(0, "recurse-submodules", &recurse_submodules, N_("on-demand"), + OPT_CALLBACK_F(0, "recurse-submodules", &recurse_submodules_cli, N_("on-demand"), N_("control recursive fetching of submodules"), PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules), OPT_BOOL(0, "dry-run", &dry_run, @@ -1996,6 +1997,27 @@ 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 (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT) + recurse_submodules = recurse_submodules_cli; + + if (negotiate_only) { + switch (recurse_submodules_cli) { + case RECURSE_SUBMODULES_OFF: + case RECURSE_SUBMODULES_DEFAULT: + /* + * --negotiate-only should never recurse into + * submodules. Skip it by setting recurse_submodules to + * RECURSE_SUBMODULES_OFF. + */ + recurse_submodules = RECURSE_SUBMODULES_OFF; + break; + + default: + die(_("--negotiate-only and --recurse-submodules cannot be used together")); + } + } + if (recurse_submodules != RECURSE_SUBMODULES_OFF) { int *sfjc = submodule_fetch_jobs_config == -1 ? &submodule_fetch_jobs_config : NULL; diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 7831a38dde..c3bd0db2f2 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -229,6 +229,18 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f test_i18ngrep "push negotiation failed" err ' +test_expect_success 'push with negotiation does not attempt to fetch submodules' ' + mk_empty submodule_upstream && + test_commit -C submodule_upstream submodule_commit && + git submodule add ./submodule_upstream submodule && + mk_empty testrepo && + git push testrepo $the_first_commit:refs/remotes/origin/first_commit && + test_commit -C testrepo unrelated_commit && + git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit && + git -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err && + ! grep "Fetching submodule" err +' + test_expect_success 'push without wildcard' ' mk_empty testrepo && diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 78f85b0714..3f3ad24299 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -628,6 +628,18 @@ test_expect_success 'usage: --negotiate-only without --negotiation-tip' ' test_cmp err.expect err.actual ' +test_expect_success 'usage: --negotiate-only with --recurse-submodules' ' + cat >err.expect <<-\EOF && + fatal: --negotiate-only and --recurse-submodules cannot be used together + EOF + + test_must_fail git -c protocol.version=2 -C client fetch \ + --negotiate-only \ + --recurse-submodules \ + origin 2>err.actual && + test_cmp err.expect err.actual +' + test_expect_success 'file:// --negotiate-only' ' SERVER="server" && URI="file://$(pwd)/server" && From de4eaae63a87ee33baf477ed10e6e97d649084cf Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 20 Jan 2022 13:58:43 -0800 Subject: [PATCH 4/4] fetch: help translators by reusing the same message template Follow the example set by 12909b6b (i18n: turn "options are incompatible" into "cannot be used together", 2022-01-05) and use the same message string to reduce the need for translation. Reported-by: Jiang Xin Helped-by: Glen Choo Signed-off-by: Junio C Hamano --- builtin/fetch.c | 3 ++- t/t5702-protocol-v2.sh | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index dc6e637428..5c329f9835 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -2014,7 +2014,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) break; default: - die(_("--negotiate-only and --recurse-submodules cannot be used together")); + die(_("options '%s' and '%s' cannot be used together"), + "--negotiate-only", "--recurse-submodules"); } } diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 3f3ad24299..b83f3f5ffe 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -630,7 +630,7 @@ test_expect_success 'usage: --negotiate-only without --negotiation-tip' ' test_expect_success 'usage: --negotiate-only with --recurse-submodules' ' cat >err.expect <<-\EOF && - fatal: --negotiate-only and --recurse-submodules cannot be used together + fatal: options '\''--negotiate-only'\'' and '\''--recurse-submodules'\'' cannot be used together EOF test_must_fail git -c protocol.version=2 -C client fetch \