cmd_fetch() performs certain tasks with the assumption that objects are fetched, but `git fetch --negotiate-only` does not fetch objects, as its name implies. This is results in behavior that is unnecessary at best, and incorrect at worst: * Submodules are updated if enabled by recurse.submodules=true, but negotiation fetch doesn't actually update the repo, so this doesn't make sense (introduced in [1]). * Commit graphs will be written if enabled by fetch.writeCommitGraph=true. But according to Documentation/config/fetch.txt [2], this should only be done if a pack-file is downloaded. * gc is run, but according to [3], we only do this because we expect `git fetch` to introduce objects. Instead of disabling these tasks piecemeal, make cmd_fetch() return early if we know for certain that objects will not be fetched. We can return early whenever objects are not fetched, but for now this only considers --negotiate-only. [1] 7dce19d374 (fetch/pull: Add the --recurse-submodules option, 2010-11-12) [2] 50f26bd035 (fetch: add fetch.writeCommitGraph config setting, 2019-09-02) [3] 131b8fcbfb (fetch: run gc --auto after fetching, 2013-01-26) Signed-off-by: Glen Choo <chooglen@xxxxxxxxxx> --- Thanks for the review, Jonathan! Changes since v1: * added more context to commit message * added a NEEDSWORK comment Interdiff against v1: diff --git a/builtin/fetch.c b/builtin/fetch.c index 01865b5c09..85091af99b 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -2122,7 +2122,14 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) string_list_clear(&list, 0); - /* skip irrelevant tasks if objects were not fetched */ + /* + * 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) return result; builtin/fetch.c | 29 ++++++++++++++++++++++++----- t/t5516-fetch-push.sh | 12 ++++++++++++ 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index f7abbc31ff..85091af99b 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1996,6 +1996,17 @@ 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 (negotiate_only) { + /* + * --negotiate-only should never recurse into + * submodules, so there is no need to read .gitmodules. + */ + recurse_submodules = RECURSE_SUBMODULES_OFF; + if (!negotiation_tip.nr) + die(_("--negotiate-only needs one or more --negotiate-tip=*")); + } + if (recurse_submodules != RECURSE_SUBMODULES_OFF) { int *sfjc = submodule_fetch_jobs_config == -1 ? &submodule_fetch_jobs_config : NULL; @@ -2005,9 +2016,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) fetch_config_from_gitmodules(sfjc, rs); } - if (negotiate_only && !negotiation_tip.nr) - die(_("--negotiate-only needs one or more --negotiate-tip=*")); - if (deepen_relative) { if (deepen_relative < 0) die(_("Negative depth in --deepen is not supported")); @@ -2112,6 +2120,19 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) result = fetch_multiple(&list, max_children); } + string_list_clear(&list, 0); + + /* + * 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) + return result; + if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) { struct strvec options = STRVEC_INIT; int max_children = max_jobs; @@ -2132,8 +2153,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 && diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 8212ca56dc..732031085e 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 && -- 2.33.GIT