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

>
>
>
> On Sat, 4 Jan 2025 at 12:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> "Shubham Kanodia via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>> > From: Shubham Kanodia <shubham.kanodia10@gmail.com>
>> >
>> > 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.
>
>
>
> 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