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@xxxxxxxxx> 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
