Toon Claes <toon@xxxxxxxxx> writes: > The changes in commit c06793a4ed (allow git-bundle to create bottomless > bundle, 2007-08-08) ensure annotated tags are properly preserved when > creating a bundle using a revision range operation. > > At the time the range notation would peel the ends to their > corresponding commit, meaning ref v2.0 would point to the v2.0^0 commit. > So the above workaround was introduced. This code looks up the ref > before it's written to the bundle, and if the ref doesn't point to the > object we expect (for tags this would be a tag object), we skip the ref > from the bundle. Instead, when the ref is a tag that's the positive end > of the range (e.g. v2.0 from the range "v1.0..v2.0"), then that ref is > written to the bundle instead. > > Later, in 895c5ba3c1 (revision: do not peel tags used in range notation, > 2013-09-19), the behavior of parsing ranges was changed and the problem > was fixed at the cause. But the workaround in bundle.c was not reverted. > Interesting to read the progression in these changes. Good digging. > Now it seems this workaround can cause a race condition. git-bundle(1) > uses setup_revisions() to parse the input into `struct rev_info`. Later, > in write_bundle_refs(), it uses this info to write refs to the bundle. > As mentioned at this point each ref is looked up again and checked > whether it points to the object we expect. If not, the ref is not > written to the bundle. But, when creating a bundle in a heavy traffic > repository (a repo with many references, and frequent ref updates) it's > possible a branch ref was updated between setup_revisions() and > write_bundle_refs() and thus the extra check causes the ref to be > skipped. > This makes sense, once the input is parsed in `setup_revisions()`, those'd be the values we want to use. Checking for values again is a definite race condition. > The workaround was originally added to deal with tags, but the code path > also gets hit by non-tag refs, causing this race condition. Because it's > no longer needed, remove it and fix the possible race condition. Nice, simple fix. [snip] > diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh > index 5d444bfe201a330527e86dde7229721fc386fc93..f398a59424dcd025ce616cadcd7eece9be5301a3 100755 > --- a/t/t6020-bundle-misc.sh > +++ b/t/t6020-bundle-misc.sh > @@ -504,6 +504,40 @@ test_expect_success 'unfiltered bundle with --objects' ' > test_cmp expect actual > ' > > +test_expect_success 'bottomless bundle upto tag' ' > + git bundle create v2.bdl \ > + v2 && > + > + git bundle verify v2.bdl | > + make_user_friendly_and_stable_output >actual && > + > + format_and_save_expect <<-EOF && > + The bundle contains this ref: > + <TAG-2> refs/tags/v2 > + The bundle records a complete history. > + $HASH_MESSAGE > + EOF > + test_cmp expect actual > +' > + > +test_expect_success 'bundle between two tags' ' > + git bundle create v1-v2.bdl \ > + v1..v2 && > + > + git bundle verify v1-v2.bdl | > + make_user_friendly_and_stable_output >actual && > + > + format_and_save_expect <<-EOF && > + The bundle contains this ref: > + <TAG-2> refs/tags/v2 > + The bundle requires these 2 refs: > + <COMMIT-E> Z > + <COMMIT-B> Z > + $HASH_MESSAGE > + EOF > + test_cmp expect actual > +' > + Shouldn't we add a test for an annotated tag and verify that the tag object is also included in the bundle? Thanks Karthik
Attachment:
signature.asc
Description: PGP signature
