{"thread":{"id":"57501","subject":"[PATCH] t5503: simplify setup of test which exercises failure of backfill","startedAt":"2022-03-03T07:20:12Z","lastAt":"2022-03-03T10:34:46Z","messageCount":2,"participants":["Patrick Steinhardt","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"450249","messageId":"cc7de043de67bb8243c764ef6764b07170b01929.1646291929.git.ps@pks.im","threadId":"57501","inReplyTo":null,"subject":"[PATCH] t5503: simplify setup of test which exercises failure of backfill","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-03-03T07:20:05Z","receivedAt":"2022-03-03T07:20:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the testcase to exercise backfilling of tags for fetches we evoke a\nfailure of the backfilling mechanism by creating a reference that later\non causes a D/F conflict. Because the assumption was that git-fetch(1)\nwould notice the D/F conflict early on this conflicting reference was\ncreated via the reference-transaction hook just when we were about to\nwrite the backfilled tag. As it turns out though this is not the case,\nand the fetch fails in the same way when we create the conflicting ref\nup front.\n\nSimplify the test setup creating the reference up front, which allows us\nto get rid of the hook script.\n\nSigned-off-by: Patrick Steinhardt \n---\n\nThis simplifies the test setup of t5503 as discussed in [1]. The patch\napplies on top of Junio's ps/fetch-atomic (583bc41923 (fetch: make\n`--atomic` flag cover pruning of refs, 2022-02-17)).\n\nPatrick\n\n t/t5503-tagfollow.sh | 20 +++++---------------\n 1 file changed, 5 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\nindex e72fdc2534..a3c01014b7 100755\n--- a/t/t5503-tagfollow.sh\n+++ b/t/t5503-tagfollow.sh\n@@ -212,21 +212,11 @@ test_expect_success 'atomic fetch with backfill should use single transaction' '\n test_expect_success 'backfill failure causes command to fail' '\n \tgit init clone5 &&\n \n-\twrite_script clone5/.git/hooks/reference-transaction <<-EOF &&\n-\t\twhile read oldrev newrev reference\n-\t\tdo\n-\t\t\tif test \"\\$reference\" = refs/tags/tag1\n-\t\t\tthen\n-\t\t\t\t# Create a nested tag below the actual tag we\n-\t\t\t\t# wanted to write, which causes a D/F conflict\n-\t\t\t\t# later when we want to commit refs/tags/tag1.\n-\t\t\t\t# We cannot just `exit 1` here given that this\n-\t\t\t\t# would cause us to die immediately.\n-\t\t\t\tgit update-ref refs/tags/tag1/nested $B\n-\t\t\t\texit \\$!\n-\t\t\tfi\n-\t\tdone\n-\tEOF\n+\t# Create a tag that is nested below the tag we are about to fetch via\n+\t# the backfill mechanism. This causes a D/F conflict when backfilling\n+\t# and should thus cause the command to fail.\n+\tempty_blob=$(git -C clone5 hash-object -w --stdin [[PGP Signed Part:Undecided]]\n> In the testcase to exercise backfilling of tags for fetches we evoke a\n> failure of the backfilling mechanism by creating a reference that later\n> on causes a D/F conflict. Because the assumption was that git-fetch(1)\n> would notice the D/F conflict early on this conflicting reference was\n> created via the reference-transaction hook just when we were about to\n> write the backfilled tag. As it turns out though this is not the case,\n> and the fetch fails in the same way when we create the conflicting ref\n> up front.\n>\n> Simplify the test setup creating the reference up front, which allows us\n> to get rid of the hook script.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n>\n> This simplifies the test setup of t5503 as discussed in [1]. The patch\n> applies on top of Junio's ps/fetch-atomic (583bc41923 (fetch: make\n> `--atomic` flag cover pruning of refs, 2022-02-17)).\n\nFWIW for something in \"next\" already the OID will be stable, so it's OK\n(and better) to mention 583bc419235 in the commit message itself.\n\n> Patrick\n>\n> t/t5503-tagfollow.sh | 20 +++++---------------\n> 1 file changed, 5 insertions(+), 15 deletions(-)\n>\n> diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\n> index e72fdc2534..a3c01014b7 100755\n> --- a/t/t5503-tagfollow.sh\n> +++ b/t/t5503-tagfollow.sh\n> @@ -212,21 +212,11 @@ test_expect_success 'atomic fetch with backfill should use single transaction' '\n> test_expect_success 'backfill failure causes command to fail' '\n> \tgit init clone5 &&\n> \n> -\twrite_script clone5/.git/hooks/reference-transaction <<-EOF &&\n> -\t\twhile read oldrev newrev reference\n> -\t\tdo\n> -\t\t\tif test \"\\$reference\" = refs/tags/tag1\n> -\t\t\tthen\n> -\t\t\t\t# Create a nested tag below the actual tag we\n> -\t\t\t\t# wanted to write, which causes a D/F conflict\n> -\t\t\t\t# later when we want to commit refs/tags/tag1.\n> -\t\t\t\t# We cannot just `exit 1` here given that this\n> -\t\t\t\t# would cause us to die immediately.\n> -\t\t\t\tgit update-ref refs/tags/tag1/nested $B\n> -\t\t\t\texit \\$!\n> -\t\t\tfi\n> -\t\tdone\n> -\tEOF\n> +\t# Create a tag that is nested below the tag we are about to fetch via\n> +\t# the backfill mechanism. This causes a D/F conflict when backfilling\n> +\t# and should thus cause the command to fail.\n> +\tempty_blob=$(git -C clone5 hash-object -w --stdin +\tgit -C clone5 update-ref refs/tags/tag1/nested $empty_blob &&\n\nThis looks better, but FWIW for the discussion about quoted\nv.s. unquoted here-doc in this case it's also OK for the pre-image (and\narguably better) to do (and any issues of $? v.s. $! etc. aside):\n\t\n\tdiff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh\n\tindex e72fdc25346..eebf0ddc4c2 100755\n\t--- a/t/t5503-tagfollow.sh\n\t+++ b/t/t5503-tagfollow.sh\n\t@@ -212,18 +212,18 @@ test_expect_success 'atomic fetch with backfill should use single transaction' '\n\t test_expect_success 'backfill failure causes command to fail' '\n\t \tgit init clone5 &&\n\t \n\t-\twrite_script clone5/.git/hooks/reference-transaction <<-EOF &&\n\t+\twrite_script clone5/.git/hooks/reference-transaction <<-\\EOF &&\n\t \t\twhile read oldrev newrev reference\n\t \t\tdo\n\t-\t\t\tif test \"\\$reference\" = refs/tags/tag1\n\t+\t\t\tif test \"$reference\" = refs/tags/tag1\n\t \t\t\tthen\n\t \t\t\t\t# Create a nested tag below the actual tag we\n\t \t\t\t\t# wanted to write, which causes a D/F conflict\n\t \t\t\t\t# later when we want to commit refs/tags/tag1.\n\t \t\t\t\t# We cannot just `exit 1` here given that this\n\t \t\t\t\t# would cause us to die immediately.\n\t-\t\t\t\tgit update-ref refs/tags/tag1/nested $B\n\t-\t\t\t\texit \\$!\n\t+\t\t\t\tgit update-ref refs/tags/tag1/nested '$B'\n\t+\t\t\t\texit $!\n\t \t\t\tfi\n\t \t\tdone\n\t \tEOF\n\nI.e. to end the quote and interpolate $B. IMO also more readable as\ne.g. shell syntax highlighting will show that we're interpolating that\noutside-test $B.\n\n> \n> \ttest_must_fail git -C clone5 fetch .. $B:refs/heads/something &&\n> \ttest $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&\n\n(In the pre-image, but..) this way of using \"test\" will hide segfaults\netc. from the invoked git command.\n\nBetter to:\n\n echo $B >expect &&\n git ... >actual &&\n test_cmp expect actual\n\nOr better yet \"test_cmp_rev\" in this case, but all of that's in \"next\"\nalready, so purely optional (and also more food for the \"hiding exit\nstatus\" microproject) :)\n"}]}