Re: [PATCH 6/9] fetch: ask server to advertise HEAD for config-less fetch

[Taylor Blau <me@ttaylorr.com>]

On Sat, Mar 08, 2025 at 10:08:47PM -0500, Jeff King wrote:
> 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.

;-).

> 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.

> 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.

>  	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.

> +	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