| Message ID | e133c14f569116156bbd3e0ca4874e8eb54b76b8.1644565025.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch: improve atomicity of `--atomic` flag | expand |
On Fri, Feb 11, 2022 at 8:38 PM Patrick Steinhardt <ps@pks.im> wrote: > > The `--atomic` flag is missing test coverage for pruning of deleted > references and backfilling of tags, and of course both aren't covered > correctly by this flag. It's not clear to me what "both aren't covered correctly by this flag" actually means here. If it means that pruning of deleted references and backfilling of tags don't work correctly when --atomic is used, then it could be stated more clearly. Otherwise this seems to just be repeating the first part of the sentence. > Furthermore, we don't have tests demonstrating > error cases for backfilling tags. > > Add tests to cover those testing gaps. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > +test_expect_success 'atomic fetch with failing backfill' ' > + git init clone3 && > + > + # We want to test whether a failure when backfilling tags correctly > + # aborts the complete transaction when `--atomic` is passed: we should > + # neither create the branch nor should we create the tag when either > + # one of both fails to update correctly. > + # > + # To trigger failure we simply abort when backfilling a tag. > + write_script clone3/.git/hooks/reference-transaction <<-\EOF && > + #!/bin/sh > + > + while read oldrev newrev reference > + do > + if test "$reference" = refs/tags/tag1 > + then > + exit 1 > + fi > + done > + EOF > + > + test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something && > + > + # Creation of the tag has failed, so ideally refs/heads/something > + # should not exist. The fact that it does is demonstrates that there is s/The fact that it does is demonstrates/The fact that it does demonstrates/ > + # missing coverage in the `--atomic` flag. Maybe s/missing coverage/a bug/ would make things clearer. > + test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)" > +' As this patch series is about fixing buggy parts of the behavior with --atomic, I think it would make more sense to use test_expect_failure, instead of test_expect_success, in this test, and to check that we have the correct behavior, instead of checking that we have the buggy behavior. Of course when later in this patch series the buggy behavior is fixed, then test_expect_failure should be replaced with test_expect_success. > +test_expect_success 'atomic fetch with backfill should use single transaction' ' > + git init clone4 && > + > + # Fetching with the `--atomic` flag should update all references in a > + # single transaction, including backfilled tags. We thus expect to see > + # a single reference transaction for the created branch and tags. > + cat >expected <<-EOF && > + prepared > + $ZERO_OID $B refs/heads/something > + $ZERO_OID $S refs/tags/tag2 > + committed > + $ZERO_OID $B refs/heads/something > + $ZERO_OID $S refs/tags/tag2 > + prepared > + $ZERO_OID $T refs/tags/tag1 > + committed > + $ZERO_OID $T refs/tags/tag1 > + EOF The comment says that we expect to see a single reference transaction, but the expected file we create seems to show 2 transactions. So I think here too, we should use test_expect_failure, instead of test_expect_success, and check that we have the correct behavior instead of a buggy one. > + write_script clone4/.git/hooks/reference-transaction <<-\EOF && Here there is no #!/bin/sh while other uses of write_script in your patch have it. If it's not necessary, it could be removed in the other uses. > + ( echo "$*" && cat ) >>actual > + EOF > + > + git -C clone4 fetch --atomic .. $B:refs/heads/something && > + test_cmp expected clone4/actual > +' I took a quick look at the 2 other tests after this one, and I think test_expect_failure should be used there too, instead of test_expect_success.
On Tue, Feb 15, 2022 at 07:19:19AM +0100, Christian Couder wrote: > On Fri, Feb 11, 2022 at 8:38 PM Patrick Steinhardt <ps@pks.im> wrote: > > > > The `--atomic` flag is missing test coverage for pruning of deleted > > references and backfilling of tags, and of course both aren't covered > > correctly by this flag. > > It's not clear to me what "both aren't covered correctly by this flag" > actually means here. If it means that pruning of deleted references > and backfilling of tags don't work correctly when --atomic is used, > then it could be stated more clearly. Otherwise this seems to just be > repeating the first part of the sentence. Yeah, the commit message was a bit lazy to be honest. I've reworded it to hopefully make clearer what one is looking at. > > Furthermore, we don't have tests demonstrating > > error cases for backfilling tags. > > > > Add tests to cover those testing gaps. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > > +test_expect_success 'atomic fetch with failing backfill' ' > > + git init clone3 && > > + > > + # We want to test whether a failure when backfilling tags correctly > > + # aborts the complete transaction when `--atomic` is passed: we should > > + # neither create the branch nor should we create the tag when either > > + # one of both fails to update correctly. > > + # > > + # To trigger failure we simply abort when backfilling a tag. > > + write_script clone3/.git/hooks/reference-transaction <<-\EOF && > > + #!/bin/sh > > + > > + while read oldrev newrev reference > > + do > > + if test "$reference" = refs/tags/tag1 > > + then > > + exit 1 > > + fi > > + done > > + EOF > > + > > + test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something && > > + > > + # Creation of the tag has failed, so ideally refs/heads/something > > + # should not exist. The fact that it does is demonstrates that there is > > s/The fact that it does is demonstrates/The fact that it does demonstrates/ > > > + # missing coverage in the `--atomic` flag. > > Maybe s/missing coverage/a bug/ would make things clearer. > > > + test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)" > > +' > > As this patch series is about fixing buggy parts of the behavior with > --atomic, I think it would make more sense to use test_expect_failure, > instead of test_expect_success, in this test, and to check that we > have the correct behavior, instead of checking that we have the buggy > behavior. > > Of course when later in this patch series the buggy behavior is fixed, > then test_expect_failure should be replaced with test_expect_success. The downside of using `test_expect_failure` is that one cannot easily see what exactly is broken in the testcase. Yes, you can document it, but when using `test_expect_success` the huge advantage is that you can see exactly what behaviour is changing in subsequent commits by just having a look at the diff of the test which adapts it from its initially-broken state to the newly-fixed behaviour. > > +test_expect_success 'atomic fetch with backfill should use single transaction' ' > > + git init clone4 && > > + > > + # Fetching with the `--atomic` flag should update all references in a > > + # single transaction, including backfilled tags. We thus expect to see > > + # a single reference transaction for the created branch and tags. > > + cat >expected <<-EOF && > > + prepared > > + $ZERO_OID $B refs/heads/something > > + $ZERO_OID $S refs/tags/tag2 > > + committed > > + $ZERO_OID $B refs/heads/something > > + $ZERO_OID $S refs/tags/tag2 > > + prepared > > + $ZERO_OID $T refs/tags/tag1 > > + committed > > + $ZERO_OID $T refs/tags/tag1 > > + EOF > > The comment says that we expect to see a single reference transaction, > but the expected file we create seems to show 2 transactions. So I > think here too, we should use test_expect_failure, instead of > test_expect_success, and check that we have the correct behavior > instead of a buggy one. Same comment as above. I've also amended the commit message to say why we're introducing the tests like this. > > + write_script clone4/.git/hooks/reference-transaction <<-\EOF && > > Here there is no #!/bin/sh while other uses of write_script in your > patch have it. If it's not necessary, it could be removed in the other > uses. Good point, I always forget that the shebang is added automatically by this helper. > > + ( echo "$*" && cat ) >>actual > > + EOF > > + > > + git -C clone4 fetch --atomic .. $B:refs/heads/something && > > + test_cmp expected clone4/actual > > +' > > I took a quick look at the 2 other tests after this one, and I think > test_expect_failure should be used there too, instead of > test_expect_success. Patrick
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh index 195fc64dd4..888305ad4d 100755 --- a/t/t5503-tagfollow.sh +++ b/t/t5503-tagfollow.sh @@ -160,4 +160,89 @@ test_expect_success 'new clone fetch main and tags' ' test_cmp expect actual ' +test_expect_success 'atomic fetch with failing backfill' ' + git init clone3 && + + # We want to test whether a failure when backfilling tags correctly + # aborts the complete transaction when `--atomic` is passed: we should + # neither create the branch nor should we create the tag when either + # one of both fails to update correctly. + # + # To trigger failure we simply abort when backfilling a tag. + write_script clone3/.git/hooks/reference-transaction <<-\EOF && + #!/bin/sh + + while read oldrev newrev reference + do + if test "$reference" = refs/tags/tag1 + then + exit 1 + fi + done + EOF + + test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something && + + # Creation of the tag has failed, so ideally refs/heads/something + # should not exist. The fact that it does is demonstrates that there is + # missing coverage in the `--atomic` flag. + test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)" +' + +test_expect_success 'atomic fetch with backfill should use single transaction' ' + git init clone4 && + + # Fetching with the `--atomic` flag should update all references in a + # single transaction, including backfilled tags. We thus expect to see + # a single reference transaction for the created branch and tags. + cat >expected <<-EOF && + prepared + $ZERO_OID $B refs/heads/something + $ZERO_OID $S refs/tags/tag2 + committed + $ZERO_OID $B refs/heads/something + $ZERO_OID $S refs/tags/tag2 + prepared + $ZERO_OID $T refs/tags/tag1 + committed + $ZERO_OID $T refs/tags/tag1 + EOF + + write_script clone4/.git/hooks/reference-transaction <<-\EOF && + ( echo "$*" && cat ) >>actual + EOF + + git -C clone4 fetch --atomic .. $B:refs/heads/something && + test_cmp expected clone4/actual +' + +test_expect_success 'backfill failure causes command to fail' ' + git init clone5 && + + write_script clone5/.git/hooks/reference-transaction <<-EOF && + #!/bin/sh + + while read oldrev newrev reference + do + if test "\$reference" = refs/tags/tag1 + then + # Create a nested tag below the actual tag we + # wanted to write, which causes a D/F conflict + # later when we want to commit refs/tags/tag1. + # We cannot just `exit 1` here given that this + # would cause us to die immediately. + git update-ref refs/tags/tag1/nested $B + exit \$! + fi + done + EOF + + # Even though we fail to create refs/tags/tag1 the below command + # unexpectedly succeeds. + git -C clone5 fetch .. $B:refs/heads/something && + test $B = $(git -C clone5 rev-parse --verify refs/heads/something) && + test $S = $(git -C clone5 rev-parse --verify tag2) && + test_must_fail git -C clone5 rev-parse --verify tag1 +' + test_done diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 20f7110ec1..93a0db3c68 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -332,6 +332,39 @@ test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' ' test_cmp expected atomic/.git/FETCH_HEAD ' +test_expect_success 'fetch --atomic --prune executes a single reference transaction only' ' + test_when_finished "rm -rf \"$D\"/atomic" && + + cd "$D" && + git branch scheduled-for-deletion && + git clone . atomic && + git branch -D scheduled-for-deletion && + git branch new-branch && + head_oid=$(git rev-parse HEAD) && + + # Fetching with the `--atomic` flag should update all references in a + # single transaction. It is currently missing coverage of pruned + # references though, and as a result those may be committed to disk + # even if updating references fails later. + cat >expected <<-EOF && + prepared + $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion + committed + $ZERO_OID $ZERO_OID refs/remotes/origin/scheduled-for-deletion + prepared + $ZERO_OID $head_oid refs/remotes/origin/new-branch + committed + $ZERO_OID $head_oid refs/remotes/origin/new-branch + EOF + + write_script atomic/.git/hooks/reference-transaction <<-\EOF && + ( echo "$*" && cat ) >>actual + EOF + + git -C atomic fetch --atomic --prune origin && + test_cmp expected atomic/actual +' + test_expect_success '--refmap="" ignores configured refspec' ' cd "$TRASH_DIRECTORY" && git clone "$D" remote-refs &&
The `--atomic` flag is missing test coverage for pruning of deleted references and backfilling of tags, and of course both aren't covered correctly by this flag. Furthermore, we don't have tests demonstrating error cases for backfilling tags. Add tests to cover those testing gaps. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/t5503-tagfollow.sh | 85 ++++++++++++++++++++++++++++++++++++++++++++ t/t5510-fetch.sh | 33 +++++++++++++++++ 2 files changed, 118 insertions(+)