Re: should git maintenance prefetch be taught to honor remote.fetch refspec?

From: Derrick Stolee <stolee@gmail.com>
Date: Apr 2, 2021, 20:43 UTC
Message-ID: <41dc2961-7ba5-a882-3416-45631e2cbb33@gmail.com>
In-Reply-To: <20210402182716.trbaflsjcvouff2y@brm-x62-17.us.oracle.com>

On 4/2/2021 2:27 PM, Tom Saeger wrote:

Show 15 quoted lines

> On Thu, Apr 01, 2021 at 06:25:36PM -0400, Derrick Stolee wrote:
>> On 4/1/2021 4:14 PM, Junio C Hamano wrote:
>>> Derrick Stolee <stolee@gmail.com> writes:
>>>
>>>> On 4/1/2021 2:49 PM, Tom Saeger wrote:
>>>
>>> So, redirecting the right-hand of configured refspec is a good idea;
>>> not copying the left-hand of configured refspec, and unconditionally
>>> using "refs/heads/*" is not.
>>  
>> This makes sense as a way to augment the feature. It doesn't seem
>> like a common scenario, but it would be good for users to have
>> that flexibility.
> 
> It's common for me, especially on repos requiring 'maintenance'.

I'm sure that once this is a tool in your belt, then it becomes common. The number of users who think about refspecs is likely a small proportion. But features should work as well as they can for as many users as possible. There's a way forward here, it just is a little tricky due to the generality of refspecs.

Show 8 quoted lines

>> Upon initial inspection, it shouldn't be too much work. However,
>> there is some generality to the refspec that might not be wholly
>> appropriate for prefetch (such as the exact_sha1 option). I'm
>> unfamiliar with the advanced forms of the refspec, so it'll take
>> some time to have confidence in this approach.
> 
> Didn't know about exact_sha1.  prefetch probably wouldn't do
> anything in that case?

My guess is that those should be dropped and ignored. But maybe the approach below will still work?

Show 6 quoted lines

> 'negative' refspecs - hmm haven't tried those.  I see how that might
> complicate things or maybe not.
> 
> generally isn't it still changing the right-hand side of refspec?
> 
> replacing ":refs/" with ":refs/prefetch/"

Right, this substring replacement might be easiest to achieve. The 'struct refspec' doesn't make it incredibly easy. Perhaps skipping the refspec parsing and just doing that substring swap directly from the config value might be the best approach.

> This would still work for refspecs with negative patterns right?

One of the issues is that negative patterns have no ":refs/" substring.

The other issue is that exact matches (no "*") have an exact string in the destination, too, so replacing the _entire_ destination with "refs/prefetch/<remote>/*" breaks the refspec. I think the substring approach will still work here.

> I'm willing to help with/test/review this, thanks for investigating.

I have a branch available [1], but I'm seeing some failures only on FreeBSD [2] and I can't understand why that platform is failing this test. The current version (as of this writing) does not do the substring replacement technique, and hence it just gives up on exact matches. I will try the substring approach as an alternative and see where that gets me.

[1] https://github.com/gitgitgadget/git/pull/924 [2] https://github.com/gitgitgadget/git/pull/924/checks?check_run_id=2256079534

Thanks, -Stolee