[PATCH 0/2] Generalize reference locking in tests

[PATCH 0/2] Generalize reference locking in tests

[Justin Tobler via GitGitGadget]

There are two tests in t1401 and t5541 that rely on writing a reference lock
file directly. While this works for the files reference backend, reftable
reference locks operate differently and are incompatible with this approach.
To be reference backend agnostic, this patch series refactors the two tests
to use git-update-ref(1) to lock refs instead.

This approach is more verbose and may warrant consideration of implementing
an abstraction to further simplify this operation. There appears to be one
other existing test in t1400 that also follows this pattern. Being that
there is only a handful of affected tests the implemented approach may be
sufficient as is.

Justin Tobler (2):
  t1401: generalize reference locking
  t5541: generalize reference locking

 t/t1401-symbolic-ref.sh    | 28 ++++++++++++++++++++++++----
 t/t5541-http-push-smart.sh | 25 +++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 6 deletions(-)


base-commit: 624eb90fa8f65a79396615f3c2842ac5a3743350
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1634%2Fjltobler%2Fjt%2Fbackend-generic-ref-lock-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1634/jltobler/jt/backend-generic-ref-lock-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1634
-- 
gitgitgadget

[PATCH 1/2] t1401: generalize reference locking

[Justin Tobler via GitGitGadget]

From: Justin Tobler <jltobler@gmail.com>

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.
Refactor the test to use git-update-ref(1) to lock refs instead so that
the test does not need to be aware of how the ref backend locks refs.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 t/t1401-symbolic-ref.sh | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index c7745e1bf69..bafe8db24df 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -105,10 +105,30 @@ test_expect_success LONG_REF 'we can parse long symbolic ref' '
 	test_cmp expect actual
 '
 
-test_expect_success 'symbolic-ref reports failure in exit code' '
-	test_when_finished "rm -f .git/HEAD.lock" &&
-	>.git/HEAD.lock &&
-	test_must_fail git symbolic-ref HEAD refs/heads/whatever
+test_expect_success PIPE 'symbolic-ref reports failure in exit code' '
+	mkfifo in out &&
+	(git 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 HEAD refs/heads/foo" >&9 &&
+
+	echo "prepare" >&9 &&
+	echo "prepare: ok" >expected &&
+	read line <&8 &&
+	echo "$line" >actual &&
+	test_cmp expected actual &&
+
+	test_must_fail git symbolic-ref HEAD refs/heads/bar
 '
 
 test_expect_success 'symbolic-ref writes reflog entry' '
-- 
gitgitgadget

[PATCH 2/2] t5541: generalize reference locking

[Justin Tobler via GitGitGadget]

From: Justin Tobler <jltobler@gmail.com>

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 <jltobler@gmail.com>
---
 t/t5541-http-push-smart.sh | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

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 &&
-- 
gitgitgadget

Re: [PATCH 0/2] Generalize reference locking in tests

[Junio C Hamano]

"Justin Tobler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> This approach is more verbose and may warrant consideration of implementing
> an abstraction to further simplify this operation. There appears to be one
> other existing test in t1400 that also follows this pattern. Being that
> there is only a handful of affected tests the implemented approach may be
> sufficient as is.

The use of two fifos and avoiding deadlocking parent and child is
tricky enough that it does feel that it warrants a helper function
to do the common part of what these two patches add.

I think I read t1401 patch carefully enough to understand what is
going on, but I cannot yet claim the same for the other one.

Thanks.

> Justin Tobler (2):
>   t1401: generalize reference locking
>   t5541: generalize reference locking
>
>  t/t1401-symbolic-ref.sh    | 28 ++++++++++++++++++++++++----
>  t/t5541-http-push-smart.sh | 25 +++++++++++++++++++++++--
>  2 files changed, 47 insertions(+), 6 deletions(-)
>
>
> base-commit: 624eb90fa8f65a79396615f3c2842ac5a3743350
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1634%2Fjltobler%2Fjt%2Fbackend-generic-ref-lock-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1634/jltobler/jt/backend-generic-ref-lock-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1634

Re: [PATCH 1/2] t1401: generalize reference locking

[Jeff King]

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

> From: Justin Tobler <jltobler@gmail.com>
> 
> 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.
> Refactor the test to use git-update-ref(1) to lock refs instead so that
> the test does not need to be aware of how the ref backend locks refs.

It looks like you re-create this situation in a backend-agnostic way by
having two simultaneous updates that conflict on the lock (but don't
care how that lock is implemented).

That works, but I think we could keep it simple. This test doesn't care
about the exact error condition we create. The point was just to die in
create_symref() and make sure the exit code was propagated. So something
like this would work:

  $ git symbolic-ref refs/heads refs/heads/foo
  error: unable to write symref for refs/heads: Is a directory

(note that you get a different error message if the refs are packed,
since there we can notice the d/f conflict manually).

There may be other ways to stimulate a failure. I thought "symbolic-ref
HEAD refs/heads/.invalid" might work, but sadly the refname format check
happens earlier.

I think it is worth avoiding the fifo magic if we can. It's complicated,
and it means that not all platforms run the test.

-Peff

Re: [PATCH 2/2] t5541: generalize reference locking

[Jeff King]

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

> From: Justin Tobler <jltobler@gmail.com>
> 
> 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

Re: [PATCH 1/2] t1401: generalize reference locking

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2054 bytes --]

On Thu, Jan 11, 2024 at 02:13:29AM -0500, Jeff King wrote:
> On Wed, Jan 10, 2024 at 06:52:29PM +0000, Justin Tobler via GitGitGadget wrote:
> 
> > From: Justin Tobler <jltobler@gmail.com>
> > 
> > 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.
> > Refactor the test to use git-update-ref(1) to lock refs instead so that
> > the test does not need to be aware of how the ref backend locks refs.
> 
> It looks like you re-create this situation in a backend-agnostic way by
> having two simultaneous updates that conflict on the lock (but don't
> care how that lock is implemented).
> 
> That works, but I think we could keep it simple. This test doesn't care
> about the exact error condition we create. The point was just to die in
> create_symref() and make sure the exit code was propagated. So something
> like this would work:
> 
>   $ git symbolic-ref refs/heads refs/heads/foo
>   error: unable to write symref for refs/heads: Is a directory
> 
> (note that you get a different error message if the refs are packed,
> since there we can notice the d/f conflict manually).

If all we care for is the exit code then this would work for the
reftable backend, too:

```
$ git init --ref-format=reftable repo
Initialized empty Git repository in /tmp/repo/.git/
$ cd repo/
$ git commit --allow-empty --message message
[main (root-commit) c2512d3] x
$ git symbolic-ref refs/heads refs/heads/foo
$ echo $?
1
```

A bit unfortunate that there is no proper error message in that case,
but that is a different topic.

Patrick

> There may be other ways to stimulate a failure. I thought "symbolic-ref
> HEAD refs/heads/.invalid" might work, but sadly the refname format check
> happens earlier.
> 
> I think it is worth avoiding the fifo magic if we can. It's complicated,
> and it means that not all platforms run the test.
> 
> -Peff
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] t5541: generalize reference locking

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
>
>> From: Justin Tobler <jltobler@gmail.com>
>> 
>> 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.

Re: [PATCH 1/2] t1401: generalize reference locking

[Justin Tobler]

Jeff King <peff@peff.net> writes:

> It looks like you re-create this situation in a backend-agnostic way by
> having two simultaneous updates that conflict on the lock (but don't
> care how that lock is implemented).
>
> That works, but I think we could keep it simple. This test doesn't care
> about the exact error condition we create. The point was just to die in
> create_symref() and make sure the exit code was propagated.

As you mentioned, the original intent was to recreate the same error
condition in a reference backend agnostic manner. Since the test doesn't
care about the exact error condition being used, I agree that creating a
d/f conflict instead is a much simpler and better approach. In the next
patch version I've updated the test in t1401 to use git-symbolic-ref(1)
to produce a d/f error.

Thanks,
Justin


On Thu, Jan 11, 2024 at 1:13 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Jan 10, 2024 at 06:52:29PM +0000, Justin Tobler via GitGitGadget wrote:
>
> > From: Justin Tobler <jltobler@gmail.com>
> >
> > 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.
> > Refactor the test to use git-update-ref(1) to lock refs instead so that
> > the test does not need to be aware of how the ref backend locks refs.
>
> It looks like you re-create this situation in a backend-agnostic way by
> having two simultaneous updates that conflict on the lock (but don't
> care how that lock is implemented).
>
> That works, but I think we could keep it simple. This test doesn't care
> about the exact error condition we create. The point was just to die in
> create_symref() and make sure the exit code was propagated. So something
> like this would work:
>
>   $ git symbolic-ref refs/heads refs/heads/foo
>   error: unable to write symref for refs/heads: Is a directory
>
> (note that you get a different error message if the refs are packed,
> since there we can notice the d/f conflict manually).
>
> There may be other ways to stimulate a failure. I thought "symbolic-ref
> HEAD refs/heads/.invalid" might work, but sadly the refname format check
> happens earlier.
>
> I think it is worth avoiding the fifo magic if we can. It's complicated,
> and it means that not all platforms run the test.
>
> -Peff

Re: [PATCH 2/2] t5541: generalize reference locking

[Justin Tobler]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
>>
>>> From: Justin Tobler <jltobler@gmail.com>
>>>
>>> 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 <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> > On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
> >
> >> From: Justin Tobler <jltobler@gmail.com>
> >>
> >> 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.

Re: [PATCH 0/2] Generalize reference locking in tests

[Justin Tobler]

Junio C Hamano <gitster@pobox.com> writes:

> The use of two fifos and avoiding deadlocking parent and child is
> tricky enough that it does feel that it warrants a helper function
> to do the common part of what these two patches add.

To avoid the trickiness that comes with introducing fifos to these
tests, in the next patch version I've followed the suggestion of Peff to
instead create d/f conflicts to create an error condition. It appears
that these tests are not particular to the exact error condition being
triggered and therefore d/f conflicts are much simpler to produce.

Thanks,
Justin

On Wed, Jan 10, 2024 at 6:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Justin Tobler via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > This approach is more verbose and may warrant consideration of implementing
> > an abstraction to further simplify this operation. There appears to be one
> > other existing test in t1400 that also follows this pattern. Being that
> > there is only a handful of affected tests the implemented approach may be
> > sufficient as is.
>
> The use of two fifos and avoiding deadlocking parent and child is
> tricky enough that it does feel that it warrants a helper function
> to do the common part of what these two patches add.
>
> I think I read t1401 patch carefully enough to understand what is
> going on, but I cannot yet claim the same for the other one.
>
> Thanks.
>
> > Justin Tobler (2):
> >   t1401: generalize reference locking
> >   t5541: generalize reference locking
> >
> >  t/t1401-symbolic-ref.sh    | 28 ++++++++++++++++++++++++----
> >  t/t5541-http-push-smart.sh | 25 +++++++++++++++++++++++--
> >  2 files changed, 47 insertions(+), 6 deletions(-)
> >
> >
> > base-commit: 624eb90fa8f65a79396615f3c2842ac5a3743350
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1634%2Fjltobler%2Fjt%2Fbackend-generic-ref-lock-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1634/jltobler/jt/backend-generic-ref-lock-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1634

[PATCH v2 0/2] Generalize reference locking in tests

[Justin Tobler via GitGitGadget]

Hello again,

This is the second version my patch series that refactors two tests to no
longer be dependent on the files reference backend. Changes compared to v1:

 * Removed reliance on fifo magic to trigger error conditions.
 * d/f conflicts now used to create errors instead.

Thanks for taking a look!

Justin

Justin Tobler (2):
  t1401: remove lockfile creation
  t5541: remove lockfile creation

 t/t1401-symbolic-ref.sh    |  5 ++---
 t/t5541-http-push-smart.sh | 18 +++++-------------
 2 files changed, 7 insertions(+), 16 deletions(-)


base-commit: 624eb90fa8f65a79396615f3c2842ac5a3743350
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1634%2Fjltobler%2Fjt%2Fbackend-generic-ref-lock-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1634/jltobler/jt/backend-generic-ref-lock-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1634

Range-diff vs v1:

 1:  cb78b549e5e ! 1:  714f713ccec t1401: generalize reference locking
     @@ Metadata
      Author: Justin Tobler <jltobler@gmail.com>
      
       ## Commit message ##
     -    t1401: generalize reference locking
     +    t1401: remove lockfile creation
      
     -    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.
     -    Refactor the test to use git-update-ref(1) to lock refs instead so that
     -    the test does not need to be aware of how the ref backend locks refs.
     +    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-symbolic-ref(1)
     +    instead so that the test is reference backend agnostic.
      
          Signed-off-by: Justin Tobler <jltobler@gmail.com>
      
       ## t/t1401-symbolic-ref.sh ##
      @@ t/t1401-symbolic-ref.sh: test_expect_success LONG_REF 'we can parse long symbolic ref' '
     - 	test_cmp expect actual
       '
       
     --test_expect_success 'symbolic-ref reports failure in exit code' '
     + test_expect_success 'symbolic-ref reports failure in exit code' '
      -	test_when_finished "rm -f .git/HEAD.lock" &&
      -	>.git/HEAD.lock &&
      -	test_must_fail git symbolic-ref HEAD refs/heads/whatever
     -+test_expect_success PIPE 'symbolic-ref reports failure in exit code' '
     -+	mkfifo in out &&
     -+	(git 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 HEAD refs/heads/foo" >&9 &&
     -+
     -+	echo "prepare" >&9 &&
     -+	echo "prepare: ok" >expected &&
     -+	read line <&8 &&
     -+	echo "$line" >actual &&
     -+	test_cmp expected actual &&
     -+
     -+	test_must_fail git symbolic-ref HEAD refs/heads/bar
     ++	# Create d/f conflict to simulate failure.
     ++	test_must_fail git symbolic-ref refs/heads refs/heads/foo
       '
       
       test_expect_success 'symbolic-ref writes reflog entry' '
 2:  11fd5091d61 ! 2:  f953a668c6a t5541: generalize reference locking
     @@ Metadata
      Author: Justin Tobler <jltobler@gmail.com>
      
       ## Commit message ##
     -    t5541: generalize reference locking
     +    t5541: remove lockfile creation
      
     -    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.
     +    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 <jltobler@gmail.com>
      
     @@ t/t5541-http-push-smart.sh: test_expect_success 'push --atomic fails on server-s
       
      -	# 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 &&
     ++	# 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 &&
     +@@ t/t5541-http-push-smart.sh: 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 &&

-- 
gitgitgadget

[PATCH v2 1/2] t1401: remove lockfile creation

[Justin Tobler via GitGitGadget]

From: Justin Tobler <jltobler@gmail.com>

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-symbolic-ref(1)
instead so that the test is reference backend agnostic.

Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
 t/t1401-symbolic-ref.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index c7745e1bf69..c67e5c134d9 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -106,9 +106,8 @@ test_expect_success LONG_REF 'we can parse long symbolic ref' '
 '
 
 test_expect_success 'symbolic-ref reports failure in exit code' '
-	test_when_finished "rm -f .git/HEAD.lock" &&
-	>.git/HEAD.lock &&
-	test_must_fail git symbolic-ref HEAD refs/heads/whatever
+	# Create d/f conflict to simulate failure.
+	test_must_fail git symbolic-ref refs/heads refs/heads/foo
 '
 
 test_expect_success 'symbolic-ref writes reflog entry' '
-- 
gitgitgadget

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

[Justin Tobler via GitGitGadget]

From: Justin Tobler <jltobler@gmail.com>

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 <jltobler@gmail.com>
---
 t/t5541-http-push-smart.sh | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

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 &&
-- 
gitgitgadget

Re: [PATCH 1/2] t1401: generalize reference locking

[Jeff King]

On Thu, Jan 11, 2024 at 12:08:44PM +0100, Patrick Steinhardt wrote:

> > (note that you get a different error message if the refs are packed,
> > since there we can notice the d/f conflict manually).
> 
> If all we care for is the exit code then this would work for the
> reftable backend, too:
> 
> ```
> $ git init --ref-format=reftable repo
> Initialized empty Git repository in /tmp/repo/.git/
> $ cd repo/
> $ git commit --allow-empty --message message
> [main (root-commit) c2512d3] x
> $ git symbolic-ref refs/heads refs/heads/foo
> $ echo $?
> 1
> ```

Yep, exactly. That should work for both and cover what the test was
originally trying to do.

> A bit unfortunate that there is no proper error message in that case,
> but that is a different topic.

Yeah, I would call that an outright bug. It does not have to be part of
this patch, but is worth fixing (and testing). I suspect it's not going
to be the only place with this problem. Most of the files-backend ref
code is very happy to spew to stdout using error(), but the reftable
code, having been written from a more lib-conscious perspective,
probably doesn't.

The obvious quick fix is to sprinkle more error() into the reftable
code. But in the longer term, I think the right direction is that the
ref code should accept an error strbuf or similar mechanism to propagate
human-readable error test to the caller.

-Peff

Re: [PATCH v2 2/2] t5541: remove lockfile creation

[Jeff King]

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

Re: [PATCH 1/2] t1401: generalize reference locking

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1943 bytes --]

On Fri, Jan 12, 2024 at 02:01:42AM -0500, Jeff King wrote:
> On Thu, Jan 11, 2024 at 12:08:44PM +0100, Patrick Steinhardt wrote:
> 
> > > (note that you get a different error message if the refs are packed,
> > > since there we can notice the d/f conflict manually).
> > 
> > If all we care for is the exit code then this would work for the
> > reftable backend, too:
> > 
> > ```
> > $ git init --ref-format=reftable repo
> > Initialized empty Git repository in /tmp/repo/.git/
> > $ cd repo/
> > $ git commit --allow-empty --message message
> > [main (root-commit) c2512d3] x
> > $ git symbolic-ref refs/heads refs/heads/foo
> > $ echo $?
> > 1
> > ```
> 
> Yep, exactly. That should work for both and cover what the test was
> originally trying to do.
> 
> > A bit unfortunate that there is no proper error message in that case,
> > but that is a different topic.
> 
> Yeah, I would call that an outright bug. It does not have to be part of
> this patch, but is worth fixing (and testing). I suspect it's not going
> to be the only place with this problem. Most of the files-backend ref
> code is very happy to spew to stdout using error(), but the reftable
> code, having been written from a more lib-conscious perspective,
> probably doesn't.

Yup, I've already fixed this bug in the reftable backend.

> The obvious quick fix is to sprinkle more error() into the reftable
> code. But in the longer term, I think the right direction is that the
> ref code should accept an error strbuf or similar mechanism to propagate
> human-readable error test to the caller.

Agreed, I think it's good that the reftable library itself does not
print error messages. In this particular case we are already able to
provide a proper error message due to the error code that the library
returns. But there are certainly going to be other cases where it might
make sense to pass in an error strbuf.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] t1401: generalize reference locking

[Jeff King]

On Fri, Jan 12, 2024 at 08:45:22AM +0100, Patrick Steinhardt wrote:

> > The obvious quick fix is to sprinkle more error() into the reftable
> > code. But in the longer term, I think the right direction is that the
> > ref code should accept an error strbuf or similar mechanism to propagate
> > human-readable error test to the caller.
> 
> Agreed, I think it's good that the reftable library itself does not
> print error messages. In this particular case we are already able to
> provide a proper error message due to the error code that the library
> returns. But there are certainly going to be other cases where it might
> make sense to pass in an error strbuf.

Oh, if there is an error code you can use already, that is even better. :)

Thanks for taking care of this case.

-Peff

Re: [PATCH v2 2/2] t5541: remove lockfile creation

[Junio C Hamano]

Jeff King <peff@peff.net> 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 &&
 

Re: [PATCH v2 2/2] t5541: remove lockfile creation

[Justin Tobler]

Great! Thanks everyone for the help!
-Justin

On Fri, Jan 12, 2024 at 11:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> 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 &&
>
>