Skip to content

Maintenance: adapt custom refspecs

Derrick Stolee via GitGitGadget git 4 versions – In review

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v4 Latest4 patches 6 repliesIn review
  2. v33 patches 9 repliesSuperseded
  3. v25 patches 9 repliesSuperseded
Earlier versions · 1
VersionPostedPatchesRepliesStatus
v15 patches27 repliesSuperseded

Patches in v4 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/4maintenance: simplify prefetch logicbuiltin/gc.c+8 -251 reply
2/4fetch: add --prefetch optionDocumentation/fetch-options.txt builtin/fetch.c t/t5582-fetch-negative-refspec.sh+106 -12 replies
3/4maintenance: use 'git fetch --prefetch'Documentation/git-maintenance.txt builtin/gc.c t/t7900-maintenance.sh+12 -150 replies
4/4maintenance: respect remote.*.skipFetchAllbuiltin/gc.c t/t7900-maintenance.sh+10 -13 replies

Reviews trailers per patch

3 of 4 patches have a Reviewed-by or Acked-by.

PatchTom SaegerRamsay Jones
1/4R-b
2/4Helped-by R-b Helped-by
3/4
4/4R-b

Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.

Read the v4 thread

Cover letter [PATCH v4 0/4] Maintenance: adapt custom refspecs

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@dhcp-10-154-148-175.vpn.oracle.com/

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 <[email protected]>
     +    Helped-by: Ramsay Jones <[email protected]>
          Signed-off-by: Derrick Stolee <[email protected]>
      
       ## 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
Signature · 1 line
gitgitgadget