GitTools / GitVersion

From git log to SemVer in no time
https://gitversion.net/docs/
MIT License
2.85k stars 650 forks source link

[Bug] Tag: useBranchName isn't working in some cases #2201

Closed zhuravelsv closed 3 years ago

zhuravelsv commented 4 years ago

Describe the bug I faced issue with property "tag: useBranchName", I'm trying to use it, but it's not working in some cases, for example even if I use tag "tag.{BranchName}" I got "FullSemVer":"3.0.0-tag..1+8".

I'm not sure is it bug or not, but all details below

Configuration:

assembly-versioning-scheme: MajorMinorPatchTag
assembly-file-versioning-scheme: MajorMinorPatchTag
assembly-informational-format: '{InformationalVersion}'
tag-prefix: '[vV]'
legacy-semver-padding: 4
build-metadata-padding: 4
commits-since-version-source-padding: 4
commit-message-incrementing: Disabled
continuous-delivery-fallback-tag: prerelease
branches:
  master:
    regex: ^master
    mode: ContinuousDelivery
    tag: useBranchName
    increment: None
    prevent-increment-of-merged-branch-version: true
    track-merge-target: false
    tracks-release-branches: false
    is-release-branch: false
  feature:
    regex: .*
    mode: ContinuousDelivery
    tag: useBranchName
    increment: None
    prevent-increment-of-merged-branch-version: false
    track-merge-target: false
    tracks-release-branches: false
    is-release-branch: false

Expected Behavior

master - tag is "master" custom/foo - tag is "foo" foo - tag is "foo" feature/name - tag is "name" feature - tag is "feature"

Actual Behavior

Tag: useBranchName isn't working for master branch and some other branches: master, custom/foo, foo, feature/name, feature - no tag ( useBranchName: "FullSemVer":"3.0.0+16", tag.{BranchName}: "FullSemVer":"3.0.0+16", tag.{BranchName}: "FullSemVer":"3.0.0-tag..1+16" (if we set tag: tag.{BranchName}) )

But, if we change regex in feature branch config to this "^features?[/-]" it will work because of this: http://regexstorm.net/tester?p=%5efeatures%3f%5b%2f-%5d&i=features%2ffoo regex cut all matched part, and remnants are used for tag, but if we configure regex to match all branches (like in case above) it will not work correctly

Context

I would like to add branch name tag to any branch (except master)

Possible Fix

BaseVersionCalculator.GetBaseVersion return incorrect version

Steps to Reproduce

Just try to use "useBranchName" or "{BranchName}" in master or configure to match all branch names

Your Environment

Azure devops (pipelines, nuget/artifacts), nect core 3.1

asbjornu commented 4 years ago

Interesting. Would it be possible for you to submit a PR that reproduces this problem in a RepositoryFixture test?

manicprogrammer commented 4 years ago

I just ran across this as well. I haven't looked at the code yet but I have a suspicion of what is happening. It seems to occur on any branch that is not namespaced, So develop, master, my-branch but not on any branch like feature/my-feature

The BranchName variable outputs correctly but I suspect how the variable is being parsed when used in a tag is wrong. I suspect it is looking for the last / and using the value after that to put into the tag - but if your branch does not have a namespace it parses the branchname for the purpose of the tag as an empty string.

I'll try to look at the code this weekend and see if my suspicions are correct and if so put in a test and a fix.

TechWatching commented 4 years ago

Any news on this topic ?

dejoost commented 3 years ago

I also ran into this issue today with v5.5.1. As soon as the regex matches the full branch name the BranchName variable returns empty. In my case branch is called "cicd". As proof of concept I tried "regex: ^random\/gitversion" and as expected the BranchName returned again as an empty string in the SemVer

asbjornu commented 3 years ago

Please submit a failing RepositoryFixture test in a PR so it's possible to see exactly what is failing.

bddckr commented 3 years ago

Looks like the suspicion raised above is correct:

The branch name is stripped of BranchPrefixToTrim: https://github.com/GitTools/GitVersion/blob/8f8cca13d6fa25ff59670b0752663edbcf3b8156/src/GitVersionCore/Configuration/ConfigExtensions.cs#L132-L135

That property is set to the Regex specified for a branch (via branches in the YAML): https://github.com/GitTools/GitVersion/blob/8f8cca13d6fa25ff59670b0752663edbcf3b8156/src/GitVersionCore/Configuration/ConfigExtensions.cs#L102

The examples and defaults work because they do not match anything after / or - in the regex (e.g. ^features?[/-]).


Fixing this looks like a breaking change to me... To not have it break existing usages I had to check for whether the branch name contains / or -. That doesn't seem to align with the interest of fixing this - it should be left up to the user to define what gets captured for replacement IMO. Perhaps the best way moving forward is to add BranchPrefixToTrim to the BranchConfig and set it to BranchConfig.Regex by default to keep backwards compatibility?

Here's what I stopped at:

diff --git a/src/GitVersionCore.Tests/IntegrationTests/FeatureBranchScenarios.cs b/src/GitVersionCore.Tests/IntegrationTests/FeatureBranchScenarios.cs
index b9129d88..69a8c4f1 100644
--- a/src/GitVersionCore.Tests/IntegrationTests/FeatureBranchScenarios.cs
+++ b/src/GitVersionCore.Tests/IntegrationTests/FeatureBranchScenarios.cs
@@ -220,6 +220,38 @@ public void ShouldUseConfiguredTag(string tag, string featureName, string preRel
             fixture.AssertFullSemver(expectedFullSemVer, config);
         }

+        [TestCase("alpha", "JIRA-123", "alpha")]
+        [TestCase("useBranchName", "JIRA-123", "JIRA-123")]
+        [TestCase("alpha.{BranchName}", "JIRA-123", "alpha.JIRA-123")]
+        public void ConfiguredTagIsBranchNameForBranchesWithoutPrefix(string tag, string branchName, string preReleaseTagName)
+        {
+            var config = new Config
+            {
+                Branches =
+                {
+                    {
+                        "other",
+                        new BranchConfig
+                        {
+                            Increment = IncrementStrategy.Patch,
+                            Regex = ".*",
+                            SourceBranches = new HashSet<string>(),
+                            Tag = tag
+                        }
+                    }
+                }
+            };
+
+            using var fixture = new EmptyRepositoryFixture();
+            fixture.Repository.MakeATaggedCommit("1.0.0");
+            fixture.Repository.CreateBranch(branchName);
+            Commands.Checkout(fixture.Repository, branchName);
+            fixture.Repository.MakeCommits(5);
+
+            var expectedFullSemVer = $"1.0.1-{preReleaseTagName}.1+5";
+            fixture.AssertFullSemver(expectedFullSemVer, config);
+        }
+
         [Test]
         public void BranchCreatedAfterFinishReleaseShouldInheritAndIncrementFromLastMasterCommitTag()
         {
diff --git a/src/GitVersionCore/Configuration/ConfigExtensions.cs b/src/GitVersionCore/Configuration/ConfigExtensions.cs
index 987795e2..e9e39ef2 100644
--- a/src/GitVersionCore/Configuration/ConfigExtensions.cs
+++ b/src/GitVersionCore/Configuration/ConfigExtensions.cs
@@ -129,7 +129,7 @@ public static string GetBranchSpecificTag(this EffectiveConfiguration configurat
                 log.Info("Using branch name to calculate version tag");

                 var branchName = branchNameOverride ?? branchFriendlyName;
-                if (!string.IsNullOrWhiteSpace(configuration.BranchPrefixToTrim))
+                if (!string.IsNullOrWhiteSpace(configuration.BranchPrefixToTrim) && (branchName.Contains('/') || branchName.Contains('-')))
                 {
                     branchName = branchName.RegexReplace(configuration.BranchPrefixToTrim, string.Empty, RegexOptions.IgnoreCase);
                 }
asbjornu commented 3 years ago

Wow, what a mess. A PR that fixes this properly would be appreciated for v6. What we should do until then, I'm not sure. Perhaps adding another configuration property is the way to go, but I'd like to avoid it. If we add the check for - or / in the branch name, that would remove some flexibility but would it be a workable compromise until the breaking change and proper fix is introduced in v6?

bddckr commented 3 years ago

If we add the check for - or / in the branch name, that would remove some flexibility but would it be a workable compromise until the breaking change and proper fix is introduced in v6?

I'm personally using some branch names that use - but do not use /, so I'm afraid that won't work for me.

I do like the idea of making this less weird to resolve as I've done in the linked PR, though - a breaking change in v6 sounds like a good idea to me. What's your idea of how this should work moving forward? Should we offer an additional config property but just remove the relationship to the existing property Regex perhaps so it's all explicit?

dejoost commented 3 years ago

as workaround: just add an extra check, if the "trimmed" branchname returns empty, use the untrimmed version (and maybe a boolean to disable the trimming altogether).

for a (breaking) permanent: fix i would make it explicit, add the branchPrefixToTrim to the config and keep it empty by default. This would also allow ppl to apply more exotic transforms.

carlwoodhouse commented 3 years ago

just ran into this :( any progress ?

asbjornu commented 3 years ago

as workaround: just add an extra check, if the "trimmed" branchname returns empty, use the untrimmed version (and maybe a boolean to disable the trimming altogether).

I think this is a nice compromise. A pull request implementing this would be accepted and appreciated. 🙏🏼

carlwoodhouse commented 3 years ago

as workaround: just add an extra check, if the "trimmed" branchname returns empty, use the untrimmed version (and maybe a boolean to disable the trimming altogether).

I think this is a nice compromise. A pull request implementing this would be accepted and appreciated. 🙏🏼

Have opened a PR that implements this suggested compromise i hope :)

github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 5.6.9 :tada: The release is available on:

Your GitReleaseManager bot :package::rocket: