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@xxxxxxxxx>
> >
> > +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
