[PATCH v3 2/3] builtin/fetch: skip unnecessary tasks when using --negotiate-only
- From
Glen Choo <chooglen@google.com>
- Date
- Dec 22, 2021, 00:11 UTC
- Message-ID
- <20211222001134.28933-3-chooglen@google.com>
- In-Reply-To
- <20211222001134.28933-1-chooglen@google.com>
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.
A similar optimization would be to skip irrelevant tasks in `git fetch --dry-run`. This optimization was not done in this commit because a dry run will actually fetch objects; we'd presumably still want to recurse into submodules and run gc.
[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@google.com> --- builtin/fetch.c | 20 ++++++++++++++++++++ t/t5516-fetch-push.sh | 12 ++++++++++++ 2 files changed, 32 insertions(+)
diff --git a/builtin/fetch.c b/builtin/fetch.c index eab73d7417..883bb1b10c 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1996,6 +1996,15 @@ 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 (recurse_submodules != RECURSE_SUBMODULES_OFF) { int *sfjc = submodule_fetch_jobs_config == -1 ? &submodule_fetch_jobs_config : NULL; @@ -2113,6 +2122,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) result = fetch_multiple(&list, max_children); } + /* + * 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; + if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) { struct strvec options = STRVEC_INIT; int max_children = max_jobs; 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