Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid version number detected in merge message #385

Closed
markpjohnson opened this issue Mar 10, 2015 · 8 comments
Closed

Invalid version number detected in merge message #385

markpjohnson opened this issue Mar 10, 2015 · 8 comments
Milestone

Comments

@markpjohnson
Copy link

I am looking into using GitVersion 3.0, but it detects the following message as a versioned release: Merge branch 'release/Sprint_2.0_Holdings_Computed_Balances'

My team used to use descriptive release branches instead of versioned release branches, and even though we would start at version 1.0, GitVersion 3.0 parses our merge messages and finds version 2.0.

@JakeGinnivan
Copy link
Contributor

Not sure how to fix this unless we drop support for parsing versions in format major.minor. PR #383 is changing this code, suggestions or thoughts for a fix would be handy.

@markpjohnson
Copy link
Author

I think it would be better if the regex check for the semantic version parser were more strict. For example, if the branch were release/2.0 instead of the branch containing 2.0, then I would expect the version parser to extract a 2.0 version.

It looks like the best change would be to modify the regex pattern in the SemanticVersion.TryParse method.

@JakeGinnivan
Copy link
Contributor

Did you want to have a go and submit a pull request to the release/3.0.0 branch?

@JakeGinnivan
Copy link
Contributor

See #395 for another case and another test case

@orjan
Copy link
Contributor

orjan commented Mar 16, 2015

Adding another failing test case

        [Test]
        public void AMergeCommitIncludingAThirdPartyLibraryVersionShouldNotBeParsedAsAVersion()
        {
            var mergeCommitWithIpNumber = @"Merge pull request #1 in FOO/bar from feature/ISSUE-1 to develop

* commit '38560a7eed06e8d3f3f1aaf091befcdf8bf50fea':
  Updated jQuery to v2.1.3";

            var parents = GetParents(true);

            AssertMergeMessage(mergeCommitWithIpNumber, null, parents);
        }

@JakeGinnivan
Copy link
Contributor

Yeah. this is definitely an issue. If someone doesn't pick this up I will try do it this weekend. PR's welcome though :)

@orjan
Copy link
Contributor

orjan commented Mar 16, 2015

The previous version of the merge message based parse was pretty fine grained with:

            var knownMergePrefixes = new[] { "Merge branch 'hotfix-", "Merge branch 'hotfix/", "Merge branch 'release-", "Merge branch 'release/" };

and some other if:s and but:s. The question is whether we should

  • Revert to the old approach
  • Make it configurable
  • Try fix the current generic version?

@JakeGinnivan
Copy link
Contributor

I am not sure really. I am tempted to try and make it generic with something like this:

var version = Regex.Match("[/-](?<PossibleMatch>.*?)[$_]", mergeMessage).Matches.Cast<Match>()
.Select(m => { SemanticVersion temp; ; return new { isMatch: SemanticVersion.TryParse(m.Groups["PossibleMatch"].Value, out temp), Version: temp});
.FirstOrDefault();

Maybe always do string split on '\n'. If we revert then the feature only works for known branches, not other types of branches people use.

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

No branches or pull requests

3 participants