{"thread":{"id":"60720","subject":"[PATCH 0/2] Generalize reference locking in tests","startedAt":"2024-01-10T18:52:33Z","lastAt":"2024-01-13T22:26:08Z","messageCount":20,"participants":["Justin Tobler via GitGitGadget","Junio C Hamano","Jeff King","Patrick Steinhardt","Justin Tobler"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"486556","messageId":"pull.1634.git.1704912750.gitgitgadget@gmail.com","threadId":"60720","inReplyTo":null,"subject":"[PATCH 0/2] Generalize reference locking in tests","fromName":"Justin Tobler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-10T18:52:28Z","receivedAt":"2024-01-10T18:52:33Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"There are two tests in t1401 and t5541 that rely on writing a reference lock\nfile directly. While this works for the files reference backend, reftable\nreference locks operate differently and are incompatible with this approach.\nTo be reference backend agnostic, this patch series refactors the two tests\nto use git-update-ref(1) to lock refs instead.\n\nThis approach is more verbose and may warrant consideration of implementing\nan abstraction to further simplify this operation. There appears to be one\nother existing test in t1400 that also follows this pattern. Being that\nthere is only a handful of affected tests the implemented approach may be\nsufficient as is.\n\nJustin Tobler (2):\n  t1401: generalize reference locking\n  t5541: generalize reference locking\n\n t/t1401-symbolic-ref.sh    | 28 ++++++++++++++++++++++++----\n t/t5541-http-push-smart.sh | 25 +++++++++++++++++++++++--\n 2 files changed, 47 insertions(+), 6 deletions(-)\n\n\nbase-commit: 624eb90fa8f65a79396615f3c2842ac5a3743350\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1634%2Fjltobler%2Fjt%2Fbackend-generic-ref-lock-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1634/jltobler/jt/backend-generic-ref-lock-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1634\n-- \ngitgitgadget\n"},{"id":"486557","messageId":"cb78b549e5e826ffef39c55bd726164e6b7bb755.1704912750.git.gitgitgadget@gmail.com","threadId":"60720","inReplyTo":"pull.1634.git.1704912750.gitgitgadget@gmail.com","subject":"[PATCH 1/2] t1401: generalize reference locking","fromName":"Justin Tobler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-10T18:52:29Z","receivedAt":"2024-01-10T18:52:33Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"From: Justin Tobler <jltobler@gmail.com>\n\nSome tests set up reference locks by directly creating the lockfile.\nWhile this works for the files reference backend, reftable reference\nlocks operate differently and are incompatible with this approach.\nRefactor the test to use git-update-ref(1) to lock refs instead so that\nthe test does not need to be aware of how the ref backend locks refs.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n t/t1401-symbolic-ref.sh | 28 ++++++++++++++++++++++++----\n 1 file changed, 24 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex c7745e1bf69..bafe8db24df 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -105,10 +105,30 @@ test_expect_success LONG_REF 'we can parse long symbolic ref' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'symbolic-ref reports failure in exit code' '\n-\ttest_when_finished \"rm -f .git/HEAD.lock\" &&\n-\t>.git/HEAD.lock &&\n-\ttest_must_fail git symbolic-ref HEAD refs/heads/whatever\n+test_expect_success PIPE 'symbolic-ref reports failure in exit code' '\n+\tmkfifo in out &&\n+\t(git update-ref --stdin <in >out &) &&\n+\n+\texec 9>in &&\n+\texec 8<out &&\n+\ttest_when_finished \"exec 9>&-\" &&\n+\ttest_when_finished \"exec 8<&-\" &&\n+\n+\techo \"start\" >&9 &&\n+\techo \"start: ok\" >expected &&\n+\tread line <&8 &&\n+\techo \"$line\" >actual &&\n+\ttest_cmp expected actual &&\n+\n+\techo \"update HEAD refs/heads/foo\" >&9 &&\n+\n+\techo \"prepare\" >&9 &&\n+\techo \"prepare: ok\" >expected &&\n+\tread line <&8 &&\n+\techo \"$line\" >actual &&\n+\ttest_cmp expected actual &&\n+\n+\ttest_must_fail git symbolic-ref HEAD refs/heads/bar\n '\n \n test_expect_success 'symbolic-ref writes reflog entry' '\n-- \ngitgitgadget\n\n"},{"id":"486558","messageId":"11fd5091d61b54d8862ab2e316bbd25fff63ce0f.1704912750.git.gitgitgadget@gmail.com","threadId":"60720","inReplyTo":"pull.1634.git.1704912750.gitgitgadget@gmail.com","subject":"[PATCH 2/2] t5541: generalize reference locking","fromName":"Justin Tobler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-10T18:52:30Z","receivedAt":"2024-01-10T18:52:33Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"From: Justin Tobler <jltobler@gmail.com>\n\nSome tests set up reference locks by directly creating the lockfile.\nWhile this works for the files reference backend, reftable reference\nlocks operate differently and are incompatible with this approach.\nGeneralize reference locking by preparing a reference transaction.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n t/t5541-http-push-smart.sh | 25 +++++++++++++++++++++++--\n 1 file changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex df758e187df..5b94c0b066f 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -232,8 +232,29 @@ test_expect_success 'push --atomic fails on server-side errors' '\n \ttest_config -C \"$d\" http.receivepack true &&\n \tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n \n-\t# break ref updates for other on the remote site\n-\tmkdir \"$d/refs/heads/other.lock\" &&\n+\tmkfifo in out &&\n+\t(git -C \"$d\" update-ref --stdin <in >out &) &&\n+\n+\texec 9>in &&\n+\texec 8<out &&\n+\ttest_when_finished \"exec 9>&-\" &&\n+\ttest_when_finished \"exec 8<&-\" &&\n+\n+\techo \"start\" >&9 &&\n+\techo \"start: ok\" >expected &&\n+\tread line <&8 &&\n+\techo \"$line\" >actual &&\n+\ttest_cmp expected actual &&\n+\n+\techo \"update refs/heads/other refs/heads/other\" >&9 &&\n+\n+\t# Prepare reference transaction on `other` reference to lock it and thus\n+\t# break updates on the remote.\n+\techo \"prepare\" >&9 &&\n+\techo \"prepare: ok\" >expected &&\n+\tread line <&8 &&\n+\techo \"$line\" >actual &&\n+\ttest_cmp expected actual &&\n \n \t# add the new commit to other\n \tgit branch -f other collateral &&\n-- \ngitgitgadget\n"},{"id":"486583","messageId":"xmqqv8807ll1.fsf@gitster.g","threadId":"60720","inReplyTo":"pull.1634.git.1704912750.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Generalize reference locking in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-11T00:36:10Z","receivedAt":"2024-01-11T00:36:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Justin Tobler via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> This approach is more verbose and may warrant consideration of implementing\n> an abstraction to further simplify this operation. There appears to be one\n> other existing test in t1400 that also follows this pattern. Being that\n> there is only a handful of affected tests the implemented approach may be\n> sufficient as is.\n\nThe use of two fifos and avoiding deadlocking parent and child is\ntricky enough that it does feel that it warrants a helper function\nto do the common part of what these two patches add.\n\nI think I read t1401 patch carefully enough to understand what is\ngoing on, but I cannot yet claim the same for the other one.\n\nThanks.\n\n> Justin Tobler (2):\n>   t1401: generalize reference locking\n>   t5541: generalize reference locking\n>\n>  t/t1401-symbolic-ref.sh    | 28 ++++++++++++++++++++++++----\n>  t/t5541-http-push-smart.sh | 25 +++++++++++++++++++++++--\n>  2 files changed, 47 insertions(+), 6 deletions(-)\n>\n>\n> base-commit: 624eb90fa8f65a79396615f3c2842ac5a3743350\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1634%2Fjltobler%2Fjt%2Fbackend-generic-ref-lock-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1634/jltobler/jt/backend-generic-ref-lock-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1634\n"},{"id":"486600","messageId":"20240111071329.GC48154@coredump.intra.peff.net","threadId":"60720","inReplyTo":"cb78b549e5e826ffef39c55bd726164e6b7bb755.1704912750.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] t1401: generalize reference locking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-11T07:13:29Z","receivedAt":"2024-01-11T07:13:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 10, 2024 at 06:52:29PM +0000, Justin Tobler via GitGitGadget wrote:\n\n> From: Justin Tobler <jltobler@gmail.com>\n> \n> Some tests set up reference locks by directly creating the lockfile.\n> While this works for the files reference backend, reftable reference\n> locks operate differently and are incompatible with this approach.\n> Refactor the test to use git-update-ref(1) to lock refs instead so that\n> the test does not need to be aware of how the ref backend locks refs.\n\nIt looks like you re-create this situation in a backend-agnostic way by\nhaving two simultaneous updates that conflict on the lock (but don't\ncare how that lock is implemented).\n\nThat works, but I think we could keep it simple. This test doesn't care\nabout the exact error condition we create. The point was just to die in\ncreate_symref() and make sure the exit code was propagated. So something\nlike this would work:\n\n  $ git symbolic-ref refs/heads refs/heads/foo\n  error: unable to write symref for refs/heads: Is a directory\n\n(note that you get a different error message if the refs are packed,\nsince there we can notice the d/f conflict manually).\n\nThere may be other ways to stimulate a failure. I thought \"symbolic-ref\nHEAD refs/heads/.invalid\" might work, but sadly the refname format check\nhappens earlier.\n\nI think it is worth avoiding the fifo magic if we can. It's complicated,\nand it means that not all platforms run the test.\n\n-Peff\n"},{"id":"486602","messageId":"20240111072828.GD48154@coredump.intra.peff.net","threadId":"60720","inReplyTo":"11fd5091d61b54d8862ab2e316bbd25fff63ce0f.1704912750.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] t5541: generalize reference locking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-11T07:28:28Z","receivedAt":"2024-01-11T07:28:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:\n\n> From: Justin Tobler <jltobler@gmail.com>\n> \n> Some tests set up reference locks by directly creating the lockfile.\n> While this works for the files reference backend, reftable reference\n> locks operate differently and are incompatible with this approach.\n> Generalize reference locking by preparing a reference transaction.\n\nAs with the first patch, I think we could use d/f conflicts to get the\nsame effect. Perhaps something like this:\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex df758e187d..7eb0e887e1 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -233,7 +233,8 @@ test_expect_success 'push --atomic fails on server-side errors' '\n \tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n \n \t# break ref updates for other on the remote site\n-\tmkdir \"$d/refs/heads/other.lock\" &&\n+\tgit -C \"$d\" update-ref -d refs/heads/other &&\n+\tgit -C \"$d\" update-ref refs/heads/other/block-me HEAD &&\n \n \t# add the new commit to other\n \tgit branch -f other collateral &&\n@@ -244,12 +245,8 @@ test_expect_success 'push --atomic fails on server-side errors' '\n \t# the new branch should not have been created upstream\n \ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n \n-\t# upstream should still reflect atomic2, the last thing we pushed\n-\t# successfully\n-\tgit rev-parse atomic2 >expected &&\n-\t# ...to other.\n-\tgit -C \"$d\" rev-parse refs/heads/other >actual &&\n-\ttest_cmp expected actual &&\n+\t# upstream should not have updated, as it cannot be written at all.\n+\ttest_must_fail git -C \"$d\" rev-parse --verify refs/heads/other &&\n \n \t# the new branch should not have been created upstream\n \ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n\nI do think that the original was slightly more interesting (since we\ncould check that \"other\" still existed but was not updated), but I think\nthe main point of the test is that \"atomic\" was not pushed at all.\n\n-Peff\n"},{"id":"486620","messageId":"ZZ_MPK2huH2j6CGd@tanuki","threadId":"60720","inReplyTo":"20240111071329.GC48154@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t1401: generalize reference locking","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-11T11:08:44Z","receivedAt":"2024-01-11T11:08:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 11, 2024 at 02:13:29AM -0500, Jeff King wrote:\n> On Wed, Jan 10, 2024 at 06:52:29PM +0000, Justin Tobler via GitGitGadget wrote:\n> \n> > From: Justin Tobler <jltobler@gmail.com>\n> > \n> > Some tests set up reference locks by directly creating the lockfile.\n> > While this works for the files reference backend, reftable reference\n> > locks operate differently and are incompatible with this approach.\n> > Refactor the test to use git-update-ref(1) to lock refs instead so that\n> > the test does not need to be aware of how the ref backend locks refs.\n> \n> It looks like you re-create this situation in a backend-agnostic way by\n> having two simultaneous updates that conflict on the lock (but don't\n> care how that lock is implemented).\n> \n> That works, but I think we could keep it simple. This test doesn't care\n> about the exact error condition we create. The point was just to die in\n> create_symref() and make sure the exit code was propagated. So something\n> like this would work:\n> \n>   $ git symbolic-ref refs/heads refs/heads/foo\n>   error: unable to write symref for refs/heads: Is a directory\n> \n> (note that you get a different error message if the refs are packed,\n> since there we can notice the d/f conflict manually).\n\nIf all we care for is the exit code then this would work for the\nreftable backend, too:\n\n```\n$ git init --ref-format=reftable repo\nInitialized empty Git repository in /tmp/repo/.git/\n$ cd repo/\n$ git commit --allow-empty --message message\n[main (root-commit) c2512d3] x\n$ git symbolic-ref refs/heads refs/heads/foo\n$ echo $?\n1\n```\n\nA bit unfortunate that there is no proper error message in that case,\nbut that is a different topic.\n\nPatrick\n\n> There may be other ways to stimulate a failure. I thought \"symbolic-ref\n> HEAD refs/heads/.invalid\" might work, but sadly the refname format check\n> happens earlier.\n> \n> I think it is worth avoiding the fifo magic if we can. It's complicated,\n> and it means that not all platforms run the test.\n> \n> -Peff\n> \n"},{"id":"486642","messageId":"xmqqa5pbhfkw.fsf@gitster.g","threadId":"60720","inReplyTo":"20240111072828.GD48154@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] t5541: generalize reference locking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-11T18:47:59Z","receivedAt":"2024-01-11T18:48:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:\n>\n>> From: Justin Tobler <jltobler@gmail.com>\n>> \n>> Some tests set up reference locks by directly creating the lockfile.\n>> While this works for the files reference backend, reftable reference\n>> locks operate differently and are incompatible with this approach.\n>> Generalize reference locking by preparing a reference transaction.\n>\n> As with the first patch, I think we could use d/f conflicts to get the\n> same effect. Perhaps something like this:\n\nThanks for a great alternative.  I agree that avoiding fifo indeed\nis a better way to go.\n"},{"id":"486649","messageId":"CAGAWz+6MEbxeWyPk5aw7jfEve0tYjvfPRXpDCqsw6pe-nGFPjA@mail.gmail.com","threadId":"60720","inReplyTo":"20240111071329.GC48154@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t1401: generalize reference locking","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-01-11T20:19:21Z","receivedAt":"2024-01-11T20:19:54Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It looks like you re-create this situation in a backend-agnostic way by\n> having two simultaneous updates that conflict on the lock (but don't\n> care how that lock is implemented).\n>\n> That works, but I think we could keep it simple. This test doesn't care\n> about the exact error condition we create. The point was just to die in\n> create_symref() and make sure the exit code was propagated.\n\nAs you mentioned, the original intent was to recreate the same error\ncondition in a reference backend agnostic manner. Since the test doesn't\ncare about the exact error condition being used, I agree that creating a\nd/f conflict instead is a much simpler and better approach. In the next\npatch version I've updated the test in t1401 to use git-symbolic-ref(1)\nto produce a d/f error.\n\nThanks,\nJustin\n\n\nOn Thu, Jan 11, 2024 at 1:13 AM Jeff King <peff@peff.net> wrote:\n>\n> On Wed, Jan 10, 2024 at 06:52:29PM +0000, Justin Tobler via GitGitGadget wrote:\n>\n> > From: Justin Tobler <jltobler@gmail.com>\n> >\n> > Some tests set up reference locks by directly creating the lockfile.\n> > While this works for the files reference backend, reftable reference\n> > locks operate differently and are incompatible with this approach.\n> > Refactor the test to use git-update-ref(1) to lock refs instead so that\n> > the test does not need to be aware of how the ref backend locks refs.\n>\n> It looks like you re-create this situation in a backend-agnostic way by\n> having two simultaneous updates that conflict on the lock (but don't\n> care how that lock is implemented).\n>\n> That works, but I think we could keep it simple. This test doesn't care\n> about the exact error condition we create. The point was just to die in\n> create_symref() and make sure the exit code was propagated. So something\n> like this would work:\n>\n>   $ git symbolic-ref refs/heads refs/heads/foo\n>   error: unable to write symref for refs/heads: Is a directory\n>\n> (note that you get a different error message if the refs are packed,\n> since there we can notice the d/f conflict manually).\n>\n> There may be other ways to stimulate a failure. I thought \"symbolic-ref\n> HEAD refs/heads/.invalid\" might work, but sadly the refname format check\n> happens earlier.\n>\n> I think it is worth avoiding the fifo magic if we can. It's complicated,\n> and it means that not all platforms run the test.\n>\n> -Peff\n"},{"id":"486650","messageId":"CAGAWz+4s0YFMfMpAr-6ugoKgFKt7mjVELpMcCEOU+0nO=DOaww@mail.gmail.com","threadId":"60720","inReplyTo":"xmqqa5pbhfkw.fsf@gitster.g","subject":"Re: [PATCH 2/2] t5541: generalize reference locking","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-01-11T20:20:01Z","receivedAt":"2024-01-11T20:20:34Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:\n>>\n>>> From: Justin Tobler <jltobler@gmail.com>\n>>>\n>>> Some tests set up reference locks by directly creating the lockfile.\n>>> While this works for the files reference backend, reftable reference\n>>> locks operate differently and are incompatible with this approach.\n>>> Generalize reference locking by preparing a reference transaction.\n>>\n>> As with the first patch, I think we could use d/f conflicts to get the\n>> same effect. Perhaps something like this:\n>\n> Thanks for a great alternative.  I agree that avoiding fifo indeed\n> is a better way to go.\n\nFor this patch, in the next version, I have also followed Peff's\nsuggestion to create d/f conflicts to trigger an error condition instead\nof using fifos.\n\nThanks to everyone for the feedback!\nJustin\n\n\nOn Thu, Jan 11, 2024 at 12:48 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Jeff King <peff@peff.net> writes:\n>\n> > On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:\n> >\n> >> From: Justin Tobler <jltobler@gmail.com>\n> >>\n> >> Some tests set up reference locks by directly creating the lockfile.\n> >> While this works for the files reference backend, reftable reference\n> >> locks operate differently and are incompatible with this approach.\n> >> Generalize reference locking by preparing a reference transaction.\n> >\n> > As with the first patch, I think we could use d/f conflicts to get the\n> > same effect. Perhaps something like this:\n>\n> Thanks for a great alternative.  I agree that avoiding fifo indeed\n> is a better way to go.\n"},{"id":"486651","messageId":"CAGAWz+5d+r97Nq_CB5BFnujo_K9bhCArPuLT2o8XNXvsJyETPQ@mail.gmail.com","threadId":"60720","inReplyTo":"xmqqv8807ll1.fsf@gitster.g","subject":"Re: [PATCH 0/2] Generalize reference locking in tests","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-01-11T20:20:14Z","receivedAt":"2024-01-11T20:20:47Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> The use of two fifos and avoiding deadlocking parent and child is\n> tricky enough that it does feel that it warrants a helper function\n> to do the common part of what these two patches add.\n\nTo avoid the trickiness that comes with introducing fifos to these\ntests, in the next patch version I've followed the suggestion of Peff to\ninstead create d/f conflicts to create an error condition. It appears\nthat these tests are not particular to the exact error condition being\ntriggered and therefore d/f conflicts are much simpler to produce.\n\nThanks,\nJustin\n\nOn Wed, Jan 10, 2024 at 6:36 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Justin Tobler via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > This approach is more verbose and may warrant consideration of implementing\n> > an abstraction to further simplify this operation. There appears to be one\n> > other existing test in t1400 that also follows this pattern. Being that\n> > there is only a handful of affected tests the implemented approach may be\n> > sufficient as is.\n>\n> The use of two fifos and avoiding deadlocking parent and child is\n> tricky enough that it does feel that it warrants a helper function\n> to do the common part of what these two patches add.\n>\n> I think I read t1401 patch carefully enough to understand what is\n> going on, but I cannot yet claim the same for the other one.\n>\n> Thanks.\n>\n> > Justin Tobler (2):\n> >   t1401: generalize reference locking\n> >   t5541: generalize reference locking\n> >\n> >  t/t1401-symbolic-ref.sh    | 28 ++++++++++++++++++++++++----\n> >  t/t5541-http-push-smart.sh | 25 +++++++++++++++++++++++--\n> >  2 files changed, 47 insertions(+), 6 deletions(-)\n> >\n> >\n> > base-commit: 624eb90fa8f65a79396615f3c2842ac5a3743350\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1634%2Fjltobler%2Fjt%2Fbackend-generic-ref-lock-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1634/jltobler/jt/backend-generic-ref-lock-v1\n> > Pull-Request: https://github.com/gitgitgadget/git/pull/1634\n"},{"id":"486652","messageId":"pull.1634.v2.git.1705004670.gitgitgadget@gmail.com","threadId":"60720","inReplyTo":"pull.1634.git.1704912750.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Generalize reference locking in tests","fromName":"Justin Tobler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-11T20:24:28Z","receivedAt":"2024-01-11T20:24:34Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Hello again,\n\nThis is the second version my patch series that refactors two tests to no\nlonger be dependent on the files reference backend. Changes compared to v1:\n\n * Removed reliance on fifo magic to trigger error conditions.\n * d/f conflicts now used to create errors instead.\n\nThanks for taking a look!\n\nJustin\n\nJustin Tobler (2):\n  t1401: remove lockfile creation\n  t5541: remove lockfile creation\n\n t/t1401-symbolic-ref.sh    |  5 ++---\n t/t5541-http-push-smart.sh | 18 +++++-------------\n 2 files changed, 7 insertions(+), 16 deletions(-)\n\n\nbase-commit: 624eb90fa8f65a79396615f3c2842ac5a3743350\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1634%2Fjltobler%2Fjt%2Fbackend-generic-ref-lock-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1634/jltobler/jt/backend-generic-ref-lock-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1634\n\nRange-diff vs v1:\n\n 1:  cb78b549e5e ! 1:  714f713ccec t1401: generalize reference locking\n     @@ Metadata\n      Author: Justin Tobler <jltobler@gmail.com>\n      \n       ## Commit message ##\n     -    t1401: generalize reference locking\n     +    t1401: remove lockfile creation\n      \n     -    Some tests set up reference locks by directly creating the lockfile.\n     -    While this works for the files reference backend, reftable reference\n     -    locks operate differently and are incompatible with this approach.\n     -    Refactor the test to use git-update-ref(1) to lock refs instead so that\n     -    the test does not need to be aware of how the ref backend locks refs.\n     +    To create error conditions, some tests set up reference locks by\n     +    directly creating its lockfile. While this works for the files reference\n     +    backend, this approach is incompatible with the reftable backend.\n     +    Refactor the test to create a d/f conflict via git-symbolic-ref(1)\n     +    instead so that the test is reference backend agnostic.\n      \n          Signed-off-by: Justin Tobler <jltobler@gmail.com>\n      \n       ## t/t1401-symbolic-ref.sh ##\n      @@ t/t1401-symbolic-ref.sh: test_expect_success LONG_REF 'we can parse long symbolic ref' '\n     - \ttest_cmp expect actual\n       '\n       \n     --test_expect_success 'symbolic-ref reports failure in exit code' '\n     + test_expect_success 'symbolic-ref reports failure in exit code' '\n      -\ttest_when_finished \"rm -f .git/HEAD.lock\" &&\n      -\t>.git/HEAD.lock &&\n      -\ttest_must_fail git symbolic-ref HEAD refs/heads/whatever\n     -+test_expect_success PIPE 'symbolic-ref reports failure in exit code' '\n     -+\tmkfifo in out &&\n     -+\t(git update-ref --stdin <in >out &) &&\n     -+\n     -+\texec 9>in &&\n     -+\texec 8<out &&\n     -+\ttest_when_finished \"exec 9>&-\" &&\n     -+\ttest_when_finished \"exec 8<&-\" &&\n     -+\n     -+\techo \"start\" >&9 &&\n     -+\techo \"start: ok\" >expected &&\n     -+\tread line <&8 &&\n     -+\techo \"$line\" >actual &&\n     -+\ttest_cmp expected actual &&\n     -+\n     -+\techo \"update HEAD refs/heads/foo\" >&9 &&\n     -+\n     -+\techo \"prepare\" >&9 &&\n     -+\techo \"prepare: ok\" >expected &&\n     -+\tread line <&8 &&\n     -+\techo \"$line\" >actual &&\n     -+\ttest_cmp expected actual &&\n     -+\n     -+\ttest_must_fail git symbolic-ref HEAD refs/heads/bar\n     ++\t# Create d/f conflict to simulate failure.\n     ++\ttest_must_fail git symbolic-ref refs/heads refs/heads/foo\n       '\n       \n       test_expect_success 'symbolic-ref writes reflog entry' '\n 2:  11fd5091d61 ! 2:  f953a668c6a t5541: generalize reference locking\n     @@ Metadata\n      Author: Justin Tobler <jltobler@gmail.com>\n      \n       ## Commit message ##\n     -    t5541: generalize reference locking\n     +    t5541: remove lockfile creation\n      \n     -    Some tests set up reference locks by directly creating the lockfile.\n     -    While this works for the files reference backend, reftable reference\n     -    locks operate differently and are incompatible with this approach.\n     -    Generalize reference locking by preparing a reference transaction.\n     +    To create error conditions, some tests set up reference locks by\n     +    directly creating its lockfile. While this works for the files reference\n     +    backend, this approach is incompatible with the reftable backend.\n     +    Refactor the test to create a d/f conflict via git-update-ref(1) instead\n     +    so that the test is reference backend agnostic.\n      \n          Signed-off-by: Justin Tobler <jltobler@gmail.com>\n      \n     @@ t/t5541-http-push-smart.sh: test_expect_success 'push --atomic fails on server-s\n       \n      -\t# break ref updates for other on the remote site\n      -\tmkdir \"$d/refs/heads/other.lock\" &&\n     -+\tmkfifo in out &&\n     -+\t(git -C \"$d\" update-ref --stdin <in >out &) &&\n     -+\n     -+\texec 9>in &&\n     -+\texec 8<out &&\n     -+\ttest_when_finished \"exec 9>&-\" &&\n     -+\ttest_when_finished \"exec 8<&-\" &&\n     -+\n     -+\techo \"start\" >&9 &&\n     -+\techo \"start: ok\" >expected &&\n     -+\tread line <&8 &&\n     -+\techo \"$line\" >actual &&\n     -+\ttest_cmp expected actual &&\n     -+\n     -+\techo \"update refs/heads/other refs/heads/other\" >&9 &&\n     -+\n     -+\t# Prepare reference transaction on `other` reference to lock it and thus\n     -+\t# break updates on the remote.\n     -+\techo \"prepare\" >&9 &&\n     -+\techo \"prepare: ok\" >expected &&\n     -+\tread line <&8 &&\n     -+\techo \"$line\" >actual &&\n     -+\ttest_cmp expected actual &&\n     ++\t# Create d/f conflict to break ref updates for other on the remote site.\n     ++\tgit -C \"$d\" update-ref -d refs/heads/other &&\n     ++\tgit -C \"$d\" update-ref refs/heads/other/conflict HEAD &&\n       \n       \t# add the new commit to other\n       \tgit branch -f other collateral &&\n     +@@ t/t5541-http-push-smart.sh: test_expect_success 'push --atomic fails on server-side errors' '\n     + \t# --atomic should cause entire push to be rejected\n     + \ttest_must_fail git push --atomic \"$up\" atomic other 2>output  &&\n     + \n     +-\t# the new branch should not have been created upstream\n     +-\ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n     +-\n     +-\t# upstream should still reflect atomic2, the last thing we pushed\n     +-\t# successfully\n     +-\tgit rev-parse atomic2 >expected &&\n     +-\t# ...to other.\n     +-\tgit -C \"$d\" rev-parse refs/heads/other >actual &&\n     +-\ttest_cmp expected actual &&\n     +-\n     +-\t# the new branch should not have been created upstream\n     ++\t# The atomic and other branches should be created upstream.\n     + \ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n     ++\ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/other &&\n     + \n     + \t# the failed refs should be indicated to the user\n     + \tgrep \"^ ! .*rejected.* other -> other .*atomic transaction failed\" output &&\n\n-- \ngitgitgadget\n"},{"id":"486653","messageId":"714f713ccece4a9dd167b3b73a71c068dbe91f53.1705004670.git.gitgitgadget@gmail.com","threadId":"60720","inReplyTo":"pull.1634.v2.git.1705004670.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] t1401: remove lockfile creation","fromName":"Justin Tobler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-11T20:24:29Z","receivedAt":"2024-01-11T20:24:34Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"From: Justin Tobler <jltobler@gmail.com>\n\nTo create error conditions, some tests set up reference locks by\ndirectly creating its lockfile. While this works for the files reference\nbackend, this approach is incompatible with the reftable backend.\nRefactor the test to create a d/f conflict via git-symbolic-ref(1)\ninstead so that the test is reference backend agnostic.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n t/t1401-symbolic-ref.sh | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex c7745e1bf69..c67e5c134d9 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -106,9 +106,8 @@ test_expect_success LONG_REF 'we can parse long symbolic ref' '\n '\n \n test_expect_success 'symbolic-ref reports failure in exit code' '\n-\ttest_when_finished \"rm -f .git/HEAD.lock\" &&\n-\t>.git/HEAD.lock &&\n-\ttest_must_fail git symbolic-ref HEAD refs/heads/whatever\n+\t# Create d/f conflict to simulate failure.\n+\ttest_must_fail git symbolic-ref refs/heads refs/heads/foo\n '\n \n test_expect_success 'symbolic-ref writes reflog entry' '\n-- \ngitgitgadget\n\n"},{"id":"486654","messageId":"f953a668c6a7e0a57adcee77ceee2d578970065e.1705004670.git.gitgitgadget@gmail.com","threadId":"60720","inReplyTo":"pull.1634.v2.git.1705004670.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] t5541: remove lockfile creation","fromName":"Justin Tobler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-11T20:24:30Z","receivedAt":"2024-01-11T20:24:36Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"From: Justin Tobler <jltobler@gmail.com>\n\nTo create error conditions, some tests set up reference locks by\ndirectly creating its lockfile. While this works for the files reference\nbackend, this approach is incompatible with the reftable backend.\nRefactor the test to create a d/f conflict via git-update-ref(1) instead\nso that the test is reference backend agnostic.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\n t/t5541-http-push-smart.sh | 18 +++++-------------\n 1 file changed, 5 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex df758e187df..9a8bed6c32b 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -232,8 +232,9 @@ test_expect_success 'push --atomic fails on server-side errors' '\n \ttest_config -C \"$d\" http.receivepack true &&\n \tup=\"$HTTPD_URL\"/smart/atomic-branches.git &&\n \n-\t# break ref updates for other on the remote site\n-\tmkdir \"$d/refs/heads/other.lock\" &&\n+\t# Create d/f conflict to break ref updates for other on the remote site.\n+\tgit -C \"$d\" update-ref -d refs/heads/other &&\n+\tgit -C \"$d\" update-ref refs/heads/other/conflict HEAD &&\n \n \t# add the new commit to other\n \tgit branch -f other collateral &&\n@@ -241,18 +242,9 @@ test_expect_success 'push --atomic fails on server-side errors' '\n \t# --atomic should cause entire push to be rejected\n \ttest_must_fail git push --atomic \"$up\" atomic other 2>output  &&\n \n-\t# the new branch should not have been created upstream\n-\ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n-\n-\t# upstream should still reflect atomic2, the last thing we pushed\n-\t# successfully\n-\tgit rev-parse atomic2 >expected &&\n-\t# ...to other.\n-\tgit -C \"$d\" rev-parse refs/heads/other >actual &&\n-\ttest_cmp expected actual &&\n-\n-\t# the new branch should not have been created upstream\n+\t# The atomic and other branches should be created upstream.\n \ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n+\ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/other &&\n \n \t# the failed refs should be indicated to the user\n \tgrep \"^ ! .*rejected.* other -> other .*atomic transaction failed\" output &&\n-- \ngitgitgadget\n"},{"id":"486687","messageId":"20240112070142.GD618729@coredump.intra.peff.net","threadId":"60720","inReplyTo":"ZZ_MPK2huH2j6CGd@tanuki","subject":"Re: [PATCH 1/2] t1401: generalize reference locking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-12T07:01:42Z","receivedAt":"2024-01-12T07:01:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 11, 2024 at 12:08:44PM +0100, Patrick Steinhardt wrote:\n\n> > (note that you get a different error message if the refs are packed,\n> > since there we can notice the d/f conflict manually).\n> \n> If all we care for is the exit code then this would work for the\n> reftable backend, too:\n> \n> ```\n> $ git init --ref-format=reftable repo\n> Initialized empty Git repository in /tmp/repo/.git/\n> $ cd repo/\n> $ git commit --allow-empty --message message\n> [main (root-commit) c2512d3] x\n> $ git symbolic-ref refs/heads refs/heads/foo\n> $ echo $?\n> 1\n> ```\n\nYep, exactly. That should work for both and cover what the test was\noriginally trying to do.\n\n> A bit unfortunate that there is no proper error message in that case,\n> but that is a different topic.\n\nYeah, I would call that an outright bug. It does not have to be part of\nthis patch, but is worth fixing (and testing). I suspect it's not going\nto be the only place with this problem. Most of the files-backend ref\ncode is very happy to spew to stdout using error(), but the reftable\ncode, having been written from a more lib-conscious perspective,\nprobably doesn't.\n\nThe obvious quick fix is to sprinkle more error() into the reftable\ncode. But in the longer term, I think the right direction is that the\nref code should accept an error strbuf or similar mechanism to propagate\nhuman-readable error test to the caller.\n\n-Peff\n"},{"id":"486688","messageId":"20240112070356.GE618729@coredump.intra.peff.net","threadId":"60720","inReplyTo":"f953a668c6a7e0a57adcee77ceee2d578970065e.1705004670.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] t5541: remove lockfile creation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-12T07:03:56Z","receivedAt":"2024-01-12T07:03:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 11, 2024 at 08:24:30PM +0000, Justin Tobler via GitGitGadget wrote:\n\n> -\t# the new branch should not have been created upstream\n> -\ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n> -\n> -\t# upstream should still reflect atomic2, the last thing we pushed\n> -\t# successfully\n> -\tgit rev-parse atomic2 >expected &&\n> -\t# ...to other.\n> -\tgit -C \"$d\" rev-parse refs/heads/other >actual &&\n> -\ttest_cmp expected actual &&\n> -\n> -\t# the new branch should not have been created upstream\n> +\t# The atomic and other branches should be created upstream.\n>  \ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n> +\ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/other &&\n\nThis last comment should say \"should not be created\", I think?\n\nOther than that, both patches look good to me.\n\n-Peff\n"},{"id":"486695","messageId":"ZaDuEufXOnwH7hT4@framework","threadId":"60720","inReplyTo":"20240112070142.GD618729@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t1401: generalize reference locking","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-12T07:45:22Z","receivedAt":"2024-01-12T07:45:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 12, 2024 at 02:01:42AM -0500, Jeff King wrote:\n> On Thu, Jan 11, 2024 at 12:08:44PM +0100, Patrick Steinhardt wrote:\n> \n> > > (note that you get a different error message if the refs are packed,\n> > > since there we can notice the d/f conflict manually).\n> > \n> > If all we care for is the exit code then this would work for the\n> > reftable backend, too:\n> > \n> > ```\n> > $ git init --ref-format=reftable repo\n> > Initialized empty Git repository in /tmp/repo/.git/\n> > $ cd repo/\n> > $ git commit --allow-empty --message message\n> > [main (root-commit) c2512d3] x\n> > $ git symbolic-ref refs/heads refs/heads/foo\n> > $ echo $?\n> > 1\n> > ```\n> \n> Yep, exactly. That should work for both and cover what the test was\n> originally trying to do.\n> \n> > A bit unfortunate that there is no proper error message in that case,\n> > but that is a different topic.\n> \n> Yeah, I would call that an outright bug. It does not have to be part of\n> this patch, but is worth fixing (and testing). I suspect it's not going\n> to be the only place with this problem. Most of the files-backend ref\n> code is very happy to spew to stdout using error(), but the reftable\n> code, having been written from a more lib-conscious perspective,\n> probably doesn't.\n\nYup, I've already fixed this bug in the reftable backend.\n\n> The obvious quick fix is to sprinkle more error() into the reftable\n> code. But in the longer term, I think the right direction is that the\n> ref code should accept an error strbuf or similar mechanism to propagate\n> human-readable error test to the caller.\n\nAgreed, I think it's good that the reftable library itself does not\nprint error messages. In this particular case we are already able to\nprovide a proper error message due to the error code that the library\nreturns. But there are certainly going to be other cases where it might\nmake sense to pass in an error strbuf.\n\nPatrick\n"},{"id":"486698","messageId":"20240112080318.GA620715@coredump.intra.peff.net","threadId":"60720","inReplyTo":"ZaDuEufXOnwH7hT4@framework","subject":"Re: [PATCH 1/2] t1401: generalize reference locking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-12T08:03:18Z","receivedAt":"2024-01-12T08:03:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 12, 2024 at 08:45:22AM +0100, Patrick Steinhardt wrote:\n\n> > The obvious quick fix is to sprinkle more error() into the reftable\n> > code. But in the longer term, I think the right direction is that the\n> > ref code should accept an error strbuf or similar mechanism to propagate\n> > human-readable error test to the caller.\n> \n> Agreed, I think it's good that the reftable library itself does not\n> print error messages. In this particular case we are already able to\n> provide a proper error message due to the error code that the library\n> returns. But there are certainly going to be other cases where it might\n> make sense to pass in an error strbuf.\n\nOh, if there is an error code you can use already, that is even better. :)\n\nThanks for taking care of this case.\n\n-Peff\n"},{"id":"486718","messageId":"xmqqo7dqbfin.fsf@gitster.g","threadId":"60720","inReplyTo":"20240112070356.GE618729@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] t5541: remove lockfile creation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-12T17:58:08Z","receivedAt":"2024-01-12T17:58:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Jan 11, 2024 at 08:24:30PM +0000, Justin Tobler via GitGitGadget wrote:\n>\n>> -\t# the new branch should not have been created upstream\n>> -\ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n>> -\n>> -\t# upstream should still reflect atomic2, the last thing we pushed\n>> -\t# successfully\n>> -\tgit rev-parse atomic2 >expected &&\n>> -\t# ...to other.\n>> -\tgit -C \"$d\" rev-parse refs/heads/other >actual &&\n>> -\ttest_cmp expected actual &&\n>> -\n>> -\t# the new branch should not have been created upstream\n>> +\t# The atomic and other branches should be created upstream.\n>>  \ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n>> +\ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/other &&\n>\n> This last comment should say \"should not be created\", I think?\n>\n> Other than that, both patches look good to me.\n\nThanks.  Will queue with the following and \"Acked-by: peff\".\n\ndiff --git c/t/t5541-http-push-smart.sh w/t/t5541-http-push-smart.sh\nindex 9a8bed6c32..71428f3d5c 100755\n--- c/t/t5541-http-push-smart.sh\n+++ w/t/t5541-http-push-smart.sh\n@@ -242,7 +242,7 @@ test_expect_success 'push --atomic fails on server-side errors' '\n \t# --atomic should cause entire push to be rejected\n \ttest_must_fail git push --atomic \"$up\" atomic other 2>output  &&\n \n-\t# The atomic and other branches should be created upstream.\n+\t# The atomic and other branches should not be created upstream.\n \ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n \ttest_must_fail git -C \"$d\" show-ref --verify refs/heads/other &&\n \n\n"},{"id":"486768","messageId":"CAGAWz+6yKzFz5bQc6OHbd+=xGsjzgEb7nkpDkht+Y-OGYVyYuw@mail.gmail.com","threadId":"60720","inReplyTo":"xmqqo7dqbfin.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] t5541: remove lockfile creation","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-01-13T22:25:56Z","receivedAt":"2024-01-13T22:26:08Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Great! Thanks everyone for the help!\n-Justin\n\nOn Fri, Jan 12, 2024 at 11:58 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Jeff King <peff@peff.net> writes:\n>\n> > On Thu, Jan 11, 2024 at 08:24:30PM +0000, Justin Tobler via GitGitGadget wrote:\n> >\n> >> -    # the new branch should not have been created upstream\n> >> -    test_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n> >> -\n> >> -    # upstream should still reflect atomic2, the last thing we pushed\n> >> -    # successfully\n> >> -    git rev-parse atomic2 >expected &&\n> >> -    # ...to other.\n> >> -    git -C \"$d\" rev-parse refs/heads/other >actual &&\n> >> -    test_cmp expected actual &&\n> >> -\n> >> -    # the new branch should not have been created upstream\n> >> +    # The atomic and other branches should be created upstream.\n> >>      test_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n> >> +    test_must_fail git -C \"$d\" show-ref --verify refs/heads/other &&\n> >\n> > This last comment should say \"should not be created\", I think?\n> >\n> > Other than that, both patches look good to me.\n>\n> Thanks.  Will queue with the following and \"Acked-by: peff\".\n>\n> diff --git c/t/t5541-http-push-smart.sh w/t/t5541-http-push-smart.sh\n> index 9a8bed6c32..71428f3d5c 100755\n> --- c/t/t5541-http-push-smart.sh\n> +++ w/t/t5541-http-push-smart.sh\n> @@ -242,7 +242,7 @@ test_expect_success 'push --atomic fails on server-side errors' '\n>         # --atomic should cause entire push to be rejected\n>         test_must_fail git push --atomic \"$up\" atomic other 2>output  &&\n>\n> -       # The atomic and other branches should be created upstream.\n> +       # The atomic and other branches should not be created upstream.\n>         test_must_fail git -C \"$d\" show-ref --verify refs/heads/atomic &&\n>         test_must_fail git -C \"$d\" show-ref --verify refs/heads/other &&\n>\n>\n"}]}