Re: [PATCH v4 1/2] rerere: wait for MERGE_RR.lock, and let the gc skip it
flat view
From: Patrick Steinhardt <hidden>
Date: 2026-09-28 08:18:20
On Mon, Sep 14, 2026 at 08:04:20AM +0000, Thomas Bachem via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
@@ -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