[v4,0/4] Maintenance: adapt custom refspecs

Message

Linus Arver via GitGitGadget April 16, 2021, 12:49 p.m. UTC

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/[e-mail]/

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 <[e-mail]>
     +    Helped-by: Ramsay Jones <[e-mail]>
          Signed-off-by: Derrick Stolee <[e-mail]>
      
       ## 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