From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 28 Sep 2026 08:18:14 GMT
Subject: Re: [PATCH v4 1/2] rerere: wait for MERGE_RR.lock, and let the gc skip it
Message-ID: <aroixgCkqbmKErng@pks.im>
In-Reply-To: <8a7a74d6aa359844a49593538ef6178cd1b02031.1789373061.git.gitgitgadget@gmail.com>

On Mon, Sep 14, 2026 at 08:04:20AM +0000, Thomas Bachem via GitGitGadget wrote:
> diff --git a/Documentation/git-rerere.adoc b/Documentation/git-rerere.adoc
> index 4e6ab9a27c..4df653367e 100644
> --- a/Documentation/git-rerere.adoc
> +++ b/Documentation/git-rerere.adoc
> @@ -70,7 +70,9 @@ occurred a long time ago.  By default, unresolved conflicts older
>  than 15 days and resolved conflicts older than 60
>  days are pruned.  These defaults are controlled via the
>  `gc.rerereUnresolved` and `gc.rerereResolved` configuration
> -variables respectively.
> +variables respectively.  If another process holds the rerere lock,
> +for example a merge or rebase that is recording a conflict, `gc`
> +does nothing and says so.

Do we maybe want to drop these examples? I don't feel like they add any
value.

> diff --git a/rerere.c b/rerere.c
> index 3d3bd0db16..7d44f3937c 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -33,6 +33,9 @@ static int rerere_enabled = -1;
>  /* automatically update cleanly resolved paths to the index */
>  static int rerere_autoupdate;
>  
> +/* how long to wait for MERGE_RR.lock, in milliseconds */
> +static int rerere_lock_timeout_ms = 1000;
> +
>  #define RR_HAS_POSTIMAGE 1
>  #define RR_HAS_PREIMAGE 2
>  struct rerere_dir {
> @@ -850,6 +853,8 @@ static void git_rerere_config(void)
>  {
>  	repo_config_get_bool(the_repository, "rerere.enabled", &rerere_enabled);
>  	repo_config_get_bool(the_repository, "rerere.autoupdate", &rerere_autoupdate);
> +	repo_config_get_int(the_repository, "rerere.locktimeout",
> +			    &rerere_lock_timeout_ms);
>  	repo_config(the_repository, git_default_config, NULL);
>  }
>  
> @@ -882,12 +887,34 @@ int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags)
>  
>  	if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE))
>  		rerere_autoupdate = !!(flags & RERERE_AUTOUPDATE);
> -	if (flags & RERERE_READONLY)
> +	if ((flags & RERERE_READONLY) && (flags & RERERE_NOWAIT))
> +		BUG("RERERE_READONLY takes no lock, so RERERE_NOWAIT does not apply");
> +	if (flags & RERERE_READONLY) {
>  		fd = 0;
> -	else
> -		fd = repo_hold_lock_file_for_update(r, &write_lock,
> -						    git_path_merge_rr(r),
> -						    LOCK_DIE_ON_ERROR);
> +	} else {
> +		const char *path = git_path_merge_rr(r);
> +		int lock_flags = LOCK_DIE_ON_ERROR;
> +		long timeout_ms = rerere_lock_timeout_ms;

Here you're using a `long` whereas `rerere_lock_timeout_ms` is an `int`.
Of course we'd ideally use a `long` consistently as that's also what
`repo_hold_lock_file_for_update_timeout()` accepts. But I guess the
reason you didn't is that we don't have `repo_config_get_long()`. So I
guess this is good enough for now.

> @@ -1211,7 +1238,7 @@ void rerere_gc(struct repository *r, struct string_list *rr)
>  	timestamp_t cutoff_resolve = now - 60 * 86400;
>  	struct strbuf buf = STRBUF_INIT;
>  
> -	if (setup_rerere(r, rr, 0) < 0)
> +	if (setup_rerere(r, rr, RERERE_NOWAIT) < 0)
>  		return;
>  
>  	repo_config_get_expiry_in_days(the_repository, "gc.rerereresolved",

I'm not a 100% sold on this change. There's two different scenarios
under which we want to perform garbage collection:

  - As part of auto-maintenance, triggered by Git automatically. Here
    I'm fully aligned that it makes sense to just silently ignore the
    case where we couldn't acquire the lock, as auto-maintenance is done
    on a best-effort basis anyway.

  - As part of `git rerere gc`, which is invoked manually by the user.
    Here I'm less so, as the user has explicitly asked us to garbage
    collect. Sure, we print a warning now, but the exit code does not
    signal that we failed garbage collecting.

So I'd argue that we should discern those two use cases. I think that in
the second use case, we'd probably want to use a timeout and if we fail
to acquire the lock, we should make `git rerere gc` fail with a non-zero
exit code.

Patrick