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

[Taylor Blau <me@ttaylorr.com>]

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 <schacon@gmail.com>
> >
> > +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