| Message ID | 081174b7a00cf094a7dacd8aba89fb137ea40f66.1645430423.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fetch: improve atomicity of `--atomic` flag | expand |
Patrick Steinhardt <ps@pks.im> writes: > +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 I think I've reviewed the previous round of these patches in detail. I by mistake sent a comment for this step in v2, but I think the same puzzlement exists in this round, too.
Junio C Hamano <gitster@pobox.com> writes: > Patrick Steinhardt <ps@pks.im> writes: > >> +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 > > I think I've reviewed the previous round of these patches in > detail. I by mistake sent a comment for this step in v2, but I > think the same puzzlement exists in this round, too. Namely: 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.
On Wed, Mar 02, 2022 at 04:32:48PM -0800, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Patrick Steinhardt <ps@pks.im> writes: > > > >> +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 > > > > I think I've reviewed the previous round of these patches in > > detail. I by mistake sent a comment for this step in v2, but I > > think the same puzzlement exists in this round, too. > > Namely: > > 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. Oof, this was supposed to be `exit \$?`, thanks for catching this. But your above comment is right: we can indeed just create the D/F conflict outside of the hook and thus avoid the hook script altogether. Thanks! Patrick
Patrick Steinhardt <ps@pks.im> writes: >> >> + # would cause us to die immediately. >> >> + git update-ref refs/tags/tag1/nested $B >> >> + exit \$! >> >> + fi >> >> + done >> >> + EOF >> > >> > I think I've reviewed the previous round of these patches in >> > detail. I by mistake sent a comment for this step in v2, but I >> > think the same puzzlement exists in this round, too. >> >> Namely: >> >> 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. > > Oof, this was supposed to be `exit \$?`, thanks for catching this. But > your above comment is right: we can indeed just create the D/F conflict > outside of the hook and thus avoid the hook script altogether. Thanks! I see. As that shell does not send anything to background, at the point of the reference $! would yield an empty string, and "exit" is equivalent to "exit $?", it is doing the right thing, I presume. The topic has been in 'next' for a while, so if you are inclined to fix it up, please send an incremental patch. If you do "exit" it would be a one-liner change, or if you use a different "cause D/F conflict outside the hook" approach, the change may become a bit more involved. Thanks.
On Wed, Mar 02, 2022 at 10:49:05PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > >> >> + # would cause us to die immediately. > >> >> + git update-ref refs/tags/tag1/nested $B > >> >> + exit \$! > >> >> + fi > >> >> + done > >> >> + EOF > >> > > >> > I think I've reviewed the previous round of these patches in > >> > detail. I by mistake sent a comment for this step in v2, but I > >> > think the same puzzlement exists in this round, too. > >> > >> Namely: > >> > >> 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. > > > > Oof, this was supposed to be `exit \$?`, thanks for catching this. But > > your above comment is right: we can indeed just create the D/F conflict > > outside of the hook and thus avoid the hook script altogether. Thanks! > > I see. > > As that shell does not send anything to background, at the point of > the reference $! would yield an empty string, and "exit" is > equivalent to "exit $?", it is doing the right thing, I presume. > > The topic has been in 'next' for a while, so if you are inclined to > fix it up, please send an incremental patch. If you do "exit" it > would be a one-liner change, or if you use a different "cause D/F > conflict outside the hook" approach, the change may become a bit > more involved. > > Thanks. Fair enough, thanks for clarifying the steps. Does it make sense to also change indentantion of the heredocs as per your review of v2 or should I just keep that as-is now? Patrick
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh index 195fc64dd4..7dadaafd4b 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 in `backfill_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(+)