Re: [PATCH 6/9] fetch: ask server to advertise HEAD for config-less fetch
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Mar 12, 2025, 21:43 UTC
- Message-ID
- <Z9H//JHtYTGqHI3n@nand.local>
- In-Reply-To
- <20250309030847.GF2334191@coredump.intra.peff.net>
On Sat, Mar 08, 2025 at 10:08:47PM -0500, Jeff King wrote:
Show 18 quoted lines
> If we're not given any refspecs (either on the command line or via > config) and we have no branch merge config, then we fetch the remote > HEAD into our local FETCH_HEAD. In that case we do not send any > ref-prefix option to the server at all, and we see the full > advertisement. > > But this is sub-optimal. We only care about HEAD, so we can just ask > for that, and ignore all of the other refs. > > The new test demonstrates a case where we see fewer refs (in this case > only one less, but in theory we could be ignoring millions of them). > > This also removes the only case where we care about seeing some refs > from the other side, but don't add anything to the ref_prefixes list. > Cleaning this up means one less maintenance burden. Before this patch, > any code which wanted to add to the list had to make sure the list was > not empty, since an empty list meant "ask for everything". Now it really > means "we are not interested in any refs".
Yes. The optimization is nice on its own, but I think this is the real benefit to this patch IMHO.
> This should let us optimize a few more cases in subsequent patches.
;-).
Show 8 quoted lines
> Note that we'll add "HEAD" to the list of prefixes, and later code for > updating "refs/remotes/<remote>/HEAD" may likewise do so. In theory this > could cause duplicates in the list, but in practice these can't both > trigger. We hit our new case only if there are no refspecs, and the > "<remote>/HEAD" feature is enabled only when we are fetching from a > remote with configured refspecs. We could be defensive with a flag, but > it didn't seem worth it to me (the absolute worse case is a useless > redundant ref-prefix line sent to the server).
Yeah, I think that we already do this in some instances that you and I talked about off-list, but I can't remember exactly what I did to provoke it. In either case, the server responds correctly, so I don't think it's so urgent to deal with ATM.
Show 16 quoted lines
> diff --git a/builtin/fetch.c b/builtin/fetch.c > index 95fd0018b9..f142756441 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1766,6 +1766,14 @@ static int do_fetch(struct transport *transport, > branch->merge[i]->src); > } > } > + > + /* > + * If there are no refs specified to fetch, then we just > + * fetch HEAD; mention that to narrow the advertisement. > + */ > + if (!transport_ls_refs_options.ref_prefixes.nr) > + strvec_push(&transport_ls_refs_options.ref_prefixes, > + "HEAD");
Makes sense. If we know that we just want to fetch HEAD into FETCH_HEAD and we haven't already limited the advertisement, we are free to do so now.
Show 12 quoted lines
> if (tags == TAGS_SET || tags == TAGS_DEFAULT) {
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index cea8f92a3d..2f0a52a72d 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -679,6 +679,21 @@ test_expect_success 'default refspec is used to filter ref when fetching' '
> grep "ref-prefix refs/tags/" log
> '
>
> +test_expect_success 'set up parent for prefix tests' '
> + git init prefix-parent &&
> + git -C prefix-parent commit --allow-empty -m foo &&Any reason to use a bona-fide "commit" here instead of "test_commit"?
Not a big deal either way, of course, I'm just curious.
Show 10 quoted lines
> + git -C prefix-parent branch unrelated-branch > +' > + > +test_expect_success 'empty refspec filters refs when fetching' ' > + git init configless-child && > + > + test_when_finished "rm -f log" && > + GIT_TRACE_PACKET="$(pwd)/log" \ > + git -C configless-child fetch ../prefix-parent && > + test_grep ! unrelated-branch log
Very clean, nice.
Thanks, Taylor