| Message ID | b4ca3f1f3baacde2aea7bae4f583f68c211a557a.1645102965.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 2a0cafd464709cfa22fe7249290c644a2a26c520 |
| Headers | show |
| Series | fetch: improve atomicity of `--atomic` flag | expand |
On Thu, Feb 17, 2022 at 2:04 PM Patrick Steinhardt <ps@pks.im> wrote: > +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 Maybe the following could save a few lines: test "$reference" = refs/tags/tag1 && exit 1 It would make the code look a bit different than in another hook script written below though, so not a big deal. > + done > + EOF Overall it looks good, and I like the improved commit message!
Patrick Steinhardt <ps@pks.im> writes: > When using git-fetch(1) with the `--atomic` flag the expectation is that > either all of the references are updated, or alternatively none are in > case the fetch fails. While we already have tests for this, we do not > have any tests which exercise atomicity either when pruning deleted refs > or when backfilling tags. This gap in test coverage hides that we indeed > don't handle atomicity correctly for both of these cases. > > Add test cases which cover these testing gaps to demonstrate the broken > behaviour. Note that tests are not marked as `test_expect_failure`: this > is done to explicitly demonstrate the current known-wrong behaviour, and > they will be fixed up as soon as we fix the underlying bugs. > > While at it this commit also adds another test case which demonstrates > that backfilling of tags does not return an error code in case the > backfill fails. This bug will also be fixed by a subsequent commit. No need to reorganize the series just to fix this, but in general, adding new tests that are designed to fail in an early commit and then give fixes in a separate commit is to be avoided for two reasons. One is obviously that it breaks bisectability. And another is that it makes it unclear what the end-user visible problem was when reading the actual fix, and makes reviewing the series harder. If a test that demonstrates an existing bug comes with the code to fix the bug in the same commit, it obviously will not break bisectability, and the new test serves also as a documentation of what the end-user visible symptoms of the bug being fixed are. Sometimes people work around the bisectability issue by starting these tests that demonstrates known-to-be-unfixed-yet bugs with test_expect_failure, but that is not a good solution for the reviewability issue, as the step that fixes the bug will show the code fix plus only the first few lines of the test being changed to expect success and it is not readily visible what the symptom actually were. A series of patches, each one of which fixes one issue and demonstrates the issue with tests that expects success when the issue is resolved, is much more pleasant to read than a series where it has tests for multiple issues, followed by a patch or two to handle each issue. And in such a series, it is much easier to see what breaks if the fix weren't there. Reviewers can apply one step of the patch and tentatively revert everything outside t/ to see how the added test breaks without the fix. It helps those who may want to cherry-pick a particular fix, together with its associated test, if a series is done that way. Thanks. > +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. What does this paragraph want the phrase `backfilling tags` to mean? Just from end-user's perspective, there is only one (i.e. if an object that is tagged gets fetched and that tag is not here, fetch it too), but at the mechanism level, there are two distinct code paths (i.e. if OPT_FOLLOWTAGS gets the job done, there is no need to call backfill_tags()). Which failure does this talk about, or it does not matter which code path gave us the 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 > + > + 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)" > +' > + > +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 Nicely done. > + > + 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 && Is there a reason why we cannot do <<-\EOF here to lose the backslashes in the here-text? > + 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 Wow, nasty ;-) > + 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
Junio C Hamano <gitster@pobox.com> writes: > Is there a reason why we cannot do <<-\EOF here to lose the > backslashes in the here-text? Yes. We refer to "$B".
On Thu, Feb 17, 2022 at 12:41:33PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: [snip] > > +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. > > What does this paragraph want the phrase `backfilling tags` to mean? > Just from end-user's perspective, there is only one (i.e. if an > object that is tagged gets fetched and that tag is not here, fetch > it too), but at the mechanism level, there are two distinct code > paths (i.e. if OPT_FOLLOWTAGS gets the job done, there is no need to > call backfill_tags()). Which failure does this talk about, or it > does not matter which code path gave us the tag? It refers to `backfill_tags()`. Should I update this comment to clarify? Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Thu, Feb 17, 2022 at 12:41:33PM -0800, Junio C Hamano wrote: >> Patrick Steinhardt <ps@pks.im> writes: > [snip] >> > +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. >> >> What does this paragraph want the phrase `backfilling tags` to mean? >> Just from end-user's perspective, there is only one (i.e. if an >> object that is tagged gets fetched and that tag is not here, fetch >> it too), but at the mechanism level, there are two distinct code >> paths (i.e. if OPT_FOLLOWTAGS gets the job done, there is no need to >> call backfill_tags()). Which failure does this talk about, or it >> does not matter which code path gave us the tag? > > It refers to `backfill_tags()`. Should I update this comment to clarify? After reading the patch, the issue is only about the case where we perform the second fetch and the case where OPT_FOLLOWTAGS does everything necessary is not relevant. So hinting that we are interested to see what a failure in the follow-on fetch does to the atomicity would be a good idea, and mentioning backfill_tags() would have been a good way to do so. Perhaps like "whether a failure in backfill_tags() correctly aborts..." or something? Thanks.
On Thu, Feb 17, 2022 at 04:18:07PM +0100, Christian Couder wrote: > On Thu, Feb 17, 2022 at 2:04 PM Patrick Steinhardt <ps@pks.im> wrote: > > > +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 > > Maybe the following could save a few lines: > > test "$reference" = refs/tags/tag1 && exit 1 > > It would make the code look a bit different than in another hook > script written below though, so not a big deal. If `$reference` does not match the tag we want to continue and eventually return successfully from this hook. But: $ while read foo; do test "$foo" = bar && echo match; done < <(echo bar) match $ while read foo; do test "$foo" = bar && echo match; done < <(echo foo) $ echo $? 1 So with the proposed change we'd now exit the hook with an error code instead of returning successfully. Patrick
Patrick Steinhardt <ps@pks.im> 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. > + 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? > + 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? > + 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
On Wed, Mar 02, 2022 at 04:25:13PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> 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
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh index 195fc64dd4..6ffe2a5719 100755 --- a/t/t5503-tagfollow.sh +++ b/t/t5503-tagfollow.sh @@ -160,4 +160,85 @@ 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 && + 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 demonstrates that there is + # a bug 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 && + 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 ef0da0a63b..70d51f343b 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -343,6 +343,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 &&
When using git-fetch(1) with the `--atomic` flag the expectation is that either all of the references are updated, or alternatively none are in case the fetch fails. While we already have tests for this, we do not have any tests which exercise atomicity either when pruning deleted refs or when backfilling tags. This gap in test coverage hides that we indeed don't handle atomicity correctly for both of these cases. Add test cases which cover these testing gaps to demonstrate the broken behaviour. Note that tests are not marked as `test_expect_failure`: this is done to explicitly demonstrate the current known-wrong behaviour, and they will be fixed up as soon as we fix the underlying bugs. While at it this commit also adds another test case which demonstrates that backfilling of tags does not return an error code in case the backfill fails. This bug will also be fixed by a subsequent commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/t5503-tagfollow.sh | 81 ++++++++++++++++++++++++++++++++++++++++++++ t/t5510-fetch.sh | 33 ++++++++++++++++++ 2 files changed, 114 insertions(+)