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