Re: [PATCH] rerere: keep a background gc from killing a rebase

[Patrick Steinhardt <ps@pks.im>]

On Wed, Sep 02, 2026 at 08:31:37AM +0000, Thomas Bachem via GitGitGadget wrote:
> From: Thomas Bachem <mail@thomasbachem.com>
> 
> Since 2.54 unscheduled maintenance uses the "geometric" strategy, so
> the "git maintenance run --auto --detach" behind every "git commit"
> runs "git rerere gc" in the background whenever rr-cache has an entry.
> That includes the "git commit" the sequencer runs for a resolved pick
> on "git rebase --continue".

I think this hints that we should tweak the default value of
"maintenance.rerere-gc.auto". The way it's currently written we indeed
are quite aggressive with spawning `git rerere gc`, and I agree that we
should tweak it. And in the best case we'd not only respect whether we
have a specific number of entries, but we should also respect whether
those would be garbage collected in the first place.

I'll send a patch series later today to do this.

[snip]
> The gc needs the lock: it removes every rr-cache directory it finds
> empty, and a rerere that has just created its directory but not yet
> written the preimage looks exactly like that. So keep the lock and fix
> both orders. When the gc finds the lock busy, let it warn and do
> nothing this time, the way "maintenance run" treats its own lock, so a
> manual "git rerere gc" sees the warning and the maintenance task and
> "git gc" see a clean exit. When the gc holds the lock, let every other
> caller wait it out instead of dying at once, for rerere.lockTimeout
> milliseconds with the semantics of core.packedRefsTimeout: 1000 by
> default, 0 for the old behaviour, -1 for an unbounded wait. Walking a
> 20000-entry rr-cache takes about 0.4 s here.

Having a locking timeout is sensible anyway, I think. It does not only
solve races with a concurrent maintenance run, but also with concurrent
writers.

> diff --git a/rerere.c b/rerere.c
> index 8232542585..22d114262b 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -32,6 +32,7 @@ static int rerere_enabled = -1;
>  
>  /* automatically update cleanly resolved paths to the index */
>  static int rerere_autoupdate;
> +static int rerere_lock_timeout_ms = 1000;
>  
>  #define RR_HAS_POSTIMAGE 1
>  #define RR_HAS_PREIMAGE 2
> @@ -876,6 +877,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);
>  }
>  
> @@ -908,12 +911,26 @@ 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) {
>  		fd = 0;
> -	else
> +	} else if (flags & RERERE_SKIP_LOCKED) {
>  		fd = hold_lock_file_for_update(&write_lock,
> -					       git_path_merge_rr(r),
> -					       LOCK_DIE_ON_ERROR);
> +					       git_path_merge_rr(r), 0);
> +		if (fd < 0) {
> +			warning_errno(_("unable to lock '%s', skipping"),
> +				      git_path_merge_rr(r));
> +			return -1;
> +		}

We should instead pass `LOCK_REPORT_ON_ERROR`, as the lockfile machinery
knows better why exactly locking has failed.

> +	} else {
> +		/*
> +		 * A background "rerere gc" holds the lock for as long as it
> +		 * takes to walk rr-cache, so wait it out rather than die.
> +		 */
> +		fd = hold_lock_file_for_update_timeout(&write_lock,
> +						       git_path_merge_rr(r),
> +						       LOCK_DIE_ON_ERROR,
> +						       rerere_lock_timeout_ms);
> +	}

I think we can easily combine those two branches and simply set the
timeout value to 0 in case we see the flag.

Patrick