Re: [PATCH] rerere: keep a background gc from killing a rebase
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 3, 2026, 07:40 UTC
- Message-ID
- <apkkVAYOqjfAsp9-@pks.im>
- In-Reply-To
- <pull.2214.git.1788337897490.gitgitgadget@gmail.com>
On Wed, Sep 02, 2026 at 08:31:37AM +0000, Thomas Bachem via GitGitGadget wrote:
Show 7 quoted lines
> 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]
Show 11 quoted lines
> 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.
Show 39 quoted lines
> 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.
Show 10 quoted lines
> + } 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