[v2,2/2] t5541: remove lockfile creation

Commit Message

Justin Tobler Jan. 11, 2024, 8:24 p.m. UTC

From: Justin Tobler <[e-mail]>

To create error conditions, some tests set up reference locks by
directly creating its lockfile. While this works for the files reference
backend, this approach is incompatible with the reftable backend.
Refactor the test to create a d/f conflict via git-update-ref(1) instead
so that the test is reference backend agnostic.

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

Comments

Jeff King Jan. 12, 2024, 7:03 a.m. UTC | #1

On Thu, Jan 11, 2024 at 08:24:30PM +0000, Justin Tobler via GitGitGadget wrote:

> -	# 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 &&
> -
> -	# the new branch should not have been created upstream
> +	# The atomic and other branches should be created upstream.
>  	test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
> +	test_must_fail git -C "$d" show-ref --verify refs/heads/other &&

This last comment should say "should not be created", I think?

Other than that, both patches look good to me.

-Peff

Junio C Hamano Jan. 12, 2024, 5:58 p.m. UTC | #2

Jeff King <[e-mail]> writes:

> On Thu, Jan 11, 2024 at 08:24:30PM +0000, Justin Tobler via GitGitGadget wrote:
>
>> -	# 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 &&
>> -
>> -	# the new branch should not have been created upstream
>> +	# The atomic and other branches should be created upstream.
>>  	test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
>> +	test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
>
> This last comment should say "should not be created", I think?
>
> Other than that, both patches look good to me.

Thanks.  Will queue with the following and "Acked-by: peff".

diff --git c/t/t5541-http-push-smart.sh w/t/t5541-http-push-smart.sh
index 9a8bed6c32..71428f3d5c 100755
--- c/t/t5541-http-push-smart.sh
+++ w/t/t5541-http-push-smart.sh
@@ -242,7 +242,7 @@ test_expect_success 'push --atomic fails on server-side errors' '
 	# --atomic should cause entire push to be rejected
 	test_must_fail git push --atomic "$up" atomic other 2>output  &&
 
-	# The atomic and other branches should be created upstream.
+	# The atomic and other branches should not be created upstream.
 	test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
 	test_must_fail git -C "$d" show-ref --verify refs/heads/other &&

Justin Tobler Jan. 13, 2024, 10:25 p.m. UTC | #3

Great! Thanks everyone for the help!
-Justin

On Fri, Jan 12, 2024 at 11:58 AM Junio C Hamano <[e-mail]> wrote:
>
> Jeff King <[e-mail]> writes:
>
> > On Thu, Jan 11, 2024 at 08:24:30PM +0000, Justin Tobler via GitGitGadget wrote:
> >
> >> -    # 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 &&
> >> -
> >> -    # the new branch should not have been created upstream
> >> +    # The atomic and other branches should be created upstream.
> >>      test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
> >> +    test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
> >
> > This last comment should say "should not be created", I think?
> >
> > Other than that, both patches look good to me.
>
> Thanks.  Will queue with the following and "Acked-by: peff".
>
> diff --git c/t/t5541-http-push-smart.sh w/t/t5541-http-push-smart.sh
> index 9a8bed6c32..71428f3d5c 100755
> --- c/t/t5541-http-push-smart.sh
> +++ w/t/t5541-http-push-smart.sh
> @@ -242,7 +242,7 @@ test_expect_success 'push --atomic fails on server-side errors' '
>         # --atomic should cause entire push to be rejected
>         test_must_fail git push --atomic "$up" atomic other 2>output  &&
>
> -       # The atomic and other branches should be created upstream.
> +       # The atomic and other branches should not be created upstream.
>         test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
>         test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
>
>

Patch

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index df758e187df..9a8bed6c32b 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -232,8 +232,9 @@  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" &&
+	# Create d/f conflict to break ref updates for other on the remote site.
+	git -C "$d" update-ref -d refs/heads/other &&
+	git -C "$d" update-ref refs/heads/other/conflict HEAD &&
 
 	# add the new commit to other
 	git branch -f other collateral &&
@@ -241,18 +242,9 @@  test_expect_success 'push --atomic fails on server-side errors' '
 	# --atomic should cause entire push to be rejected
 	test_must_fail git push --atomic "$up" atomic other 2>output  &&
 
-	# 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 &&
-
-	# the new branch should not have been created upstream
+	# The atomic and other branches should be created upstream.
 	test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
+	test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
 
 	# the failed refs should be indicated to the user
 	grep "^ ! .*rejected.* other -> other .*atomic transaction failed" output &&