Improve ExecuteCore.ExecuteInternal when HEAD is not pointing at the desired branch #1291
Conversation
… pointing at the desired branch
|
@Jericho Thanks for the PR! Would it be possible that you can also create a test case similar to the scenario with your demo repo (branch without a commit on it) and repo checked out to specific commit? |
Jericho
force-pushed
the
head_not_pointing_at_desired_branch
branch
from
adb8ec5
to
9da272c
Compare
…or on the GitVersionContext class This allows this new logic introduced by the PR to be shared by the ExecuteCore class and the the unit testing fixture without having to duplicate code.
Jericho
force-pushed
the
head_not_pointing_at_desired_branch
branch
from
9da272c
to
dfa6c79
Compare
…a given commit is on multiple branches and the desired branch is not specified
| public GitVersionContext(IRepository repository, Config configuration, bool isForTrackingBranchOnly = true, string commitId = null) | ||
| : this(repository, repository.Head, configuration, isForTrackingBranchOnly, commitId) | ||
| public GitVersionContext(IRepository repository, string targetBranch, Config configuration, bool onlyEvaluateTrackedBranches = true, string commitId = null) | ||
| : this(repository, repository.Branches.SingleOrDefault(b => b.CanonicalName == targetBranch || b.FriendlyName == targetBranch) ?? repository.Head, configuration, onlyEvaluateTrackedBranches, commitId) |
There was a problem hiding this comment.
Can you please reduce the number of statements per line here? It's a bit hard to read with so much going on in one line of code. 😃
|
@asbjornu done. |
|
@pascalberger done. I created a file called Speaking of this scenario, I mentioned it before but I want to repeat it because it's been bothering me: if you remove the fix I wrote and run the unit test for my scenario, you would hope to see the version being miscalculated like I observed when using GitVersion on AppVeyor. But that's not the case. The testing fixture throws an exception because it can't handle a detached head in a git repo. I don't know why the behavior is different and, to be honest, I don't have the appetite to investigate but I want everyone to be aware. |
| : this(repository, repository.Branches.SingleOrDefault(b => b.CanonicalName == targetBranch || b.FriendlyName == targetBranch) ?? repository.Head, configuration, onlyEvaluateTrackedBranches, commitId) | ||
| : this(repository, | ||
| repository.Branches.SingleOrDefault(b => b.CanonicalName == targetBranch || b.FriendlyName == targetBranch) ?? repository.Head, | ||
| configuration, onlyEvaluateTrackedBranches, commitId) |
There was a problem hiding this comment.
I only now discovered that this all happens within a constructor. Sorry for not catching that earlier. Could you please add a static method that wraps repository.Branches.SingleOrDefault(b => b.CanonicalName == targetBranch || b.FriendlyName == targetBranch) ?? repository.Head and invoke that method from the constructor?
|
@asbjornu done. Also, instead of just a one-liner, I broke it up into multiple lines which allowed me to add comments to explain each step in the logic. |
|
@Jericho: Beautiful! As soon as the tests are green, this will be merged. Thank you so much for your persistence and tireless effort! 😄 👍 ❤️ |
|
@Jericho Thanks for getting this done and your persistence 👍 |
This resolves #1289