From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 03 Sep 2024 09:48:47 GMT
Subject: Re: Improvement: `git-maintenance` to allow configuring of remotes to fetch
Message-ID: <ZtbbejUrUO2tdsUQ@pks.im>
In-Reply-To: <CAG=Um+17JYyqC6n0gU3GSNaVz1PaB1U50M1hy87zObB+Rc03Qg@mail.gmail.com>

On Tue, Sep 03, 2024 at 11:31:04AM +0530, Shubham Kanodia wrote:
> On Tue, Sep 3, 2024 at 10:48 AM Patrick Steinhardt <ps@pks.im> wrote:
> > On Mon, Sep 02, 2024 at 09:16:24PM +0530, Shubham Kanodia wrote:
> > > > Patrick Steinhardt <ps@pks.im> writes:
> > > >
> > > > > I'm not aware of any discussion around this...
> > > >
> > > > I do not think so, either.
> > > >
> > > > I agree that it makes as much sense to limit prefetches to a subset
> > > > of remotes as it makes sense to limit to certain hierarchies (e.g.
> > > > excluding refs/changes/ or even limiting to refs/heads/seen and
> > > > nothing else).
> > >
> > > I'm seeking advice on the configuration option structure for this
> > > feature. The typical config format for maintenance tasks seems to be
> > > as follows:
> > >
> > > `maintenance.<task-name>.<option>`
> > >
> > > A natural extension of this for the prefetch task could be:
> > >
> > > ```
> > > git config maintenance.prefetch.<remote-name>.refs refs/heads/master
> > > ```
> > >
> > > In this structure, the 'refs' value represents only the source part of
> > > a refspec, and both remote and refs can be configured.
> > > Specifying a full refspec isn't necessary since the --prefetch option
> > > may override the destination anyway.
> > >
> > > While I've successfully implemented this approach, I'm open to
> > > suggestions for alternative configuration options. My concerns are:
> > >
> > > 1. Most Git configurations are nested up to three levels deep, whereas
> > > this proposal introduces a fourth level.
> > > 2. This configuration appears in the config file as:
> > >
> > > ```
> > > [maintenance "prefetch.origin"]
> > >        refs = refs/heads/master
> > > ```
> > > which might look odd?
> >
> > Agreed, it does. To me, the most natural way to configure this would be
> > as part of the remotes themselves:
> >
> > ```
> > [remote "origin"]
> >     url = https://example.com/repo.git
> >     fetch = +refs/heads/*:refs/remotes/origin/*
> >     # Whether or not the prefetch task shall fatch this repository.
> >     # Defaults to `true`.
> >     prefetch = true
> >     # An arbitrary number of refspecs used by the prefetch task.
> >     # Overrides the fetch refspec if given, otherwise we fall back to
> >     # using the fetch refspec.
> >     prefetchRefspec = +refs/heads/main:refs/remotes/origin/main
> > ```
> >
> > The prefetch refspec would be rewritten by git-maintenance(1) such that
> > the destination part (the right-hand side of the refspec) is prefixed
> > with `refs/prefetch/`, same as the fetch refspec would be changed in
> > this way.
> >
> > An alternative would be to _not_ rewrite the prefetch refspec at all and
> > thus allow the user to prefetch into arbitrary hierarchies. But I'm a
> > bit worried that this might cause users to misconfigure prefetches by
> > accident, causing it to overwrite their usual set of refs.
> >
> > > Also, hopefully my mail is formatted better this time!
> >
> > It is, thanks!
> >
> > Patrick
> 
> Interesting. I guess if we put this in `remote.*` instead of
> `maintenance.*` what's unclear then is if this setting should also be
> respected by `git fetch --prefetch`
> when used outside the context of a maintenance task — since that'd
> probably be a bigger change.

I would've expected it to already do, given that it is explicitly
documented in git-fetch(1) to be for git-maintenance(1). I don't have
enough knowledge around the prefetching task though to be able to tell
whether it should or shouldn't.

> For instance, the `skipFetchAll` remote config option seems to apply
> to prefetches (within maintenance & outside) and normal fetches.
> 
> Additionally, we'd need to discuss what to do with backward compatibility:
> 
> If we were designing maintenance prefetch right now, it'd probably
> make sense not to fetch it for any remote / refspec by default unless
> explicitly enabled since
> fetching all refs can be a waste of space in more cases than not.
> 
> However, since the current behaviour already fetches all remotes and
> refs, I don't know if breaking this is something we could do? If not,
> the behavior would be —
> 
> 1. If none of the remotes specify a `prefetch: true`, then prefetch
> all remotes and refs (backwards compat)
> 2. If at least one of the remotes specifies `prefetch: true` then only
> that remote should be fetched
> 3. Two-tier `fetch` / `prefetchRefSpec` hierarchy to decide which refs
> to fetch (we can decide on the name later as `fetch` and
> `prefetchRefSpec` seem asymmetrical)

I don't think we should change the current default. As mentioned in my
mail, `prefetch` should default to `true` such that the behaviour as we
have it right now is unchanged. Thus any remote that doesn't have it
gets fetched by default, and to disable fetching a specific remote you
would have to set `remote.<name>.prefetch = false`.

While the second item, namely change the default to `false` when there
is at least one `prefetch` config, might be something to consider. But
in the end I think it would lead to confusing behaviour, so I'd not go
there personally.

Patrick