fix(tags): do not peel tag when cloning

[SamVerschueren]

I'm fixing a bug or typo

• if this is your first time contributing, run npm run add-contributor and follow the prompts to add yourself to the README
• squash merge the PR with commit message "fix: [Description of fix]"

This PR fixes #1441. I think a tag should always use the serverref, and not the peeled object id instead as explained in the original issue.

[jcubic]

I have no idea if this correct @mojavelinux Do you know if this change should be added to the library. The tests are passing, so it should be right.

[mojavelinux]

I have tests in Antora that would be able to give me insight into what impact this has and whether it's doing the right thing (as I'm sure it is...but it's always good to test). I'll try to look at it this week. I vaguely remember having to workaround the lack of tag peeling...but it's been awhile so I need to refresh my memory.

[mojavelinux]

I can confirm that this is the correct behavior. If I clone the repository without this patch, I see the following tag for the repository I cloned when I run git for-each-ref refs/tags:

098c323a9bd63eb615ef3ddd37a8ae98e2f61353 commit	refs/tags/v2.0.16

However, that's not correct. That's showing a lightweight tag. It should be an annotated tag. If I apply this patch, reclone, then run git for-each-ref refs/tags again, I see:

7de18ac8b4e1512e6067e5bb590d3db16bd46595 tag	refs/tags/v2.0.16

That's consistent with the result when cloning with the git command.

What concerns me about this patch is that there is no test. I would like to see a test that verifies that the annotate tag is preserved during a clone.

[SamVerschueren]

What concerns me about this patch is that there is no test. I would like to see a test that verifies that the annotate tag is preserved during a clone.

I'll see what I can do. Not super familiar with the codebase but would be awesome if I could get a test in.

[mojavelinux]

Excellent. Usually, I try to see if I can find a test which is close, perhaps one already doing the work, then just build on that. If you need help, don't hesitate to ask.

[SamVerschueren]

So I tried setting up a test, but it's a pain. Or maybe, there's another way of doing it.

So I looked at the isomorphic-git repository to find an annotated tag, which v0.0.10 is. So I tried it with this test first

const { fs, dir, gitdir } = await makeFixture('isomorphic-git')
await clone({
  fs,
  http,
  dir,
  gitdir,
  depth: 1,
  singleBranch: true,
  ref: 'v0.0.10',
  url: 'https://github.com/isomorphic-git/isomorphic-git.git',
  corsProxy: process.browser ? `http://${localhost}:9999` : undefined,
})
const oid = await fs._readFile(`${gitdir}/refs/tags/v0.0.10`, 'utf8')
expect(oid.trim()).toBe('0a117b8378f5e5323d15694c7eb8f62c4bea152b')

This doesn't fail because it only checks out the tag v0.0.10 and thus not the peeled tag. So that test wouldn't be broken on master currently.

So for it to test correctly, I should have to clone the entire isomorphic-git repository, which takes a while...

const { fs, dir, gitdir } = await makeFixture('isomorphic-git')
await clone({
  fs,
  http,
  dir,
  gitdir,
  url: 'https://github.com/isomorphic-git/isomorphic-git.git',
  corsProxy: process.browser ? `http://${localhost}:9999` : undefined,
})
const oid = await fs._readFile(`${gitdir}/refs/tags/v0.0.10`, 'utf8')
expect(oid.trim()).toBe('0a117b8378f5e5323d15694c7eb8f62c4bea152b')

So not entirely sure how to proceed from here. Any ideas?

[mojavelinux]

There are two approaches you could take. First clone a smaller repository, such as https://github.com/isomorphic-git/lightning-fs. But better would be to keep the test self contained. The test suite has a git server in it. You can use that to serve a repository you use isomorphic-git to create, then check the clone behavior. See https://github.com/isomorphic-git/isomorphic-git/blob/main/__tests__/test-clone.js#L117-L138

...though I wonder why you couldn't just add an assertion to this test: https://github.com/isomorphic-git/isomorphic-git/blob/main/__tests__/test-clone.js#L71-L92

[SamVerschueren]

...though I wonder why you couldn't just add an assertion to this test: main/tests/test-clone.js#L71-L92

It's the same thing. It specifically checks out that tag, which means that test-tag^{} will not be in the refs list, because test-tag will be the only one and thus it will work as expected.

I think it would be a good idea to use that mock github server to give me something back that I want.

[mojavelinux]

It's the same thing. It specifically checks out that tag, which means that test-tag^{} will not be in the refs list, because test-tag will be the only one and thus it will work as expected.

What you're saying here is not consistent with the output I showed above using git for-each-ref. Clearly, isomorphic-git is peeling the tag, so it should be possible to detect and assert this. Otherwise, what is it that git for-each-ref is seeing?

[SamVerschueren]

You are correct, it does peel the tag. But the behaviour is different if you change the input parameters.

If you use depth=1, it does not pull everything (which makes sense). This is the output of git for-each-ref if you clone with depth=1.

$ git for-each-ref
76d0d69b9044bb6b38d247ba34c3a9a88b70036c commit refs/heads/main
76d0d69b9044bb6b38d247ba34c3a9a88b70036c commit refs/remotes/origin/HEAD
76d0d69b9044bb6b38d247ba34c3a9a88b70036c commit refs/remotes/origin/main

But what we want here is to verify that that specific tag is not peeled without cloning the entire repository. So passing depth=1 and ref=v0.0.10. This is the outcome of for-each-ref if you do that.

$ git for-each-ref
0a117b8378f5e5323d15694c7eb8f62c4bea152b tag    refs/tags/v0.0.10

Which is the annotated object id, and not the peeled one. This behaviour is different if you clone the entire repo.

$ git for-each-ref refs/tags/v0.0.10
ce03143bd6567fc7063549c204e877834cda5645 commit refs/tags/v0.0.10

The reason for this behaviour is that if you pass in the ref, the refs Map passed into the function only contains the key-value pair 'refs/tags/v0.0.10': '0a117b8378f5e5323d15694c7eb8f62c4bea152b'. Which means that when it tries to read the peeled tag refs/tags/v0.0.10^{} here, it cannot find it and will just use the annotated object id instead.

The reason is probably due to the fact it takes a different code path when fetching a repository when only cloning a single branch. Because if we only clone a single branch, it will not use the remoteRefs list but construct a refs list locally which only contains the ref of the branch you are cloning.

I hope this makes a bit of sense? I might be missing some bits and pieces because as said before, not super familiair with the codebase.

[mojavelinux]

Thank you for sharing the analysis. That does make sense to me.

What I'm not sure about is how this relates to the request to write a test. If the code is being changed, it must be possible to assert that there a behavior change in the output under that condition. What I showed above was the output before and after applying this patch. All I'm requesting is that there's a test that verifies that isomorphic-git no longer peels the tag in that scenario. Then we can move forward.

[SamVerschueren]

All I'm requesting is that there's a test that verifies that isomorphic-git no longer peels the tag in that scenario.

Yes, and this is what I'm trying to do. But in order to verify this behavior change I need to clone an entire repo with all it's tags and what not. So as you suggested, I'm looking into writing a self-contained test or cloning a smaller repo. I looked at lightning-fs, but they don't use annotated tags. However, git-http-mock-server does. Or someone should add an annotated tag to the test.empty repository. But that might cause side effects.

[mojavelinux]

Yes, and this is what I'm trying to do.

Got it. 👍

However, git-http-mock-server does.

In Antora, I use node-git-server, which works extremely well. Instead of messing with making a mock, it's a real git server. Here's how I use it: https://gitlab.com/antora/antora/-/blob/main/packages/cli/test/cli-test.js#L71-77

[mojavelinux]

Or someone should add an annotated tag to the test.empty repository.

Ironically, the test.empty repository is not actually empty.

Since this test isn't really about the behavior of a repository service like GitHub or GitLab, I think it's best to keep it self-contained.

[mojavelinux]

However, git-http-mock-server does.

I now realize you were referring to cloning the repository, not using it.

Still, I think having to use a remote repository is really unnecessary on the whole. I think most of isomorphic-git tests should be self-contained...only using GitHub when the intent is to very exchange with GitHub specifically.

[mojavelinux]

I think most of isomorphic-git tests should be self-contained

But for the purpose of this MR, I think it's fine to use the git-http-mock-server repository as a fixture.

[SamVerschueren]

I was able to add a self-contained test based on git-http-mock-server. Without the fix, the oid of the v1.0.0 tag is incorrect and the test fails. With the fix, the test returns the correct oid.

[mojavelinux]

Great work!

[jcubic]

Thanks for great work.

[isomorphic-git-bot]

🎉 This PR is included in version 1.10.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[SamVerschueren]

Awesome! Thanks for helping me out and for the feedback 🙏 ! Highly appreciated.