Re: [PATCH 1/6] fetch: increase test coverage of fetches

[Christian Couder <christian.couder@gmail.com>]

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.