Tom Saeger rightly pointed out [1] that the prefetch task ignores custom refspecs. This can lead to downloading more data than requested, and it doesn't even help the future foreground fetches that use that custom refspec. [1] https://lore.kernel.org/git/20210401184914.qmr7jhjbhp2mt3h6@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ This series fixes this problem by carefully replacing the start of each refspec's destination with "refs/prefetch/". If the destination already starts with "refs/", then that is replaced. Otherwise "refs/prefetch/" is just prepended. This happens inside of git fetch when a --prefetch option is given. This allows us to maniuplate a struct refspec_item instead of a full refspec string. It also simplifies our logic in testing the prefetch task. Updates in V4 ============= * Two bugs were fixed. Thanks, Ramsay and Tom, for pointing out the issues. Tests are added that prevent regressions. * A new patch is added to respect remote.<name>.skipFetchAll. This is added at the end to take advantage of the simpler test design after --prefetch is added. Update in V3 ============ * The fix is almost completely rewritten as an update to 'git fetch'. See the new PATCH 2 for this update. * There was some discussion of rewriting test_subcommand, but that can be delayed until a proper solution is found to complications around softer matches. Updates in V2 ============= Thanks for the close eye on this series. I appreciate the recommendations, which I believe I have responded to them all: * Fixed typos. * Made refspec_item_format() re-entrant. Consumers must free the buffer. * Cleaned up style (quoting and tabbing). Thanks, -Stolee Derrick Stolee (4): maintenance: simplify prefetch logic fetch: add --prefetch option maintenance: use 'git fetch --prefetch' maintenance: respect remote.*.skipFetchAll Documentation/fetch-options.txt | 5 +++ Documentation/git-maintenance.txt | 6 ++-- builtin/fetch.c | 59 ++++++++++++++++++++++++++++++- builtin/gc.c | 39 +++++++------------- t/t5582-fetch-negative-refspec.sh | 43 ++++++++++++++++++++++ t/t7900-maintenance.sh | 22 +++++++----- 6 files changed, 134 insertions(+), 40 deletions(-) base-commit: 89b43f80a514aee58b662ad606e6352e03eaeee4 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-924%2Fderrickstolee%2Fmaintenance%2Frefspec-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-924/derrickstolee/maintenance/refspec-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/924 Range-diff vs v3: 1: 4c0e983ba56f = 1: 4c0e983ba56f maintenance: simplify prefetch logic 2: 7f488eea6dbd ! 2: 73b4e8496746 fetch: add --prefetch option @@ Commit message Finally, we add the 'force' option to ensure that prefetch refs are replaced as necessary. + There are some interesting cases that are worth testing. + + An earlier version of this change dropped the "i--" from the loop that + deletes a refspec item and shifts the remaining entries down. This + allowed some refspecs to not be modified. The subtle part about the + first --prefetch test is that the "refs/tags/*" refspec appears directly + before the "refs/heads/bogus/*" refspec. Without that "i--", this + ordering would remove the "refs/tags/*" refspec and leave the last one + unmodified, placing the result in "refs/heads/*". + + It is possible to have an empty refspec. This is typically the case for + remotes other than the origin, where users want to fetch a specific tag + or branch. To correctly test this case, we need to further remove the + upstream remote for the local branch. Thus, we are testing a refspec + that will be deleted, leaving nothing to fetch. + + Helped-by: Tom Saeger <tom.saeger@xxxxxxxxxx> + Helped-by: Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> ## Documentation/fetch-options.txt ## @@ builtin/fetch.c: static void find_non_local_tags(const struct ref *refs, + rs->raw[j - 1] = rs->raw[j]; + } + rs->nr--; ++ i--; + continue; + } + @@ builtin/fetch.c: static struct ref *get_ref_map(struct remote *remote, if (rs->nr) { struct refspec *fetch_refspec; +@@ builtin/fetch.c: static struct ref *get_ref_map(struct remote *remote, + if (has_merge && + !strcmp(branch->remote_name, remote->name)) + add_merge_config(&ref_map, remote_refs, branch, &tail); +- } else { ++ } else if (!prefetch) { + ref_map = get_remote_ref(remote_refs, "HEAD"); + if (!ref_map) + die(_("Couldn't find remote ref HEAD")); ## t/t5582-fetch-negative-refspec.sh ## @@ t/t5582-fetch-negative-refspec.sh: test_expect_success "push with matching +: and negative refspec" ' @@ t/t5582-fetch-negative-refspec.sh: test_expect_success "push with matching +: an +test_expect_success '--prefetch correctly modifies refspecs' ' + git -C one config --unset-all remote.origin.fetch && -+ git -C one config --add remote.origin.fetch "refs/tags/*:refs/tags/*" && + git -C one config --add remote.origin.fetch ^refs/heads/bogus/ignore && ++ git -C one config --add remote.origin.fetch "refs/tags/*:refs/tags/*" && + git -C one config --add remote.origin.fetch "refs/heads/bogus/*:bogus/*" && + + git tag -a -m never never-fetch-tag HEAD && @@ t/t5582-fetch-negative-refspec.sh: test_expect_success "push with matching +: an + git -C one rev-parse refs/prefetch/bogus/fetched && + test_must_fail git -C one rev-parse refs/prefetch/bogus/ignore +' ++ ++test_expect_success '--prefetch succeeds when refspec becomes empty' ' ++ git checkout bogus/fetched && ++ test_commit extra && ++ ++ git -C one config --unset-all remote.origin.fetch && ++ git -C one config --unset branch.main.remote && ++ git -C one config remote.origin.fetch "+refs/tags/extra" && ++ git -C one config remote.origin.skipfetchall true && ++ git -C one config remote.origin.tagopt "--no-tags" && ++ ++ git -C one fetch --prefetch ++' + test_done 3: ed055d772452 = 3: 565ed8a18929 maintenance: use 'git fetch --prefetch' -: ------------ > 4: 92652fd9e6e1 maintenance: respect remote.*.skipFetchAll -- gitgitgadget
