libgit2 / libgit2sharp

Git + .NET = ❤
http://libgit2.github.com
MIT License
3.21k stars 889 forks source link

TrackedBranch does not exist for empty repositories #276

Closed jamill closed 11 years ago

jamill commented 11 years ago

I wanted to open this issue about some corner cases when dealing with the TrackedBranch property on the Branch class for "new" repositories to see if there were any suggestions for how (if?) to deal with the following scenarios:

  1. If I clone from an empty repository, TrackedBranch property of the master branch returns null.
  2. If I clone from an empty repository, and then make a commit, TrackedBranch poperty of the master branch still returns null.
  3. If I initialize an empty repository, set an upstream remote, then perform a fetch, TrackedBranch property of master returns null.

Part of the reason for this behavior is that the resolution path goes through libgit2 and requires "real" branches. As there is not a proper "Branch" I am not sure what the expected behavior is here - however it would be nice for Libgit2Sharp to still handle the resolution of the "Upstream" branch name.

Possible resolution: 1) Add another property TrackedBranchName (or maybe UpstreamBranchName) that just reads the branch name from the config file.

Note - the Remote property has a similar issue dealing with "local" remote (i.e. the "." remote for a branch.{branch_name}.remote" configuration key in the config file)

dahlbyk commented 11 years ago

I remember thinking when I saw libgit2's implementation that we'd probably run into a case where returning a proper branch might be problematic. As you've shown, it's perfectly legitimate to be tracking a branch that isn't yet a valid reference in the current repository.

Rather than introduce a TrackedBranchName property, I think it would be preferable to return a proper Branch instance that just doesn't reference a commit yet - similar to Head on a new repo.

We could probably handle local remotes with a special subclass of Remote that throws on Fetch().

nulltoken commented 11 years ago

Rather than introduce a TrackedBranchName property, I think it would be preferable to return a proper Branch instance that just doesn't reference a commit yet - similar to Head on a new repo.

I concur with @dahlbyk's approach

jamill commented 11 years ago

@nulltoken, @dahlbyk - Thanks! this seems reasonable.

nulltoken commented 11 years ago

@dahlbyk Would you have some time to work on this?

dahlbyk commented 11 years ago

I've pushed a failing test that explicitly covers scenarios 1 & 2. Scenario 3 has identical symptoms to scenario 1.

https://github.com/dahlbyk/libgit2sharp/compare/gh276

I'm not sure if it's too late to change the API, or if we should just add a new one, but all we really need from libgit2 is a function to turn "master" into either null or "refs/remotes/origin/master". From there, our bindings can either return null or an appropriately-named Branch (with or without Tip).

On a related note, I noticed Branch.Remote manually resolves its branch.*.remote config - as that's one component of the tracking branch equation, should retrieving that particular config value be part of the libgit2 public API?

nulltoken commented 11 years ago

Wouldn't retrieving the refspec from the remote (with git_remote_fetchspec()) and applying the refspec onto refs/heads/master (with git_refspec_transform()) do the trick?

nulltoken commented 11 years ago

@jamill @dahlbyk Forget about my previous comment. This would work, but would replicate most of libgit2 logic.

I sent PR libgit2/libgit2#1245 to rather expose a native method able to perform this refspec transformation.

dahlbyk commented 11 years ago

Cool, that PR looks like what I had in mind.

dahlbyk commented 11 years ago

On a related note, I noticed Branch.Remote manually resolves its branch.*.remote config - as that's one component of the tracking branch equation, should retrieving that particular config value be part of the libgit2 public API?

Thoughts on this question?

nulltoken commented 11 years ago

On a related note, I noticed Branch.Remote manually resolves its branch.*.remote config - as that's one component of the tracking branch equation, should retrieving that particular config value be part of the libgit2 public API?

Thoughts on this question?

Wouldn't it only be a wrapper for accessing the config entry? If that's the case, I really don't think this would get merged.

/cc @carlosmn

carlosmn commented 11 years ago

Yeah, that's all it would be, as that's the only way to determine the branch's upstream repository.

nulltoken commented 11 years ago

@dahlbyk @jamill I've worked a bit on this (cf . nulltoken/topic/tracked).

I was thinking about creating a new type: The VoidReference. It would only be used for internal purpose, would bear a name, return a null TargetIdentifier and resolve to a null GitObject. This would be used to create an orphaned Branch.

How do you feel about this?

jamill commented 11 years ago

@nulltoken Thanks! that seems reasonable

dahlbyk commented 11 years ago

:+1: for VoidReference

Quick note on https://github.com/nulltoken/libgit2sharp/compare/topic/tracked#L3R165: I don't see any reason to coalesce to "" - an empty name seems less correct than no name.

nulltoken commented 11 years ago

Quick note on https://github.com/nulltoken/libgit2sharp/compare/topic/tracked#L3R165: I don't see any reason to coalesce to "" - an empty name seems less correct than no name.

@dahlbyk Once again, you're right! :heart: Please see #305 for final review.

nulltoken commented 11 years ago

Fixed by #305