From: Patrick Steinhardt Date: Thu, 03 Sep 2026 07:40:04 GMT Subject: Re: [PATCH] rerere: keep a background gc from killing a rebase Message-ID: In-Reply-To: On Wed, Sep 02, 2026 at 08:31:37AM +0000, Thomas Bachem via GitGitGadget wrote: > From: Thomas Bachem > > 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