git: Reimplement cloneRefName using cloneTag's implementation to support annotated tags

[bburky]

cloneRefName() failed to support annotated tags and would attempt to clone the annotated tag object's id as if it were a commit object. This would cause the following error (with the tag's hash, not the commit):

unable to resolve commit object for 'da39a3ee5e6b4b0d3255bfef95601890afd80709': object not found

cloneTag() already had support for annotated tags, and could actually be modified to support cloning any ref with no modification. The implementation was moved to cloneRefName() and now cloneTag() is a wrapper for cloneRefName().

Sharing an implementation between cloneRefName() and cloneTag() means error messages are slightly worse: they always say "ref" now instead of "tag" when appropriate.

Due to a bug(?) in go-git a ref like refs/pull/1/head can't be cloned used with SingleBranch == True. Go-git's Repository.cloneRefSpec() will try to build a refspec using .Short() on the ref, which doesn't behave the same for refs named like this (that don't match any RefRevParseRules).
https://github.com/src-d/go-git/blob/v4.13.1/repository.go#L826-L829

A test for cloneRefName() using an annotated tag was added. Arguably the tests could be merged somewhat now that they share an implementation. There is one change in the cloneTag tests for expectConcreteCommit that I don't quite understand.

There is a TODO in this code about SingleBranch. Other that, I think this approach works. I've tested using the unit tests and they pass. If you have a different approach you'd like to use to fix this, please feel free to close this PR if you want an alternate solution.

[bburky]

I don't really know what caused this to change.

[bburky]

As mentioned in the PR message, go-git CloneOptions.SingleBranch does not support refs/pull/1/head because it builds the refpec to clone wrong.

I'm not entirely sure, but this may be a bug in go-git.

Ignoring singleBranch would be a regression for branches and other non-tag refs. Even with singleBranch=false go-git actually will detect tags automatically (it checks ReferenceName.IsTag() and only fetches one ref). I don't have a great fix to work around this. If it is a go-git bug it could be fixed upstream.

[aryan9600]

Hi @bburky, thanks for discovering this issue and opening this PR! I did manage to repro this issue on my local.

These changes seem to have a lot of blast radius, bringing forward other issues in go-git and also seems to be breaking existing tests. An easier and straightforward solution might be to instead detect if the ref name is pointing to a tag and then just do g.cloneTag(). We could do the same for branches too.
One thing we'd need to make sure about how Reference is set on the resultant git.Commit object. When using a branch or a tag directly, the Reference starts with the short version of the ref, such as main, v0.0.1, etc. So the logic to quit early by checking opts.LastObservedCommit needs to account for this.

asking @hiddeco and @darkowlzz to weigh in here also.

[bburky]

An easier and straightforward solution might be to instead detect if the ref name is pointing to a tag and then just do g.cloneTag()

If you have a good way to do this with a simpler change, it may be for the best. This could definitely introduce unexpected issues, I don't know why the expectConcreteCommit test changed for example (but I didn't debug it too closely).

I suppose you could do what you suggest based on a ref name matching refs/tags/. That may be a reliable enough heuristic (technically ref names are by convention only and you can put a git tag object under a nonstandard ref). The approach I tried to take was doing g.cloneTag() for all refs (which required refactoring done in this PR, because it previously only handled tag names not any ref).

[aryan9600]

Now that go-git has support for peeled refs (see: go-git/go-git#750), solving this issue becomes a lot easier. We can use the AppendPeeled option when we call remote.ListContext() to include the commit ID that all annotated tags point to with the ^{} syntax. The way I see it we have two options:

1. Instruct users in docs to always use the ^{} syntax when specifying an annotated tag under a ref name.
2. If we could find the ref && ref is a tag && the ref doesn't end with ^{}, then we try and see if the ref list contains an entry for that ref appended with ^{}. If such an entry exists, then we use that instead and if it doesn't we use the original entry.

[hiddeco]

My vote would to go to option 2 to reduce the burden on users, as not everyone actually knows what an annotated tag is.

My suggestion would be to use AppendPeeled from go-git/go-git#750, and look if there is an exact match or one suffixed with ^{} to reduce the burden on users.

[bburky]

Option 2 sounds good to me from the UX perspective too. We accidentally stumbled across this when a new developer made the tag and they used an annotated tag not expecting there to be any difference at all for Flux. We had to a little bit of debugging to figure out it an annotated tag that caused the issue. I think most users would expect any ref to work with names:.

I'm not familiar enough with go-git to implement a solution using peeled refs. I can close this PR if someone else can pick this up. Let me know if you want me to create an issue to track this, but someone else may want to with more info about go-git than I know.

I'd summarize the original issue I reported as:

---

cloneRefName() failed to support annotated tags and would attempt to clone the annotated tag object's id as if it were a commit object. This would cause the following error (with the tag's hash, not the commit):

unable to resolve commit object for 'da39a3ee5e6b4b0d3255bfef95601890afd80709': object not found

Currently Flux's cloneRefName() passes the ref to getRemoteHEAD() to resolve the ref to an object id, and then passes the id to cloneCommit() which only supports commit objects, not annotated tags.

[aryan9600]

yes, it'd be great if you could open an issue for this. since you won't be picking this up, i'm closing this PR. thanks again for finding this issue 🙇