Repository navigation
rerere: wait for MERGE_RR.lock, but not in auto maintenance #2214
+165
−12
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,3 +10,13 @@ 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. |
||
| `$GIT_DIR`, e.g. if "rerere" was previously used in the | ||
| repository. | ||
|
|
||
| rerere.lockTimeout:: | ||
| The length of time, in milliseconds, to wait for the rerere | ||
| lock when another process holds it, typically a background | ||
|
There was a problem hiding this comment. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Mon, Sep 14, 2026 at 08:04:21AM +0000, Thomas Bachem via GitGitGadget wrote:
> diff --git a/rerere.c b/rerere.c
> index 7d44f3937c..a996d39159 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -900,18 +902,26 @@ int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags)
> * 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. The gc itself never
> - * waits: skipping one of its runs costs nothing.
> + * waits: skipping one of its runs costs nothing. A
> + * command that stops at a conflict must not die here
> + * either, so it warns and goes on without rerere.
> */
> if (flags & RERERE_NOWAIT) {
> lock_flags = 0;
> timeout_ms = 0;
> }
> + if (flags & RERERE_SKIP_LOCKED)
> + lock_flags = 0;
> fd = repo_hold_lock_file_for_update_timeout(r, &write_lock,
> path, lock_flags,
> timeout_ms);
> if (fd < 0) {
> warning_errno(_("skipping rerere, "
> "unable to create '%s.lock'"), path);
> + if (flags & RERERE_SKIP_LOCKED)
> + advise(_("run \"git rerere\" before resolving "
> + "the conflict to record or replay "
> + "its resolution"));
> return -1;
> }
> }
Should this use `advise_if_enabled()`?
PatrickThere was a problem hiding this comment. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Fri, Oct 02, 2026 at 11:11:32AM +0000, Thomas Bachem via GitGitGadget wrote:
> From: Thomas Bachem <mail@thomasbachem.com>
>
> When a merge, rebase, cherry-pick, revert, am, stash or apply stops
> at a conflict, it runs rerere right before it returns to the user.
> If MERGE_RR.lock is still held when rerere.lockTimeout runs out, the
> command dies there. In a rebase, the sequencer has not yet written the
> state that "git rebase --continue" needs. A later
> "git rebase --continue" fails, and the "git commit --amend" that its
> message offers first folds the conflicted pick into the previous
> commit.
>
> So warn and go on without rerere. The conflict is still in place, and
> a hint tells the user to run "git rerere" before resolving it. That
> records the preimage or replays a known resolution, as the command
> would have. The hint is under advice.mergeConflict like other hints
> printed at a conflict stop.
>
> Everything else that waits for the lock is left as it is and still
> fails if the wait times out. That includes "git commit" and
> "git am --continue", which run rerere after a resolution. When they
> fail, the rebase or am can still be continued.
Is this a commit that we maybe want to defer to a later point in time?
I'm not yet convinced that it's really necessary with the other changes
that you've done, and it feels fishy to me to just skip some operations.
So I'd propose that we drop the commit for now, but keep the option open
to reintroduce it at a later point in time in case where we have users
actually hit the issue in the wild.
PatrickThere was a problem hiding this comment. Thomas Bachem wrote on the Git mailing list (how to reply to this email): Hi Patrick,
On 09/10/2026 11:06, Patrick Steinhardt wrote:
> Is this a commit that we maybe want to defer to a later point in time?
> I'm not yet convinced that it's really necessary with the other changes
> that you've done, and it feels fishy to me to just skip some operations.
> So I'd propose that we drop the commit for now, but keep the option open
> to reintroduce it at a later point in time in case where we have users
> actually hit the issue in the wild.
Agreed, I'll drop it in v7. With the wait, a rebase only dies at a
conflict when another process holds the lock for longer than
rerere.lockTimeout. And since tb/rerere-lock-grace, a rebase's own
commits no longer start auto maintenance. If users do hit it, I'll
bring the patch back.
Thanks,
Thomas |
||
| `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 Mon, Sep 28, 2026 at 11:58:21AM +0000, Thomas Bachem via GitGitGadget wrote:
> diff --git a/Documentation/git-rerere.adoc b/Documentation/git-rerere.adoc
> index 4e6ab9a27c..da7a1d093e 100644
> --- a/Documentation/git-rerere.adoc
> +++ b/Documentation/git-rerere.adoc
> @@ -63,14 +63,17 @@ Print paths with conflicts that have not been autoresolved by rerere.
> This includes paths whose resolutions cannot be tracked by rerere,
> such as conflicting submodules.
>
> -'gc'::
> +'gc' [--auto]::
>
> Prune records of conflicted merges that
> 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. With `--auto`, which `git maintenance run
> +--auto` and `git gc --auto` pass, `gc` does nothing while another
> +process holds the rerere lock. Without it, `gc` waits for the lock
> +as long as `rerere.lockTimeout` allows and then fails.
It's a bit weird to have git-rerere(1) document who calls it. We may
want to document why specifically this is useful though.
> diff --git a/builtin/rerere.c b/builtin/rerere.c
> index a056cb791b..2a8871df41 100644
> --- a/builtin/rerere.c
> +++ b/builtin/rerere.c
> @@ -56,16 +57,21 @@ int cmd_rerere(int argc,
> struct repository *repo UNUSED)
> {
> struct string_list merge_rr = STRING_LIST_INIT_DUP;
> - int autoupdate = -1, flags = 0;
> + int autoupdate = -1, auto_flag = 0, flags = 0;
>
> struct option options[] = {
> OPT_SET_INT(0, "rerere-autoupdate", &autoupdate,
> N_("register clean resolutions in index"), 1),
> + OPT_BOOL(0, "auto", &auto_flag,
> + N_("skip gc while another process holds the lock")),
> OPT_END(),
> };
Thinking about this a bit... I know it was my suggestion, but I wonder
whether "auto" is misnamed. We don't let any heuristics kick in like we
typically do for other commands like `git pack-refs --auto`, we only
know to skip garbage collection if the lock is taken. So there is a bit
of a mismatch here.
How about we instead call this "--skip-locked"? We could even mark it as
a hidden option and not even document it, as it feels very specific to
how git-maintenance(1) wants to invoke it. If so, we could maybe remove
it again at a later point.
An alternative could be to instead call `rerere_gc()` directly, and if
so we wouldn't have to add this flag at all. But that may result in some
bigger changes, so I'll leave it up to you to decide.
> argc = parse_options(argc, argv, prefix, options, rerere_usage, 0);
>
> + if (auto_flag && (argc < 1 || strcmp(argv[0], "gc")))
> + die(_("the option '%s' requires '%s'"), "--auto", "gc");
> +
> repo_config(the_repository, git_xmerge_config, NULL);
>
> if (autoupdate == 1)
Oh dear, this is a mess. The file could really use a refactoring to use
proper subcommands.
But anyway, that's certainly outside the scope of this patch series.
PatrickThere was a problem hiding this comment. Thomas Bachem wrote on the Git mailing list (how to reply to this email): Hi Patrick,
On 30/09/2026 17:00, Patrick Steinhardt wrote:
> It's a bit weird to have git-rerere(1) document who calls it. We may
> want to document why specifically this is useful though.
I'll take that out of git-rerere(1) again. I'd keep the last sentence
of the rerere.lockTimeout entry, since that is where I say what each
command does when the time is up, but name the two commands there
instead of the option:
"A `git rerere gc` run by `git maintenance run --auto` or
`git gc --auto` does not wait and does nothing while the lock is held."
> How about we instead call this "--skip-locked"? We could even mark it as
> a hidden option and not even document it, as it feels very specific to
> how git-maintenance(1) wants to invoke it. If so, we could maybe remove
> it again at a later point.
I'll take both, the name and hiding it.
Patch 3 has a RERERE_SKIP_LOCKED flag for the conflict-time callers.
I'll rename that one to RERERE_WARN_LOCKED so it doesn't look like the
option's flag, which stays RERERE_NOWAIT.
> An alternative could be to instead call `rerere_gc()` directly, and if
> so we wouldn't have to add this flag at all. But that may result in some
> bigger changes, so I'll leave it up to you to decide.
I tried it. It is six lines in builtin/gc.c, but rerere_gc() dies when
it can't take the lock. A manual or scheduled "git maintenance run"
then dies with the lockfile's message and exit code 128, where it now
reports "task 'rerere-gc' failed" and exits with 1. The rerere-gc
tests in t7900 fail too, since their helper looks for the
"git rerere gc" child. So I'd keep the option for this series. Say if
you'd rather have the direct call.
I'll wait a day or two for other comments before I send v6.
Thanks,
ThomasThere was a problem hiding this comment. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Thu, Oct 01, 2026 at 10:08:16AM +0200, Thomas Bachem wrote:
> Hi Patrick,
>
> On 30/09/2026 17:00, Patrick Steinhardt wrote:
> > It's a bit weird to have git-rerere(1) document who calls it. We may
> > want to document why specifically this is useful though.
>
> I'll take that out of git-rerere(1) again. I'd keep the last sentence
> of the rerere.lockTimeout entry, since that is where I say what each
> command does when the time is up, but name the two commands there
> instead of the option:
>
> "A `git rerere gc` run by `git maintenance run --auto` or
> `git gc --auto` does not wait and does nothing while the lock is held."
>
> > How about we instead call this "--skip-locked"? We could even mark it as
> > a hidden option and not even document it, as it feels very specific to
> > how git-maintenance(1) wants to invoke it. If so, we could maybe remove
> > it again at a later point.
>
> I'll take both, the name and hiding it.
>
> Patch 3 has a RERERE_SKIP_LOCKED flag for the conflict-time callers.
> I'll rename that one to RERERE_WARN_LOCKED so it doesn't look like the
> option's flag, which stays RERERE_NOWAIT.
>
> > An alternative could be to instead call `rerere_gc()` directly, and if
> > so we wouldn't have to add this flag at all. But that may result in some
> > bigger changes, so I'll leave it up to you to decide.
>
> I tried it. It is six lines in builtin/gc.c, but rerere_gc() dies when
> it can't take the lock. A manual or scheduled "git maintenance run"
> then dies with the lockfile's message and exit code 128, where it now
> reports "task 'rerere-gc' failed" and exits with 1. The rerere-gc
> tests in t7900 fail too, since their helper looks for the
> "git rerere gc" child. So I'd keep the option for this series. Say if
> you'd rather have the direct call.
Ah, right, that makes sense. Let's keep the hidden option in that case.
Thanks!
PatrickThere was a problem hiding this comment. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Fri, Oct 02, 2026 at 11:11:31AM +0000, Thomas Bachem via GitGitGadget wrote:
> From: Thomas Bachem <mail@thomasbachem.com>
>
> Since the previous commit, "git rerere gc" waits for MERGE_RR.lock
> like every other command that takes it, and fails only if the wait
> times out. That suits a user who runs it by hand and wants to know
> when nothing was pruned.
Two nits:
- We already took the lock before the preceding commit, the only
difference is that we now have a timeout. Your message sounds as if
the whole lock were new.
- The user don't necessarily care that nothing was pruned, but they do
care that pruning has failed.
> But the user did not ask for the gc that auto
> maintenance starts after a commit, and the next commit starts another
> one.
And this reads quite awkward, too. How about:
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.
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 57a3520263..7ad3987b71 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -385,12 +385,14 @@ out:
> return should_prune;
> }
>
> -static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts UNUSED,
> +static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts,
> struct gc_config *cfg UNUSED)
> {
> struct child_process rerere_cmd = CHILD_PROCESS_INIT;
> rerere_cmd.git_cmd = 1;
> strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL);
> + if (opts->auto_flag)
> + strvec_push(&rerere_cmd.args, "--skip-locked");
> return run_command(&rerere_cmd);
> }
Makes sense, as this is what drives both `git gc --auto` and `git
maintenance run --auto`.
> diff --git a/rerere.c b/rerere.c
> index 64fac07c71..43c8eb04db 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -887,18 +887,30 @@ 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) && (flags & RERERE_NOWAIT))
> + BUG("RERERE_NOWAIT does not apply with RERERE_READONLY");
> if (flags & RERERE_READONLY) {
> fd = 0;
> } else {
> + int lock_flags = LOCK_DIE_ON_ERROR;
> + int timeout_ms = rerere_lock_timeout_ms;
> +
> /*
> * 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.
> + * it instead of dying right away. The gc of an automatic
> + * maintenance run does not wait, since skipping one of
> + * its runs costs nothing.
> */
This comment is basically a layering violation, as you now assume who
passes `RERERE_NOWAIT`. It's a generic mechanism though, so I'd just
drop that part.
> + if (flags & RERERE_NOWAIT) {
> + lock_flags = 0;
> + timeout_ms = 0;
> + }
> fd = repo_hold_lock_file_for_update_timeout(r, &write_lock,
> git_path_merge_rr(r),
> - LOCK_DIE_ON_ERROR,
> - rerere_lock_timeout_ms);
> + lock_flags, timeout_ms);
> + if (fd < 0)
> + return -1;
It's a tiny bit fishy that we return an error in the case where we have
been asked to skip locking and we indeed weren't able to acquire the
lock. To me it doesn't really indicate an error, as it matches the
intent of the caller. But I guess that's debatable.
> diff --git a/rerere.h b/rerere.h
> index feeb0e2c9f..d54c53d0d4 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
> +/* Take MERGE_RR.lock only if it is free, and return quietly otherwise */
> +#define RERERE_NOWAIT 010
"free" is a bit unusual for a term for a lock.
> @@ -37,7 +39,7 @@ 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 *);
> +void rerere_gc(struct repository *, struct string_list *, int);
Given that these flags are new now, and given that none of the other
flags apply to `rerere_gc`, shouldn't we instead have a separate list of
flags specific to this function?
enum rerere_gc_flags {
/* Skip the operation in case the MERGE_RR.lock is already taken. */
RERERE_GC_NOWAIT = (1 << 0),
};
void rerere_gc(struct repository *, struct string_list *,
enum rerere_gc_flags flags);
> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
> index 7bd92235dc..28152bf456 100755
> --- a/t/t4200-rerere.sh
> +++ b/t/t4200-rerere.sh
> @@ -242,6 +242,27 @@ test_expect_success 'old records rest in peace' '
> test_path_is_missing $rr2/preimage
> '
>
> +test_expect_success 'gc --skip-locked 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 --skip-locked 2>err &&
> + test_must_be_empty err &&
> + test_path_is_file $rr2/preimage &&
> +
> + rm .git/MERGE_RR.lock &&
> + git rerere gc --skip-locked &&
> + test_path_is_missing $rr2/preimage
> +'
> +
> +test_expect_success '--skip-locked is only accepted by gc' '
> + test_must_fail git rerere --skip-locked clear 2>err &&
> + test_grep "option .--skip-locked. requires .gc." err
> +'
You verify that --skip-locked skips when locked, but you don't verify
that it doesn't skip when unlocked.
PatrickThere was a problem hiding this comment. Thomas Bachem wrote on the Git mailing list (how to reply to this email): Hi Patrick,
On 09/10/2026 11:06, Patrick Steinhardt wrote:
> And this reads quite awkward, too. How about:
I'll take your message as it is, thanks. I'd only add a last
paragraph on why the flag is hidden:
Only auto-maintenance needs that flag, so hide it, like the
"--skip-foreground-tasks" flag that `git maintenance run` passes to
`git gc`.
> This comment is basically a layering violation, as you now assume who
> passes `RERERE_NOWAIT`. It's a generic mechanism though, so I'd just
> drop that part.
Right, I'll drop it.
> It's a tiny bit fishy that we return an error in the case where we have
> been asked to skip locking and we indeed weren't able to acquire the
> lock. To me it doesn't really indicate an error, as it matches the
> intent of the caller. But I guess that's debatable.
setup_rerere() already returns -1 when rerere is disabled, and every
caller takes that as nothing to do rather than as an error. So I'd
keep the -1 and say so in the comment on RERERE_NOWAIT.
> "free" is a bit unusual for a term for a lock.
That's the comment I'd change anyway, so it would read:
/* If MERGE_RR.lock is taken, return -1 as if rerere were disabled */
> Given that these flags are new now, and given that none of the other
> flags apply to `rerere_gc`, shouldn't we instead have a separate list of
> flags specific to this function?
Yes, I'll add enum rerere_gc_flags as you wrote it, and have
rerere_gc() pass RERERE_NOWAIT to setup_rerere() for it.
> You verify that --skip-locked skips when locked, but you don't verify
> that it doesn't skip when unlocked.
The second half of that test does: it removes the lock, runs
"git rerere gc --skip-locked" again and checks that the preimage is
gone. That's easy to miss, so I'll make it a test of its own.
Thanks,
Thomas |
||
| to wait indefinitely. Default is 1000 (i.e., wait for 1 | ||
| second). When the time is up, the command fails as it does | ||
| for any other lock it cannot take. A `git rerere gc` run by | ||
| `git maintenance run --auto` or `git gc --auto` does not wait | ||
| and does nothing while the lock is held. | ||
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
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):