RFC regarding the calculation of Base Versions

[franknarf8]

Dear Reviewers,

Please take a look at the discussion here : #2798

TL;DR : I'm proposing we use the BaseVersionSource of Tags before MergeMessage, if there are any.

I think there might be implications I am not foreseeing and would like your input on this proposition.

Related Issue

Resolves #2798.
Related to #1526.
Fixes #2394.

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.

[asbjornu]

I think this is an improvement, but as it is a breaking change, I think we need to postpone this to GitVersion 6.

[ni-fgenois]

That's a fair point; so when is v6 planned for? And what are the next steps to get this in?

Also, I'm having some trouble fixing the last 2 failing tests, could someone have a look?

[asbjornu]

I think that if you rebase on main, a new run of the tests should turn them green.

[ni-fgenois]

Still seems broken for some other reason :S

[asbjornu]

Just a note for your next PR: Create a disposable branch from main and do your commits in it and submit your pull request based on that branch instead of using main, since main should preferably be identical across forks to make submitting new pull requests easier.

[franknarf8]

@asbjornu Woo-hoo! Got the merge conflict resolved and the test to turn green!

[ni-fgenois]

I don't mind fixing this merge conflict again, but will it be merged afterwards? Does it need more approvals?

[asbjornu]

@ni-fgenois, sorry about not merging this, but as mentioned in #2799 (comment) we need to postpone this to version 6 of GitVersion as it is a breaking change. We don't have a v6 branch yet, but perhaps we should create one so we can merge v6 features like this. Thoughts @arturcic?

[arturcic]

@asbjornu I would go the other way, I'd create a v5.x branch and main would be for the v6. But first I want to finish with some warnings fixing. Version 5.x will be only for bug fixing and no new features will be added.

[ni-fgenois]

Ok, please tell me when the 5.x release will be kicked, and I'll gladly fix the merge conflict :)

Thank you two for the quick response :)

[asbjornu]

Sounds like a plan, @arturcic! 👍🏼

[ni-fgenois]

The 5.x release seem to have been kicked, is it time to merge this in? (If so, I'll rebase the code and fix the tests)

[arturcic]

@ni-fgenois not yet, I'm working on making the branch release\5.x release-able from the build infrastructure, and want to enable the nullable for the entire solution. After that we'll focus on main for v6. For now main and release\5.x will be the same

[asbjornu]

main is now primed for v6. Can you please rebase and solve conflicts so we can merge this, @franknarf8?

[ni-fgenois]

Sorry for the delay, the merge conflict was a bit more challenging this time around.

You might want to also take a look to this new test case I have added here : https://github.com/GitTools/GitVersion/pull/2799/files#diff-d43fa8360d2034a4a95db32f5376f489764f0cb4b4ceec376940a7797a699528R205

Thanks for the notification and all the support :)

[asbjornu]

Awesome, @ni-fgenois. Thanks! Just one thing I thought when reviewing this is that there has been some discussion leading to #3041 lately. I think this PR affects that and want to have your thoughts on the matter.

[ni-fgenois]

This PR would indeed partially impact that other topic; as we would now be taking versioned tags over any other VersionSources (merge commits, etc). But if there is a case where there are no tags present in the VersionSources, the old behavior will still apply.

This relates to another interrogation I had on this topic the other day about the behavior of GitVersion when encoutering tags. I thought that, running GitVersion on a commit with a tag that is a valid SemVer should simply return that version (even when there is some Meta information in the tag). What do you think? I thought this would be a convenient way to lock/override a version (helps with replayability of GitVersion when the CI is automatically tagging the repo).

This came to me when dealing with tags on master containing Release Candidate meta information. I wanted to setup automatic tagging on master while using ContinuousDelivery versioning mode, but GitVersion doesn't use the Metadata of tags, which leads in a different version before and after tagging a version with RC metadata. For example, when running GitVersion a first time results in 2.0.0+0, but adding the 2.0.0+0 tag on that same commit causes the next version to be 2.0.0 instead of staying at 2.0.0+0. I guess we could also reload the CommitCountSource from those tags (if present) and count from there, what do you think?

Here's a test that illustrates this scenario :

[Test]
public void ShouldUseTheSemVerMetadataOfTags()
{
    using var fixture = new EmptyRepositoryFixture();
    fixture.Repository.MakeATaggedCommit("1.0.0");
    fixture.Repository.MakeCommits(1);
    fixture.Repository.CreateBranch("release/2.0.0");
    fixture.Checkout("release/2.0.0");
    fixture.Repository.MakeCommits(4);
    fixture.Checkout(MainBranch);
    fixture.Repository.MergeNoFF("release/2.0.0", Generate.SignatureNow());

    fixture.AssertFullSemver("2.0.0+0");
    fixture.Repository.MakeCommits(1);
    fixture.AssertFullSemver("2.0.0+1");
    fixture.Repository.ApplyTag("2.0.0+1");
    fixture.AssertFullSemver("2.0.0"); // but it should be 2.0.0+1
    fixture.Repository.MakeCommits(1);
    fixture.AssertFullSemver("2.0.1+1"); // but it should be 2.0.0+2
}

[ni-fgenois]

So, I've updated the failing test specified in #2394 to use the new fixtures (see below), and it seems my PR fixes this behavior (the test below is green on my branch; haven't pushed it though).

I haven't found a test case to repro the issue mentioned in : #2394

[Test]
    public void VersionSource()
    {
        using var fixture = new EmptyRepositoryFixture();
        fixture.Repository.MakeATaggedCommit("0.1.0");
        fixture.Checkout(MainBranch);
        fixture.Repository.CreateBranch("develop");
        fixture.Checkout("develop");
        fixture.MakeACommit("Feature commit 1");
        fixture.BranchTo("release/0.2.0");
        fixture.MakeACommit("Release commit 1");
        fixture.Checkout(MainBranch);
        fixture.MergeNoFF("release/0.2.0");
        fixture.ApplyTag("0.2.0");
        var tag = fixture.Repository.Head.Tip;
        fixture.Checkout("develop");
        fixture.MergeNoFF(MainBranch);
        fixture.AssertFullSemver("0.3.0-alpha.1");
        var version = fixture.GetVersion();
        version.VersionSourceSha.ShouldBeEquivalentTo(tag.Sha);
    }

[asbjornu]

That sounds great, @ni-fgenois! Please include the test from #2394 in this PR and write in the description Fixes #2394 so it'll be closed when this is merged.

[ni-fgenois]

Done! :)

On another subject, while I've got you here, what do you think of this behavior illustrated above?

fixture.AssertFullSemver("2.0.0+1");
fixture.Repository.ApplyTag("2.0.0+1");
fixture.AssertFullSemver("2.0.0"); // shouldn't it be 2.0.0+1?

Is there already an explaination for that, or should I open another ticket?

[asbjornu]

Done! :)

Great!

On another subject, while I've got you here, what do you think of this behavior illustrated above?

fixture.AssertFullSemver("2.0.0+1");
fixture.Repository.ApplyTag("2.0.0+1");
fixture.AssertFullSemver("2.0.0"); // shouldn't it be 2.0.0+1?

Is there already an explaination for that, or should I open another ticket?

I'm not sure there is a good explanation other than that everything after + is just metadata and something that is provided by GitVersion, but not something that GitVersion sources from somewhere else. So when GitVersion reads the tag 2.0.0+1 it just ignores +1. Then, when the FullSemVer is generated, the result is 2.0.0 since this is a stable version provided by a tag.

Although I don't think careful consideration has been applied to this behavior, I would dare to say that this is by design and not a bug. Whether the design is correct or not, can of course be discussed.

[asbjornu]

The only thing to point out are some minor code style changes. Also, I think it would be great to document how this works once and for all. Could you please attempt to write something up? I'll help with the formulation, but I would appreciate if you could get the ball rolling. 🙏🏼

[asbjornu]

I believe #3190 touched upon many of the same files as this PR. Can you please rebase and assess whether this PR is relevant and needed anymore?

[ni-fgenois]

To be honest, I kinda lost track of this change over the last couple of months. I'm not even sure if this PR is relevant anymore; I would suggest closing it for now. I can always re-open it if needed.

[HHobeck]

Hmm I see... With the refactoring of #3190 the BaseVersionCalculator class has not been survived. That means your refactoring is obsolete IMO. Sorry for that! But if I see your unit tests then this is almost fixed in 6.x and the behavior is like you would expecting it.

[HHobeck]

On another subject, while I've got you here, what do you think of this behavior illustrated above?

fixture.AssertFullSemver("2.0.0+1");
fixture.Repository.ApplyTag("2.0.0+1");
fixture.AssertFullSemver("2.0.0"); // shouldn't it be 2.0.0+1?

Is there already an explaination for that, or should I open another ticket?

I totally agree with you that the output of GitVersion should yield the same result before and after the tagging has been done. I would go one step further and say: Even if you tag the commit with 2.0.0.

[HHobeck]

Please integrate your ideas and help us to realize #3041 and contribute for version 6.x. I'm going to close this PR because it makes no sense anymore. Thank you very very very much for your input and your work.