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

[Junio C Hamano <gitster@pobox.com>]

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