Fix bug: VersionInBranchNameVersionStrategy only considers the release branch #3455

HHobeck · 2023-03-31T15:46:35Z

Fix [Bug] VersionInBranchNameVersionStrategy only considers the release branch #2693

Is related to #2336

Close #2693

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

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.

HHobeck · 2023-04-01T08:44:25Z

Okay I'm done please review and merge to main.

asbjornu

Nice! Does version-in-branch-pattern require is-release-branch to be true? If so, we should say something about that in the documentation.

docs/input/docs/reference/configuration.md

asbjornu · 2023-04-01T10:38:25Z

src/GitVersion.Core.Tests/IntegrationTests/HotfixBranchScenarios.cs

@@ -167,7 +167,7 @@ public void FeatureOnHotfixFeatureBranchDeleted()
        fixture.Checkout(hotfix451);
        fixture.MergeNoFF(featureBranch); // commit 2
        fixture.Repository.Branches.Remove(featureBranch);
-        fixture.AssertFullSemver("4.5.1-beta.2", configuration);
+        fixture.AssertFullSemver("4.5.1-beta.3", configuration);


Why did this go from 4.5.1-beta.2 to 4.5.1-beta.3?

Hmm very good question. I think it has something to do with the logic in GitVersionVariables.cs which you know will be changed in PR #2347. I have checked the source of the resulting base version and it looks okay for me:

But the .3 in 4.5.1-beta.3 indicates that there's 3 commits since the version source, but it is only 2 indicated by the previous .2 as in 4.5.1-beta.2. So this feels like a bug to me? Why does it count to 3 and not 2?

I have take a look and I think it's correct. If you take a look to the following figure:

then you see that GitVersion uses the highest version (4.5.1). If there are ambiguous results (like in this case) the oldest commit as a source will be taken. Here it is the commit with the identifier bf7a8f7f. Thus if you have this in mind 4.5.1-beta.3 is totally correct. See yor comment here #2394 (comment)

If you don't agree then we need to create a bug or feature issue and change the logic. But this has nothing to do with this PR IMO.

asbjornu · 2023-04-01T10:39:24Z

src/GitVersion.Core.Tests/IntegrationTests/HotfixBranchScenarios.cs

@@ -60,7 +60,7 @@ public void CanTakeVersionFromHotfixesBranch()

        // create hotfix branch
        Commands.Checkout(fixture.Repository, fixture.Repository.CreateBranch("hotfixes/1.1.1"));
-        fixture.AssertFullSemver("1.1.0"); // We are still on a tagged commit
+        fixture.AssertFullSemver("1.1.1+0");


Are we not still on a tagged commit here?

Hmm very good question. I think you have found a bug. ;) The same question applies to the following scenario:

[Test] public void __Just_A_Test__() { using var fixture = new EmptyRepositoryFixture(); fixture.MakeATaggedCommit("1.0.0"); fixture.BranchTo("release/1.1.0"); fixture.AssertFullSemver("1.1.0+0"); }

Why it yields to 1.1.0 and not to 1.0.0?

I think this is one of those edge-cases that no matter which solution we pick, we are going to make some people unhappy. I'm in the "a tag should always win" camp, but I can definitely see how some people would expect release/1.1.0 to yield 1.1.0 even though the same commit is tagged 1.0.0. I don't think there's a right answer here, we just need to pick one way or the other, stick to it and document it well.

I think this discussion is related to this issue here: Same version computed on different branches #3453

We need to introduce a new branch related property (like take-incremented-version) or make the behavior dependent on the used version-mode option and make it configurable. The point is if you are on the hotfix branch you don't want to have properly the tagged version you would like to have the next version 1.1.0+0. Other way around if you are on the main branch you would like to have the tagged version 1.0.0. In both cases you are on the same commit with different result dependent on what branch you are. If you don't want this behavior neither then you are able to change it. I see the following enum values: TakeAlwaysBaseVersion, TakeTaggedOtherwiseIncrementedVersion, TakeAlwaysIncrementedVersion

If we were to base this on mode, how would you suggest we do it?

Hmm I think it would be not a good idea to put it on the version-mode option because it is very confusing and would lead to issues complaining about that the tag is not recognice correctly.

Also if I think about the behavior of real live deplyoment scenarios (trunk-based, continues-deployment, continues-delivery and manually-deployment) this question is totally independent of that. Because it depends on the fact: Do I create a artifact after I have tagged the commit or before. Or do I take a pre-release version and just re-declare it to production version or not.

What is your opinion about that?

asbjornu · 2023-04-01T18:31:22Z

Nice! Does version-in-branch-pattern require is-release-branch to be true? If so, we should say something about that in the documentation.

Can you please answer this, @HHobeck? ☝🏼

HHobeck · 2023-04-02T07:09:24Z

Nice! Does version-in-branch-pattern require is-release-branch to be true? If so, we should say something about that in the documentation.

Can you please answer this, @HHobeck? ☝🏼

Yes you are right the property version-in-branch-pattern effects only branches where the is-release-branch option is set to true. I have changed the documentation :)

asbjornu · 2023-04-03T09:36:19Z

...sion.Core.Tests/VersionCalculation/Strategies/VersionInBranchNameBaseVersionStrategyTests.cs

@@ -80,6 +82,7 @@ public void CanTakeVersionFromNameOfConfiguredReleaseBranch(string branchName, s
        var repository = fixture.Repository.ToGitRepository();

        var configuration = GitFlowConfigurationBuilder.New
+            .WithLabelPrefix("([vV]|lts-)?")


Do we need to document this change being required, perhaps?

What is requirement behind it to support lts before the number will be present? For sure if you define support/2.0.0-lts it works by default if you define support/lts-2.0.0 it's not working by default and a change in the configuration needs to be done (either changing label-prefix or version-in-branch-name). I think this line is not necessary anymore because I have changed the test case. Done.

Perhaps we can add a negative test for this, then? To illustrate that hotfix/downgrade-some-lib-to-3.2.1 don't generate the version number 3.2.1 by default. Such a test case would also provide as good documentation of why this change is made in the first place.

I have added a new test DoesNotTakeVersionFromNameWhenItHasBeenAccidentalSpecifiedInBranch

...sion.Core.Tests/VersionCalculation/Strategies/VersionInBranchNameBaseVersionStrategyTests.cs

src/GitVersion.Core/Configuration/GitFlowConfigurationBuilder.cs

src/GitVersion.Core/VersionCalculation/SemanticVersioning/ReferenceNameExtensions.cs

HHobeck · 2023-04-03T14:01:02Z

Okay I'm done with the integration of review comments. Please check and merge to main.

asbjornu

There's suddenly a lot of formatting changes here. Please do formatting and refactoring in separate pull requests from bug fixes and feature implementations.

Also, I would love to see a test case added for hotfix/downgrade-library-to-3.1.2 not yielding the version number 3.1.2 as that seems to be one of the primary reasons behind this PR.

src/GitVersion.Core/Git/ReferenceName.cs

src/GitVersion.Core/Git/SemanticVersion.cs

src/GitVersion.Core/Git/SemanticVersionBuildMetaData.cs

HHobeck · 2023-04-04T06:24:59Z

There's suddenly a lot of formatting changes here. Please do formatting and refactoring in separate pull requests from bug fixes and feature implementations.

Also, I would love to see a test case added for hotfix/downgrade-library-to-3.1.2 not yielding the version number 3.1.2 as that seems to be one of the primary reasons behind this PR.

You mean such tests!? There are already existing. Nevermind let me know if you are missing test scenarios.

[Test]
public void DoesNotTakeVersionFromNameOfNonReleaseBranch()
{
    using var fixture = new BaseGitFlowRepositoryFixture("1.0.0");
    fixture.CreateAndMergeBranchIntoDevelop("pull-request/improved-by-upgrading-some-lib-to-4.5.6");
    fixture.CreateAndMergeBranchIntoDevelop("hotfix/downgrade-some-lib-to-3.2.1-to-avoid-breaking-changes");

    fixture.AssertFullSemver("1.1.0-alpha.5");
}

[TestCase("release-2.0.0", "2.0.0")]
[TestCase("release/3.0.0", "3.0.0")]
[TestCase("support/2.0.0-lts", "2.0.0")]
[TestCase("support-3.0.0-lts", "3.0.0")]
[TestCase("hotfix/2.0.0", "2.0.0")]
[TestCase("hotfix-3.0.0", "3.0.0")]
[TestCase("hotfix/2.0.0-lts", "2.0.0")]
[TestCase("hotfix-3.0.0-lts", "3.0.0")]
public void CanTakeVersionFromNameOfConfiguredReleaseBranch(string branchName, string expectedBaseVersion)
{
    using var fixture = new EmptyRepositoryFixture();

    fixture.MakeACommit();
    fixture.CreateBranch(branchName);

    var repository = fixture.Repository.ToGitRepository();

    var configuration = GitFlowConfigurationBuilder.New
        .WithBranch("support", builder => builder.WithIsReleaseBranch(true))
   ...
}

…e branch

./build.ps1 -Stage build -Target BuildPrepare ./build.ps1 -Stage docs -Target GenerateSchemas

…it test

…enceName.cs

…ions

…dent: add 2 spaces list-item-indent remark-lint

.github/ISSUE_TEMPLATE/documentation.md

src/GitVersion.Core.Tests/IntegrationTests/VersionInMergedBranchNameScenarios.cs

…chNameScenarios.cs Co-authored-by: Asbjørn Ulsberg <[email protected]>

asbjornu

Fabulous! 🚀

mergify · 2023-04-06T05:34:39Z

Thank you @HHobeck for your contribution!

HHobeck force-pushed the feature/2693_VersionInBranchNameVersionStrategy_only_considers_the_release_branch branch 2 times, most recently from bd3c051 to ec14032 Compare April 1, 2023 08:18

arturcic requested a review from asbjornu April 1, 2023 10:32

asbjornu requested changes Apr 1, 2023

View reviewed changes

asbjornu changed the title ~~Fix bug: VersionInBranchNameVersionStrategy only considers the releae branch~~ Fix bug: VersionInBranchNameVersionStrategy only considers the release branch Apr 3, 2023

asbjornu requested changes Apr 3, 2023

View reviewed changes

HHobeck force-pushed the feature/2693_VersionInBranchNameVersionStrategy_only_considers_the_release_branch branch from 844f00e to ad5f11d Compare April 3, 2023 11:15

asbjornu requested changes Apr 3, 2023

View reviewed changes

HHobeck force-pushed the feature/2693_VersionInBranchNameVersionStrategy_only_considers_the_release_branch branch from 0560fd4 to 4c10925 Compare April 4, 2023 08:08

HHobeck added 13 commits April 4, 2023 20:14

Fix bug: VersionInBranchNameVersionStrategy only considers the releas…

4531e3c

…e branch

Fix VersionInBranchNameVersionStrategy only considers the releae branch

9faa790

Fix some unit tests

1992f42

Gnerating the json schemas with:

0ea5c52

./build.ps1 -Stage build -Target BuildPrepare ./build.ps1 -Stage docs -Target GenerateSchemas

Extract TryGetSemanticVersion method into extensions class

d54cd69

Change documentation

26b5465

minor refactoring in MergeMessage.cs

a50892a

Change breaking change documentation and remove WithLabelPrefix in un…

14d33d8

…it test

Remove ReferenceNameExtensions and integrate functionality into Refer…

cbfce46

…enceName.cs

Integrate some StringExtensions function into GitVersionCommon/Extens…

fa54904

…ions

Add additional unit test

3ade5b3

Link StringExtensions class as reference to new-cli solution

fec1a63

Try to fix [remark-lint] reported by reviewdog Incorrect list-item in…

907579c

…dent: add 2 spaces list-item-indent remark-lint

asbjornu requested changes Apr 4, 2023

View reviewed changes

.github/ISSUE_TEMPLATE/documentation.md Outdated Show resolved Hide resolved

src/GitVersion.Core.Tests/IntegrationTests/VersionInMergedBranchNameScenarios.cs Outdated Show resolved Hide resolved

Update src/GitVersion.Core.Tests/IntegrationTests/VersionInMergedBran…

3795785

…chNameScenarios.cs Co-authored-by: Asbjørn Ulsberg <[email protected]>

arturcic force-pushed the feature/2693_VersionInBranchNameVersionStrategy_only_considers_the_release_branch branch from 137647f to 3795785 Compare April 5, 2023 07:49

arturcic requested a review from asbjornu April 5, 2023 08:15

asbjornu approved these changes Apr 5, 2023

View reviewed changes

arturcic merged commit 03271d6 into GitTools:main Apr 6, 2023

HHobeck mentioned this pull request Feb 6, 2024

[Feature] Same version computed on different branches #3898

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug: VersionInBranchNameVersionStrategy only considers the release branch #3455

Fix bug: VersionInBranchNameVersionStrategy only considers the release branch #3455

HHobeck commented Mar 31, 2023 •

edited

Loading

HHobeck commented Apr 1, 2023

asbjornu left a comment

asbjornu Apr 1, 2023

HHobeck Apr 1, 2023

asbjornu Apr 1, 2023

HHobeck Apr 2, 2023 •

edited

Loading

asbjornu Apr 1, 2023

HHobeck Apr 1, 2023 •

edited

Loading

asbjornu Apr 1, 2023

HHobeck Apr 2, 2023 •

edited

Loading

asbjornu Apr 3, 2023

HHobeck Apr 3, 2023 •

edited

Loading

asbjornu commented Apr 1, 2023

HHobeck commented Apr 2, 2023

asbjornu Apr 3, 2023

HHobeck Apr 3, 2023 •

edited

Loading

asbjornu Apr 3, 2023

HHobeck Apr 4, 2023

HHobeck commented Apr 3, 2023

asbjornu left a comment

HHobeck commented Apr 4, 2023

asbjornu left a comment

mergify bot commented Apr 6, 2023

Fix bug: VersionInBranchNameVersionStrategy only considers the release branch #3455

Fix bug: VersionInBranchNameVersionStrategy only considers the release branch #3455

Conversation

HHobeck commented Mar 31, 2023 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

HHobeck commented Apr 1, 2023

asbjornu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HHobeck Apr 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HHobeck Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HHobeck Apr 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HHobeck Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

asbjornu commented Apr 1, 2023

HHobeck commented Apr 2, 2023

Choose a reason for hiding this comment

HHobeck Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HHobeck commented Apr 3, 2023

asbjornu left a comment

Choose a reason for hiding this comment

HHobeck commented Apr 4, 2023

asbjornu left a comment

Choose a reason for hiding this comment

mergify bot commented Apr 6, 2023

HHobeck commented Mar 31, 2023 •

edited

Loading

HHobeck Apr 2, 2023 •

edited

Loading

HHobeck Apr 1, 2023 •

edited

Loading

HHobeck Apr 2, 2023 •

edited

Loading

HHobeck Apr 3, 2023 •

edited

Loading

HHobeck Apr 3, 2023 •

edited

Loading