[2/2] t5541: generalize reference locking

Commit Message

Justin Tobler Jan. 10, 2024, 6:52 p.m. UTC

From: Justin Tobler <[e-mail]>

Some tests set up reference locks by directly creating the lockfile.
While this works for the files reference backend, reftable reference
locks operate differently and are incompatible with this approach.
Generalize reference locking by preparing a reference transaction.

Signed-off-by: Justin Tobler <[e-mail]>
---
 t/t5541-http-push-smart.sh | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

Comments

Jeff King Jan. 11, 2024, 7:28 a.m. UTC | #1

On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:

> From: Justin Tobler <[e-mail]>
> 
> Some tests set up reference locks by directly creating the lockfile.
> While this works for the files reference backend, reftable reference
> locks operate differently and are incompatible with this approach.
> Generalize reference locking by preparing a reference transaction.

As with the first patch, I think we could use d/f conflicts to get the
same effect. Perhaps something like this:

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index df758e187d..7eb0e887e1 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -233,7 +233,8 @@ test_expect_success 'push --atomic fails on server-side errors' '
 	up="$HTTPD_URL"/smart/atomic-branches.git &&
 
 	# break ref updates for other on the remote site
-	mkdir "$d/refs/heads/other.lock" &&
+	git -C "$d" update-ref -d refs/heads/other &&
+	git -C "$d" update-ref refs/heads/other/block-me HEAD &&
 
 	# add the new commit to other
 	git branch -f other collateral &&
@@ -244,12 +245,8 @@ test_expect_success 'push --atomic fails on server-side errors' '
 	# the new branch should not have been created upstream
 	test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
 
-	# upstream should still reflect atomic2, the last thing we pushed
-	# successfully
-	git rev-parse atomic2 >expected &&
-	# ...to other.
-	git -C "$d" rev-parse refs/heads/other >actual &&
-	test_cmp expected actual &&
+	# upstream should not have updated, as it cannot be written at all.
+	test_must_fail git -C "$d" rev-parse --verify refs/heads/other &&
 
 	# the new branch should not have been created upstream
 	test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&

I do think that the original was slightly more interesting (since we
could check that "other" still existed but was not updated), but I think
the main point of the test is that "atomic" was not pushed at all.

-Peff

Junio C Hamano Jan. 11, 2024, 6:47 p.m. UTC | #2

Jeff King <[e-mail]> writes:

> On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
>
>> From: Justin Tobler <[e-mail]>
>> 
>> Some tests set up reference locks by directly creating the lockfile.
>> While this works for the files reference backend, reftable reference
>> locks operate differently and are incompatible with this approach.
>> Generalize reference locking by preparing a reference transaction.
>
> As with the first patch, I think we could use d/f conflicts to get the
> same effect. Perhaps something like this:

Thanks for a great alternative.  I agree that avoiding fifo indeed
is a better way to go.

Justin Tobler Jan. 11, 2024, 8:20 p.m. UTC | #3

Junio C Hamano <[e-mail]> writes:

> Jeff King <[e-mail]> writes:
>
>> On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
>>
>>> From: Justin Tobler <[e-mail]>
>>>
>>> Some tests set up reference locks by directly creating the lockfile.
>>> While this works for the files reference backend, reftable reference
>>> locks operate differently and are incompatible with this approach.
>>> Generalize reference locking by preparing a reference transaction.
>>
>> As with the first patch, I think we could use d/f conflicts to get the
>> same effect. Perhaps something like this:
>
> Thanks for a great alternative.  I agree that avoiding fifo indeed
> is a better way to go.

For this patch, in the next version, I have also followed Peff's
suggestion to create d/f conflicts to trigger an error condition instead
of using fifos.

Thanks to everyone for the feedback!
Justin


On Thu, Jan 11, 2024 at 12:48 PM Junio C Hamano <[e-mail]> wrote:
>
> Jeff King <[e-mail]> writes:
>
> > On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
> >
> >> From: Justin Tobler <[e-mail]>
> >>
> >> Some tests set up reference locks by directly creating the lockfile.
> >> While this works for the files reference backend, reftable reference
> >> locks operate differently and are incompatible with this approach.
> >> Generalize reference locking by preparing a reference transaction.
> >
> > As with the first patch, I think we could use d/f conflicts to get the
> > same effect. Perhaps something like this:
>
> Thanks for a great alternative.  I agree that avoiding fifo indeed
> is a better way to go.

Patch

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index df758e187df..5b94c0b066f 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -232,8 +232,29 @@  test_expect_success 'push --atomic fails on server-side errors' '
 	test_config -C "$d" http.receivepack true &&
 	up="$HTTPD_URL"/smart/atomic-branches.git &&
 
-	# break ref updates for other on the remote site
-	mkdir "$d/refs/heads/other.lock" &&
+	mkfifo in out &&
+	(git -C "$d" update-ref --stdin <in >out &) &&
+
+	exec 9>in &&
+	exec 8<out &&
+	test_when_finished "exec 9>&-" &&
+	test_when_finished "exec 8<&-" &&
+
+	echo "start" >&9 &&
+	echo "start: ok" >expected &&
+	read line <&8 &&
+	echo "$line" >actual &&
+	test_cmp expected actual &&
+
+	echo "update refs/heads/other refs/heads/other" >&9 &&
+
+	# Prepare reference transaction on `other` reference to lock it and thus
+	# break updates on the remote.
+	echo "prepare" >&9 &&
+	echo "prepare: ok" >expected &&
+	read line <&8 &&
+	echo "$line" >actual &&
+	test_cmp expected actual &&
 
 	# add the new commit to other
 	git branch -f other collateral &&