Repository navigation
rerere: wait for MERGE_RR.lock, but not in auto maintenance - #2214
Open
thomasbachem wants to merge 2 commits into
Open
thomasbachem wants to merge 2 commits into
thomasbachem wants to merge 2 commits into
Conversation
Welcome to GitGitGadgetHi @thomasbachem, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
Member
|
/allow |
|
User thomasbachem is now allowed to use GitGitGadget. |
Author
|
/preview |
|
Preview email sent as pull.2214.git.1788337239398.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2214.git.1788337897490.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Phillip Wood wrote on the Git mailing list (how to reply to this email): Hi Thomas
On 02/09/2026 09:31, Thomas Bachem via GitGitGadget wrote:
> From: Thomas Bachem <mail@thomasbachem.com>
> > Since 2.54 unscheduled maintenance uses the "geometric" strategy, so
That change really is the gift that keeps on giving
> 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".
> > rerere_gc() takes MERGE_RR.lock through setup_rerere(), which uses
> LOCK_DIE_ON_ERROR, and so does the sequencer's repo_rerere() at the
> next conflict a few milliseconds later. Whichever comes second dies.
To me this is another reason why we should disable gc.auto while rebasing. To do that we need to pass "-c gc.auto=false -c maintenance.auto=false" when running "git commit" in run_git_commit() and also when running "git merge" in do_merge(). We should also pass those settings via GIT_CONFIG_PARAMETERS when running a exec command in do_exec(). That is largly papering over the cracks but until we have a systematic solution it does at least stop exposing users to this bug.
> When it is the rebase, it dies in do_pick_commit() That's a bug us well - we should be returning errors, not dying -rerere_setup() should be returning an error, so we can clean up and reschedule the pick.
There is a lot of detail here about what causes the problem which is helpful, but there is very little discussion about the fix. As I understand it we now block the sequencer until the background maintenance has completed, or continue to die in an inconvenient state we timeout before the background maintenance finishes. That seems rather unfortunate as the idea of running the maintenance in the background is to prevent it from interfering with other commands.
I think my preferred solution is to disable gc while rebasing. Returning an error from rerere_setup() would also help in the case where the user runs "git commit" and then continues the rebase. I'd be interested to hear what Junio and Patrick think about that. I'm also not clear why gc.auto has to fork a separate process just to check if it needs to run or not, I've not been following closely but my impression is that that is the cause of quite a lot of the lock contention bugs we've seen.
Thanks
Phillip
> with the index
> written but before make_patch() writes rebase-merge/{message,patch,
> stopped-sha}, and every later "git rebase --continue" refuses with
> "you have staged changes in your working tree". When it is the "git
> commit" of a later continue, that one dies in its post-commit
> repo_rerere() after the commit was made. Before 2.54 the same
> collision needed an auto gc to actually run, since gc runs
> "rerere gc" at its end.
> > A rebase with two conflicts in a row shows it. The filler makes the
> pick slower than the ~5 ms the background task needs to take the
> lock, and keeps the lock held for about 0.4 s. It hit 6 of 6 runs
> here on 2.55.0, and a test suite driving rebases on toy repositories
> with a single rr-cache entry hit it in both runs that were traced:
> > git init -q -b main r && cd r
> git config rerere.enabled true
> git config maintenance.auto false
> mkdir pad && seq 20000 | (cd pad && split -l 1 -a 5)
> echo base >f && git add -A && git commit -qm base
> git checkout -q -b topic
> echo b >f && git commit -qam B
> echo c >f && git commit -qam C
> git checkout -q main
> echo a >f && git commit -qam A
> git repack -adq
> seq 20000 | awk '{printf ".git/rr-cache/%040x\n", $1}' \
> | xargs mkdir -p
> for d in .git/rr-cache/*/; do echo x >$d/preimage; done
> git config --unset maintenance.auto
> git checkout -q topic
> git rebase main
> echo ab >f && git add f
> GIT_EDITOR=true git rebase --continue
> > The second continue dies with "Unable to create '.git/MERGE_RR.lock':
> File exists" while the gc spawned by its own commit holds the lock,
> and after resolving C every further continue refuses. Maintenance
> stays off during the setup so that no repack is pending: a repack due
> at that commit runs ahead of rerere-gc in the task list and would
> spend the window.
> > 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.
> > That rebase now completes. The tests cover the gc under a held lock,
> directly and through the maintenance task, a merge that waits a lock
> out within a five second rerere.lockTimeout, and one that fails at
> once with a timeout of 0.
> > Assisted-by: Claude Fable 5.1
> Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
> ---
> rerere: keep a background gc from killing a rebase
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2214%2Fthomasbachem%2Frerere-gc-lock-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2214/thomasbachem/rerere-gc-lock-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2214
> > Documentation/config/rerere.adoc | 8 +++++++
> Documentation/git-rerere.adoc | 4 +++-
> rerere.c | 27 +++++++++++++++++----
> rerere.h | 1 +
> t/t4200-rerere.sh | 40 ++++++++++++++++++++++++++++++++
> t/t7900-maintenance.sh | 8 +++++++
> 6 files changed, 82 insertions(+), 6 deletions(-)
> > diff --git a/Documentation/config/rerere.adoc b/Documentation/config/rerere.adoc
> index 3a78b5ebb1..8041a1587b 100644
> --- a/Documentation/config/rerere.adoc
> +++ b/Documentation/config/rerere.adoc
> @@ -10,3 +10,11 @@ rerere.enabled::
> enabled if there is an `rr-cache` directory under the
> `$GIT_DIR`, e.g. if "rerere" was previously used in the
> repository.
> +
> +rerere.lockTimeout::
> + The length of time, in milliseconds, to retry when trying to
> + take the rerere lock while another process holds it, typically
> + a background `git rerere gc`. Value 0 means not to retry at
> + all; -1 means to try indefinitely. Default is 1000 (i.e.,
> + retry for 1 second). `git rerere gc` itself does not wait and
> + skips its run instead.
> diff --git a/Documentation/git-rerere.adoc b/Documentation/git-rerere.adoc
> index 4e6ab9a27c..05935b0603 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 lock on the
> +recorded resolutions, for example a merge or rebase that is recording
> +a conflict, `gc` does nothing and reports so.
> > > DISCUSSION
> 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;
> + }
> + } 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);
> + }
> read_rr(r, merge_rr);
> return fd;
> }
> @@ -1237,7 +1254,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_SKIP_LOCKED) < 0)
> return;
> > repo_config_get_expiry_in_days(the_repository, "gc.rerereresolved",
> diff --git a/rerere.h b/rerere.h
> index d4b5f7c932..87964bb3c5 100644
> --- a/rerere.h
> +++ b/rerere.h
> @@ -10,6 +10,7 @@ struct repository;
> #define RERERE_AUTOUPDATE 01
> #define RERERE_NOAUTOUPDATE 02
> #define RERERE_READONLY 04
> +#define RERERE_SKIP_LOCKED 010
> > /*
> * Marks paths that have been hand-resolved and added to the
> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
> index 1717f407c8..6b90294435 100755
> --- a/t/t4200-rerere.sh
> +++ b/t/t4200-rerere.sh
> @@ -242,6 +242,46 @@ test_expect_success 'old records rest in peace' '
> test_path_is_missing $rr2/preimage
> '
> > +test_expect_success 'gc does nothing while MERGE_RR is locked' '
> + mkdir -p $rr2 &&
> + echo Hello >$rr2/preimage &&
> + test-tool chmtime =$just_over_15_days_ago $rr2/preimage &&
> +
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + git rerere gc 2>err &&
> + test_grep "MERGE_RR" err &&
> + test_path_is_file $rr2/preimage &&
> +
> + rm .git/MERGE_RR.lock &&
> + git rerere gc &&
> + test_path_is_missing $rr2/preimage
> +'
> +
> +test_expect_success 'a held lock is waited out within rerere.lockTimeout' '
> + git reset --hard &&
> + rm -rf $rr &&
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + {
> + (sleep 1 && rm -f .git/MERGE_RR.lock) &
> + } &&
> + test_must_fail git -c rerere.lockTimeout=5000 merge first 2>err &&
> + wait &&
> + test_grep ! "Unable to create" err &&
> + grep "^=======\$" $rr/preimage
> +'
> +
> +test_expect_success 'rerere.lockTimeout=0 fails at once on a held lock' '
> + git reset --hard &&
> + rm -rf $rr &&
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + test_must_fail git -c rerere.lockTimeout=0 merge first 2>err &&
> + test_grep "Unable to create" err &&
> + test_path_is_missing $rr/preimage
> +'
> +
> rerere_gc_custom_expiry_test () {
> five_days="$1" right_now="$2"
> test_expect_success "rerere gc with custom expiry ($five_days, $right_now)" '
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index d7f82e1bec..a55ca2e829 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -885,6 +885,14 @@ test_expect_success 'rerere-gc task with --auto honors maintenance.rerere-gc.aut
> test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=0 maintenance run --auto --task=rerere-gc
> '
> > +test_expect_success 'rerere-gc task succeeds while MERGE_RR is locked' '
> + test_when_finished "rm -rf .git/rr-cache .git/MERGE_RR.lock" &&
> + mkdir .git/rr-cache &&
> + : >.git/rr-cache/entry &&
> + >.git/MERGE_RR.lock &&
> + test_expect_rerere_gc git maintenance run --task=rerere-gc
> +'
> +
> test_expect_success '--auto and --schedule incompatible' '
> test_must_fail git maintenance run --auto --schedule=daily 2>err &&
> test_grep "cannot be used together" err
> > base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc |
|
User |
|
Thomas Bachem wrote on the Git mailing list (how to reply to this email): Hi Phillip,
On 02/09/2026 15:27, Phillip Wood wrote:
> To me this is another reason why we should disable gc.auto while
> rebasing. To do that we need to pass "-c gc.auto=false -c
> maintenance.auto=false" when running "git commit" in run_git_commit()
> and also when running "git merge" in do_merge(). We should also pass
> those settings via GIT_CONFIG_PARAMETERS when running a exec command in
> do_exec(). That is largly papering over the cracks but until we have a
> systematic solution it does at least stop exposing users to this bug.
OK, I'll do that. It is also more consistent than it looks: the
commits the sequencer creates in-process via try_to_commit() don't run
auto maintenance at all, only the "git commit" child does (for a
resolved, reworded or squashed commit). What surprised me is that a
rebase with the merge backend then never runs maintenance, not even at
the end, because it doesn't go through finish_rebase() where the apply
backend runs it. Do you want a single run at the end of the sequence
in that patch, or keep it minimal?
FWIW, the tool I hit this with has been setting both for its whole
process tree since, and the failures stopped.
>> When it is the rebase, it dies in do_pick_commit()
>
> That's a bug us well - we should be returning errors, not dying
> -rerere_setup() should be returning an error, so we can clean up and
> reschedule the pick.
Yes. I don't think we even need to reschedule: when repo_rerere() is
called there, the merge result is already in the index and worktree,
the error and advice have been printed, and the return value is
ignored. If setup_rerere() reports the lock and returns -1, the pick
just stops at the conflict like any other, minus rerere's recording
and replay, and --continue works. I went through the callers of
setup_rerere(): all of them handle a negative return, because that is
what a disabled rerere returns, so this is close to a one-branch
change. It also fixes the stale-lock case (crashed process), which
disabling gc can't.
> As I understand it we now block the sequencer until the background
> maintenance has completed, or continue to die in an inconvenient state
> we timeout before the background maintenance finishes. That seems rather
> unfortunate as the idea of running the maintenance in the background is
> to prevent it from interfering with other commands.
Right, that's what it does. I copied the timeout from
core.packedRefsTimeout, but a ref update can't be skipped and a rerere
can, so the wait buys little. I'll drop rerere.lockTimeout.
What it did buy: the gc spawned by the continue's own commit needs
~5ms to take the lock, the next pick usually longer to reach its
rerere, so the gc is normally holding it by then. With only the
gc-side skip my repro still died 3 of 3 times; with the error return
those runs would survive but lose rerere at that stop. Tolerable, but
it is why I'd rather have the sequencer patch in the same series than
leave it for later.
> I think my preferred solution is to disable gc while rebasing. Returning
> an error from rerere_setup() would also help in the case where the user
> runs "git commit" and then continues the rebase. I'd be interested to
> hear what Junio and Patrick think about that.
So v2 would be two patches: rerere returning an error on a busy lock
(with "rerere gc" still warning and skipping as in v1, and a commit
message that talks about the fix instead of the trace), and the
sequencer disabling gc.auto/maintenance.auto for "git commit", "git
merge" and exec. I'll wait for Junio and Patrick before rerolling in
case they see it differently.
Patrick, one thing I noticed on the way: since 452b12c2e0
(builtin/maintenance: use "geometric" strategy by default, 2026-02-24)
every "maintenance run --auto" runs rerere-gc as soon as rr-cache has
even a single entry, stale or not. The doc for
maintenance.rerere-gc.auto says the heuristic may be refined; that
would make this rare for every command, not only the sequencer. Not
touching it in this series, just mentioning it.
Thanks,
Tom
Am Mi., 2. Sept. 2026 um 15:27 Uhr schrieb Phillip Wood
<phillip.wood123@gmail.com>:
>
> Hi Thomas
>
> On 02/09/2026 09:31, Thomas Bachem via GitGitGadget wrote:
> > From: Thomas Bachem <mail@thomasbachem.com>
> >
> > Since 2.54 unscheduled maintenance uses the "geometric" strategy, so
>
> That change really is the gift that keeps on giving
>
> > 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".
> >
> > rerere_gc() takes MERGE_RR.lock through setup_rerere(), which uses
> > LOCK_DIE_ON_ERROR, and so does the sequencer's repo_rerere() at the
> > next conflict a few milliseconds later. Whichever comes second dies.
>
> To me this is another reason why we should disable gc.auto while
> rebasing. To do that we need to pass "-c gc.auto=false -c
> maintenance.auto=false" when running "git commit" in run_git_commit()
> and also when running "git merge" in do_merge(). We should also pass
> those settings via GIT_CONFIG_PARAMETERS when running a exec command in
> do_exec(). That is largly papering over the cracks but until we have a
> systematic solution it does at least stop exposing users to this bug.
>
> > When it is the rebase, it dies in do_pick_commit()
>
> That's a bug us well - we should be returning errors, not dying
> -rerere_setup() should be returning an error, so we can clean up and
> reschedule the pick.
>
> There is a lot of detail here about what causes the problem which is
> helpful, but there is very little discussion about the fix. As I
> understand it we now block the sequencer until the background
> maintenance has completed, or continue to die in an inconvenient state
> we timeout before the background maintenance finishes. That seems rather
> unfortunate as the idea of running the maintenance in the background is
> to prevent it from interfering with other commands.
>
> I think my preferred solution is to disable gc while rebasing. Returning
> an error from rerere_setup() would also help in the case where the user
> runs "git commit" and then continues the rebase. I'd be interested to
> hear what Junio and Patrick think about that. I'm also not clear why
> gc.auto has to fork a separate process just to check if it needs to run
> or not, I've not been following closely but my impression is that that
> is the cause of quite a lot of the lock contention bugs we've seen.
>
> Thanks
>
> Phillip
>
> > with the index
> > written but before make_patch() writes rebase-merge/{message,patch,
> > stopped-sha}, and every later "git rebase --continue" refuses with
> > "you have staged changes in your working tree". When it is the "git
> > commit" of a later continue, that one dies in its post-commit
> > repo_rerere() after the commit was made. Before 2.54 the same
> > collision needed an auto gc to actually run, since gc runs
> > "rerere gc" at its end.
> >
> > A rebase with two conflicts in a row shows it. The filler makes the
> > pick slower than the ~5 ms the background task needs to take the
> > lock, and keeps the lock held for about 0.4 s. It hit 6 of 6 runs
> > here on 2.55.0, and a test suite driving rebases on toy repositories
> > with a single rr-cache entry hit it in both runs that were traced:
> >
> > git init -q -b main r && cd r
> > git config rerere.enabled true
> > git config maintenance.auto false
> > mkdir pad && seq 20000 | (cd pad && split -l 1 -a 5)
> > echo base >f && git add -A && git commit -qm base
> > git checkout -q -b topic
> > echo b >f && git commit -qam B
> > echo c >f && git commit -qam C
> > git checkout -q main
> > echo a >f && git commit -qam A
> > git repack -adq
> > seq 20000 | awk '{printf ".git/rr-cache/%040x\n", $1}' \
> > | xargs mkdir -p
> > for d in .git/rr-cache/*/; do echo x >$d/preimage; done
> > git config --unset maintenance.auto
> > git checkout -q topic
> > git rebase main
> > echo ab >f && git add f
> > GIT_EDITOR=true git rebase --continue
> >
> > The second continue dies with "Unable to create '.git/MERGE_RR.lock':
> > File exists" while the gc spawned by its own commit holds the lock,
> > and after resolving C every further continue refuses. Maintenance
> > stays off during the setup so that no repack is pending: a repack due
> > at that commit runs ahead of rerere-gc in the task list and would
> > spend the window.
> >
> > 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.
> >
> > That rebase now completes. The tests cover the gc under a held lock,
> > directly and through the maintenance task, a merge that waits a lock
> > out within a five second rerere.lockTimeout, and one that fails at
> > once with a timeout of 0.
> >
> > Assisted-by: Claude Fable 5.1
> > Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
> > ---
> > rerere: keep a background gc from killing a rebase
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2214%2Fthomasbachem%2Frerere-gc-lock-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2214/thomasbachem/rerere-gc-lock-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/2214
> >
> > Documentation/config/rerere.adoc | 8 +++++++
> > Documentation/git-rerere.adoc | 4 +++-
> > rerere.c | 27 +++++++++++++++++----
> > rerere.h | 1 +
> > t/t4200-rerere.sh | 40 ++++++++++++++++++++++++++++++++
> > t/t7900-maintenance.sh | 8 +++++++
> > 6 files changed, 82 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/config/rerere.adoc b/Documentation/config/rerere.adoc
> > index 3a78b5ebb1..8041a1587b 100644
> > --- a/Documentation/config/rerere.adoc
> > +++ b/Documentation/config/rerere.adoc
> > @@ -10,3 +10,11 @@ rerere.enabled::
> > enabled if there is an `rr-cache` directory under the
> > `$GIT_DIR`, e.g. if "rerere" was previously used in the
> > repository.
> > +
> > +rerere.lockTimeout::
> > + The length of time, in milliseconds, to retry when trying to
> > + take the rerere lock while another process holds it, typically
> > + a background `git rerere gc`. Value 0 means not to retry at
> > + all; -1 means to try indefinitely. Default is 1000 (i.e.,
> > + retry for 1 second). `git rerere gc` itself does not wait and
> > + skips its run instead.
> > diff --git a/Documentation/git-rerere.adoc b/Documentation/git-rerere.adoc
> > index 4e6ab9a27c..05935b0603 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 lock on the
> > +recorded resolutions, for example a merge or rebase that is recording
> > +a conflict, `gc` does nothing and reports so.
> >
> >
> > DISCUSSION
> > 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;
> > + }
> > + } 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);
> > + }
> > read_rr(r, merge_rr);
> > return fd;
> > }
> > @@ -1237,7 +1254,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_SKIP_LOCKED) < 0)
> > return;
> >
> > repo_config_get_expiry_in_days(the_repository, "gc.rerereresolved",
> > diff --git a/rerere.h b/rerere.h
> > index d4b5f7c932..87964bb3c5 100644
> > --- a/rerere.h
> > +++ b/rerere.h
> > @@ -10,6 +10,7 @@ struct repository;
> > #define RERERE_AUTOUPDATE 01
> > #define RERERE_NOAUTOUPDATE 02
> > #define RERERE_READONLY 04
> > +#define RERERE_SKIP_LOCKED 010
> >
> > /*
> > * Marks paths that have been hand-resolved and added to the
> > diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
> > index 1717f407c8..6b90294435 100755
> > --- a/t/t4200-rerere.sh
> > +++ b/t/t4200-rerere.sh
> > @@ -242,6 +242,46 @@ test_expect_success 'old records rest in peace' '
> > test_path_is_missing $rr2/preimage
> > '
> >
> > +test_expect_success 'gc does nothing while MERGE_RR is locked' '
> > + mkdir -p $rr2 &&
> > + echo Hello >$rr2/preimage &&
> > + test-tool chmtime =$just_over_15_days_ago $rr2/preimage &&
> > +
> > + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> > + >.git/MERGE_RR.lock &&
> > + git rerere gc 2>err &&
> > + test_grep "MERGE_RR" err &&
> > + test_path_is_file $rr2/preimage &&
> > +
> > + rm .git/MERGE_RR.lock &&
> > + git rerere gc &&
> > + test_path_is_missing $rr2/preimage
> > +'
> > +
> > +test_expect_success 'a held lock is waited out within rerere.lockTimeout' '
> > + git reset --hard &&
> > + rm -rf $rr &&
> > + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> > + >.git/MERGE_RR.lock &&
> > + {
> > + (sleep 1 && rm -f .git/MERGE_RR.lock) &
> > + } &&
> > + test_must_fail git -c rerere.lockTimeout=5000 merge first 2>err &&
> > + wait &&
> > + test_grep ! "Unable to create" err &&
> > + grep "^=======\$" $rr/preimage
> > +'
> > +
> > +test_expect_success 'rerere.lockTimeout=0 fails at once on a held lock' '
> > + git reset --hard &&
> > + rm -rf $rr &&
> > + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> > + >.git/MERGE_RR.lock &&
> > + test_must_fail git -c rerere.lockTimeout=0 merge first 2>err &&
> > + test_grep "Unable to create" err &&
> > + test_path_is_missing $rr/preimage
> > +'
> > +
> > rerere_gc_custom_expiry_test () {
> > five_days="$1" right_now="$2"
> > test_expect_success "rerere gc with custom expiry ($five_days, $right_now)" '
> > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> > index d7f82e1bec..a55ca2e829 100755
> > --- a/t/t7900-maintenance.sh
> > +++ b/t/t7900-maintenance.sh
> > @@ -885,6 +885,14 @@ test_expect_success 'rerere-gc task with --auto honors maintenance.rerere-gc.aut
> > test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=0 maintenance run --auto --task=rerere-gc
> > '
> >
> > +test_expect_success 'rerere-gc task succeeds while MERGE_RR is locked' '
> > + test_when_finished "rm -rf .git/rr-cache .git/MERGE_RR.lock" &&
> > + mkdir .git/rr-cache &&
> > + : >.git/rr-cache/entry &&
> > + >.git/MERGE_RR.lock &&
> > + test_expect_rerere_gc git maintenance run --task=rerere-gc
> > +'
> > +
> > test_expect_success '--auto and --schedule incompatible' '
> > test_must_fail git maintenance run --auto --schedule=daily 2>err &&
> > test_grep "cannot be used together" err
> >
> > base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
> |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): 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 |
|
Thomas Bachem wrote on the Git mailing list (how to reply to this email): Hi Patrick,
On Thu, Sep 03, 2026 at 09:40:04AM +0200, Patrick Steinhardt wrote:
> 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.
Thanks. Checking whether anything would actually be pruned sounds
right to me. It takes the frequency away, not the race, so I'd still
do the sequencer part Phillip asked for.
> 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.
Phillip found the wait unfortunate and I offered to drop it. You would
keep it. I think the two fit together: wait up to rerere.lockTimeout,
then warn and return -1 instead of dying, so the caller goes on
without rerere this once. The gc passes 0 and does not wait. That
takes the die out, which is what broke the rebase. The wait stays,
bounded to a second, but skipping rerere is not free either: it can
mean resolving a conflict again that rerere had already recorded, and
a second is cheap next to that. With the sequencer no longer spawning
the gc and your heuristic change, it should rarely come to either.
Phillip, would that work for you?
> We should instead pass `LOCK_REPORT_ON_ERROR`, as the lockfile machinery
> knows better why exactly locking has failed.
Agreed on the text, which also names a stale lock. But the callers
that go on without rerere then exit as if it were disabled, "git
commit" with 0, so for them I'd print it as a warning through
unable_to_lock_message() rather than let LOCK_REPORT_ON_ERROR call it
an error. An explicit "git rerere forget" or "clear" fails as before.
> I think we can easily combine those two branches and simply set the
> timeout value to 0 in case we see the flag.
Yes, that folds into one call.
So v2: setup_rerere() waits up to rerere.lockTimeout, 0 for the gc,
then warns and returns -1 where the caller can go on, with the
sequencer patch on top. I'll reroll once Phillip has had a look.
Thanks,
Tom |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Thu, Sep 03, 2026 at 10:11:05AM +0200, Thomas Bachem wrote:
> Hi Patrick,
>
> On Thu, Sep 03, 2026 at 09:40:04AM +0200, Patrick Steinhardt wrote:
> > 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.
>
> Thanks. Checking whether anything would actually be pruned sounds
> right to me. It takes the frequency away, not the race, so I'd still
> do the sequencer part Phillip asked for.
Yes. Ideally, I'd think that we should both introduce the grace period
for locking the file and adapting the heuristic used by the maintenance
strategy. Whether we should completely disable auto-maintenance when in
the sequencer... I dunno. In any case, that feels like another separate
topic that should probably be discussed in its own series.
> > 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.
>
> Phillip found the wait unfortunate and I offered to drop it. You would
> keep it. I think the two fit together: wait up to rerere.lockTimeout,
> then warn and return -1 instead of dying, so the caller goes on
> without rerere this once. The gc passes 0 and does not wait. That
> takes the die out, which is what broke the rebase. The wait stays,
> bounded to a second, but skipping rerere is not free either: it can
> mean resolving a conflict again that rerere had already recorded, and
> a second is cheap next to that. With the sequencer no longer spawning
> the gc and your heuristic change, it should rarely come to either.
> Phillip, would that work for you?
I think that having the wait is a sensible thing to do, as the race was
a preexisting one that was only uncovered by the change to the default
maintenance strategy. It can also happen with two concurrent processes
that both happen to write rerere entries. You wouldn't normally see the
wait anyway, so in the happy path nobody will really care. And in the
cases where you would see it the user is probably more happy to wait a
bit than having Git die (or just not write a rerere entry at all).
Patrick |
|
Thomas Bachem wrote on the Git mailing list (how to reply to this email): Hi Patrick,
On Thu, Sep 03, 2026 at 10:32:36AM +0200, Patrick Steinhardt wrote:
> Yes. Ideally, I'd think that we should both introduce the grace period
> for locking the file and adapting the heuristic used by the maintenance
> strategy. Whether we should completely disable auto-maintenance when in
> the sequencer... I dunno. In any case, that feels like another separate
> topic that should probably be discussed in its own series.
Phillip, this is the part I said I'd do in this series, so I'd
rather answer it here than just drop it. I think Patrick is right
that it's a topic of its own. My reason for wanting it in the same
series was the recording lost at a stop while the gc holds the lock,
and that was for the variant without the wait. With the wait kept,
the next pick waits the gc out and records as before, so the
sequencer patch no longer buys the rebase anything the rerere patch
doesn't, short of a prune that outlasts the timeout.
What it would still decide is whether a rebase with the merge backend
runs maintenance at all, the question from my last mail, and that is
a discussion of its own. So I'd make v2 the rerere patch alone and
send the sequencer change separately if you still want it. Say if
you would rather keep them together.
> I think that having the wait is a sensible thing to do, as the race was
> a preexisting one that was only uncovered by the change to the default
> maintenance strategy. It can also happen with two concurrent processes
> that both happen to write rerere entries. You wouldn't normally see the
> wait anyway, so in the happy path nobody will really care. And in the
> cases where you would see it the user is probably more happy to wait a
> bit than having Git die (or just not write a rerere entry at all).
Agreed, and that is the order v2 keeps: wait first, skip only once
the wait has run out. Since your series means the gc now only runs
when there is something to prune, I measured how long that wait can
get: pruning 20000 stale entries holds the lock for 2.7 s here,
walking 20000 fresh ones takes 0.4 s, so the one second default
covers a prune of roughly 7000 entries if it scales. I'd keep the
default. A backlog that size is a one-off, and where it does hit,
the timeout now skips one recording where it used to kill the
rebase.
My patch is based on maint since the bug is there, and I'd keep it
that way unless Junio would rather have it on master. Merged up it
conflicts with d43f701d32 (lockfile: add
repo_hold_lock_file_for_update{,_timeout}{,_mode}(), 2026-07-14) in
setup_rerere(). The resolution is to take the repo-scoped helper, and
with that t4200 and t7900 pass on top of your series. I'll wait a
day or two for Phillip before rerolling.
Thanks,
Tom |
|
Phillip Wood wrote on the Git mailing list (how to reply to this email): Hi Patrick and Thomas
On 03/09/2026 09:32, Patrick Steinhardt wrote:
> On Thu, Sep 03, 2026 at 10:11:05AM +0200, Thomas Bachem wrote:
>> Hi Patrick,
>>
>> On Thu, Sep 03, 2026 at 09:40:04AM +0200, Patrick Steinhardt wrote:
>>> 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.
>>
>> Thanks. Checking whether anything would actually be pruned sounds
>> right to me. It takes the frequency away, not the race, so I'd still
>> do the sequencer part Phillip asked for.
> > Yes. Ideally, I'd think that we should both introduce the grace period
> for locking the file and adapting the heuristic used by the maintenance
> strategy. I agree
> Whether we should completely disable auto-maintenance when in
> the sequencer... I dunno. In any case, that feels like another separate
> topic that should probably be discussed in its own series.
We've seen other bugs reported related to auto-maintenance triggered during a rebase such as the one dscho fixed recently. While I can see repacking might be helpful during a very large rebase, I do not think garbage collection is useful - all the objects and rerere entries that are created during the rebase are going to be too fresh to be collected. So I think it would be a good idea to disable auto maintenance in a rebase and see if anyone complains. If it turns out to be a problem we can figure out how to make it repack incrementally.
>>> 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.
>>
>> Phillip found the wait unfortunate and I offered to drop it. You would
>> keep it. I think the two fit together: wait up to rerere.lockTimeout,
>> then warn and return -1 instead of dying, so the caller goes on
>> without rerere this once. The gc passes 0 and does not wait. That
>> takes the die out, which is what broke the rebase. The wait stays,
>> bounded to a second, but skipping rerere is not free either: it can
>> mean resolving a conflict again that rerere had already recorded, and
>> a second is cheap next to that. With the sequencer no longer spawning
>> the gc and your heuristic change, it should rarely come to either.
>> Phillip, would that work for you?
> > I think that having the wait is a sensible thing to do, as the race was
> a preexisting one that was only uncovered by the change to the default
> maintenance strategy. It can also happen with two concurrent processes
> that both happen to write rerere entries. You wouldn't normally see the
> wait anyway, so in the happy path nobody will really care. And in the
> cases where you would see it the user is probably more happy to wait a
> bit than having Git die (or just not write a rerere entry at all).
I don't object to the timeout as part of the solution. My objection was based on it being the only solution as it is inconvenient to the user if they have to wait for background maintenance jobs and it does not stop the rebase from failing if the timeout is too short.
Thanks
Phillip |
|
Phillip Wood wrote on the Git mailing list (how to reply to this email): Hi Thomas
On 02/09/2026 16:07, Thomas Bachem wrote:
> On 02/09/2026 15:27, Phillip Wood wrote:
>> To me this is another reason why we should disable gc.auto while
>> rebasing. To do that we need to pass "-c gc.auto=false -c
>> maintenance.auto=false" when running "git commit" in run_git_commit()
>> and also when running "git merge" in do_merge(). We should also pass
>> those settings via GIT_CONFIG_PARAMETERS when running a exec command in
>> do_exec(). That is largly papering over the cracks but until we have a
>> systematic solution it does at least stop exposing users to this bug.
> > OK, I'll do that. It is also more consistent than it looks: the
> commits the sequencer creates in-process via try_to_commit() don't run
> auto maintenance at all, only the "git commit" child does (for a
> resolved, reworded or squashed commit). What surprised me is that a
> rebase with the merge backend then never runs maintenance, not even at
> the end, because it doesn't go through finish_rebase() where the apply
> backend runs it. Do you want a single run at the end of the sequence
> in that patch, or keep it minimal?
We should be consistent between the backends, so yes we should be calling run_auto_maintenance() at the end of a rebase with the merge backend.
> FWIW, the tool I hit this with has been setting both for its whole
> process tree since, and the failures stopped.
> >>> When it is the rebase, it dies in do_pick_commit()
>>
>> That's a bug us well - we should be returning errors, not dying
>> -rerere_setup() should be returning an error, so we can clean up and
>> reschedule the pick.
> > Yes. I don't think we even need to reschedule: when repo_rerere() is
> called there, the merge result is already in the index and worktree,
> the error and advice have been printed, and the return value is
> ignored. If setup_rerere() reports the lock and returns -1, the pick
> just stops at the conflict like any other, minus rerere's recording
> and replay, and --continue works. Oh good point, if we get an error then we'll write the files to get "git rebase --continue" to commit the conflict resolution so we don't need to reschedule.
Thanks
Phillip
>> I think my preferred solution is to disable gc while rebasing. Returning
>> an error from rerere_setup() would also help in the case where the user
>> runs "git commit" and then continues the rebase. I'd be interested to
>> hear what Junio and Patrick think about that.
> > So v2 would be two patches: rerere returning an error on a busy lock
> (with "rerere gc" still warning and skipping as in v1, and a commit
> message that talks about the fix instead of the trace), and the
> sequencer disabling gc.auto/maintenance.auto for "git commit", "git
> merge" and exec. I'll wait for Junio and Patrick before rerolling in
> case they see it differently.
> > Patrick, one thing I noticed on the way: since 452b12c2e0
> (builtin/maintenance: use "geometric" strategy by default, 2026-02-24)
> every "maintenance run --auto" runs rerere-gc as soon as rr-cache has
> even a single entry, stale or not. The doc for
> maintenance.rerere-gc.auto says the heuristic may be refined; that
> would make this rare for every command, not only the sequencer. Not
> touching it in this series, just mentioning it.
> > Thanks,
> Tom
> > > Am Mi., 2. Sept. 2026 um 15:27 Uhr schrieb Phillip Wood
> <phillip.wood123@gmail.com>:
>>
>> Hi Thomas
>>
>> On 02/09/2026 09:31, Thomas Bachem via GitGitGadget wrote:
>>> From: Thomas Bachem <mail@thomasbachem.com>
>>>
>>> Since 2.54 unscheduled maintenance uses the "geometric" strategy, so
>>
>> That change really is the gift that keeps on giving
>>
>>> 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".
>>>
>>> rerere_gc() takes MERGE_RR.lock through setup_rerere(), which uses
>>> LOCK_DIE_ON_ERROR, and so does the sequencer's repo_rerere() at the
>>> next conflict a few milliseconds later. Whichever comes second dies.
>>
>> To me this is another reason why we should disable gc.auto while
>> rebasing. To do that we need to pass "-c gc.auto=false -c
>> maintenance.auto=false" when running "git commit" in run_git_commit()
>> and also when running "git merge" in do_merge(). We should also pass
>> those settings via GIT_CONFIG_PARAMETERS when running a exec command in
>> do_exec(). That is largly papering over the cracks but until we have a
>> systematic solution it does at least stop exposing users to this bug.
>>
>>> When it is the rebase, it dies in do_pick_commit()
>>
>> That's a bug us well - we should be returning errors, not dying
>> -rerere_setup() should be returning an error, so we can clean up and
>> reschedule the pick.
>>
>> There is a lot of detail here about what causes the problem which is
>> helpful, but there is very little discussion about the fix. As I
>> understand it we now block the sequencer until the background
>> maintenance has completed, or continue to die in an inconvenient state
>> we timeout before the background maintenance finishes. That seems rather
>> unfortunate as the idea of running the maintenance in the background is
>> to prevent it from interfering with other commands.
>>
>> I think my preferred solution is to disable gc while rebasing. Returning
>> an error from rerere_setup() would also help in the case where the user
>> runs "git commit" and then continues the rebase. I'd be interested to
>> hear what Junio and Patrick think about that. I'm also not clear why
>> gc.auto has to fork a separate process just to check if it needs to run
>> or not, I've not been following closely but my impression is that that
>> is the cause of quite a lot of the lock contention bugs we've seen.
>>
>> Thanks
>>
>> Phillip
>>
>>> with the index
>>> written but before make_patch() writes rebase-merge/{message,patch,
>>> stopped-sha}, and every later "git rebase --continue" refuses with
>>> "you have staged changes in your working tree". When it is the "git
>>> commit" of a later continue, that one dies in its post-commit
>>> repo_rerere() after the commit was made. Before 2.54 the same
>>> collision needed an auto gc to actually run, since gc runs
>>> "rerere gc" at its end.
>>>
>>> A rebase with two conflicts in a row shows it. The filler makes the
>>> pick slower than the ~5 ms the background task needs to take the
>>> lock, and keeps the lock held for about 0.4 s. It hit 6 of 6 runs
>>> here on 2.55.0, and a test suite driving rebases on toy repositories
>>> with a single rr-cache entry hit it in both runs that were traced:
>>>
>>> git init -q -b main r && cd r
>>> git config rerere.enabled true
>>> git config maintenance.auto false
>>> mkdir pad && seq 20000 | (cd pad && split -l 1 -a 5)
>>> echo base >f && git add -A && git commit -qm base
>>> git checkout -q -b topic
>>> echo b >f && git commit -qam B
>>> echo c >f && git commit -qam C
>>> git checkout -q main
>>> echo a >f && git commit -qam A
>>> git repack -adq
>>> seq 20000 | awk '{printf ".git/rr-cache/%040x\n", $1}' \
>>> | xargs mkdir -p
>>> for d in .git/rr-cache/*/; do echo x >$d/preimage; done
>>> git config --unset maintenance.auto
>>> git checkout -q topic
>>> git rebase main
>>> echo ab >f && git add f
>>> GIT_EDITOR=true git rebase --continue
>>>
>>> The second continue dies with "Unable to create '.git/MERGE_RR.lock':
>>> File exists" while the gc spawned by its own commit holds the lock,
>>> and after resolving C every further continue refuses. Maintenance
>>> stays off during the setup so that no repack is pending: a repack due
>>> at that commit runs ahead of rerere-gc in the task list and would
>>> spend the window.
>>>
>>> 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.
>>>
>>> That rebase now completes. The tests cover the gc under a held lock,
>>> directly and through the maintenance task, a merge that waits a lock
>>> out within a five second rerere.lockTimeout, and one that fails at
>>> once with a timeout of 0.
>>>
>>> Assisted-by: Claude Fable 5.1
>>> Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
>>> ---
>>> rerere: keep a background gc from killing a rebase
>>>
>>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2214%2Fthomasbachem%2Frerere-gc-lock-v1
>>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2214/thomasbachem/rerere-gc-lock-v1
>>> Pull-Request: https://github.com/gitgitgadget/git/pull/2214
>>>
>>> Documentation/config/rerere.adoc | 8 +++++++
>>> Documentation/git-rerere.adoc | 4 +++-
>>> rerere.c | 27 +++++++++++++++++----
>>> rerere.h | 1 +
>>> t/t4200-rerere.sh | 40 ++++++++++++++++++++++++++++++++
>>> t/t7900-maintenance.sh | 8 +++++++
>>> 6 files changed, 82 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/config/rerere.adoc b/Documentation/config/rerere.adoc
>>> index 3a78b5ebb1..8041a1587b 100644
>>> --- a/Documentation/config/rerere.adoc
>>> +++ b/Documentation/config/rerere.adoc
>>> @@ -10,3 +10,11 @@ rerere.enabled::
>>> enabled if there is an `rr-cache` directory under the
>>> `$GIT_DIR`, e.g. if "rerere" was previously used in the
>>> repository.
>>> +
>>> +rerere.lockTimeout::
>>> + The length of time, in milliseconds, to retry when trying to
>>> + take the rerere lock while another process holds it, typically
>>> + a background `git rerere gc`. Value 0 means not to retry at
>>> + all; -1 means to try indefinitely. Default is 1000 (i.e.,
>>> + retry for 1 second). `git rerere gc` itself does not wait and
>>> + skips its run instead.
>>> diff --git a/Documentation/git-rerere.adoc b/Documentation/git-rerere.adoc
>>> index 4e6ab9a27c..05935b0603 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 lock on the
>>> +recorded resolutions, for example a merge or rebase that is recording
>>> +a conflict, `gc` does nothing and reports so.
>>>
>>>
>>> DISCUSSION
>>> 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;
>>> + }
>>> + } 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);
>>> + }
>>> read_rr(r, merge_rr);
>>> return fd;
>>> }
>>> @@ -1237,7 +1254,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_SKIP_LOCKED) < 0)
>>> return;
>>>
>>> repo_config_get_expiry_in_days(the_repository, "gc.rerereresolved",
>>> diff --git a/rerere.h b/rerere.h
>>> index d4b5f7c932..87964bb3c5 100644
>>> --- a/rerere.h
>>> +++ b/rerere.h
>>> @@ -10,6 +10,7 @@ struct repository;
>>> #define RERERE_AUTOUPDATE 01
>>> #define RERERE_NOAUTOUPDATE 02
>>> #define RERERE_READONLY 04
>>> +#define RERERE_SKIP_LOCKED 010
>>>
>>> /*
>>> * Marks paths that have been hand-resolved and added to the
>>> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
>>> index 1717f407c8..6b90294435 100755
>>> --- a/t/t4200-rerere.sh
>>> +++ b/t/t4200-rerere.sh
>>> @@ -242,6 +242,46 @@ test_expect_success 'old records rest in peace' '
>>> test_path_is_missing $rr2/preimage
>>> '
>>>
>>> +test_expect_success 'gc does nothing while MERGE_RR is locked' '
>>> + mkdir -p $rr2 &&
>>> + echo Hello >$rr2/preimage &&
>>> + test-tool chmtime =$just_over_15_days_ago $rr2/preimage &&
>>> +
>>> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
>>> + >.git/MERGE_RR.lock &&
>>> + git rerere gc 2>err &&
>>> + test_grep "MERGE_RR" err &&
>>> + test_path_is_file $rr2/preimage &&
>>> +
>>> + rm .git/MERGE_RR.lock &&
>>> + git rerere gc &&
>>> + test_path_is_missing $rr2/preimage
>>> +'
>>> +
>>> +test_expect_success 'a held lock is waited out within rerere.lockTimeout' '
>>> + git reset --hard &&
>>> + rm -rf $rr &&
>>> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
>>> + >.git/MERGE_RR.lock &&
>>> + {
>>> + (sleep 1 && rm -f .git/MERGE_RR.lock) &
>>> + } &&
>>> + test_must_fail git -c rerere.lockTimeout=5000 merge first 2>err &&
>>> + wait &&
>>> + test_grep ! "Unable to create" err &&
>>> + grep "^=======\$" $rr/preimage
>>> +'
>>> +
>>> +test_expect_success 'rerere.lockTimeout=0 fails at once on a held lock' '
>>> + git reset --hard &&
>>> + rm -rf $rr &&
>>> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
>>> + >.git/MERGE_RR.lock &&
>>> + test_must_fail git -c rerere.lockTimeout=0 merge first 2>err &&
>>> + test_grep "Unable to create" err &&
>>> + test_path_is_missing $rr/preimage
>>> +'
>>> +
>>> rerere_gc_custom_expiry_test () {
>>> five_days="$1" right_now="$2"
>>> test_expect_success "rerere gc with custom expiry ($five_days, $right_now)" '
>>> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
>>> index d7f82e1bec..a55ca2e829 100755
>>> --- a/t/t7900-maintenance.sh
>>> +++ b/t/t7900-maintenance.sh
>>> @@ -885,6 +885,14 @@ test_expect_success 'rerere-gc task with --auto honors maintenance.rerere-gc.aut
>>> test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=0 maintenance run --auto --task=rerere-gc
>>> '
>>>
>>> +test_expect_success 'rerere-gc task succeeds while MERGE_RR is locked' '
>>> + test_when_finished "rm -rf .git/rr-cache .git/MERGE_RR.lock" &&
>>> + mkdir .git/rr-cache &&
>>> + : >.git/rr-cache/entry &&
>>> + >.git/MERGE_RR.lock &&
>>> + test_expect_rerere_gc git maintenance run --task=rerere-gc
>>> +'
>>> +
>>> test_expect_success '--auto and --schedule incompatible' '
>>> test_must_fail git maintenance run --auto --schedule=daily 2>err &&
>>> test_grep "cannot be used together" err
>>>
>>> base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
>> |
thomasbachem
force-pushed
the
rerere-gc-lock
branch
from
September 4, 2026 07:39
ecd9e5b to
bbb2338
Compare
Author
|
/preview |
|
Preview email sent as pull.2214.v2.git.1788507662888.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2214.v2.git.1788507876543.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
thomasbachem
force-pushed
the
rerere-gc-lock
branch
from
September 4, 2026 15:47
bbb2338 to
5bfda65
Compare
Author
|
/preview |
|
Preview email sent as pull.2214.v3.git.1788536934397.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2214.v3.git.1788537081930.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Phillip Wood wrote on the Git mailing list (how to reply to this email): Hi Thomas
On 04/09/2026 08:44, 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.
With Patricks patches that's no-longer true I think. I think a better motivation, as the cache is per-repository, rather than per-worktree, is concurrent writers running in different worktrees. That makes the timeout much more sensible as we expect writing a conflict resolution to be much faster than gc.
Overall, this commit message is rather long and it would be helpful if you could distill it to remove unnecessary and unrelated details.
> > Documentation/config/rerere.adoc | 10 ++++
> Documentation/git-rerere.adoc | 4 +-
> builtin/am.c | 2 +-
> builtin/rebase.c | 6 +-
> builtin/rerere.c | 7 ++-
> rerere.c | 42 ++++++++++----
> rerere.h | 8 ++-
> t/t4200-rerere.sh | 96 ++++++++++++++++++++++++++++++++
> t/t7900-maintenance.sh | 8 +++
> 9 files changed, 163 insertions(+), 20 deletions(-)
> > diff --git a/Documentation/config/rerere.adoc b/Documentation/config/rerere.adoc
> index 3a78b5ebb1..b67323fc46 100644
> --- a/Documentation/config/rerere.adoc
> +++ b/Documentation/config/rerere.adoc
> @@ -10,3 +10,13 @@ rerere.enabled::
> enabled if there is an `rr-cache` directory under the
> `$GIT_DIR`, e.g. if "rerere" was previously used in the
> repository.
> +
> +rerere.lockTimeout::
> + The length of time, in milliseconds, to retry when trying to
> + take the rerere lock while another process holds it, typically
> + a background `git rerere gc`. When the time is up, the command
> + warns and goes on without rerere. Value 0 means not to retry
> + at all; -1 means to try indefinitely. Default is 1000 (i.e.,
> + retry for 1 second). `git rerere gc` does not retry, and
> + `git rerere`, `git rerere forget` and `git rerere clear` fail
> + instead of going on.
Why do those commands fail rather than wait?
> @@ -908,12 +911,31 @@ 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
> - fd = hold_lock_file_for_update(&write_lock,
> - git_path_merge_rr(r),
> - LOCK_DIE_ON_ERROR);
> + } else {
> + int lock_flags = 0;
> + long timeout_ms = rerere_lock_timeout_ms;
> +
> + if (flags & RERERE_LOCK_OR_DIE)
> + lock_flags = LOCK_DIE_ON_ERROR;
> + if (flags & RERERE_NOWAIT)
> + timeout_ms = 0;
It might be worth adding a check above here that BUG()s out if the caller passes an incompatible set of flags.
> + /*
> + * A background "rerere gc" holds the lock for as long as it
> + * takes to prune rr-cache, so wait it out rather than fail
> + * at once. The gc itself has nothing to lose from a skipped
> + * run and never waits.
> + */
> + fd = hold_lock_file_for_update_timeout(&write_lock,
> + git_path_merge_rr(r),
> + lock_flags, timeout_ms);
> + if (fd < 0) {
> + warning_errno(_("skipping rerere, unable to create '%s.lock'"),
> + git_path_merge_rr(r));
A background job that the user did not explicitly start printing to the terminal is rather confusing as it is likely to get mixed in with the output of whatever is running in the foreground.
Thanks
Phillip
> + return -1;
> + }
> + }
> read_rr(r, merge_rr);
> return fd;
> }
> @@ -1124,7 +1146,7 @@ fail_exit:
> return -1;
> }
> > -int rerere_forget(struct repository *r, struct pathspec *pathspec)
> +int rerere_forget(struct repository *r, struct pathspec *pathspec, int flags)
> {
> int i, fd, ret;
> struct string_list conflict = STRING_LIST_INIT_DUP;
> @@ -1133,7 +1155,7 @@ int rerere_forget(struct repository *r, struct pathspec *pathspec)
> if (repo_read_index(r) < 0)
> return error(_("index file corrupt"));
> > - fd = setup_rerere(r, &merge_rr, RERERE_NOAUTOUPDATE);
> + fd = setup_rerere(r, &merge_rr, RERERE_NOAUTOUPDATE | flags);
> if (fd < 0)
> return 0;
> > @@ -1237,7 +1259,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",
> @@ -1289,11 +1311,11 @@ void rerere_gc(struct repository *r, struct string_list *rr)
> *
> * NEEDSWORK: shouldn't we be calling this from "reset --hard"?
> */
> -void rerere_clear(struct repository *r, struct string_list *merge_rr)
> +void rerere_clear(struct repository *r, struct string_list *merge_rr, int flags)
> {
> int i;
> > - if (setup_rerere(r, merge_rr, 0) < 0)
> + if (setup_rerere(r, merge_rr, flags) < 0)
> return;
> > for (i = 0; i < merge_rr->nr; i++) {
> diff --git a/rerere.h b/rerere.h
> index d4b5f7c932..3a9f58acd9 100644
> --- a/rerere.h
> +++ b/rerere.h
> @@ -10,6 +10,10 @@ struct repository;
> #define RERERE_AUTOUPDATE 01
> #define RERERE_NOAUTOUPDATE 02
> #define RERERE_READONLY 04
> +/* Do not wait for the lock when another process holds it */
> +#define RERERE_NOWAIT 010
> +/* Die on a lock that cannot be taken instead of going on without rerere */
> +#define RERERE_LOCK_OR_DIE 020
> > /*
> * Marks paths that have been hand-resolved and added to the
> @@ -34,9 +38,9 @@ int repo_rerere(struct repository *, int);
> */
> const char *rerere_path(struct strbuf *buf, const struct rerere_id *,
> const char *file);
> -int rerere_forget(struct repository *, struct pathspec *);
> +int rerere_forget(struct repository *, struct pathspec *, int);
> int rerere_remaining(struct repository *, struct string_list *);
> -void rerere_clear(struct repository *, struct string_list *);
> +void rerere_clear(struct repository *, struct string_list *, int);
> void rerere_gc(struct repository *, struct string_list *);
> > #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
> index 1717f407c8..243b3ebed3 100755
> --- a/t/t4200-rerere.sh
> +++ b/t/t4200-rerere.sh
> @@ -242,6 +242,102 @@ test_expect_success 'old records rest in peace' '
> test_path_is_missing $rr2/preimage
> '
> > +test_expect_success 'gc does nothing while MERGE_RR is locked' '
> + mkdir -p $rr2 &&
> + echo Hello >$rr2/preimage &&
> + test-tool chmtime =$just_over_15_days_ago $rr2/preimage &&
> +
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + git rerere gc 2>err &&
> + test_grep "MERGE_RR.lock" err &&
> + test_path_is_file $rr2/preimage &&
> +
> + rm .git/MERGE_RR.lock &&
> + git rerere gc &&
> + test_path_is_missing $rr2/preimage
> +'
> +
> +test_expect_success 'a held lock is waited out within rerere.lockTimeout' '
> + git reset --hard &&
> + rm -rf $rr &&
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + {
> + ( sleep 1 && rm -f .git/MERGE_RR.lock ) &
> + } &&
> + test_must_fail git -c rerere.lockTimeout=5000 merge first 2>err &&
> + wait &&
> + test_grep ! "MERGE_RR" err &&
> + test_grep "^=======\$" $rr/preimage
> +'
> +
> +test_expect_success 'merge goes on without rerere once rerere.lockTimeout is up' '
> + git reset --hard &&
> + rm -rf $rr &&
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + test_must_fail git -c rerere.lockTimeout=0 merge first 2>err &&
> + test_grep "skipping rerere" err &&
> + test_grep "^=======\$" a1 &&
> + test_path_is_missing $rr/preimage
> +'
> +
> +test_expect_success 'commit goes on without rerere once rerere.lockTimeout is up' '
> + git reset --hard &&
> + rm -rf $rr &&
> + git checkout -b lock-held-commit third &&
> + test_when_finished "git checkout third && git branch -D lock-held-commit" &&
> + test_must_fail git merge first &&
> + test_path_is_file $rr/preimage &&
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + echo resolved >a1 &&
> + git add a1 &&
> + git -c rerere.lockTimeout=0 commit -qm resolved 2>err &&
> + test_grep "skipping rerere" err &&
> + test_path_is_missing $rr/postimage
> +'
> +
> +test_expect_success 'rerere, forget and clear fail on a lock they cannot take' '
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + test_must_fail git -c rerere.lockTimeout=0 rerere 2>err &&
> + test_grep "Unable to create" err &&
> + test_must_fail git -c rerere.lockTimeout=0 rerere forget a1 2>err &&
> + test_grep "Unable to create" err &&
> + test_must_fail git -c rerere.lockTimeout=0 rerere clear 2>err &&
> + test_grep "Unable to create" err
> +'
> +
> +test_expect_success 'rebase goes on without rerere once rerere.lockTimeout is up' '
> + git reset --hard &&
> + rm -rf $rr &&
> + git checkout -b lock-held third &&
> + test_when_finished "git checkout third && git branch -D lock-held" &&
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + test_must_fail git -c rerere.lockTimeout=0 rebase first 2>err &&
> + test_grep "skipping rerere" err &&
> + test_path_is_file .git/rebase-merge/stopped-sha &&
> + echo resolved >a1 &&
> + git add a1 &&
> + git -c rerere.lockTimeout=0 rebase --continue &&
> + test_path_is_missing .git/rebase-merge &&
> + test_path_is_missing $rr/preimage
> +'
> +
> +test_expect_success 'rebase --abort goes on without rerere on a held lock' '
> + git checkout -b lock-held-abort third &&
> + test_when_finished "git checkout third && git branch -D lock-held-abort" &&
> + test_must_fail git rebase first &&
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + git -c rerere.lockTimeout=0 rebase --abort 2>err &&
> + test_grep "skipping rerere" err &&
> + test_path_is_missing .git/rebase-merge
> +'
> +
> rerere_gc_custom_expiry_test () {
> five_days="$1" right_now="$2"
> test_expect_success "rerere gc with custom expiry ($five_days, $right_now)" '
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index d7f82e1bec..a55ca2e829 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -885,6 +885,14 @@ test_expect_success 'rerere-gc task with --auto honors maintenance.rerere-gc.aut
> test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=0 maintenance run --auto --task=rerere-gc
> '
> > +test_expect_success 'rerere-gc task succeeds while MERGE_RR is locked' '
> + test_when_finished "rm -rf .git/rr-cache .git/MERGE_RR.lock" &&
> + mkdir .git/rr-cache &&
> + : >.git/rr-cache/entry &&
> + >.git/MERGE_RR.lock &&
> + test_expect_rerere_gc git maintenance run --task=rerere-gc
> +'
> +
> test_expect_success '--auto and --schedule incompatible' '
> test_must_fail git maintenance run --auto --schedule=daily 2>err &&
> test_grep "cannot be used together" err
> > base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc |
|
User |
|
Thomas Bachem wrote on the Git mailing list (how to reply to this email): Hi Phillip,
On 04/09/2026 16:21, Phillip Wood wrote:
> With Patricks patches that's no-longer true I think. I think a better
> motivation, as the cache is per-repository, rather than per-worktree, is
> concurrent writers running in different worktrees.
MERGE_RR is per worktree, though, and so is its lock:
$ git -C linked rev-parse --git-path MERGE_RR
/path/to/main/.git/worktrees/linked/MERGE_RR
so writers in different worktrees never meet on it. What they share is
rr-cache, which a gc in one worktree prunes under its own worktree's
lock only. That is a gap of its own, and not one this patch closes.
What remains after Patrick's series is any "git rerere gc" that runs
while a command records a conflict, from "git gc", from a maintenance
run, or from auto maintenance once enough entries are stale. The v3
message says it that way.
> Overall, this commit message is rather long and it would be helpful if
> you could distill it to remove unnecessary and unrelated details.
Done, it is a quarter of the size now.
> Why do those commands fail rather than wait?
They wait like everything else, and once the time is up they fail
instead of going on without rerere, which is all they are for. That
way a stale lock gets the usual advice to remove it. The config text
said otherwise, fixed.
> It might be worth adding a check above here that BUG()s out if the
> caller passes an incompatible set of flags.
Added, for RERERE_NOWAIT with RERERE_LOCK_OR_DIE and for
RERERE_READONLY with either.
> A background job that the user did not explicitly start printing to the
> terminal is rather confusing as it is likely to get mixed in with the
> output of whatever is running in the foreground.
The detached maintenance run has no terminal: daemonize() closes the
standard descriptors and reopens them on /dev/null, so the gc's
warning goes nowhere when it loses the lock. Where it cannot detach,
on Windows, it runs in the foreground of the commit that started it
and there is no race to lose. The warning the user does see is the
foreground command's own, when it gives up waiting.
Thanks,
Thomas |
|
Submitted as pull.2214.v7.git.1791627204.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series is no longer integrated into seen. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -10,3 +10,11 @@ rerere.enabled:: | |||
| enabled if there is an `rr-cache` directory under the | |||
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/Documentation/config/rerere.adoc b/Documentation/config/rerere.adoc
> index 3a78b5ebb1..30e827f32b 100644
> --- a/Documentation/config/rerere.adoc
> +++ b/Documentation/config/rerere.adoc
> @@ -10,3 +10,11 @@ rerere.enabled::
> enabled if there is an `rr-cache` directory under the
> `$GIT_DIR`, e.g. if "rerere" was previously used in the
> repository.
> +
> +rerere.lockTimeout::
$ git grep -A4 -i '^[^ ]*timeout[a-z]*:' Documentation/
tells me that all "timeout" configuration variables are measured in
milliseconds, which justifies the choice of milliseconds as the unit
for this new variable too.
Side note. Only one of the other ones has unit in its name
(i.e., credentialStore.lockTimeoutMS). We probably want to give
it a synonym without MS suffix to make everything uniform.
#leftoverbits
> + The length of time, in milliseconds, to wait for the rerere
> + lock when another process holds it, typically a background
> + `git rerere gc`. Value 0 means not to wait at all; -1 means
> + to wait indefinitely. Default is 1000 (i.e., wait for 1
> + second).
OK.
> + When the time is up, the command fails as it does
> + for any other lock it cannot take.
Is it necessary to say this? If we invent a new lock on 'foo' whose
behavior is to wait for N milliseconds and then proceed anyway,
ignoring the lock after the timer expires, we should name such a
setting differently from a simple 'fooLockTimeout'. This would make
it easier for users to tell the difference, perhaps using
'fooLockBreakTimeout' or something similar.
In any case, we should ensure that we do not have to single out
'rerere.lockTimeout' and describe what happens after the timer
expires. The timeout behavior on locks should be consistent. That
may be slightly outside the scope of this topic, but since none of
the configuration variables whose names end with 'timeout' say the
above, leaving it out of this would be a good first step. We can
leave a '#leftoverbits' task to describe the overall rule for
timeout settings for locks (i.e., "if you still cannot take the lock
after the timeout expires, you will give up and fail") in some
central place to make it clear that the same rule applies to
everyone.
> @@ -882,12 +887,19 @@ 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
> - fd = repo_hold_lock_file_for_update(r, &write_lock,
> - git_path_merge_rr(r),
> - LOCK_DIE_ON_ERROR);
> + } else {
> + /*
> + * Another process may hold the lock for a while, e.g.
> + * "git rerere gc" while it prunes rr-cache, so wait for
> + * it instead of dying right away.
> + */
> + fd = repo_hold_lock_file_for_update_timeout(r, &write_lock,
> + git_path_merge_rr(r),
> + LOCK_DIE_ON_ERROR,
> + rerere_lock_timeout_ms);
> + }
> read_rr(r, merge_rr);
> return fd;
> }
OK. Very straight-forward.
> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
> index 7bb601e117..7bd92235dc 100755
> --- a/t/t4200-rerere.sh
> +++ b/t/t4200-rerere.sh
> @@ -242,6 +242,59 @@ test_expect_success 'old records rest in peace' '
> test_path_is_missing $rr2/preimage
> '
>
> +test_expect_success 'a held lock is waited out within rerere.lockTimeout' '
> + git reset --hard &&
> + rm -rf $rr &&
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + {
> + ( sleep 1 && rm -f .git/MERGE_RR.lock ) &
> + } &&
> + test_must_fail git -c rerere.lockTimeout=5000 merge first 2>err &&
The backgrounded unlocker sleeps for a second. Is the idea that,
even in a heavily loaded CI environment, the backgrounded unlocker
will have a sufficient chance to sleep for a second and unlock while
the 5000-millisecond timeout waits for it?
> + wait &&
Is the idea behind this 'wait' that the 'rerere.locktimeout'
implementation might break in the future and we could reach this
point before the background unlocker has finished sleeping for a
full second? And we want to ensure it has exited before proceeding
by waiting for it ourselves. If that is the case, perhaps a comment
is warranted after '&&', such as:
wait && # just in case the background unlocker is still active
or something similar.
> + test_grep ! "MERGE_RR" err &&
It is a bit unclear what error message this is looking for.
repo_hold_lock_file_for_update_timeout() is fed the path to
MERGE_RR, and eventually calls unable_to_lock_message() to format
the error message, which starts with "Unable to create '...'" to
state the path. Is the idea that this message will contain MERGE_RR
as part of that path and we will catch it if we failed to acquire
the lock?
This deserves a short comment to clarify that we are looking for the
lack of "unable to lock" comment, if that is indeed what is
happening. Or make the string a bit more specific, such as:
test_grep ! "Unable to create.*MERGE_RR\.lock" err &&
or something along those line.
Should we ensure that there is no leftover pid file by removing it
before creating .git/MERGE_RR.lock, by the way?
> + test_grep "^=======\$" $rr/preimage
The merge still has to fail (which is ensured by test_must_fail in
the earlier step) and leave the preimage of the conflicted state,
which makes sense.
I'll stop here, but you can grasp the principles used in reviewing
this test and apply them to the remaining tests to ensure they are
clearly written.
Thanks.
> +
> +test_expect_success 'merge fails once rerere.lockTimeout is up' '
> + git reset --hard &&
> + rm -rf $rr &&
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + test_must_fail git -c rerere.lockTimeout=0 merge first 2>err &&
> + test_grep "Unable to create" err &&
> + test_grep "^=======\$" a1 &&
> + test_path_is_missing $rr/preimage
> +'
> +
> +test_expect_success 'rerere, forget, clear and gc fail on a lock they cannot take' '
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + test_must_fail git -c rerere.lockTimeout=0 rerere 2>err &&
> + test_grep "Unable to create" err &&
> + test_must_fail git -c rerere.lockTimeout=0 rerere forget a1 2>err &&
> + test_grep "Unable to create" err &&
> + test_must_fail git -c rerere.lockTimeout=0 rerere clear 2>err &&
> + test_grep "Unable to create" err &&
> + test_must_fail git -c rerere.lockTimeout=0 rerere gc 2>err &&
> + test_grep "Unable to create" err
> +'
> +
> +test_expect_success 'rebase --abort fails on a lock it cannot take' '
> + git reset --hard &&
> + git checkout -b lock-held-abort third &&
> + test_when_finished "git checkout third && git branch -D lock-held-abort" &&
> + test_must_fail git rebase first &&
> + test_when_finished "rm -f .git/MERGE_RR.lock" &&
> + >.git/MERGE_RR.lock &&
> + test_must_fail git -c rerere.lockTimeout=0 rebase --abort 2>err &&
> + test_grep "Unable to create" err &&
> + test_path_is_dir .git/rebase-merge &&
> + rm .git/MERGE_RR.lock &&
> + git rebase --abort &&
> + test_path_is_missing .git/rebase-merge
> +'
> +
> rerere_gc_custom_expiry_test () {
> five_days="$1" right_now="$2"
> test_expect_success "rerere gc with custom expiry ($five_days, $right_now)" 'There was a problem hiding this comment.
Thomas Bachem wrote on the Git mailing list (how to reply to this email):
Hi Junio,
Thanks for the careful review.
On 11/10/2026 02:39, Junio C Hamano wrote:
>> + When the time is up, the command fails as it does
>> + for any other lock it cannot take.
>
> Is it necessary to say this?
No, I'll drop it.
> The backgrounded unlocker sleeps for a second. Is the idea that,
> even in a heavily loaded CI environment, the backgrounded unlocker
> will have a sufficient chance to sleep for a second and unlock while
> the 5000-millisecond timeout waits for it?
Yes, the unlocker can be up to four seconds late. t0610 and t1300 use
the same numbers.
> Is the idea behind this 'wait' that the 'rerere.locktimeout'
> implementation might break in the future and we could reach this
> point before the background unlocker has finished sleeping for a
> full second? And we want to ensure it has exited before proceeding
> by waiting for it ourselves. If that is the case, perhaps a comment
> is warranted after '&&', such as:
>
> wait && # just in case the background unlocker is still active
It is. Instead of the comment, I'll move the wait into
test_when_finished, as pack-refs-tests.sh does, so it runs however
the test ends.
> Or make the string a bit more specific, such as:
>
> test_grep ! "Unable to create.*MERGE_RR\.lock" err &&
I'll use your pattern there and in the other tests.
> Should we ensure that there is no leftover pid file by removing it
> before creating .git/MERGE_RR.lock, by the way?
I don't think we need to. Git only writes MERGE_RR~pid.lock with
core.lockfilePid set, and t4200 never sets it. Even a stale one would
only change the text after the "Unable to create" line, not whether
git gets the lock.
> I'll stop here, but you can grasp the principles used in reviewing
> this test and apply them to the remaining tests to ensure they are
> clearly written.
Will do.
Thanks,
ThomasThere was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
Thomas Bachem <mail@thomasbachem.com> writes:
>> wait && # just in case the background unlocker is still active
>
> It is. Instead of the comment, I'll move the wait into
> test_when_finished, as pack-refs-tests.sh does, so it runs however
> the test ends.
Ah, that sounds quite sensible.
Thanks.There was a problem hiding this comment.
Okhuomon Ajayi wrote on the Git mailing list (how to reply to this email):
On Sun, Oct 11, 2026 at 1:39 AM Junio C Hamano <gitster@pobox.com> wrote:
> Side note. Only one of the other ones has unit in its name
> (i.e., credentialStore.lockTimeoutMS). We probably want to give
> it a synonym without MS suffix to make everything uniform.
> #leftoverbits
Hi Junio,
I would like to work on this #leftoverbits.
OkhuomonThere was a problem hiding this comment.
Okhuomon Ajayi wrote on the Git mailing list (how to reply to this email):
Support credentialStore.lockTimeout as the preferred setting
for credential lock retries, retaining lockTimeoutMS as a
deprecated alias.
Signed-off-by: Okhuomon Ajayi <okhuomonajayi54@gmail.com>
---
Documentation/config/credential.adoc | 5 ++-
builtin/credential-store.c | 4 +-
t/t0302-credential-store.sh | 56 ++++++++++++++++++++++++++++
3 files changed, 63 insertions(+), 2 deletions(-)
diff --git a/Documentation/config/credential.adoc b/Documentation/config/credential.adoc
index 80a7c77772..6fa40560be 100644
--- a/Documentation/config/credential.adoc
+++ b/Documentation/config/credential.adoc
@@ -48,8 +48,11 @@ credential.<url>.*::
credentialCache.ignoreSIGHUP::
Tell git-credential-cache--daemon to ignore SIGHUP, instead of quitting.
-credentialStore.lockTimeoutMS::
+credentialStore.lockTimeout::
The length of time, in milliseconds, for git-credential-store to retry
when trying to lock the credentials file. A value of 0 means not to retry at
all; -1 means to try indefinitely. Default is 1000 (i.e., retry for
1s).
+
+credentialStore.lockTimeoutMS (deprecated)::
+ This is a deprecated synonym for `credentialStore.lockTimeout`.
diff --git a/builtin/credential-store.c b/builtin/credential-store.c
index bc1453c6b2..9fef54d218 100644
--- a/builtin/credential-store.c
+++ b/builtin/credential-store.c
@@ -67,7 +67,9 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
{
int timeout_ms = 1000;
- repo_config_get_int(the_repository, "credentialstore.locktimeoutms", &timeout_ms);
+ /* lockTimeoutMS is a deprecated synonym of lockTimeout. */
+ if (repo_config_get_int(the_repository, "credentialstore.locktimeout", &timeout_ms))
+ repo_config_get_int(the_repository, "credentialstore.locktimeoutms", &timeout_ms);
if (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout_ms) < 0)
die_errno(_("unable to get credential storage lock in %d ms"), timeout_ms);
if (extra)
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index c1cd60edd0..408f26bb10 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -210,4 +210,60 @@ test_expect_success 'get: store file can contain empty/bogus lines' '
EOF
'
+# Holding creds.lock makes the next store fail. The error reports the
+# timeout that was actually used, so "in 0 ms" means that name won.
+test_expect_success 'credentialStore.lockTimeoutMS=0 gives up immediately' '
+ test_when_finished "rm -f creds creds.lock" &&
+ >"creds.lock" &&
+ test_must_fail git -c credentialStore.lockTimeoutMS=0 \
+ credential-store --file=creds store 2>err <<-\EOF &&
+ protocol=https
+ host=example.com
+ username=store-user
+ password=store-pass
+ EOF
+ test_grep "unable to get credential storage lock in 0 ms" err
+'
+
+test_expect_success 'credentialStore.lockTimeout=0 gives up immediately' '
+ test_when_finished "rm -f creds creds.lock" &&
+ >"creds.lock" &&
+ test_must_fail git -c credentialStore.lockTimeout=0 \
+ credential-store --file=creds store 2>err <<-\EOF &&
+ protocol=https
+ host=example.com
+ username=store-user
+ password=store-pass
+ EOF
+ test_grep "unable to get credential storage lock in 0 ms" err
+'
+
+test_expect_success 'credentialStore.lockTimeout wins when both are set' '
+ test_when_finished "rm -f creds creds.lock" &&
+ >"creds.lock" &&
+ test_must_fail git \
+ -c credentialStore.lockTimeout=0 \
+ -c credentialStore.lockTimeoutMS=500 \
+ credential-store --file=creds store 2>err <<-\EOF &&
+ protocol=https
+ host=example.com
+ username=store-user
+ password=store-pass
+ EOF
+ test_grep "unable to get credential storage lock in 0 ms" err
+'
+
+test_expect_success 'credentialStore.lockTimeout allows a store' '
+ test_when_finished "rm -f creds" &&
+ git -c credentialStore.lockTimeout=1000 \
+ credential-store --file=creds store <<-\EOF &&
+ protocol=https
+ host=example.com
+ username=store-user
+ password=store-pass
+ EOF
+ echo "https://store-user:store-pass@example.com" >expect &&
+ test_cmp expect creds
+'
+
test_done
--
2.54.0
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
Okhuomon Ajayi <okhuomonajayi54@gmail.com> writes:
> diff --git a/builtin/credential-store.c b/builtin/credential-store.c
> index bc1453c6b2..9fef54d218 100644
> --- a/builtin/credential-store.c
> +++ b/builtin/credential-store.c
> @@ -67,7 +67,9 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
> {
> int timeout_ms = 1000;
>
> - repo_config_get_int(the_repository, "credentialstore.locktimeoutms", &timeout_ms);
> + /* lockTimeoutMS is a deprecated synonym of lockTimeout. */
> + if (repo_config_get_int(the_repository, "credentialstore.locktimeout", &timeout_ms))
> + repo_config_get_int(the_repository, "credentialstore.locktimeoutms", &timeout_ms);
> if (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout_ms) < 0)
> die_errno(_("unable to get credential storage lock in %d ms"), timeout_ms);
Around this point the file is already full of overly long lines, but
the patch is making it even worse.
> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index c1cd60edd0..408f26bb10 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -210,4 +210,60 @@ test_expect_success 'get: store file can contain empty/bogus lines' '
> EOF
> '
>
> +# Holding creds.lock makes the next store fail. The error reports the
> +# timeout that was actually used, so "in 0 ms" means that name won.
> +test_expect_success 'credentialStore.lockTimeoutMS=0 gives up immediately' '
> + test_when_finished "rm -f creds creds.lock" &&
> + >"creds.lock" &&
> + test_must_fail git -c credentialStore.lockTimeoutMS=0 \
> + credential-store --file=creds store 2>err <<-\EOF &&
> + protocol=https
> + host=example.com
> + username=store-user
> + password=store-pass
> + EOF
> + test_grep "unable to get credential storage lock in 0 ms" err
> +'
> +
> +test_expect_success 'credentialStore.lockTimeout=0 gives up immediately' '
> + test_when_finished "rm -f creds creds.lock" &&
> + >"creds.lock" &&
> + test_must_fail git -c credentialStore.lockTimeout=0 \
> + credential-store --file=creds store 2>err <<-\EOF &&
> + protocol=https
> + host=example.com
> + username=store-user
> + password=store-pass
> + EOF
> + test_grep "unable to get credential storage lock in 0 ms" err
> +'
As it is necessary to keep the above two in sync forever, it may
make sense to do this instead:
for v lockTimeout lockTimeoutMS
do
test_expect_success "credentialStore.$v=0 gives up immediately" '
...
test_must_fail git -c credentialStore.$v=0 \
credential-store --file=creds store 2>err <<-\EOF &&
...
test_grep "unable to get ..." err
'
done
It is a common mistake to use double-quote in an attempt to have $v
expanded before it is passed to text_expect_success function, but
the variable $v is dereferenced correctly inside the body of the
test that is quoted with single-quote as it is eval'ed, so the way
the above is written is right.
> +test_expect_success 'credentialStore.lockTimeout wins when both are set' '
> + test_when_finished "rm -f creds creds.lock" &&
> + >"creds.lock" &&
> + test_must_fail git \
> + -c credentialStore.lockTimeout=0 \
> + -c credentialStore.lockTimeoutMS=500 \
> + credential-store --file=creds store 2>err <<-\EOF &&
> + protocol=https
> + host=example.com
> + username=store-user
> + password=store-pass
> + EOF
> + test_grep "unable to get credential storage lock in 0 ms" err
> +'
This is understandable. The presense of locktimeout=0 hides
whatever locktimeoutms is set to, so it should fail the same way as
the earlier two tests. Wait, nobody unlocks creds.lock, so what
should happen if the line that sets locktimeout=0 is removed?
Ahh, the key to this test is that we make sure "in 0 ms" part of the
error message does not say "in 500 ms" to ensure which of the two
variables are being used. It is not like the credential-store command
would fail with 0ms timeout but succees with 500ms timeout.
> +test_expect_success 'credentialStore.lockTimeout allows a store' '
> + test_when_finished "rm -f creds" &&
> + git -c credentialStore.lockTimeout=1000 \
> + credential-store --file=creds store <<-\EOF &&
> + protocol=https
> + host=example.com
> + username=store-user
> + password=store-pass
> + EOF
> + echo "https://store-user:store-pass@example.com" >expect &&
> + test_cmp expect creds
> +'
This one I I am not sure what it is trying to test, though. Would
the test start failing as the machine is loaded more and 1000 ms
becomes insufficient? No. The thing is not locked, nobody takes a
lock competing with our test, so with any value of the timeout we
should succeed, no? Or would it fail if the timeout is set to 0ms?
Assuming that there is a reason to run this test, should it also be
testing locktimeoutms to ensure both variables behave the same way
when set independently, similarly to the two tests I rewrote into a
for loop?
Thanks.There was a problem hiding this comment.
Okhuomon Ajayi wrote on the Git mailing list (how to reply to this email):
On Thu, Oct 15, 2026 at 6:50 PM Junio C Hamano <gitster@pobox.com> wrote:
> As it is necessary to keep the above two in sync forever, it may
> make sense to do this instead:
>
> for v lockTimeout lockTimeoutMS
Thanks, I've turned the two tests into a loop like this for v2, and
wrapped the long lines in credential-store.c.
> Ahh, the key to this test is that we make sure "in 0 ms" part of the
> error message does not say "in 500 ms" to ensure which of the two
> variables are being used. It is not like the credential-store command
> would fail with 0ms timeout but succees with 500ms timeout.
Yes, that is the point of it. I've added a comment above the test to
say so.
> This one I I am not sure what it is trying to test, though. Would
> the test start failing as the machine is loaded more and 1000 ms
> becomes insufficient? No. The thing is not locked, nobody takes a
> lock competing with our test, so with any value of the timeout we
> should succeed, no? Or would it fail if the timeout is set to 0ms?
You're right. Nothing holds the lock there, so it passes with any
timeout and does not test lockTimeout at all. I've dropped it in v2.
I'll send v2 shortly.
Thanks,
OkhuomonThere was a problem hiding this comment.
Okhuomon Ajayi wrote on the Git mailing list (how to reply to this email):
Support credentialStore.lockTimeout as the preferred setting
for credential lock retries, retaining lockTimeoutMS as a
deprecated alias.
Signed-off-by: Okhuomon Ajayi <okhuomonajayi54@gmail.com>
---
Changes since v1:
- Turned the two "lockTimeout=0" and "lockTimeoutMS=0" tests into a
single loop over both names, so they cannot drift apart.
- Wrapped the long lines the patch added in credential-store.c.
- Added a comment explaining that the "both are set" test checks for
"in 0 ms" rather than "in 500 ms".
- Dropped the "lockTimeout allows a store" test: nothing holds the
lock there, so it passes with any timeout.
Documentation/config/credential.adoc | 5 +++-
builtin/credential-store.c | 7 +++++-
t/t0302-credential-store.sh | 35 ++++++++++++++++++++++++++++
3 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/Documentation/config/credential.adoc b/Documentation/config/credential.adoc
index 80a7c77772..6fa40560be 100644
--- a/Documentation/config/credential.adoc
+++ b/Documentation/config/credential.adoc
@@ -48,8 +48,11 @@ credential.<url>.*::
credentialCache.ignoreSIGHUP::
Tell git-credential-cache--daemon to ignore SIGHUP, instead of quitting.
-credentialStore.lockTimeoutMS::
+credentialStore.lockTimeout::
The length of time, in milliseconds, for git-credential-store to retry
when trying to lock the credentials file. A value of 0 means not to retry at
all; -1 means to try indefinitely. Default is 1000 (i.e., retry for
1s).
+
+credentialStore.lockTimeoutMS (deprecated)::
+ This is a deprecated synonym for `credentialStore.lockTimeout`.
diff --git a/builtin/credential-store.c b/builtin/credential-store.c
index bc1453c6b2..afd2e4f3ce 100644
--- a/builtin/credential-store.c
+++ b/builtin/credential-store.c
@@ -67,7 +67,12 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
{
int timeout_ms = 1000;
- repo_config_get_int(the_repository, "credentialstore.locktimeoutms", &timeout_ms);
+ /* lockTimeoutMS is a deprecated synonym of lockTimeout. */
+ if (repo_config_get_int(the_repository, "credentialstore.locktimeout",
+ &timeout_ms))
+ repo_config_get_int(the_repository,
+ "credentialstore.locktimeoutms",
+ &timeout_ms);
if (hold_lock_file_for_update_timeout(&credential_lock, fn, 0, timeout_ms) < 0)
die_errno(_("unable to get credential storage lock in %d ms"), timeout_ms);
if (extra)
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index c1cd60edd0..e1bbca3fcc 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -210,4 +210,39 @@ test_expect_success 'get: store file can contain empty/bogus lines' '
EOF
'
+# Holding creds.lock makes the next store fail. The error reports the
+# timeout that was actually used, so "in 0 ms" means that name won.
+for v in lockTimeout lockTimeoutMS
+do
+ test_expect_success "credentialStore.$v=0 gives up immediately" '
+ test_when_finished "rm -f creds creds.lock" &&
+ >"creds.lock" &&
+ test_must_fail git -c credentialStore.$v=0 \
+ credential-store --file=creds store 2>err <<-\EOF &&
+ protocol=https
+ host=example.com
+ username=store-user
+ password=store-pass
+ EOF
+ test_grep "unable to get credential storage lock in 0 ms" err
+ '
+done
+
+# lockTimeout=0 must win over lockTimeoutMS=500: the error says
+# "in 0 ms", not "in 500 ms".
+test_expect_success 'credentialStore.lockTimeout wins when both are set' '
+ test_when_finished "rm -f creds creds.lock" &&
+ >"creds.lock" &&
+ test_must_fail git \
+ -c credentialStore.lockTimeout=0 \
+ -c credentialStore.lockTimeoutMS=500 \
+ credential-store --file=creds store 2>err <<-\EOF &&
+ protocol=https
+ host=example.com
+ username=store-user
+ password=store-pass
+ EOF
+ test_grep "unable to get credential storage lock in 0 ms" err
+'
+
test_done
--
2.54.0
There was a problem hiding this comment.
Okhuomon Ajayi wrote on the Git mailing list (how to reply to this email):
Friendly ping on this :)
Okhuomon
|
This patch series was integrated into seen via git@2ed1264. |
setup_rerere() takes MERGE_RR.lock with LOCK_DIE_ON_ERROR, so when two processes want it at the same time, the second one dies. That used to be rare. Since 452b12c (builtin/maintenance: use "geometric" strategy by default, 2026-02-24) the auto maintenance after a commit runs "git rerere gc" whenever rr-cache has enough stale entries, and the gc holds the lock while it prunes. If a rebase's next pick conflicts while the gc holds the lock, the rebase dies inside repo_rerere(), before the sequencer has written what "git rebase --continue" needs. Every later "git rebase --continue" then fails with "you have staged changes in your working tree". Wait for the lock for up to rerere.lockTimeout milliseconds, 1000 by default, and only then fail as before. Pruning a few thousand entries takes well under a second, so the default covers a rebase that runs into the gc. Assisted-by: Claude Fable 5.1 Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
Starting with the preceding commit, processes that want to acquire the rerere cache's MERGE_RR.lock by default know to wait up to one second until that lock has been released. This is a sensible default for many commands that happen to write rerere entries, as we would otherwise die immediately when the lock is taken by another process. But for repository maintenance it's a bit more complicated, as there are two cases that we have to care about. When the user explicitly asks us to garbage collect rerere entries via `git rerere gc` they probably want us to try our best to perform this operation. It's thus sensible to wait for the lock and then die if we weren't able to acquire it. But we also prune rerere entries as part of auto-maintenance, which is only executed on a best-effort basis anyway. Delaying the whole operation to acquire the lock is somewhat heavy-handed, and neither does it make sense to die in case we haven't been able to garbage collect rerere entries as that would impede other housekeeping tasks. Furthermore, it's totally fine to skip the operation when the rerere cache is locked already, as we will retry during the next run anyway. But we do not have an easy way to tell `git rerere gc` to skip the operation in case the cache is locked already. Add a new "--skip-locked" flag to plug that gap and have auto-maintenance pass that flag. Only auto-maintenance needs that flag, so hide it, like the "--skip-foreground-tasks" flag that `git maintenance run` passes to `git gc`. Helped-by: Patrick Steinhardt <ps@pks.im> Assisted-by: Claude Fable 5.1 Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
thomasbachem
force-pushed
the
rerere-gc-lock
branch
from
October 13, 2026 11:03
0002881 to
4202361
Compare
Author
|
/preview |
|
Preview email sent as pull.2214.v8.git.1791889623.gitgitgadget@gmail.com |
| rerere.lockTimeout:: | ||
| The length of time, in milliseconds, to wait for the rerere | ||
| lock when another process holds it, typically a background | ||
| `git rerere gc`. Value 0 means not to wait at all; -1 means |
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Sat, Oct 10, 2026 at 10:13:24AM +0000, Thomas Bachem via GitGitGadget wrote:
> diff --git a/rerere.h b/rerere.h
> index feeb0e2c9f..61e986475b 100644
> --- a/rerere.h
> +++ b/rerere.h
> @@ -10,6 +10,8 @@ struct repository;
> #define RERERE_AUTOUPDATE 01
> #define RERERE_NOAUTOUPDATE 02
> #define RERERE_READONLY 04
> +/* If MERGE_RR.lock is taken, return -1 as if rerere were disabled */
> +#define RERERE_NOWAIT 010
>
> /*
> * Marks paths that have been hand-resolved and added to the
> @@ -37,7 +39,14 @@ const char *rerere_path(struct strbuf *buf, const struct rerere_id *,
> int rerere_forget(struct repository *, struct pathspec *);
> int rerere_remaining(struct repository *, struct string_list *);
> void rerere_clear(struct repository *, struct string_list *);
> -void rerere_gc(struct repository *, struct string_list *);
> +
> +enum rerere_gc_flags {
> + /* Skip the operation in case the MERGE_RR.lock is already taken. */
> + RERERE_GC_NOWAIT = (1 << 0),
> +};
Hm. I was hoping that we could avoid introducing both RERERE_NOWAIT and
RERERE_GC_NOWAIT. But seeing the result I'm way less sure about my
earlier suggestion to introduce the gc-specific flags, as translating
the flags is a bit awkward. I think that your version that only has the
RERERE_NOWAIT flag was better, sorry.
Patrick
Author
|
/submit |
|
Submitted as pull.2214.v8.git.1791893274.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Oct 13, 2026 at 12:07:51PM +0000, Thomas Bachem via GitGitGadget wrote:
> Changes since v7:
>
> * The rerere.lockTimeout entry no longer says that the command fails when
> the time is up (Junio).
>
> * The lock tests grep for the "Unable to create" message about
> MERGE_RR.lock (Junio).
>
> * The wait for the background unlocker is now in test_when_finished
> (Junio).
>
> * The rebase --abort test aborts its rebase in test_when_finished, so a
> failed check cannot break later tests.
>
> * The test "merge fails once rerere.lockTimeout is up" never waits, so it
> is now called "merge fails on a lock it cannot take".
>
> * In patch 2, rerere_gc() takes RERERE_NOWAIT again instead of its own enum
> rerere_gc_flags (Patrick).
>
> * The comment about waiting in setup_rerere() now says "by default", and as
> in patch 1 it sits directly above the call that takes the lock.
>
> * A comment in t7900 says why the tests with a held lock create rr-cache.
I think this is in a good enough state by now. Thanks!
Patrick |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Patrick Steinhardt <ps@pks.im> writes:
> On Tue, Oct 13, 2026 at 12:07:51PM +0000, Thomas Bachem via GitGitGadget wrote:
>> Changes since v7:
>>
>> * The rerere.lockTimeout entry no longer says that the command fails when
>> the time is up (Junio).
>>
>> * The lock tests grep for the "Unable to create" message about
>> MERGE_RR.lock (Junio).
>>
>> * The wait for the background unlocker is now in test_when_finished
>> (Junio).
>>
>> * The rebase --abort test aborts its rebase in test_when_finished, so a
>> failed check cannot break later tests.
>>
>> * The test "merge fails once rerere.lockTimeout is up" never waits, so it
>> is now called "merge fails on a lock it cannot take".
>>
>> * In patch 2, rerere_gc() takes RERERE_NOWAIT again instead of its own enum
>> rerere_gc_flags (Patrick).
>>
>> * The comment about waiting in setup_rerere() now says "by default", and as
>> in patch 1 it sits directly above the call that takes the lock.
>>
>> * A comment in t7900 says why the tests with a held lock create rr-cache.
>
> I think this is in a good enough state by now. Thanks!
OK, thanks for writing and reviewing. Shall we mark the topic ready
for 'next' now?
Thanks. |
|
This patch series is no longer integrated into seen. |
|
There was a status update in the "Cooking" section about the branch Instead of failing to record conflicts to be resolved immediately, wait while "rerere gc" is ongoing. Will merge to 'next'? cf. <as4ikRUi0NZv_jKr@pks.im> cf. <xmqqy0c1zi6j.fsf@gitster.g> source: <pull.2214.v8.git.1791893274.gitgitgadget@gmail.com> |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Oct 13, 2026 at 09:20:04AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > On Tue, Oct 13, 2026 at 12:07:51PM +0000, Thomas Bachem via GitGitGadget wrote:
> >> Changes since v7:
> >>
> >> * The rerere.lockTimeout entry no longer says that the command fails when
> >> the time is up (Junio).
> >>
> >> * The lock tests grep for the "Unable to create" message about
> >> MERGE_RR.lock (Junio).
> >>
> >> * The wait for the background unlocker is now in test_when_finished
> >> (Junio).
> >>
> >> * The rebase --abort test aborts its rebase in test_when_finished, so a
> >> failed check cannot break later tests.
> >>
> >> * The test "merge fails once rerere.lockTimeout is up" never waits, so it
> >> is now called "merge fails on a lock it cannot take".
> >>
> >> * In patch 2, rerere_gc() takes RERERE_NOWAIT again instead of its own enum
> >> rerere_gc_flags (Patrick).
> >>
> >> * The comment about waiting in setup_rerere() now says "by default", and as
> >> in patch 1 it sits directly above the call that takes the lock.
> >>
> >> * A comment in t7900 says why the tests with a held lock create rr-cache.
> >
> > I think this is in a good enough state by now. Thanks!
>
> OK, thanks for writing and reviewing. Shall we mark the topic ready
> for 'next' now?
Yeah, let's. Thanks!
Patrick |
|
Thomas Bachem wrote on the Git mailing list (how to reply to this email): Hi Patrick, hi Junio,
Thank you both, and Phillip too, for all the reviews over the eight
rounds.
Thomas |
|
User |
|
This patch series was integrated into seen via git@05b1f36. |
|
There was a status update in the "Cooking" section about the branch Instead of failing to record conflicts to be resolved immediately, wait while "rerere gc" is ongoing. Will merge to 'next'? cf. <as4ikRUi0NZv_jKr@pks.im> cf. <xmqqy0c1zi6j.fsf@gitster.g> source: <pull.2214.v8.git.1791893274.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch Instead of failing to record conflicts to be resolved immediately, wait while "rerere gc" is ongoing. Will merge to 'next'? cf. <as4ikRUi0NZv_jKr@pks.im> cf. <xmqqy0c1zi6j.fsf@gitster.g> cf. <as8U2Ao_oX5KsI1b@pks.im> source: <pull.2214.v8.git.1791893274.gitgitgadget@gmail.com> |
|
This patch series was integrated into next via git@2479322. |
|
There was a status update in the "Cooking" section about the branch Instead of failing to record conflicts to be resolved immediately, wait while "rerere gc" is ongoing. Will merge to 'master'. cf. <as4ikRUi0NZv_jKr@pks.im> cf. <xmqqy0c1zi6j.fsf@gitster.g> cf. <as8U2Ao_oX5KsI1b@pks.im> source: <pull.2214.v8.git.1791893274.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch Instead of failing to record conflicts to be resolved immediately, wait while "rerere gc" is ongoing. Will merge to 'master'. cf. <as4ikRUi0NZv_jKr@pks.im> cf. <xmqqy0c1zi6j.fsf@gitster.g> cf. <as8U2Ao_oX5KsI1b@pks.im> source: <pull.2214.v8.git.1791893274.gitgitgadget@gmail.com> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A rebase dies at a conflict when a background "git rerere gc" holds MERGE_RR.lock at that moment. With the first patch, rerere waits for the lock instead of dying at once. With the second, the "git rerere gc" started by auto maintenance does nothing while the lock is held.
For v8 I took Junio's suggestions on patch 1 and Patrick's on patch 2. Changes since v7:
The rerere.lockTimeout entry no longer says that the command fails when the time is up (Junio).
The lock tests grep for the "Unable to create" message about MERGE_RR.lock (Junio).
The wait for the background unlocker is now in test_when_finished (Junio).
The rebase --abort test aborts its rebase in test_when_finished, so a failed check cannot break later tests.
The test "merge fails once rerere.lockTimeout is up" never waits, so it is now called "merge fails on a lock it cannot take".
In patch 2, rerere_gc() takes RERERE_NOWAIT again instead of its own enum rerere_gc_flags (Patrick).
The comment about waiting in setup_rerere() now says "by default", and as in patch 1 it sits directly above the call that takes the lock.
A comment in t7900 says why the tests with a held lock create rr-cache.
Cc: Patrick Steinhardt ps@pks.im
Cc: Phillip Wood phillip.wood@dunelm.org.uk
Cc: Junio C Hamano gitster@pobox.com
cc: Phillip Wood phillip.wood123@gmail.com
cc: Okhuomon Ajayi okhuomonajayi54@gmail.com