Re: [PATCH v7 1/2] rerere: wait for MERGE_RR.lock before giving up

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 11, 2026, 00:39 UTC
Message-ID: <xmqqqzhxf4uo.fsf@gitster.g>
In-Reply-To: <3dc3d02f12a3118ac9e270c19960815f6b8170cb.1791627204.git.gitgitgadget@gmail.com>

"Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines

> 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

Show 5 quoted lines

> +	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.

Show 25 quoted lines

> @@ -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.

Show 17 quoted lines

> 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.

Show 43 quoted lines

> +
> +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)" '