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@xxxxxx> 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@xxxxxx> > > > +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
Attachment:
signature.asc
Description: PGP signature
