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

[Junio C Hamano <gitster@pobox.com>]

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