Be more git compliant when resolving references

[nulltoken]

It looks like LibGit2Sharp is not that git.git compliant when it comes to weirdly named references. Let's fix this!

Given a repo with a tag refs/tags/refs/tags/v1.0

• TagCollection["v1.0"] shouldn't resolve anything
• TagCollection["refs/tags/v1.0"] should correctly resolve it
• TagCollection["refs/tags/refs/tags/v1.0"] should correctly resolve it

Given a repo with two tags refs/tags/refs/tags/v1.0 and refs/tags/v1.0

• TagCollection["v1.0"] should resolve refs/tags/v1.0
• TagCollection["refs/tags/v1.0"] should resolve refs/tags/v1.0
• TagCollection["refs/tags/refs/tags/v1.0"] should resolve refs/tags/refs/tags/v1.0

It would make sense to make BranchCollection consistent with this behavior as well.

Given a repo with a tag refs/tags/v1.0 and a branch refs/heads/v1.0

• TagCollection["v1.0"] should resolve refs/tags/v1.0
• BranchCollection["v1.0"] should resolve refs/heads/v1.0

[nulltoken]

/cc @arthurschreiber I'll be waiting for your pull request 😉

[arthurschreiber]

TagCollection["v1.0"] should correctly resolve it.

That's actually not right. This should not resolve anything in the first case.

[nulltoken]

TagCollection["v1.0"] should correctly resolve it.

That's actually not right. This should not resolve anything in the first case.

👍 You're completely correct. Thanks for having pointed at this!

Fixed.

[nulltoken]

It would make sense to make BranchCollection and ReferenceCollection consistent with this behavior as well.

Actually, I think I was a little to hasty when writing this. References are a much lower level concept than tags and branches, and I'm not sure it's be wise to let the engine guess. The current implementation considers the given entry as a canonical name and I think it makes much more sense.

Going a little bit farther, a repo may contain a branch named test and a tag named test. In that case git show will try to "dwim" the reference (see libgit2/libgit2#1560 for equivalent libgit2 feature) and will prefer refs/tags/test over refs/heads/test (this can be reproduced against the libgit2 testrepo.git test repository, for instance).

In this context, we have to ensure that resolving test against the TagCollection will retrieve the tag, and the head through BranchCollection.

Updated the question to include this.

[nulltoken]

I'm starting to wonder if we shouldn't enhance git_reference_dwim() a little bit... Maybe could we pass an additional parameter to this function that would describe the probing context. This would greatly reduce the logic in TagCollection, BranchCollection.

typedef enum {
    GIT_DWIM_STANDARD = 0,
    GIT_DWIM_TAGS = 1,
    GIT_DWIM_LOCAL_HEADS = 2,
    GIT_DWIM_REMOTE_HEADS = 3
} git_dwim;

/cc @carlosmn Thoughts?

[carlosmn]

I'm sceptical of something that tries to be so generic and specific at the same time. The purpose of git_reference_dwim is to deal with the situation of "the user gave me this string to act on, let's see what it could mean". In this case you already know what you're dealing with and I'm not sure what the situation would be where you are given a string that could mean anything but you absolutely know that it's restricted to a particular namespace.

From the examples above, it looks like you could check the string you're given verbatim if it starts with refs/ and then add the appropriate prefix and check again, without any involvement of _dwim.

The only clear indication of something git_reference_dwim might not be doing correctly is to prefer tags, and we should definitely make it have the same ordering as git.

[nulltoken]

The only clear indication of something git_reference_dwim might not be doing correctly is to prefer tags, and we should definitely make it have the same ordering as git.

The way git_reference_dwim() works is heavily inspired from this portion of git.git code

In this case you already know what you're dealing with and I'm not sure what the situation would be where you are given a string that could mean anything but you absolutely know that it's restricted to a particular namespace.

The idea would be that when working with branches, test would be dwim'ed as a branch whereas when working with tags, it would be rather be dwim'ed as a tag (ie. %s, refs/%s then refs/tags/%s. We would not check refs/heads/%s, refs/remotes/%s nor refs/remotes/%s/HEAD).

[arthurschreiber]

So, what's the plan here? In rugged, I solved this simply by treating the given tagname as canonical first, and if that fails prepend refs/tags and try again. Is that how you prefer it to be fixed in lg2#, too?

[nulltoken]

In rugged, I solved this simply by treating the given tagname as canonical first, and if that fails prepend refs/tags and try again. Is that how you prefer it to be fixed in lg2#, too?

@arthurschreiber 👍