Re: [PATCH v4] remote: allow specifying refs to prefetch

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 6, 2024, 06:46 UTC
Message-ID: <ZysQvUyxgdRqjvj2@pks.im>
In-Reply-To: <CAG=Um+3D3iGPRv4t4DcCP=Bh7gbAQ6cSJ79jmg2h-qbSiQRkUA@mail.gmail.com>

On Tue, Nov 05, 2024 at 09:56:52PM +0530, Shubham Kanodia wrote:

Show 45 quoted lines

> On Tue, Nov 5, 2024 at 8:17 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > On 05/11/2024 06:45, Patrick Steinhardt wrote:
> > > On Fri, Oct 04, 2024 at 08:21:32PM +0000, Shubham Kanodia via GitGitGadget wrote:
> > >> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt
> > >> index 8efc53e836d..186f439ed7b 100644
> > >> --- a/Documentation/config/remote.txt
> > >> +++ b/Documentation/config/remote.txt
> > >> @@ -33,6 +33,13 @@ remote.<name>.fetch::
> > >>      The default set of "refspec" for linkgit:git-fetch[1]. See
> > >>      linkgit:git-fetch[1].
> > >>
> > >> +remote.<name>.prefetchref::
> > >> +    Specify the refs to be prefetched when fetching from this
> > >> +    remote. The value is a space-separated list of ref patterns
> > >> +    (e.g., "refs/heads/main !refs/heads/develop*"). This can be
> > >> +    used to optimize fetch operations by specifying exactly which
> > >> +    refs should be prefetched.
> > >
> > > I'm a bit surprised that we use a space-separated list here instead of
> > > accepting a multi-valued config like we do for "remote.<name>.fetch".
> > > Shouldn't we use the format here to make things more consistent?
> >
> > I agree that would be a good idea. I also think that it would be helpful
> > to expand the documentation to explain exactly how the patterns are
> > matched. I think "remote.<name>.prefetch" would better match the
> > existing "remote.<name>.fetch" and "remote.<push>.name" config key names.
> >
> > Best Wishes
> >
> > Phillip
> >
> 
> Thanks for taking the time to look at this!
> 
> Let me add context here based on a few things that have already been discussed
> as a part of this thread or an earlier discussion on
> https://lore.kernel.org/git/CAG=Um+1wTbXn_RN+LOCrpZpSNR_QF582PszxNyhz5anVHtBp+w@mail.gmail.com/
> 
> > I'm coming rather late to the party and simply want to review this so
> > that the thread gets revived. So my context may be lacking, please
> > forgive me if I am reopening things that were already discussed.
> 
> I don't have a particular preference here, and this was discussed in
> an earlier thread
> where Junio opined (https://lore.kernel.org/git/xmqq5xrcn2k1.fsf@gitster.g/—

Fair enough, thanks for the pointer! The reason stated by Junio is that having a space-separated list of refspecs makes it easier to reset the refspec again, as we can simply use a "last-one-wins" pattern. And while I understand that, I would claim that we already have a properly established way to reset multi-valued lists by first assigning an empty value:

    [remote "origin"]
        prefetchref = refs/heads/*
        prefetchref = refs/tags/*
        prefetchref =
        prefetchref = refs/heads/*

The final value would be only "refs/heads/*" due to the reset.

So overall I'm still leaning into the direction of making this a multi-valued list for the sake of consistency with other refspec configs. @Junio Let me know in case you disagree though.

Show 27 quoted lines

> > I agree that it is the right place to configure this as attributes
> > to remotes.  It would make it handy if we could give a catch-all
> > configuration, though.  For example:
> >
> > [remote "origin"]
> >       prefetch = true
> >        prefetchref = refs/heads/* refs/tags/*
> > [remote "*"]
> >        prefetch = false
> >
> > may toggle prefetch off for all remotes, except that the tags and
> > the local branches of the remote "origin" are prefetched.  Instead
> > of a multi-value configuration variable (like remote.*.fetch) where
> > we need to worry about clearing convention, we can use a regular
> > "last one wins" variable that is whitespace separated patterns, as
> > such a pattern can never have a whitespace in it.
> 
> which is what my implementation is based on.
> 
> > This is essentially open-coding a bunch of logic around how we parse
> > refspecs. I'd propose to instead use the APIs we already have in this
> > area, namely those in "refspec.h".
> 
> Is there any that you would suggest? I don't see anything in `refspec.h` that
> might cover a similar logic. I'd previously used `wildmatch.h`, but
> that is really
> only suitable for glob-style wildcards.

The logic around refspecs is somewhat split up between different headers indeed. "refspec.h" really only concerns parsing of refspecs, but does not provide the interfaces to also use them to translate or match refs.

I think the closest to what you need is `omit_name_by_refspec()`: you give it a refspec and a refname, and it returns whether any of the refspec items matches that ref. This also takes into oaccount whether the refspec item is negated ("!" prefix") or whether it is a pattern (contains "*").

Ideally, we'd move interfaces that provide basic refspec functionality into "refspec.h" so that they are easier to discover. But I'm fine with making this a #leftoverbit, I don't want to push the burden on you.

Patrick