maintenance: add prune-remote-refs task #1838

pastelsky · 2024-12-17T10:36:28Z

As discussed previously on:
https://lore.kernel.org/git/[email protected]/T/#t

Remote-tracking refs can accumulate in local repositories even as branches
are deleted on remotes, impacting git performance negatively. Existing
alternatives to keep refs pruned have a few issues —

The fetch.prune config automatically cleans up remote ref on fetch,
but also pulls in new ref from remote which is an undesirable side-effect.

2.git remote prune cleans up refs without adding to the existing list
but requires periodic user intervention.

This adds a new maintenance task 'prune-remote-refs' that runs
'git remote prune' for each configured remote daily. This provides an
automated way to clean up stale remote-tracking refs — especially when
users may not do a full fetch.

This task is disabled by default.

CC: Junio C Hamano [email protected], Patrick Steinhardt [email protected]

gitgitgadget · 2024-12-17T10:37:27Z

There are issues in commit 7076784:
maintenance: add prune-remote-refs task
Commit not signed off

gitgitgadget · 2024-12-18T03:40:58Z

There are issues in commit 1af8661:
maintenance: add prune-remote-refs task
Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
Indented lines, and lines without whitespace, are exempt

gitgitgadget · 2024-12-23T09:05:07Z

There are issues in commit b67f04c:
maintenance: add prune-remote-refs task
Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
Indented lines, and lines without whitespace, are exempt

pastelsky · 2024-12-23T09:35:03Z

/submit

gitgitgadget · 2024-12-23T09:36:15Z

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1838/pastelsky/sk/add-remote-prune-maintenance-v1

To fetch this version to local tag pr-1838/pastelsky/sk/add-remote-prune-maintenance-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1838/pastelsky/sk/add-remote-prune-maintenance-v1

gitgitgadget · 2024-12-27T09:11:58Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Thanks for a patch.


"Shubham Kanodia via GitGitGadget" <[email protected]> writes:

You'd want to check your procedure to tell GGG about addresses; I am
seeing these

    From: "Shubham Kanodia via GitGitGadget" <[email protected]>
    To: [email protected]
    Cc: "mailto:[email protected]" <[[email protected]]>,
            "mailto:[email protected]" <[[email protected]]>,
            Shubham Kanodia <[email protected]>,
            Shubham Kanodia <[email protected]>

and Cc addresses in it would probably not work as-is (I've fixed
them up manually).

> From: Shubham Kanodia <[email protected]>
>
> Remote-tracking refs can accumulate in local repositories even as branches
> are deleted on remotes, impacting git performance negatively. Existing
> alternatives to keep refs pruned have a few issues:
>
> 1. The `fetch.prune` config automatically cleans up remote ref on fetch,
> but also pulls in new ref from remote which is an undesirable side-effect.

This makes it sound as if fetch.prune configuration makes new refs
pulled, but that is not what happens and that is not what you wanted
to hint.

	If you run "git fetch" with the "--prune" option (or with
	the fetch.prune configuration set to true) while having the
	default refspec "+refs/heads/*:refs/remotes/$name/*"
	configured in remote.$name.fetch, then ...

> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
> index 6e6651309d3..0c8f1e01ccd 100644
> --- a/Documentation/git-maintenance.txt
> +++ b/Documentation/git-maintenance.txt
> @@ -158,6 +158,26 @@ pack-refs::
>  	need to iterate across many references. See linkgit:git-pack-refs[1]
>  	for more information.
>  
> +prune-remote-refs::
> +	The `prune-remote-refs` task runs `git remote prune` on each remote
> +	repository registered in the local repository. This task helps clean
> +	up deleted remote branches, improving the performance of operations
> +	that iterate through the refs. See linkgit:git-remote[1] for more
> +	information. This task is disabled by default.
> ++
> +NOTE: This task is opt-in to prevent unexpected removal of remote refs
> +for users of git-maintenance. For most users, configuring `fetch.prune=true`
> +is a acceptable solution, as it will automatically clean up stale remote-tracking
> +branches during normal fetch operations. However, this task can be useful in
> +specific scenarios:
> ++
> +--
> +* When using selective fetching (e.g., `git fetch origin +foo:refs/remotes/origin/foo`)
> +  where `fetch.prune` would not affect refs outside the fetched hierarchy

The word "hierarchy" hints that things under refs/remotes/origin/
(which is the hierarchy 'foo' is fetched into) that went away would
be pruned, but that is not what happens (otherwise you would not be
adding this feature).

> +* When third-party tools might perform unexpected full fetches, and you want
> +  periodic cleanup independently of fetch operations

You'd want a full-stop after these two sentences, by the way.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 4ae5196aedf..9acf1d29895 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -20,6 +20,7 @@
>  #include "lockfile.h"
>  #include "parse-options.h"
>  #include "run-command.h"
> +#include "remote.h"
>  #include "sigchain.h"
>  #include "strvec.h"
>  #include "commit.h"
> @@ -913,6 +914,40 @@ static int maintenance_opt_schedule(const struct option *opt, const char *arg,
>  	return 0;
>  }
>  
> +static int collect_remote(struct remote *remote, void *cb_data)
> +{
> +	struct string_list *list = cb_data;
> +
> +	if (!remote->url.nr)
> +		return 0;
> +
> +	string_list_append(list, remote->name);
> +	return 0;
> +}
> +
> +static int maintenance_task_prune_remote(struct maintenance_run_opts *opts UNUSED,
> +					 struct gc_config *cfg UNUSED)
> +{
> +	struct string_list_item *item;
> +	struct string_list remotes_list = STRING_LIST_INIT_NODUP;
> +	struct child_process child = CHILD_PROCESS_INIT;
> +	int result = 0;
> +
> +	for_each_remote(collect_remote, &remotes_list);
> +
> +	for_each_string_list_item (item, &remotes_list) {
> +		const char *remote_name = item->string;
> +		child.git_cmd = 1;
> +		strvec_pushl(&child.args, "remote", "prune", remote_name, NULL);
> +
> +		if (run_command(&child))
> +			result = error(_("failed to prune '%s'"), remote_name);
> +	}

Hmph, is there a reason why you need two loops, instead of
for-each-remote calling a function that does the run_command()
thing?

"git grep for_each_string_list_item \*.c" tells me that we almost
never write SP between the macro name and the opening parenthesis.

This loop does not stop at the first error, but returns a non-zero
error after noticing even a single remote fail to run prune, which
sounds like a seneible design.  Would an error percolate up the same
way when two different tasks run and one of them fails in the
control folow in "git maintenance"?  Just want to see if we are
being consistent with the surrounding code.

Thanks.

gitgitgadget · 2024-12-28T10:02:07Z

On the Git mailing list, Shubham Kanodia wrote (reply to this):

On Fri, Dec 27, 2024 at 2:37 PM Junio C Hamano <[email protected]> wrote:
>
> Thanks for a patch.
>
>
> "Shubham Kanodia via GitGitGadget" <[email protected]> writes:
>
> You'd want to check your procedure to tell GGG about addresses; I am
> seeing these
>
>     From: "Shubham Kanodia via GitGitGadget" <[email protected]>
>     To: [email protected]
>     Cc: "mailto:[email protected]" <[[email protected]]>,
>             "mailto:[email protected]" <[[email protected]]>,
>             Shubham Kanodia <[email protected]>,
>             Shubham Kanodia <[email protected]>
>
> and Cc addresses in it would probably not work as-is (I've fixed
> them up manually).

I think the GGG comment had a few formatting errors. Thanks for fixing the cc.

> Hmph, is there a reason why you need two loops, instead of
> for-each-remote calling a function that does the run_command()
> thing?

It can be collapsed into one.

> This loop does not stop at the first error, but returns a non-zero
> error after noticing even a single remote fail to run prune, which
> sounds like a seneible design.  Would an error percolate up the same
> way when two different tasks run and one of them fails in the
> control folow in "git maintenance"?  Just want to see if we are
> being consistent with the surrounding code.

Fair point. I'll make the process flow identical to the prefetch refs
task that works similarly across remotes.
It returns as soon as the first remote fails (without necessarily
affecting other tasks).

pastelsky · 2024-12-28T10:07:03Z

/submit

gitgitgadget · 2024-12-28T10:07:50Z

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1838/pastelsky/sk/add-remote-prune-maintenance-v2

To fetch this version to local tag pr-1838/pastelsky/sk/add-remote-prune-maintenance-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1838/pastelsky/sk/add-remote-prune-maintenance-v2

gitgitgadget · 2024-12-28T16:08:13Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Shubham Kanodia <[email protected]> writes:

>> Hmph, is there a reason why you need two loops, instead of
>> for-each-remote calling a function that does the run_command()
>> thing?
>
> It can be collapsed into one.

Sorry, but that is not an answer, as my question was not a
suggestion to change anything.

It was a question asking you if there was a specific reason why the
code was structured the way it was written.  If there is another way
to write it, you need to answer why the alternative wasn't picked.

>> This loop does not stop at the first error, but returns a non-zero
>> error after noticing even a single remote fail to run prune, which
>> sounds like a seneible design.  Would an error percolate up the same
>> way when two different tasks run and one of them fails in the
>> control folow in "git maintenance"?  Just want to see if we are
>> being consistent with the surrounding code.
>
> Fair point. I'll make the process flow identical to the prefetch refs
> task that works similarly across remotes.
> It returns as soon as the first remote fails (without necessarily
> affecting other tasks).

... and the first failure signals the caller a failure?  That would
match what you did in your new feature, which is perfect.

Thanks.

gitgitgadget · 2024-12-28T16:29:14Z

On the Git mailing list, Shubham Kanodia wrote (reply to this):

On Sat, Dec 28, 2024 at 9:35 PM Junio C Hamano <[email protected]> wrote:
>
> Shubham Kanodia <[email protected]> writes:
>
> >> Hmph, is there a reason why you need two loops, instead of
> >> for-each-remote calling a function that does the run_command()
> >> thing?
> >
> > It can be collapsed into one.
>
> Sorry, but that is not an answer, as my question was not a
> suggestion to change anything.
>
> It was a question asking you if there was a specific reason why the
> code was structured the way it was written.  If there is another way
> to write it, you need to answer why the alternative wasn't picked.

There wasn't a good reason for doing it that way. I guess I was trying
to understand the second argument for `for_each_remote` would be best
used if the command was called directly (while avoiding a compilation
warning), but looking at a few other usages of `for_each_remote` I
realised that it could just be marked unused in this case (since we
aren't doing anything with it).

I should've probably looked deeper and learnt from existing patterns
(e.g. `maintenance_task_prefetch`) — which I have in my last patch.

> >> This loop does not stop at the first error, but returns a non-zero
> >> error after noticing even a single remote fail to run prune, which
> >> sounds like a seneible design.  Would an error percolate up the same
> >> way when two different tasks run and one of them fails in the
> >> control folow in "git maintenance"?  Just want to see if we are
> >> being consistent with the surrounding code.
> >
> > Fair point. I'll make the process flow identical to the prefetch refs
> > task that works similarly across remotes.
> > It returns as soon as the first remote fails (without necessarily
> > affecting other tasks).
>
> ... and the first failure signals the caller a failure?  That would
> match what you did in your new feature, which is perfect.

Exactly — the first failing remote will signal that the
`prune-remote-refs` task has failed via an immediate `return 1`.
The maintenance command uses this to register the exit code of the top
level command to 1, while continuing to execute all other tasks
anyway.

Thanks,
Shubham K

gitgitgadget · 2024-12-28T16:29:15Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Shubham Kanodia via GitGitGadget" <[email protected]> writes:

> From: Shubham Kanodia <[email protected]>
>
> Remote-tracking refs can accumulate in local repositories even as branches
> are deleted on remotes, impacting git performance negatively. Existing
> alternatives to keep refs pruned have a few issues — 
>
> 1. Running `git fetch` with either `--prune` or `fetch.prune=true` set will
> prune stale refs, but requires a manual operation and also pulls in new
> refs from remote which can be an undesirable side-effect.

It is only true if you cloned without any tweaks.  For example, if
you cloned with the single-branch option, you would not pull in new
refs, wouldn't you?  Also "requires a manual operation" is not quite
a good rationale, as you could have placed such a "git fetch"
instead of "git remote prune", in the maintenance schedule.

For this to become an issue, the condition we saw in earlier
discussion, i.e.

    while having the *default* refspec
    "+refs/heads/*:refs/remotes/$name/*" configured in
    remote.$name.fetch

is crucial.  Since that is the default refspec "git clone" gives
you, your "git fetch --prune" would give you full set of refs while
pruning, and the end result is that you have up-to-date set of
remote-tracking branches (which you may not want).

> 2.`git remote prune` cleans up refs without adding to the existing list
> but requires periodic user intervention.

You have a SP after "1." but not after "2.".

> This adds a new maintenance task 'prune-remote-refs' that runs
> 'git remote prune' for each configured remote daily. This provides an
> automated way to clean up stale remote-tracking refs — especially when
> users may not do a full fetch.

"This adds" -> "Add".

I'd strike the latter sentence.  Regardless of what users do or do
not do, the automated clean-up is performed.

> This task is disabled by default.
>
> Signed-off-by: Shubham Kanodia <[email protected]>
> ---

> +NOTE: This task is opt-in to prevent unexpected removal of remote refs
> +for users of git-maintenance. For most users, configuring `fetch.prune=true`
> +is a acceptable solution, as it will automatically clean up stale remote-tracking

"a acceptable" -> "an acceptable".

> +branches during normal fetch operations. However, this task can be useful in
> +specific scenarios:
> ++
> +--
> +* When using selective fetching (e.g., `git fetch origin +foo:refs/remotes/origin/foo`)
> +  where `fetch.prune` would only affect refs that are explicitly fetched.
> +* When third-party tools might perform unexpected full fetches, and you want
> +  periodic cleanup independently of fetch operations.
> +--

Very well written.

> @@ -913,6 +914,30 @@ static int maintenance_opt_schedule(const struct option *opt, const char *arg,
>  	return 0;
>  }
>  
> +static int prune_remote(struct remote *remote, void *cb_data UNUSED)
> +{
> +	struct child_process child = CHILD_PROCESS_INIT;
> +
> +	if (!remote->url.nr)
> +		return 0;
> +
> +	child.git_cmd = 1;
> +	strvec_pushl(&child.args, "remote", "prune", remote->name, NULL);
> +
> +	return !!run_command(&child);
> +}
> +
> +static int maintenance_task_prune_remote(struct maintenance_run_opts *opts,
> +					 struct gc_config *cfg UNUSED)
> +{
> +	if (for_each_remote(prune_remote, opts)) {
> +		error(_("failed to prune remotes"));
> +		return 1;
> +	}
> +
> +	return 0;
> +}

Nice reuse of the program structure, which is very clean and easy to read.

Overall very well written.  Will queue, with attached range-diff.
Thanks.


--- >8 ---
1:  0ae9668b5c ! 1:  8a40f8b319 maintenance: add prune-remote-refs task
    @@ Commit message
     
         Remote-tracking refs can accumulate in local repositories even as branches
         are deleted on remotes, impacting git performance negatively. Existing
    -    alternatives to keep refs pruned have a few issues — 
    +    alternatives to keep refs pruned have a few issues:
     
    -    1. Running `git fetch` with either `--prune` or `fetch.prune=true` set will
    -    prune stale refs, but requires a manual operation and also pulls in new
    -    refs from remote which can be an undesirable side-effect.
    +      1. Running `git fetch` with either `--prune` or `fetch.prune=true`
    +         set, with the default refspec to copy all their branches into
    +         our remote-tracking branches, will prune stale refs, but also
    +         pulls in new branches from remote.  That is undesirable if the
    +         user wants to only work with a selected few remote branches.
     
    -    2.`git remote prune` cleans up refs without adding to the existing list
    -    but requires periodic user intervention.
    +      2. `git remote prune` cleans up refs without adding to the
    +         existing list but requires periodic user intervention.
     
    -    This adds a new maintenance task 'prune-remote-refs' that runs
    -    'git remote prune' for each configured remote daily. This provides an
    -    automated way to clean up stale remote-tracking refs — especially when
    -    users may not do a full fetch.
    -
    -    This task is disabled by default.
    +    Add a new maintenance task 'prune-remote-refs' that runs 'git remote
    +    prune' for each configured remote daily.  Leave the task disabled by
    +    default, as it may be unexpected to see their remote-tracking
    +    branches to disappear while they are not watching for unsuspecting
    +    users.
     
         Signed-off-by: Shubham Kanodia <[email protected]>
         Signed-off-by: Junio C Hamano <[email protected]>
    @@ Documentation/git-maintenance.txt: pack-refs::
     ++
     +NOTE: This task is opt-in to prevent unexpected removal of remote refs
     +for users of git-maintenance. For most users, configuring `fetch.prune=true`
    -+is a acceptable solution, as it will automatically clean up stale remote-tracking
    ++is an acceptable solution, as it will automatically clean up stale remote-tracking
     +branches during normal fetch operations. However, this task can be useful in
     +specific scenarios:
     ++

gitgitgadget · 2024-12-28T19:21:22Z

This patch series was integrated into seen via git@6898464.

gitgitgadget · 2024-12-28T21:11:15Z

This patch series was integrated into seen via git@c020efd.

gitgitgadget · 2024-12-30T07:18:42Z

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Sat, Dec 28, 2024 at 10:07:41AM +0000, Shubham Kanodia via GitGitGadget wrote:
> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
> index 6e6651309d3..8b3e496c8ef 100644
> --- a/Documentation/git-maintenance.txt
> +++ b/Documentation/git-maintenance.txt
> @@ -158,6 +158,26 @@ pack-refs::
>  	need to iterate across many references. See linkgit:git-pack-refs[1]
>  	for more information.
>  
> +prune-remote-refs::
> +	The `prune-remote-refs` task runs `git remote prune` on each remote
> +	repository registered in the local repository. This task helps clean
> +	up deleted remote branches, improving the performance of operations
> +	that iterate through the refs. See linkgit:git-remote[1] for more
> +	information. This task is disabled by default.
> ++
> +NOTE: This task is opt-in to prevent unexpected removal of remote refs
> +for users of git-maintenance. For most users, configuring `fetch.prune=true`

Do we want to make this linkgit:git-maintenance[1] even though this is
self-referential?

> +is a acceptable solution, as it will automatically clean up stale remote-tracking
> +branches during normal fetch operations. However, this task can be useful in
> +specific scenarios:
> ++
> +--
> +* When using selective fetching (e.g., `git fetch origin +foo:refs/remotes/origin/foo`)
> +  where `fetch.prune` would only affect refs that are explicitly fetched.
> +* When third-party tools might perform unexpected full fetches, and you want
> +  periodic cleanup independently of fetch operations.
> +--

Nicely explained. I wish we had more such documentation that is taking
the user by their hand and explains why they may or may not want to have
a specific thing.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 4ae5196aedf..329c764f300 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -20,6 +20,7 @@
>  #include "lockfile.h"
>  #include "parse-options.h"
>  #include "run-command.h"
> +#include "remote.h"
>  #include "sigchain.h"
>  #include "strvec.h"
>  #include "commit.h"
> @@ -913,6 +914,30 @@ static int maintenance_opt_schedule(const struct option *opt, const char *arg,
>  	return 0;
>  }
>  
> +static int prune_remote(struct remote *remote, void *cb_data UNUSED)
> +{
> +	struct child_process child = CHILD_PROCESS_INIT;
> +
> +	if (!remote->url.nr)
> +		return 0;
> +
> +	child.git_cmd = 1;
> +	strvec_pushl(&child.args, "remote", "prune", remote->name, NULL);
> +
> +	return !!run_command(&child);
> +}
> +
> +static int maintenance_task_prune_remote(struct maintenance_run_opts *opts,
> +					 struct gc_config *cfg UNUSED)
> +{
> +	if (for_each_remote(prune_remote, opts)) {
> +		error(_("failed to prune remotes"));
> +		return 1;

I wonder whether we should adapt the loop to be eager. Erroring out on
the first failed remote would potentially mean that none of the other
remotes may get pruned. So if you had a now-unreachable remote as first
remote then none of your remotes would be pruned.

If so, we may want to collect the names of failed remotes and print
them, as well.

Patrick

gitgitgadget · 2024-12-30T14:09:48Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Patrick Steinhardt <[email protected]> writes:

> On Sat, Dec 28, 2024 at 10:07:41AM +0000, Shubham Kanodia via GitGitGadget wrote:
>> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
>> index 6e6651309d3..8b3e496c8ef 100644
>> --- a/Documentation/git-maintenance.txt
>> +++ b/Documentation/git-maintenance.txt
>> @@ -158,6 +158,26 @@ pack-refs::
>>  	need to iterate across many references. See linkgit:git-pack-refs[1]
>>  	for more information.
>>  
>> +prune-remote-refs::
>> +	The `prune-remote-refs` task runs `git remote prune` on each remote
>> +	repository registered in the local repository. This task helps clean
>> +	up deleted remote branches, improving the performance of operations
>> +	that iterate through the refs. See linkgit:git-remote[1] for more
>> +	information. This task is disabled by default.
>> ++
>> +NOTE: This task is opt-in to prevent unexpected removal of remote refs
>> +for users of git-maintenance. For most users, configuring `fetch.prune=true`
>
> Do we want to make this linkgit:git-maintenance[1] even though this is
> self-referential?

That certainly is a thought---the rule could be "whenever we refer
to a Git command, we refer to it in a uniform way".  An alternative
would be "of git-maintenance" -> "of this command" to weaken it.

This refers to those users who want to use the command for other
reasons (you use the scheduled tasks driven by 'git maintenance'
only because you wanted the 'gc' and 'pack-refs' tasks to run, you
do not necessarily want to run a new kind of task the new version of
Git started supporting, especially when the task is destructive,
like this one).  We might want to stress that point, perhaps?  If a
reader reads this part of the documentation, finds this task useful
and decides to use 'git maintenance', the note would sound somewhat
nonsensical to them---"I thought about the ramifications, I decided
I wanted to use the command, why would it be opt-in?" is a plausible
confusion.

>> +is a acceptable solution, as it will automatically clean up stale remote-tracking
>> +branches during normal fetch operations. However, this task can be useful in
>> +specific scenarios:
>> ++
>> +--
>> +* When using selective fetching (e.g., `git fetch origin +foo:refs/remotes/origin/foo`)
>> +  where `fetch.prune` would only affect refs that are explicitly fetched.
>> +* When third-party tools might perform unexpected full fetches, and you want
>> +  periodic cleanup independently of fetch operations.
>> +--
>
> Nicely explained. I wish we had more such documentation that is taking
> the user by their hand and explains why they may or may not want to have
> a specific thing.

Yes, a configuration or an option that are not for everybody and for
every situation need such a guidance, and this one is done nicely.

>> +static int maintenance_task_prune_remote(struct maintenance_run_opts *opts,
>> +					 struct gc_config *cfg UNUSED)
>> +{
>> +	if (for_each_remote(prune_remote, opts)) {
>> +		error(_("failed to prune remotes"));
>> +		return 1;
>
> I wonder whether we should adapt the loop to be eager. Erroring out on
> the first failed remote would potentially mean that none of the other
> remotes may get pruned. So if you had a now-unreachable remote as first
> remote then none of your remotes would be pruned.

I think the structure, hence the behaviour, is shared with an
existing prefetch task.  I think the current way is OK-ish, but
given that we are not in a hurry, we may want to correct the
semantics for both of them before unleashing this new task to the
world.

For that, we need the callback functions given to for_each_remote
(i.e., fetch_remote and prune_remote) to always return "success" in
the sense to tell "I am done with this remote" to allow the loop to
continue to the next remote, and convey the failure from the
subcommand via some other means (like flipping a bit in the cbdata).

Thanks.

gitgitgadget · 2024-12-30T15:16:43Z

This branch is now known as sk/maintenance-remote-prune.

gitgitgadget · 2024-12-30T17:15:38Z

This patch series was integrated into seen via git@1c85b88.

gitgitgadget · 2024-12-30T17:36:51Z

There was a status update in the "Cooking" section about the branch sk/maintenance-remote-prune on the Git mailing list:

A new periodic maintenance task to run "git remote prune" has been
introduced.

Expecting a reroll.
cf. <[email protected]>
source: <[email protected]>

gitgitgadget · 2025-01-01T22:30:59Z

This patch series was integrated into seen via git@78f907d.

gitgitgadget · 2025-01-02T00:08:29Z

This patch series was integrated into seen via git@53cc91b.

gitgitgadget · 2025-01-02T16:42:39Z

This patch series was integrated into seen via git@49096a1.

gitgitgadget · 2025-01-02T21:59:54Z

This patch series was integrated into seen via git@d81b897.

gitgitgadget · 2025-01-03T06:54:05Z

On the Git mailing list, Shubham Kanodia wrote (reply to this):

On Mon, Dec 30, 2024 at 7:35 PM Junio C Hamano <[email protected]> wrote:
>
> Patrick Steinhardt <[email protected]> writes:
>
> > On Sat, Dec 28, 2024 at 10:07:41AM +0000, Shubham Kanodia via GitGitGadget wrote:
> >> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
> >> index 6e6651309d3..8b3e496c8ef 100644
> >> --- a/Documentation/git-maintenance.txt
> >> +++ b/Documentation/git-maintenance.txt
> >> @@ -158,6 +158,26 @@ pack-refs::
> >>      need to iterate across many references. See linkgit:git-pack-refs[1]
> >>      for more information.
> >>
> >> +prune-remote-refs::
> >> +    The `prune-remote-refs` task runs `git remote prune` on each remote
> >> +    repository registered in the local repository. This task helps clean
> >> +    up deleted remote branches, improving the performance of operations
> >> +    that iterate through the refs. See linkgit:git-remote[1] for more
> >> +    information. This task is disabled by default.
> >> ++
> >> +NOTE: This task is opt-in to prevent unexpected removal of remote refs
> >> +for users of git-maintenance. For most users, configuring `fetch.prune=true`
> >
> > Do we want to make this linkgit:git-maintenance[1] even though this is
> > self-referential?
>
> That certainly is a thought---the rule could be "whenever we refer
> to a Git command, we refer to it in a uniform way".  An alternative
> would be "of git-maintenance" -> "of this command" to weaken it.
>
> This refers to those users who want to use the command for other
> reasons (you use the scheduled tasks driven by 'git maintenance'
> only because you wanted the 'gc' and 'pack-refs' tasks to run, you
> do not necessarily want to run a new kind of task the new version of
> Git started supporting, especially when the task is destructive,
> like this one).  We might want to stress that point, perhaps?  If a
> reader reads this part of the documentation, finds this task useful
> and decides to use 'git maintenance', the note would sound somewhat
> nonsensical to them---"I thought about the ramifications, I decided
> I wanted to use the command, why would it be opt-in?" is a plausible
> confusion.
>
> >> +is a acceptable solution, as it will automatically clean up stale remote-tracking
> >> +branches during normal fetch operations. However, this task can be useful in
> >> +specific scenarios:
> >> ++
> >> +--
> >> +* When using selective fetching (e.g., `git fetch origin +foo:refs/remotes/origin/foo`)
> >> +  where `fetch.prune` would only affect refs that are explicitly fetched.
> >> +* When third-party tools might perform unexpected full fetches, and you want
> >> +  periodic cleanup independently of fetch operations.
> >> +--
> >
> > Nicely explained. I wish we had more such documentation that is taking
> > the user by their hand and explains why they may or may not want to have
> > a specific thing.
>
> Yes, a configuration or an option that are not for everybody and for
> every situation need such a guidance, and this one is done nicely.
>
> >> +static int maintenance_task_prune_remote(struct maintenance_run_opts *opts,
> >> +                                     struct gc_config *cfg UNUSED)
> >> +{
> >> +    if (for_each_remote(prune_remote, opts)) {
> >> +            error(_("failed to prune remotes"));
> >> +            return 1;
> >
> > I wonder whether we should adapt the loop to be eager. Erroring out on
> > the first failed remote would potentially mean that none of the other
> > remotes may get pruned. So if you had a now-unreachable remote as first
> > remote then none of your remotes would be pruned.
>
> I think the structure, hence the behaviour, is shared with an
> existing prefetch task.  I think the current way is OK-ish, but
> given that we are not in a hurry, we may want to correct the
> semantics for both of them before unleashing this new task to the
> world.
>
> For that, we need the callback functions given to for_each_remote
> (i.e., fetch_remote and prune_remote) to always return "success" in
> the sense to tell "I am done with this remote" to allow the loop to
> continue to the next remote, and convey the failure from the
> subcommand via some other means (like flipping a bit in the cbdata).
>
> Thanks.

Curious — I submitted my patches through GGG, but Junio was kind
enough to apply a few other fixes to it.
Is there a place I can now get the whole diff (with the range diff
patched in) so I can pull that into GGG?

Thanks,
Shubham

gitgitgadget · 2025-01-03T07:41:54Z

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Fri, Jan 03, 2025 at 12:20:07PM +0530, Shubham Kanodia wrote:
> Curious — I submitted my patches through GGG, but Junio was kind
> enough to apply a few other fixes to it.
> Is there a place I can now get the whole diff (with the range diff
> patched in) so I can pull that into GGG?

Junio publishes his branches at [1], and yours specifically is called
"sk/maintenance-remote-prune". I'd typically just update my local branch
to match what he has in there.

Patrick

[1]: https://github.com/gitster/git.git

Remote-tracking refs can accumulate in local repositories even as branches are deleted on remotes, impacting git performance negatively. Existing alternatives to keep refs pruned have a few issues: 1. Running `git fetch` with either `--prune` or `fetch.prune=true` set, with the default refspec to copy all their branches into our remote-tracking branches, will prune stale refs, but also pulls in new branches from remote. That is undesirable if the user wants to only work with a selected few remote branches. 2. `git remote prune` cleans up refs without adding to the existing list but requires periodic user intervention. Add a new maintenance task 'prune-remote-refs' that runs 'git remote prune' for each configured remote daily. Leave the task disabled by default, as it may be unexpected to see their remote-tracking branches to disappear while they are not watching for unsuspecting users. Signed-off-by: Shubham Kanodia <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>

gitgitgadget · 2025-01-03T18:08:10Z

This patch series was integrated into seen via git@1cfcd06.

pastelsky · 2025-01-03T18:09:19Z

/submit

gitgitgadget · 2025-01-03T18:14:02Z

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1838/pastelsky/sk/add-remote-prune-maintenance-v3

To fetch this version to local tag pr-1838/pastelsky/sk/add-remote-prune-maintenance-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1838/pastelsky/sk/add-remote-prune-maintenance-v3

gitgitgadget · 2025-01-03T19:06:15Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Shubham Kanodia via GitGitGadget" <[email protected]> writes:

> From: Shubham Kanodia <[email protected]>
>
> Remote-tracking refs can accumulate in local repositories even as branches
> are deleted on remotes, impacting git performance negatively. Existing
> alternatives to keep refs pruned have a few issues:
>
>   1. Running `git fetch` with either `--prune` or `fetch.prune=true`
>      set, with the default refspec to copy all their branches into
>      our remote-tracking branches, will prune stale refs, but also
>      pulls in new branches from remote.  That is undesirable if the
>      user wants to only work with a selected few remote branches.
>
>   2. `git remote prune` cleans up refs without adding to the
>      existing list but requires periodic user intervention.
>
> Add a new maintenance task 'prune-remote-refs' that runs 'git remote
> prune' for each configured remote daily.  Leave the task disabled by
> default, as it may be unexpected to see their remote-tracking
> branches to disappear while they are not watching for unsuspecting
> users.

There is no description on how and why the prefetch job has been
modified here.

I haven't formed a strong opinion on the "should we keep going after
the first failure?" question yet, and if this topic is modifying the
way how the prefetch operates, the patch(es) should be CC'ed to the
author of that feature (The author of 28cb5e66 (maintenance: add
prefetch task, 2020-09-25) CC'ed).

If it turns out to be a good idea to do so, I would expect the topic
to consist of at least two patches:

 - [PATCH 1/2] to argue that it is a bug that the prefetch job stops
   at the first failed remote, and change its behaviour to prefetch
   from all remotes and then report a failure if the prefetch failed
   for any remote.  With some additional tests to check the updated
   behaviour.

 - [PATCH 2/2] to argue the need for periodic `remote prune`, and do
   the part of this patch that relates to that new feature.

> +struct remote_cb_data {
> +	struct maintenance_run_opts *maintenance_opts;
> +	struct string_list failed_remotes;
> +};
> +
> +static void report_failed_remotes(struct string_list *failed_remotes,
> +				  const char *action_name)
> +{
> +	if (failed_remotes->nr) {
> +		int i;
> +		struct strbuf msg = STRBUF_INIT;
> +		strbuf_addf(&msg, _("failed to %s the following remotes: "),
> +			    action_name);
> +		for (i = 0; i < failed_remotes->nr; i++) {
> +			if (i)
> +				strbuf_addstr(&msg, ", ");
> +			strbuf_addstr(&msg, failed_remotes->items[i].string);
> +		}
> +		error("%s", msg.buf);
> +		strbuf_release(&msg);
> +	}
> +}

A few comments:

 - The message pretends to be _("localizable"), but the sentence
   logo inserts action_name that is not translated.  If the
   operation failed only for a single remote, "following remotes" is
   grammatically incorrect.

 - Would it be useful to force this message to a single line, with
   multiple remote names concatenated with ","?  Computer output of
   a listing often is more useful without "," if it is meant to be
   consumable for cut-and-paste users.

Overall, I am fairly negative on the report this helper tries to
give the users.  Because we are going to do the operation on all
remotes anyway, wouldn't we have let the underlying operations (like
"git fetch" or "git remore prune") already issue error messages to
the user?  Do we need this extra reporting on top at all?

Thanks.

gitgitgadget · 2025-01-03T21:28:25Z

There was a status update in the "Cooking" section about the branch sk/maintenance-remote-prune on the Git mailing list:

A new periodic maintenance task to run "git remote prune" has been
introduced.

Expecting a reroll.
cf. <[email protected]>
source: <[email protected]>

gitgitgadget · 2025-01-03T22:36:18Z

This patch series was integrated into seen via git@5411572.

gitgitgadget · 2025-01-06T18:51:43Z

This patch series was integrated into seen via git@aef6dde.

gitgitgadget · 2025-01-06T20:06:01Z

There was a status update in the "Cooking" section about the branch sk/maintenance-remote-prune on the Git mailing list:

A new periodic maintenance task to run "git remote prune" has been
introduced.

Expecting a reroll.
source: <[email protected]>

pastelsky force-pushed the sk/add-remote-prune-maintenance branch 7 times, most recently from 849e20a to 1af8661 Compare December 18, 2024 03:38

pastelsky force-pushed the sk/add-remote-prune-maintenance branch from 1af8661 to b67f04c Compare December 23, 2024 09:04

pastelsky force-pushed the sk/add-remote-prune-maintenance branch 2 times, most recently from b66f827 to 72e27d3 Compare December 23, 2024 09:33

pastelsky force-pushed the sk/add-remote-prune-maintenance branch 2 times, most recently from 547f14d to 4d6c143 Compare December 28, 2024 09:56

gitgitgadget bot added the seen label Dec 28, 2024

pastelsky force-pushed the sk/add-remote-prune-maintenance branch 2 times, most recently from 9ebcee7 to 362a069 Compare January 3, 2025 08:52

pastelsky force-pushed the sk/add-remote-prune-maintenance branch from 362a069 to 7954df1 Compare January 3, 2025 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

maintenance: add prune-remote-refs task #1838

maintenance: add prune-remote-refs task #1838

pastelsky commented Dec 17, 2024 •

edited

Loading

gitgitgadget bot commented Dec 17, 2024

gitgitgadget bot commented Dec 18, 2024

gitgitgadget bot commented Dec 23, 2024

pastelsky commented Dec 23, 2024

gitgitgadget bot commented Dec 23, 2024

gitgitgadget bot commented Dec 27, 2024

gitgitgadget bot commented Dec 28, 2024

pastelsky commented Dec 28, 2024

gitgitgadget bot commented Dec 28, 2024

gitgitgadget bot commented Dec 28, 2024

gitgitgadget bot commented Dec 28, 2024

gitgitgadget bot commented Dec 28, 2024

gitgitgadget bot commented Dec 28, 2024

gitgitgadget bot commented Dec 28, 2024

gitgitgadget bot commented Dec 30, 2024

gitgitgadget bot commented Dec 30, 2024

gitgitgadget bot commented Dec 30, 2024

gitgitgadget bot commented Dec 30, 2024

gitgitgadget bot commented Dec 30, 2024

gitgitgadget bot commented Jan 1, 2025

gitgitgadget bot commented Jan 2, 2025

gitgitgadget bot commented Jan 2, 2025

gitgitgadget bot commented Jan 2, 2025

gitgitgadget bot commented Jan 3, 2025

gitgitgadget bot commented Jan 3, 2025

gitgitgadget bot commented Jan 3, 2025

pastelsky commented Jan 3, 2025

gitgitgadget bot commented Jan 3, 2025

gitgitgadget bot commented Jan 3, 2025

gitgitgadget bot commented Jan 3, 2025

gitgitgadget bot commented Jan 3, 2025

gitgitgadget bot commented Jan 6, 2025

gitgitgadget bot commented Jan 6, 2025

maintenance: add prune-remote-refs task #1838

Are you sure you want to change the base?

maintenance: add prune-remote-refs task #1838

Conversation

pastelsky commented Dec 17, 2024 • edited Loading

gitgitgadget bot commented Dec 17, 2024

gitgitgadget bot commented Dec 18, 2024

gitgitgadget bot commented Dec 23, 2024

pastelsky commented Dec 23, 2024

gitgitgadget bot commented Dec 23, 2024

gitgitgadget bot commented Dec 27, 2024

gitgitgadget bot commented Dec 28, 2024

pastelsky commented Dec 28, 2024

gitgitgadget bot commented Dec 28, 2024

gitgitgadget bot commented Dec 28, 2024

gitgitgadget bot commented Dec 28, 2024

gitgitgadget bot commented Dec 28, 2024

gitgitgadget bot commented Dec 28, 2024

gitgitgadget bot commented Dec 28, 2024

gitgitgadget bot commented Dec 30, 2024

gitgitgadget bot commented Dec 30, 2024

gitgitgadget bot commented Dec 30, 2024

gitgitgadget bot commented Dec 30, 2024

gitgitgadget bot commented Dec 30, 2024

gitgitgadget bot commented Jan 1, 2025

gitgitgadget bot commented Jan 2, 2025

gitgitgadget bot commented Jan 2, 2025

gitgitgadget bot commented Jan 2, 2025

gitgitgadget bot commented Jan 3, 2025

gitgitgadget bot commented Jan 3, 2025

gitgitgadget bot commented Jan 3, 2025

pastelsky commented Jan 3, 2025

gitgitgadget bot commented Jan 3, 2025

gitgitgadget bot commented Jan 3, 2025

gitgitgadget bot commented Jan 3, 2025

gitgitgadget bot commented Jan 3, 2025

gitgitgadget bot commented Jan 6, 2025

gitgitgadget bot commented Jan 6, 2025

pastelsky commented Dec 17, 2024 •

edited

Loading