On Wed, Mar 02, 2022 at 04:25:13PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@xxxxxx> writes: > > > +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 && > > + while read oldrev newrev reference > > + do > > + if test "$reference" = refs/tags/tag1 > > + then > > + exit 1 > > + fi > > + done > > + EOF > > Without the extra indentation level, all your <<here-doc would > become easier to read. You are consistently doing this in your > patches, which it is better than random mixes of indentation style, > though. Personally I think it's easier to read this way, but grepping through the codebase shows that what you're proposing is used consistently. I'll change it. > > + 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 demonstrates that there is > > + # a bug in the `--atomic` flag. > > + test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)" > > +' > > Makes sense. > > > +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 > > I think we see two transactions here, even though the comment says > otherwise. Is this expecting a known breakage? Yes, it is. I've added a comment in this patch to document the breakage, which is then removed when the bug is fixed. > > + 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 > > Nice way to make sure what is and is not in each transaction. I > feels a bit too rigid (e.g. in the first transaction, there is no > inherent reason to expect that the update to head/something is > mentioned before the update to tags/tag2, for example). > > > +' > > + > > +test_expect_success 'backfill failure causes command to fail' ' > > + git init clone5 && > > + > > + write_script clone5/.git/hooks/reference-transaction <<-EOF && > > + 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 > > I have been wondering if we need to do this from the hook? If we > have this ref before we start "fetch", would it have the same > effect, or "fetch" notices that this interfering ref exists and > removes it to make room for storing refs/tags/tag1, making the whole > thing fail to fail? No, it indeed is not, thanks! Patrick > > + exit \$! > > In any case, "exit 0" or "exit \$?" would be understandable, but > exit with "$!", which is ...? The process ID of the most recent > background command? Puzzled. > > > + fi > > + done > > + EOF
Attachment:
signature.asc
Description: PGP signature
