Incorrect calculated version on new branch without any commits

[Jericho]

Here's an issue I am experiencing when building on AppVeyor but I am unable to reproduce locally.

Let's say the current build number is 0.4.2, let's also say that every push triggers a build on AppVeyor and finally let's say that a step in the build script uses GitVersion to calculate the build version.

Repro steps

• Create a new branch, say release/0.4.3
• Push to GitHub
• Check your AppVeyor build. The calculated version is 0.5.0 which is incorrect
• Here's an example that demonstrates this incorrect result

Steps to restore expected behavior

• Commit a change to the new branch
• Push to GitHub
• Check your AppVeyor build. The calculated version is 0.4.3 which is correct
• Here's an example that demonstrates this result.

I'm guessing GitVersion is confused because the latest commit on the new branch is the same as the latest on the develop branch, but that's just a guess on my part.

I have confirmed this incorrect behavior with GitVersion 3.6.2 I'll double check and confirm if 4.0 beta also suffers from this issue.

[Jericho]

I created a GitHub repo with a simplistic build script to reproduce this issue.

I confirm that I was able to reproduce this issue with the following version of GitVersion:

• 3.6.2
• 3.6.3
• 3.6.4
• 3.6.5
• 4.0.0-beta0012

In the following screenshot (taken from here), I have circled in red the build versions that were calculated incorrectly and circled in green the build versions that were calculated correctly:

The pattern is consistent: every time I push a new branch without any commits, the version is incorrect. As soon as I push a commit to this new branch the version is calculated correctly.

[gep13]

@Jericho to rule out any issues with caching on AppVeyor, can you remove this line:

https://github.com/Jericho/GitVersion_Issue_Repro/blob/master/appveyor.yml#L24

And then re-run the tests again?

Even though you are changing the version number of the tool in the pre-processor directive, if GitVersion is already resolved locally in the tools folder, it won't be updated.

This issue is being corrected in 0.22.0 of Cake.

[gep13]

i.e. I want to rule out this:

https://ci.appveyor.com/project/Jericho/gitversion-issue-repro/build/4.0.12-rc.1.build.18#L6

[Jericho]

@gep13 as you suggested I removed the caching from appveyor.yml and ran my tests again. Unfortunately, same result:

[gep13]

@Jericho Can I suggest that you use this:

https://www.gep13.co.uk/blog/how-to-use-appveyor-remote-desktop-connection

To try checking directly on the server to see if there is something "strange" going on when running on AppVeyor? The fact that it is working locally means it has to be something on the environment.

[Jericho]

I RDP'd to the appveyor box and manually ran the GitVersion tool from command prompt with /diag and here's the result:

PS C:\projects\gitversion-issue-repro> C:/projects/gitversion-issue-repro/tools/GitVersion.CommandLine/tools/GitVersion.
exe -output json /diag
INFO [09/06/17 12:50:30:45] Dumping commit graph:
INFO [09/06/17 12:50:30:63] * bdd1463 41 minutes ago  (HEAD -> master, origin/release/4.0.123, origin/master, origin/HEA
D, release/4.0.123)
| * 422adf8 19 hours ago  (origin/release/4.0.122, release/4.0.122)
|/
* 1f8592f 19 hours ago
| * 71e0deb 19 hours ago  (origin/release/3.6.52, release/3.6.52)
|/
* 9059cfd 19 hours ago
| * 9688bad 19 hours ago  (origin/release/3.6.42, release/3.6.42)
|/
* 1da2936 19 hours ago
| * aa1fee8 19 hours ago  (origin/release/3.6.32, release/3.6.32)
|/
* e5b685b 19 hours ago
| * d4c8fbb 19 hours ago  (origin/release/3.6.22, release/3.6.22)
|/
* 14c074e 19 hours ago
* c21c9db 21 hours ago
* 67dc0a5 21 hours ago
| * 2676432 23 hours ago  (origin/release/4.0.12, release/4.0.12)
|/
* 81784e9 23 hours ago
* 9a2020f 23 hours ago
| * ee43147 23 hours ago  (origin/release/3.6.5, release/3.6.5)
|/
* e70e066 23 hours ago
| * bdb6f7f 23 hours ago  (origin/release/3.6.4, release/3.6.4)
|/
* 90f32b2 23 hours ago
| * 00a239b 23 hours ago  (origin/release/3.6.3, release/3.6.3)
|/
* 02c300e 23 hours ago
| * a4e9c15 23 hours ago  (origin/release/3.6.2, release/3.6.2)
|/
* 5a10e05 23 hours ago
* b1009c6 24 hours ago

INFO [09/06/17 12:50:30:64] Working directory: C:\projects\gitversion-issue-repro
INFO [09/06/17 12:50:30:65] IsDynamicGitRepository: False
INFO [09/06/17 12:50:30:69] Returning Project Root from DotGitDirectory: C:\projects\gitversion-issue-repro\.git - C:\pr
ojects\gitversion-issue-repro
INFO [09/06/17 12:50:30:70] Running on Windows.
INFO [09/06/17 12:50:30:71] IsDynamicGitRepository: False
INFO [09/06/17 12:50:30:71] Returning Project Root from DotGitDirectory: C:\projects\gitversion-issue-repro\.git - C:\pr
ojects\gitversion-issue-repro
INFO [09/06/17 12:50:30:71] Project root is: C:\projects\gitversion-issue-repro
INFO [09/06/17 12:50:30:71] DotGit directory is: C:\projects\gitversion-issue-repro\.git
INFO [09/06/17 12:50:30:72] IsDynamicGitRepository: False
INFO [09/06/17 12:50:30:72] Returning Project Root from DotGitDirectory: C:\projects\gitversion-issue-repro\.git - C:\pr
ojects\gitversion-issue-repro
INFO [09/06/17 12:50:30:77] IsDynamicGitRepository: False
INFO [09/06/17 12:50:30:77] Returning Project Root from DotGitDirectory: C:\projects\gitversion-issue-repro\.git - C:\pr
ojects\gitversion-issue-repro
INFO [09/06/17 12:50:30:90] Using latest commit on specified branch
INFO [09/06/17 12:50:30:92] Running against branch: master (bdd146337409ea0e9d4375dd23199cd107a92aac)
INFO [09/06/17 12:50:30:93] Begin: Calculating base versions
  INFO [09/06/17 12:50:30:97] Fallback base version: 0.1.0 with commit count source b1009c6fb0636cf2a5809978f312f6f9bdb0
10f4 (Incremented: None)
  INFO [09/06/17 12:50:31:08] Found multiple base versions which will produce the same SemVer (0.1.0), taking oldest sou
rce for commit counting (Fallback base version)
  INFO [09/06/17 12:50:31:08] Base version used: Fallback base version: 0.1.0 with commit count source b1009c6fb0636cf2a
5809978f312f6f9bdb010f4 (Incremented: None)
INFO [09/06/17 12:50:31:08] End: Calculating base versions (Took: 153.15ms)
INFO [09/06/17 12:50:31:10] Skipping version increment
INFO [09/06/17 12:50:31:10] 14 commits found between b1009c6fb0636cf2a5809978f312f6f9bdb010f4 and bdd146337409ea0e9d4375
dd23199cd107a92aac

[Jericho]

one thing I find odd is: INFO [09/06/17 12:50:30:92] Running against branch: master (bdd146337409ea0e9d4375dd23199cd107a92aac). If I understand this message correctly, it means that GitVersion thinks it's building the master branch but that's not the case, I am trying to build a release branch.

This goes back to what I said when I opened this issue: I think GitVersion is confused by the fact that the latest commit on the release branch is also the latest commit in the master branch since I haven't committed anything to the release yet.

[Jericho]

Wait, something just occurred to me: AppVeyor checks out a specific commit like so:

Build started
git config --global core.autocrlf true
git clone -q --branch=master https://github.com/Jericho/GitVersion_Issue_Repro.git C:\projects\gitversion-issue-repro
git checkout -qf bdd146337409ea0e9d4375dd23199cd107a92aac
.\build.ps1 -Target "AppVeyor"

but, as I said, this commit is both the latest on the master branch and the latest on my release branch so how is GitVersion supposed to figure out that I want the release branch? When I run GitVersion locally, it probably looks at some git metadata to figure it out.

[Jericho]

I noticed something else: in the clone command, AppVeyor specifically specifies "master".

git clone -q --branch=master https://github.com/Jericho/GitVersion_Issue_Repro.git C:\projects\gitversion-issue-repro

Could this issue be caused by AppVeyor rather than GitVersion?

[Jericho]

I'll see if I can get @FeodorFitsner help with the investigation.

[Jericho]

I opened a ticket with AppVeyor, you can follow the discussion here: http://help.appveyor.com/discussions/problems/7739-appveyor-picks-the-wrong-branch

[Jericho]

@IlyaFinkelshteyn responded to my AppVeyor ticket and pointed out that I may have confused the numerous builds. The --branch=master is not from one of the builds where the version is miscalculated. So I'm back to suspecting GitVersion from selecting the wrong branch.

Looking at the output of the GitVersion command with diag flag, I notice the following:

INFO [09/06/17 12:41:23:98] One remote found (origin -> 'https://github.com/Jericho/GitVersion_Issue_Repro.git').
  INFO [09/06/17 12:41:23:99] Fetching from remote 'origin' using the following refspecs: +refs/heads/*:refs/remotes/origin/*.
  INFO [09/06/17 12:41:24:22] Skipping update of 'refs/remotes/origin/release/3.6.2' as it already matches the remote ref.
  INFO [09/06/17 12:41:24:22] Skipping update of 'refs/remotes/origin/release/3.6.22' as it already matches the remote ref.
  INFO [09/06/17 12:41:24:22] Skipping update of 'refs/remotes/origin/release/3.6.3' as it already matches the remote ref.
  INFO [09/06/17 12:41:24:23] Skipping update of 'refs/remotes/origin/release/3.6.32' as it already matches the remote ref.
  INFO [09/06/17 12:41:24:23] Skipping update of 'refs/remotes/origin/release/3.6.4' as it already matches the remote ref.
  INFO [09/06/17 12:41:24:23] Skipping update of 'refs/remotes/origin/release/3.6.42' as it already matches the remote ref.
  INFO [09/06/17 12:41:24:23] Skipping update of 'refs/remotes/origin/release/3.6.5' as it already matches the remote ref.
  INFO [09/06/17 12:41:24:23] Skipping update of 'refs/remotes/origin/release/3.6.52' as it already matches the remote ref.
  INFO [09/06/17 12:41:24:24] Skipping update of 'refs/remotes/origin/release/4.0.12' as it already matches the remote ref.
  INFO [09/06/17 12:41:24:24] Skipping update of 'refs/remotes/origin/release/4.0.122' as it already matches the remote ref.
  INFO [09/06/17 12:41:24:24] Skipping update of 'refs/remotes/origin/release/4.0.123' as it already matches the remote ref.
  INFO [09/06/17 12:41:24:24] HEAD points at branch 'refs/heads/master'.

Is it normal to see HEAD points at branch 'refs/heads/master'. despite the fact that I want a release branch to be built?

[Jericho]

Just to make sure we are on the same page, let me reiterate that I created a new branch, pushed it to GitHub but I haven't committed anything to this new branch. Visually, my repo tree looks like this:

and GitVersion visually displays this same information like so:

(HEAD -> master, origin/release/4.0.123, origin/master, origin/HEAD, release/4.0.123)

I'm mentioning this again because I think it's important. GitVersion seems to think the 'HEAD' points to the master branch. With that in mind, please consider the following code in GitVersionCore\ExecuteCore.cs

return gitPreparer.WithRepository(repo =>
{
    var gitVersionContext = new GitVersionContext(repo, configuration, commitId: commitId);
    var semanticVersion = versionFinder.FindVersion(gitVersionContext);

    return VariableProvider.GetVariablesFor(semanticVersion, gitVersionContext.Configuration, gitVersionContext.IsCurrentCommitTagged);
});

and more specifically, line 111:

var gitVersionContext = new GitVersionContext(repo, configuration, commitId: commitId);

This line creates a new GetVersionContext but it does not specify the desired branch and therefore the constructor assumes that the desired branch is the HEAD. Here is the code in the constructor that makes the assumption (notice: repository.Head):

this(repository, repository.Head, configuration, isForTrackingBranchOnly, commitId)

In my scenario, this assumption is wrong: the desired branch is not the HEAD because it points to "master". Therefore, I propose the following change which attempts to pick the desired branch instead of simply assuming the HEAD:

return gitPreparer.WithRepository(repo =>
{
    var currentBranch = repo.Branches.SingleOrDefault(b => b.CanonicalName == targetBranch) ?? repo.Head;
    var gitVersionContext = new GitVersionContext(repo, currentBranch, configuration, commitId: commitId);
    var semanticVersion = versionFinder.FindVersion(gitVersionContext1);

    return VariableProvider.GetVariablesFor(semanticVersion, gitVersionContext.Configuration, gitVersionContext.IsCurrentCommitTagged);
});

[asbjornu]

@Jericho: If you make that change to GitVersion, run all the tests and they are all green, we'll happily merge a PR with that change in it. Even better if that PR includes a test that verifies your specific scenario.

[Jericho]

@asbjornu all tests are green and I was having a discussion with @pascalberger last night about writing a unit test but I pointed out to him that current testing fixtures don't seem to exhibit the "bug" I'm trying to solve. For instance, take this existing unit test:

[Test]
public void CanTakeVersionFromReleaseBranch()
{
    using (var fixture = new EmptyRepositoryFixture())
    {
        fixture.Repository.MakeATaggedCommit("1.0.3");
        fixture.Repository.MakeCommits(5);
        fixture.Repository.CreateBranch("release-2.0.0");
        fixture.Checkout("release-2.0.0");

        fixture.AssertFullSemver("2.0.0-beta.1+0");
        fixture.Repository.MakeCommits(2);
        fixture.AssertFullSemver("2.0.0-beta.1+2");
    }
}

If I understand this test correctly, branch "release-2.0.0" is checked out immediately after being created and the version number is successfully asserted to be "2.0.0-beta.1+0" which, based on my investigation, is not the case in reality (and that's specifically what my PR is trying to fix). In other words, I can't seem to be able to reproduce the issue I'm trying to solve. So (and I hope I'm wrong) it seems like the testing fixtures do not 100% match reality.

Thoughts?

[Jericho]

Can I get some guidance on unit testing? As I pointed out, unless I'm wrong, I think the testing fixture does not exhibit the issue that my PR is solving so I don't know how I can effectively demonstrate that my PR indeed solves the issue.

[asbjornu]

I believed the RepositoryFixture based tests were performing actual Git commands against the file system, which makes it seem strange that the issue can't be reproduced in the tests. @JakeGinnivan, can you shed some light on this?

Relatedly, this is one of many areas in GitVersion where #1244 might have a huge impact.

[Jericho]

I continued investigating and, it took me while, but I realized the RepositoryFixtureBase has a Checkout method that accept the name of a branch but it doesn't have a method that accepts the SHA of a commit.

What I think I would need in order to be able to fully reproduce the issue is something like this:

[Test]
public void CanTakeVersionFromReleaseBranch()
{
    using (var fixture = new EmptyRepositoryFixture())
    {
        fixture.Repository.MakeATaggedCommit("1.0.3");
        var sha = fixture.Repository.MakeCommit();
        fixture.Repository.CreateBranch("release-2.0.0"); // At this point, the commit is both on master and on my new branch
        fixture.Checkout("release-2.0.0");
        fixture.Checkout(sha);

        fixture.AssertFullSemver("2.0.0-beta.1+0"); // Before my PR this assertion would fail and it will pass with my PR
    }
}

[Jericho]

Continuing my research, I made some progress: I found out that the existing Checkout accepts a branch name OR sha and I also found out fixture.Repository.MakeATaggedCommit returns a Commit which allows me to get the SHA of the new commit. Therefore I can write a unit test like this:

using (var fixture = new EmptyRepositoryFixture())
{
    fixture.Repository.MakeATaggedCommit("1.0.3");
    var commit = fixture.Repository.MakeACommit();
    fixture.Repository.CreateBranch("release-2.0.0");
    fixture.Checkout("release-2.0.0");
    fixture.Checkout(commit.Sha);

    fixture.AssertFullSemver("2.0.0-beta.1+0");
}

However, I get an exception: It looks like the branch being examined is a detached Head pointing to commit '01274a9'. Without a proper branch name GitVersion cannot determine the build version.

I'm continuing my research to try to understand why AssertFullSemver throws this error when GitVersion is in fact able to calculate a version (albeit an incorrect one, which is the bug I'm fixing).

[Jericho]

Continuing my research I made some more progress: it looks like the ExecuteCore.ExecuteInternal method that I am trying to unit test is not being invoked by the testing fixture but instead the logic has been re-written in GitToolsTestingExtensions.GetVersion.

Specifically, it appears that ExecuteCore.ExecuteInternal and GitToolsTestingExtensions.GetVersion both create an instance of GitVersionContext and they both do it in a way that makes it so master is the default branch despite the fact that some other branch is desired. This leads me to conclude that I must copy my fix from ExecuteCore.ExecuteInternal and paste it in GitToolsTestingExtensions.GetVersion. Doesn't seem right to me but if that's how it needs to be done in GitVersion, then so be it.

One last thing: ExecuteCore.ExecuteInternal has a parameter called targetBranch which I use in my fix, but GitToolsTestingExtensions.GetVersion does not have such a parameter. If I want to be able to paste my fix, I also need this parameter.

Please note that this doesn't explain why this issue results in GitVersion miscalculating the version while it causes the testing fixture to throw an exception but at this point I'm loosing patience and I don't really feel like investigating why they behave differently.

[pascalberger]

Doesn't make sense for me too. @JakeGinnivan @asbjornu Do you have any idea why it was implemented like this?

[Jericho]

By the way, in case I was not 100% clear, the duplicated code is here and here.

[asbjornu]

Duplicated code should be unnecessary. Sounds a bit like it touches on #883 and any change in the codebase that avoids duplication and ensures centralized logic receives a rain of 👍 😄 .

[Jericho]

ok, but what does it mean for my PR?

• Do I copy/paste my fix into GitToolsTestingExtensions so I can write a unit test and thereby contributing to the duplication issue?
• Can my PR be accepted without a new unit test? All existing tests are green. @asbjornu previously suggested this might be the way to go.

[asbjornu]

Is it not possible to encapsulate the duplicated code within ExecuteCore so it doesn't need to be duplicated in GitToolsTestingExtensions? I would really like to have a test that covers this.

[Jericho]

ok, challenge accepted. Let me see how I can encapsulate the duplicate code.

[Jericho]

I came up with a good solution: I moved the logic to properly select the desired branch to a new GitVersionContext constructor and I made sure this constructor is invoked both in ExecuteCore and in the testing fixture.

I was also going to suggest adding the obsolete attribute to the GetVersionContext constructor that is causing the issue. What do you think? Something like this:

[Obsolete("This constructor does not always handle scenarios where the desired commit is on multiple branches. Therefore you should use one of the constructors that allow you to specify the desired branch.")]
public GitVersionContext(IRepository repository, Config configuration, bool isForTrackingBranchOnly = true, string commitId = null)
    : this(repository, repository.Head, configuration, isForTrackingBranchOnly, commitId)
{
}

[asbjornu]

Excellent! How many places is that obsolete constructor in use? Is it a massive task to just delete it and update its usage to you newly created constructor?

[Jericho]

I only found the obsolete constructor used in two places: the "Build" method in GitVersionCore.Test/GitVersionContextBuilder.cs and the "CanFindParentBranchForInheritingIncrementStrategy" unit test in GitVersionCore.Test/GitVersionContextTests.cs.

I'll be happy to delete the constructor and adjust these two methods to invoke the new constructor.

[asbjornu]

Please do!

[Jericho]

Done. Hopefully the PR is now ready to be merged!