Improvement: `git-maintenance` to allow configuring of remotes to fetch

Improvement: `git-maintenance` to allow configuring of remotes to fetch

[Shubham Kanodia]

`git-maintenance` aims to improve the health & performance of
especially large git repositories.
A part of git maintenance tasks is `prefetch`.

However, at the moment, it prefetches all objects from all branches —

> The prefetch task updates the object directory with the latest objects from all registered remotes.

This seems non-optimal for repositories where users use the main
repository to fork branches (instead of personal forks) as the remote
could have thousands of refs and objects from branches that a user may
not care about, leading to a very large prefetch dir.

For large repositories, it might instead be helpful to only fetch
objects on `main` or `master` because those are the ones most likely
to be used by users.

To this effect, git maintenance should perhaps expose a
maintenance.prefetch.refs setting that subsets prefetching only to
certain references.

Is this something that has been raised before / is in works by any
chance or something that would be an acceptable contribution?

-- 
Regards,
Shubham Kanodia

Re: Improvement: `git-maintenance` to allow configuring of remotes to fetch

[Patrick Steinhardt]

On Mon, Aug 26, 2024 at 05:20:50PM +0530, Shubham Kanodia wrote:
> `git-maintenance` aims to improve the health & performance of
> especially large git repositories.
> A part of git maintenance tasks is `prefetch`.
> 
> However, at the moment, it prefetches all objects from all branches —
> 
> > The prefetch task updates the object directory with the latest objects from all registered remotes.
> 
> This seems non-optimal for repositories where users use the main
> repository to fork branches (instead of personal forks) as the remote
> could have thousands of refs and objects from branches that a user may
> not care about, leading to a very large prefetch dir.
> 
> For large repositories, it might instead be helpful to only fetch
> objects on `main` or `master` because those are the ones most likely
> to be used by users.
> 
> To this effect, git maintenance should perhaps expose a
> maintenance.prefetch.refs setting that subsets prefetching only to
> certain references.

I think that this makes sense as an addition. We would have to think
about things like globbing and whatnot, such that we can say e.g. to
prefetch only tags, only branches or only a subset thereof. We may even
decide to use refspecs here, which could be the most flexible way of
configuring this. We already use the default refspecs anyway -- the only
thing we change is that we prefix the refspec destination with
"refs/prefetch/". So allowing the user to override the refspec used for
prefetching makes sense.

It opens up another question though: if you've got multiple remotes in a
repository, do we also want to make it configurable which of these
should be fetched from? And if you do, do you want to allow configuring
refs to fetch per remote? I guess the answer to both of these questions
would be "yes".

> Is this something that has been raised before / is in works by any
> chance or something that would be an acceptable contribution?

I'm not aware of any discussion around this, and think that this feature
does make sense indeed.

Patrick

Re: Improvement: `git-maintenance` to allow configuring of remotes to fetch

[Junio C Hamano]

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).

Re: Improvement: `git-maintenance` to allow configuring of remotes to fetch

[Shubham Kanodia]

> 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?

Also, hopefully my mail is formatted better this time!

Re: Improvement: `git-maintenance` to allow configuring of remotes to fetch

[Patrick Steinhardt]

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

Re: Improvement: `git-maintenance` to allow configuring of remotes to fetch

[Shubham Kanodia]

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.

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)

Re: Improvement: `git-maintenance` to allow configuring of remotes to fetch

[Patrick Steinhardt]

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

Re: Improvement: `git-maintenance` to allow configuring of remotes to fetch

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

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.

As you too agree with the position to consider "prefetch" should be
invisible to the end-users, we should not allow users to specify the
full refspec at all, or if it is forced or not with "+" prefix.
Only accept a set of patterns to match, and keep it opaque where in
the local refs/* hierarchy they are stored.  It is an implementation
detail that the users should not have to know about and rely on.

Thanks.

Re: Improvement: `git-maintenance` to allow configuring of remotes to fetch

[Shubham Kanodia]

On Tue, Sep 3, 2024 at 9:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > 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.
>
> 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.
>
> As you too agree with the position to consider "prefetch" should be
> invisible to the end-users, we should not allow users to specify the
> full refspec at all, or if it is forced or not with "+" prefix.
> Only accept a set of patterns to match, and keep it opaque where in
> the local refs/* hierarchy they are stored.  It is an implementation
> detail that the users should not have to know about and rely on.
>
> Thanks.

Right so I have good direction to start working on this. I'm going to
tackle this in a few parts since this is my first contribution —

1. adding a boolean `prefetch` attribute to `remote` — allows control
over remotes that prefetch
2. adding `prefetchref` — allows control over refs per remote.
3. Check if adding a ` [remote "*"]` catchall is a possibility

For the boolean `prefetch` value the —
1. The default would be assumed to be `true`
2. No changes in behavior to any variation of  `git fetch --prefetch`
command itself.
If no remote is supplied to `git fetch` then it means default remote,
or if `--all` is used, it means all remotes and `--prefetch` for both
always
prefetches irrespective of the config value.

If I understand the codebase well, the changes would primarily affect
`remote.c` and `gc.c`.

Also much thanks for the feedback both of you've given thus far.

Re: Improvement: `git-maintenance` to allow configuring of remotes to fetch

[Shubham Kanodia]

On Wed, Sep 4, 2024 at 9:59 PM Shubham Kanodia <shubhamsizzles@gmail.com> wrote:
>
> On Tue, Sep 3, 2024 at 9:37 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > > 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.
> >
> > 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.
> >
> > As you too agree with the position to consider "prefetch" should be
> > invisible to the end-users, we should not allow users to specify the
> > full refspec at all, or if it is forced or not with "+" prefix.
> > Only accept a set of patterns to match, and keep it opaque where in
> > the local refs/* hierarchy they are stored.  It is an implementation
> > detail that the users should not have to know about and rely on.
> >
> > Thanks.
>
> Right so I have good direction to start working on this. I'm going to
> tackle this in a few parts since this is my first contribution —
>
> 1. adding a boolean `prefetch` attribute to `remote` — allows control
> over remotes that prefetch
> 2. adding `prefetchref` — allows control over refs per remote.
> 3. Check if adding a ` [remote "*"]` catchall is a possibility
>
> For the boolean `prefetch` value the —
> 1. The default would be assumed to be `true`
> 2. No changes in behavior to any variation of  `git fetch --prefetch`
> command itself.
> If no remote is supplied to `git fetch` then it means default remote,
> or if `--all` is used, it means all remotes and `--prefetch` for both
> always
> prefetches irrespective of the config value.
>
> If I understand the codebase well, the changes would primarily affect
> `remote.c` and `gc.c`.
>
> Also much thanks for the feedback both of you've given thus far.

I've published my first patch using gitgitgadget. I'm still figuring
out what the best way to link to that is but
I'll add both for now:

https://lore.kernel.org/git/pull.1779.git.1725472799637.gitgitgadget@gmail.com/T/#u
https://github.com/gitgitgadget/git/pull/1779

-- 
Regards,
Shubham Kanodia