Skip to content

Conversation

@ermshiperete
Copy link
Contributor

@ermshiperete ermshiperete commented May 20, 2020

Description

This PR fixes the calculation of the version and branch when using git worktree. This fixes #2165.

Related Issue

#2165

This PR re-uses the tests from and enhances #1848

Motivation and Context

How Has This Been Tested?

git init testgv
cd testgv
git commit -m "Initial commit" --allow-empty
git tag v1.0 -m "v1.0"
git worktree add ../testgv2
cd ../testgv2
git commit -m "Commit on other branch
+semver:major" --allow-empty

When run with an unpatched version of gitversion this will output

...
  "FullSemVer":"1.0.0",
  "InformationalVersion":"1.0.0+Branch.master.Sha.1cb7c58dd04425f3a6aabe7431bf9f78061548f4",
  "BranchName":"master",
...

with these changes the output is:

...
  "FullSemVer":"2.0.0-testgv2.1+1",
  "InformationalVersion":"2.0.0-testgv2.1+1.Branch.testgv2.Sha.222d9fdef4732e7a3800f7b1964437a8e11e2d5d",
  "BranchName":"testgv2",
...

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed (that were passing previously).

This change is Reviewable

Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, @ermshiperete! 👏 If @tpaxatb and @chuseman could do a quick review of this as well, that would be great! 🙏

@arturcic
Copy link
Member

arturcic commented May 21, 2020

@ermshiperete do you think you can move this code

var isDynamicRepo = !string.IsNullOrWhiteSpace(options.Value.DynamicGitRepositoryPath);
return isDynamicRepo ? options.Value.DotGitDirectory :  options.Value.ProjectRootDirectory;

to GitVersionOptionsExtensions and then similarly use it in GitVersionOptions as Lazy property?

It is used in 3 places, that means we can move the code into one place

@arturcic
Copy link
Member

@asbjornu I will merge this one. Looks to be fine. I have done the change I was suggesting and also rebased on top of master.

@arturcic arturcic merged commit be48ce3 into GitTools:master May 21, 2020
@ermshiperete ermshiperete deleted the feat/worktree branch May 22, 2020 06:50
@ermshiperete
Copy link
Contributor Author

Thanks for the quick review & merge + release of a new version 5.3.4 with that fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Worktrees incorrectly calculated

4 participants