Re: [PATCH v2] maintenance: improve handling of lock acquisition failures

[Patrick Steinhardt <ps@pks.im>]

On Mon, Nov 02, 2026 at 12:29:56PM +0000, Pushkar Singh wrote:
> Changes in v2:
> - Report lock acquisition failures for manual invocations as errors.
> - Print a warning for automatic invocations while preserving successful exit status.
> - Trace the reason for all automatic lock acquisition failures, not just EEXIST.
> - Add tests for automatic and manual lock failure behavior.

This really should be two patches now:

  - One patch that changes how we handle the failure to lock.

  - One patch that introduces your trace2 string.

One note though: please wait a bit before rerolling again so that others
can chime in first. You should ideally reroll at most once per day.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 81cecf6b6c..9223409750 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1402,6 +1402,8 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
>  	enum auto_gc_hook_result auto_gc_hook_result = AUTO_GC_HOOK_UNDECIDED;
>  
>  	if (repo_hold_lock_file_for_update(r, &lk, lock_path, LOCK_NO_DEREF) < 0) {
> +		int err = errno;
> +
>  		/*
>  		 * Another maintenance command is running.
>  		 *
> @@ -1409,11 +1411,29 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
>  		 * recursive process stack. Do not report an error in
>  		 * that case.
>  		 */
> -		if (!opts->auto_flag && !opts->quiet)
> -			warning(_("lock file '%s' exists, skipping maintenance"),
> -				lock_path);
> +		if (opts->auto_flag) {
> +			trace2_data_string("maintenance", r, "lock/skip",
> +				(err == EEXIST) ? "exists" : strerror(err));

Why don't you use strerror(3p) for both cases?

> +			if (!opts->quiet) {
> +				struct strbuf buf = STRBUF_INIT;
> +				unable_to_lock_message(lock_path, err, &buf);
> +				warning("%s", buf.buf);
> +				strbuf_release(&buf);
> +			}
> +
> +			free(lock_path);
> +			return 0;
> +		}
> +
> +		if (!opts->quiet) {
> +			struct strbuf buf = STRBUF_INIT;
> +			unable_to_lock_message(lock_path, err, &buf);
> +			error("%s", buf.buf);
> +			strbuf_release(&buf);

There's some duplication here with the auto-flag case. Can't we
deduplicate that?

Thanks!

Patrick