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

Message

John Passaro via GitGitGadget Jan. 11, 2024, 8:24 p.m. UTC

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 <[e-mail]>
      
       ## 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 <[e-mail]>
      
       ## 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 <[e-mail]>
      
       ## 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 <[e-mail]>
      
     @@ 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 &&