[v3,2/2] bundle-uri: add test for bundle-uri clones with tags

Commit Message

Scott Chacon March 18, 2025, 3:36 p.m. UTC

From: Scott Chacon <[e-mail]>

The change to the bundle-uri unbundling refspec now includes tags, so this
adds a simple test to make sure that tags in a bundle are properly added to
the cloned repository and will be included in ref negotiation with the
subsequent fetch.

Signed-off-by: Scott Chacon <[e-mail]>
---
 t/t5558-clone-bundle-uri.sh | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

Phillip Wood March 19, 2025, 10:33 a.m. UTC | #1

Hi Scott

On 18/03/2025 15:36, Scott Chacon via GitGitGadget wrote:
> From: Scott Chacon <[e-mail]>
> 
> +test_expect_success 'clone with tags bundle' '
> +	git clone --bundle-uri="clone-from-tags/ALL.bundle" \
> +		clone-from-tags clone-tags-path &&
> +	git -C clone-tags-path for-each-ref --format="%(refname)" >refs &&
> +	grep "refs/bundles/tags/" refs >actual &&

Thanks for adding this test. Calling "git for-each-ref" followed by 
"grep" follows the pattern of the existing tests but I'm not sure why 
they don't just pass the pattern to "for-each-ref" and avoid the extra 
process.

Do we want to just test for tags or are we really interested to see all 
the bundle refs created when cloning? This applies to the previous patch 
as well - we obviously need to change the expected output but I'm not 
sure changing the ref pattern is necessarily a good idea. After all the 
point of this series is to create refs under refs/bundles for all the 
refs in the bundle.

Best Wishes

Phillip

> +	cat >expect <<-\EOF &&
> +	refs/bundles/tags/A
> +	refs/bundles/tags/B
> +	refs/bundles/tags/tag-A
> +	EOF
> +	test_cmp expect actual
> +'
> +
>   # To get interesting tests for bundle lists, we need to construct a
>   # somewhat-interesting commit history.
>   #

Taylor Blau March 19, 2025, 5:50 p.m. UTC | #2

On Wed, Mar 19, 2025 at 10:33:48AM +0000, Phillip Wood wrote:
> Hi Scott
>
> On 18/03/2025 15:36, Scott Chacon via GitGitGadget wrote:
> > From: Scott Chacon <[e-mail]>
> >
> > +test_expect_success 'clone with tags bundle' '
> > +	git clone --bundle-uri="clone-from-tags/ALL.bundle" \
> > +		clone-from-tags clone-tags-path &&
> > +	git -C clone-tags-path for-each-ref --format="%(refname)" >refs &&
> > +	grep "refs/bundles/tags/" refs >actual &&
>
> Thanks for adding this test. Calling "git for-each-ref" followed by "grep"
> follows the pattern of the existing tests but I'm not sure why they don't
> just pass the pattern to "for-each-ref" and avoid the extra process.

Indeed.

> Do we want to just test for tags or are we really interested to see all the
> bundle refs created when cloning? This applies to the previous patch as well
> - we obviously need to change the expected output but I'm not sure changing
> the ref pattern is necessarily a good idea. After all the point of this
> series is to create refs under refs/bundles for all the refs in the bundle.

I think we should be testing that all of the refs we expect to have made
it over actually did so. This diff (applied on top of your series) does
that:

--- 8< ---
diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
index b1276ba295..9b211a626b 100755
--- a/t/t5558-clone-bundle-uri.sh
+++ b/t/t5558-clone-bundle-uri.sh
@@ -128,13 +128,12 @@ test_expect_success 'create bundle with tags' '
 test_expect_success 'clone with tags bundle' '
 	git clone --bundle-uri="clone-from-tags/ALL.bundle" \
 		clone-from-tags clone-tags-path &&
-	git -C clone-tags-path for-each-ref --format="%(refname)" >refs &&
-	grep "refs/bundles/tags/" refs >actual &&
-	cat >expect <<-\EOF &&
-	refs/bundles/tags/A
-	refs/bundles/tags/B
-	refs/bundles/tags/tag-A
-	EOF
+
+	git -C clone-from-tags for-each-ref --format="%(refname:lstrip=1)" \
+		>expect &&
+	git -C clone-tags-path for-each-ref --format="%(refname:lstrip=2)" \
+		refs/bundles >actual &&
+
 	test_cmp expect actual
 '
--- >8 ---

While writing the above, I wasn't quite sure how to follow the test
setup. It looks like it creates the following structure:

    $ git log --oneline --graph
    * d9df450 (HEAD -> base, tag: B) B
    * 0ddfaf1 (tag: tag-A, tag: A) A

, which we could do with just:

    test_commit A &&
    test_commit B

But even then, I don't think we really need to have more than one tag
here to exercise this functionality. So I think it would be fine to
simplify the test to just create a single tag, which a simple
"test_commit A" should do.

Thanks,
Taylor

Junio C Hamano March 21, 2025, 6:31 a.m. UTC | #3

Phillip Wood <[e-mail]> writes:

> Hi Scott
>
> On 18/03/2025 15:36, Scott Chacon via GitGitGadget wrote:
>> From: Scott Chacon <[e-mail]>
>> +test_expect_success 'clone with tags bundle' '
>> +	git clone --bundle-uri="clone-from-tags/ALL.bundle" \
>> +		clone-from-tags clone-tags-path &&
>> +	git -C clone-tags-path for-each-ref --format="%(refname)" >refs &&
>> +	grep "refs/bundles/tags/" refs >actual &&
>
> Thanks for adding this test. Calling "git for-each-ref" followed by
> "grep" follows the pattern of the existing tests but I'm not sure why
> they don't just pass the pattern to "for-each-ref" and avoid the extra
> process.

Yup, if we really care about a single ref, we should just go in the
clone and check with 'git show-ref'.

> Do we want to just test for tags or are we really interested to see
> all the bundle refs created when cloning? This applies to the previous
> patch as well - we obviously need to change the expected output but
> I'm not sure changing the ref pattern is necessarily a good
> idea. After all the point of this series is to create refs under
> refs/bundles for all the refs in the bundle.

The really interesting case that the original behaviour could have
been "working around existing bugs" would be

 - prepare an annotated tag.

 - include that annotated tag in the bundle specified by the
   bundle-uri feature (whose SOLE purpose is to speed up the main
   part of the tranfer, WITHOUT affecting the resulting remote
   tracking refs and global tag namespace).

 - prepare a repository that uses that bundle as bundle-uri when
   getting cloned (let's call it the 'origin' repository).  It
   probably is convenient if you make this a repository  separate
   from where you prepared the bundle above.  Make sure that this
   repository does *not* (yet) have that annotated tag.

 - clone from the 'origin' repository with bundle-uri feature.  The
   annotated tag would be held under refs/bundles/tags/ hierarchy but
   that is *not* the interesting part.  Make sure that the annotated
   tag does *not* appear in refs/tags/ hierarchy of the clone, since
   it does not exist (yet) at the 'origin' repository.

 - Now add that annotated tag to the 'origin' repository.

 - fetch from the 'origin' repository again, with the default
   configuration (i.e. allowing "tags follow when commits they
   reference are fetched" feature to kick in).  As the annotated tag
   appears in refs/tags/ of the 'origin' repository, the commit
   pointed at by that annotated tag now appears in one of its
   branches, and the history leading to that commit (and possibly
   others) are transferred to refs/remotes/origin/* remote-tracking
   branches, the tag-following feature should kick in and copy the
   annotated tag in refs/tags/ hierarchy as well.

The interesting part to verify is that in the cloned repository the
annotated tag does not appear in refs/tags/ immediately after
cloneing, but does appear there after the 'origin' is updated to
have the tag under refs/tags/ and then fetch

Patch

diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
index 33a7009e9a2..b1276ba295c 100755
--- a/t/t5558-clone-bundle-uri.sh
+++ b/t/t5558-clone-bundle-uri.sh
@@ -107,6 +107,37 @@  test_expect_success 'clone with file:// bundle' '
 	test_cmp expect actual
 '
 
+test_expect_success 'create bundle with tags' '
+	git init clone-from-tags &&
+	(
+		cd clone-from-tags &&
+		git checkout -b base &&
+		git checkout -b topic &&
+
+		test_commit A &&
+		git tag tag-A &&
+		git checkout -b base &&
+		git branch -d topic &&
+		test_commit B &&
+
+		git bundle create ALL.bundle --all &&
+		git bundle verify ALL.bundle
+	)
+'
+
+test_expect_success 'clone with tags bundle' '
+	git clone --bundle-uri="clone-from-tags/ALL.bundle" \
+		clone-from-tags clone-tags-path &&
+	git -C clone-tags-path for-each-ref --format="%(refname)" >refs &&
+	grep "refs/bundles/tags/" refs >actual &&
+	cat >expect <<-\EOF &&
+	refs/bundles/tags/A
+	refs/bundles/tags/B
+	refs/bundles/tags/tag-A
+	EOF
+	test_cmp expect actual
+'
+
 # To get interesting tests for bundle lists, we need to construct a
 # somewhat-interesting commit history.
 #