On Tue, Nov 5, 2024 at 8:17 PM Phillip Wood <phillip.wood123@xxxxxxxxx> 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@xxxxxxxxxxxxxx/ > 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/—; > 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.
