-
Notifications
You must be signed in to change notification settings - Fork 662
Fix HEAD for worktrees #1848
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix HEAD for worktrees #1848
Conversation
|
I'd love if @ermshiperete could have a look at this since this touches on the same code as his (which in turn touches on your previous work, @chuseman). To avoid going too many rounds on this, it would be great if you could agree that this is the right behavior. 😃 |
|
@ermshiperete let me know if you've got any input on this. Here's how to setup a demo of what my patch changes:
GitVersion.exe v5.0.2-beta.1+99.Branch.master.Sha.89f8cb14681020ab9c32ea5e9870e220c1d3905f output: {
"Major":5,
"Minor":0,
"Patch":2,
"PreReleaseTag":"",
"PreReleaseTagWithDash":"",
"PreReleaseLabel":"",
"PreReleaseNumber":"",
"WeightedPreReleaseNumber":"",
"BuildMetaData":99,
"BuildMetaDataPadded":"0099",
"FullBuildMetaData":"99.Branch.master.Sha.89f8cb14681020ab9c32ea5e9870e220c1d3905f",
"MajorMinorPatch":"5.0.2",
"SemVer":"5.0.2",
"LegacySemVer":"5.0.2",
"LegacySemVerPadded":"5.0.2",
"AssemblySemVer":"5.0.2.0",
"AssemblySemFileVer":"5.0.2.0",
"FullSemVer":"5.0.2+99",
"InformationalVersion":"5.0.2+99.Branch.master.Sha.89f8cb14681020ab9c32ea5e9870e220c1d3905f",
"BranchName":"master",
"Sha":"89f8cb14681020ab9c32ea5e9870e220c1d3905f",
"ShortSha":"89f8cb1",
"NuGetVersionV2":"5.0.2",
"NuGetVersion":"5.0.2",
"NuGetPreReleaseTagV2":"",
"NuGetPreReleaseTag":"",
"VersionSourceSha":"c71b8fc9f6d7b6adffe82fef588e717beb864e91",
"CommitsSinceVersionSource":99,
"CommitsSinceVersionSourcePadded":"0099",
"CommitDate":"2019-10-15"
}GitVersion.exe from my branch output: {
"Major":3,
"Minor":6,
"Patch":5,
"PreReleaseTag":"",
"PreReleaseTagWithDash":"",
"PreReleaseLabel":"",
"PreReleaseNumber":"",
"WeightedPreReleaseNumber":"",
"BuildMetaData":"",
"BuildMetaDataPadded":"",
"FullBuildMetaData":"Branch.support-3.6.0.Sha.15ef651d24091f7530fda847c9373774754d8ec1",
"MajorMinorPatch":"3.6.5",
"SemVer":"3.6.5",
"LegacySemVer":"3.6.5",
"LegacySemVerPadded":"3.6.5",
"AssemblySemVer":"3.6.5.0",
"AssemblySemFileVer":"3.6.5.0",
"FullSemVer":"3.6.5",
"InformationalVersion":"3.6.5+Branch.support-3.6.0.Sha.15ef651d24091f7530fda847c9373774754d8ec1",
"BranchName":"support/3.6.0",
"Sha":"15ef651d24091f7530fda847c9373774754d8ec1",
"ShortSha":"15ef651",
"NuGetVersionV2":"3.6.5",
"NuGetVersion":"3.6.5",
"NuGetPreReleaseTagV2":"",
"NuGetPreReleaseTag":"",
"VersionSourceSha":"15ef651d24091f7530fda847c9373774754d8ec1",
"CommitsSinceVersionSource":4,
"CommitsSinceVersionSourcePadded":"0004",
"CommitDate":"2016-11-27"
}You can see here that the current prerelease of GitVersion uses |
|
Looks good. Some unit tests wouldn't be bad though to verify that things work as expected, IMHO... |
|
Do you think you could add some tests to this and update it to |
If you've got a local branch and remote branch with the same name, SingleOrDefault() throws.
GetDotGitDirectory() returns the base repository .git, not the the worktree's .git, so HEAD was wrong. For dynamic repositories, there isn't a .git at all so it still needs to use the discovered .git directory.
|
Can you rebase and fix the merge conflicts so we hopefully get this PR into a green state, @chuseman? :) |
|
I wish I had seen this before, as I just did #2166 to fix the worktree issue (and in the same way had the issue with dynamic repos and independently solved it the same way) |
|
Any updates on this one? |
When running inside a worktree, the HEAD considered is from the default worktree's branch/commit instead of the worktree that GitVersion is executed for. This behavior was introduced in #1793 when
GitPreparer.GetDotGitDirectory()was changed to be more worktree friendly.This commit switches to use
GetProjectRootDirectory()instead ofGetDotGitDirectory()so that we can get the correct branch. I can't think of a scenario where this isn't what you'd expect.Also, when troubleshooting I found a problem when specifying
/bfor a branch. If you have a local branch and a remote branch of the same name, you'll get an exception becauseSingleOrDefault()is used to find the branch you meant. I changed it over to useFirstOrDefault()while preferring local branches first.