Subject: Re: [PATCH] remote: introduce config to set prefetch refs

From: Junio C Hamano <[email-masked]>
Date: Fri, 13 Sep 2024 09:58:54 -0700

Shubham Kanodia <[email-masked]> writes:

> Ideally, a repository should be able to specify (say):
>
> remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
> remote.origin.prefetchref=refs/heads/main
>
> This configuration would maintain the normal behavior for fetches, but
> only prefetch the main branch.
> The rationale for this is that the main branch typically serves as the
> HEAD from which future branches will be forked in an active
> repository.

Oh, that is 100% agreeable.  All I wanted to caution you about was
what should happen when remote.origin.prefetchref in the above is
replaced to something like:

    [remote "origin"]
	fetch = +refs/heads/*:refs/remotes/origin/*
        prefetchref = refs/notes/*

That is, if your refspec used for your real fetch (i.e. "git fetch"
without the "--prefetch" option) does not fetch anything from
"refs/notes/" hierarchy, prefetching from the hierarchy does not
help the real fetch.  I do not have a strong preference between
marking it as an error and silently ignoring the prefetch but
leaning towards the latter, and that is why my suggestion to
implement this new "prefetchref" as something that extends the
existing filter_prefetch_refspec(), which already filters out
refspec that fetches from the refs/tags/ namespace (and the ones
that do not store by having NULL in the .dst side).

> Regarding:
>
>> If prefetch_refs contains only positive patterns, then a refspec whose source
>> doesn't match any of these patterns is rejected.
>
> Simply rejecting a source refspec pattern in `remote.<remote>.fetch`
> wouldn't achieve our goal here.

I used the verb "reject" to mean "filter out", just like a refspec
with left-hand-side that begins with "refs/tags/" is filtered out
in the current filter_prefetch_refspec().  And that is exactly what
we want to achieve our goal here.

IOW, you would

 * read their ref advertisement, and pick only the ones that have a
   matching pattern in the left-hand-side of a remote.$name.fetch
   element.  With a more recent protocol, remote.$name.fetch may
   have already participated in narrowing what they advertise to
   begin with, but the end result is the same.

 * give it to filter_prefetch_refspec().

 * filter_prefetch_refspec() inspects the refspec elements, and
   rejects ones with no right-hand-side, and ones with
   left-hand-side that begin with refs/tags/.  The current code
   without your patch already works this way up to this point.

 * We extend the above filtering so that in addition to the two
   kinds we currently reject, reject the ones that do not match the
   prefetchref criteria.  This is what is needed to implement
   "prefetchref configuration limits the set of refs that get
   prefetched".

And what you quoted is a beginning of how "prefetchref configuration
limits".  It cannot be "add to what filter_prefetch_refspec() did",
like done by the implementation in the patch we are discussing.

If your configuration were this:

    [remote "origin"]
        fetch = +refs/heads/*:refs/remotes/origin/*

you would want a way to say things like

 (1) I want to prefetch everything I usually fetch

 (2) Among the ones I usually fetch, I only want to prefetch master
     and next branches.

 (3) I want to prefetch only refs/heads/jk/* branches, but not
     refs/heads/jk/wip/* branches.

 (4) I want to prefetch everything I usually fetch, except for
     refs/heads/wip/* branches.

The case (1) is the simplest.  You will leave .prefetchref empty.

For the case (2), you would write something like

    [remote "origin"]
	prefetchref = refs/heads/master
	prefetchref = refs/heads/next

So, when your prefetchref has all positive patterns, after the
existing conditional in filter_prefetch_refspec() passes a refspec
whose right-hand-side (i.e., .dst) is not NULL and whose
left-hand-side (i.e., .src) does not begin with "refs/tags/", you
further inspect and make sure it matches one of these prefetchref
patterns.  In example (2), if they advertised master, next, and seen
branches, refs/heads/seen would be filtered out because it matches
neither of the two patterns, so we would end up prefetching master
and next branches.

For the case (3), you would want to say something like

    [remote "origin"]
	prefetchref = refs/heads/jk/*
	prefetchref = !refs/heads/jk/wip/*

Now your prefetchref has some negative pattern.  When filtering what
the existing conditional in filter_prefetch_refspec() passed, you'd
inspect the refspec element and see if it matches any of the
positive patterns, and also if it does not match any of the negative
ones.  refs/heads/next does not match any positive ones and gets
rejected.  refs/heads/jk/main does match the positive pattern
'refs/heads/jk/*', and it does not match the negative pattern
'refs/heads/jk/wip/*', so it passes and will get prefetched.

For the case (4), you would write something like

    [remote "origin"]
	prefetchref = !refs/heads/wip/*

There is no positive pattern, so if you blindly apply the rule you
used for (3) above, everything will get rejected, which is not what
you want.  refs/heads/main does not match any positive patterns
(because there are no positive patterns given), but it does not
match any negative ones, so it passes and will get prefetched.

The condition to implement the above four cases (which I think
covers all the cases we care about, but I won't guarantee it is
exhaustive---you'd need to sanity check) would be

 - If there is 1 or more positive prefetchref patterns, the refspec
   element must match one of them to be considered for the next
   rule.  Otherwise, it will not be prefetched.

 - If the refspec element matches any of negative prefetchref
   patterns, it will not be prefetched.