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

From: Shubham Kanodia <shubham.kanodia10@gmail.com>
Date: Nov 5, 2024, 16:26 UTC
Message-ID: <CAG=Um+3D3iGPRv4t4DcCP=Bh7gbAQ6cSJ79jmg2h-qbSiQRkUA@mail.gmail.com>
In-Reply-To: <b548c532-e54a-4edc-94aa-f3c2925602e6@gmail.com>

On Tue, Nov 5, 2024 at 8:17 PM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 32 quoted lines

>
> 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/—

Show 16 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.

> Is this rename really necessary? It is not mentioned in the commit
> message, so this is surprising to me

What was previously `match_name_with_pattern` is now exposed out from `remote.c` to be used outside. I thought given that its now applicable outside `remote.c` it might be clearer to rename it to be more explicit (match name of what? remote? refspec? etc)

However I agree that the higher level `refspec_match()` is a better function to expose anyway, and am happy to make that change.