Re: [PATCH v3] maintenance: add prune-remote-refs task

On Sat, Jan 4, 2025 at 1:23 AM Shubham Kanodia
[off-list ref] wrote:

On Sat, 4 Jan 2025 at 12:32 AM, Junio C Hamano [off-list ref] wrote:

quoted

"Shubham Kanodia via GitGitGadget" [off-list ref] writes:

quoted

From: Shubham Kanodia <redacted>

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.

quoted

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

If it’s fine, I’d like to discuss the change to process all remotes separately from the initial change I submitted.

I took an attempt at it in my last patch, but I don’t know if I’m really going to have the time to iterate on it as it looks more involved.

In any case the implementation isn’t any worse off than the current maintenance command.

Also, as a new contributor I’m unsure of recalling / patching a change that’s already in seen at the moment unless it is an unworkable solution.

Thanks again.
Shubham K

Noticed an update on What's Cooking —

* sk/maintenance-remote-prune (2025-01-03) 1 commit
- maintenance: add prune-remote-refs task
 A new periodic maintenance task to run "git remote prune" has been

 introduced.

Expecting a reroll.

Junio, what do you think about my previous suggestion — do you think
that changing the remote behaviours is a blocker for this change to
make its way to master?