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

[Tom Saeger <tom.saeger@oracle.com>]

On Fri, Apr 02, 2021 at 04:43:16PM -0400, Derrick Stolee wrote:
> On 4/2/2021 2:27 PM, Tom Saeger wrote:
> > 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.
> 
> >> 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?
> 
> > '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.

I also worked up a patch, not nearly as elegant as yours, but it did
work.
I didn't think about changing fetch_remote to take struct remote like what
you've done.

Thanks - I'll give this a try.

--Tom

> 
> [1] https://github.com/gitgitgadget/git/pull/924
> [2] https://github.com/gitgitgadget/git/pull/924/checks?check_run_id=2256079534
> 
> Thanks,
> -Stolee