Skip to content

Add support for indicating change severity in commit messages #489

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

Merged
merged 1 commit into from
Aug 20, 2015

Conversation

TomGillen
Copy link
Contributor

Adds two new increment strategies: CommitMessage and MergeMessage.

While active, the version increment is determined by reading commit/merge commit messages for markers describing the change severity. All new commits introduced since the base version are scanned.

For example, "+semver: breaking" will force a major version bump.

New Branch Configuration Options

  • major-version-bump-message: a regex used for parsing breaking changes in commit messages. Defaults to \+semver:\s?(breaking|major).
  • minor-version-bump-message: a regex used for parsing minor changes in commit messages. Defaults to \+semver:\s?(feature|minor).
  • message-fallback: the increment strategy to fall back into if no change severity indicators are found in commit messages, while using the CommitMessage or MergeMessage strategies. Defaults to Patch.

Potential Issues

  • I changed the default increment strategy for the master branch to CommitMessage. This will behave the same as it did prior to this change if the user does not have markers in this commit messages. However this could certainly be considered a breaking change.
  • This change does not work for new repositories, as the base version of a new repository will be determined by the FallbackBaseVersionStrategy, which never increments versions. Certainly, If I am using GitHub Flow, this is not what I would expect. Changing the behaviour of FallbackBaseVersionStrategy when using commit message increments would be an option, but I do not know if this would make sense for other work flows.
  • Breaking changes always increment the major version. This may not be desirable for < 1.0.0 builds. We could add an option to treat breaking changes as minor version bumps for 0.x.x builds.

Addresses #137.

@JakeGinnivan
Copy link
Contributor

Nice, I will hold off on this until 3.0 has been released (which should be soon)

@TomGillen TomGillen force-pushed the commit-message-strategy branch from 098af0d to 765a7b5 Compare August 3, 2015 15:55
@TomGillen
Copy link
Contributor Author

I've rebased onto master after the 3.0.2 release. I would like some feedback on those potential issues before a merge, though.

This also still needs some unit test coverage.

@JakeGinnivan
Copy link
Contributor

Awesome, will look through it later today.

@JakeGinnivan
Copy link
Contributor

Sorry for the delay, just started back at work last week since moving back to Australia.

Rather than changing config I think that maybe we change the increment strategy to have something like this (obv this code would not compile/work, but hopefully give you the idea)

public static VersionField DetermineIncrementedField(GitVersionContext context, BaseVersion baseVersion)
{
    var incrementFromCommitMessage = FindIncrementsFromMessagesSince(context, baseVersion.Source); //baseVersion.Source is the commit used for commit counting, it is a good from filter of the commits we need to scan
    if (incrementingCommitOrMergeMessages.Any()) {
        var max = incrementFromCommitMessage.Max();
        var configIncrement = SimpleIncrement(context.Configuration.Increment);
        return (max >  ? max : configIncrement);
    }
    return SimpleIncrement(context.Configuration.Increment);
}

This way we don;t have to update any config, it will just light up?

@JakeGinnivan
Copy link
Contributor

This change does not work for new repositories, as the base version of a new repository will be determined by the FallbackBaseVersionStrategy, which never increments versions. Certainly, If I am using GitHub Flow, this is not what I would expect. Changing the behaviour of FallbackBaseVersionStrategy when using commit message increments would be an option, but I do not know if this would make sense for other work flows.

If we use my approach the commit messages override the increment from the config, solving this issue

@JakeGinnivan
Copy link
Contributor

Actually, I think my approach fixes all the issues listed under potential issues?

What do you think?

@TomGillen
Copy link
Contributor Author

You are essentially proposing that we always respect commit message severities, rather than have the user specify it via new increment strategies. I have no particular objection to this, but it does not fix all of the problems.

This code overrides the normal increment strategy when it finds relevant commit messages. However, the increment strategy is ignored entirely if the version strategy returns a BaseVersion with ShouldIncrement set to false. The FallbackBaseVersionStrategy always does this, and as such commit messages will never increment the version while that strategy is in effect (e.g. a clean repository with not yet any releases). Is this expected behaviour? As soon as the user does something which changes the version strategy (e.g. tags v0.2.0), commit messages will begin to affect the version number.

There is also still the issue of alpha (< 1.0.0) versions. There could easily be many commits tagged as breaking changes, but many users would not expect their version to be bumped up to or beyond 1.0.0 automatically. We could maybe do with having an option to treat breaking changes as minor version increments while the base version is < 1.0.0.

@JakeGinnivan
Copy link
Contributor

However, the increment strategy is ignored entirely if the version strategy returns a BaseVersion with ShouldIncrement set to false.

If we made this strategy always run before the normal increment code. If it finds an increment message then it increments the version, even if the branch setting says not to.

There is also still the issue of alpha (< 1.0.0) versions

Agreed, if the version we are incrementing is < 1.0 we should just bump minor instead.

@JakeGinnivan
Copy link
Contributor

What do you think @thogil?

@TomGillen
Copy link
Contributor Author

Sounds reasonable. I'll have a look at updating the PR later today.

@TomGillen TomGillen force-pushed the commit-message-strategy branch 2 times, most recently from 65e9ea5 to 42a2a1e Compare August 18, 2015 10:28
@TomGillen
Copy link
Contributor Author

The code now looks for major, minor and patch commit messages. If it finds one, it will use that increment, regardless of the base version ShouldIncrement property.

There are branch configuration values for the three message regexes, which default to:

  1. \+semver:\s?(breaking|major) e.g. +semver: breaking
  2. \+semver:\s?(feature|minor) e.g. +semver: feature
  3. \+semver:\s?(fix|patch) e.g. +semver: fix

There is also a commit-message-version-bump-mode setting, which controls which messages are read. It defaults to All, but can also be MergeMessageOnly or None (disable this feature).

If no commit message increments are found, it falls back to the default behaviour. If the version would have been incremented by branchConfig.Increment, had there been no commit messages, then it will always increment by at least branchConfig.Increment. It is also capped at minor for versions < 1.0.0.

@TomGillen
Copy link
Contributor Author

@JakeGinnivan I don't know why the AppVeyor build is failing. The only thing which looks like an error is coming from Fody, but that error also exists in the (passing) master build. The build just appears to halt silently.

@JakeGinnivan
Copy link
Contributor

I'll check it out tomorrow

Sent from my Windows Phone


From: Thomas Gillenmailto:[email protected]
Sent: ‎18/‎08/‎2015 6:51 PM
To: GitTools/GitVersionmailto:[email protected]
Cc: Jake Ginnivanmailto:[email protected]
Subject: Re: [GitVersion] Add support for indicating change severity in commit messages (#489)

@JakeGinnivanhttps://github.com/JakeGinnivan I don't know why the AppVeyor build is failing. The only thing which looks like an error is coming from Fody, but that error also exists in the (passing) master build. The build just appears to halt silently.


Reply to this email directly or view it on GitHubhttps://github.com//pull/489#issuecomment-132170377.

@@ -9,18 +9,30 @@ branches:
increment: Patch
prevent-increment-of-merged-branch-version: true
track-merge-target: false
major-version-bump-message: '\+semver:\s?(breaking|major)'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This config shouldn't be per branch I don't think? Can it be pulled up to root?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, sure.

@SimonCropp
Copy link
Contributor

@TomGillen TomGillen force-pushed the commit-message-strategy branch from 101033e to 5fbb8ea Compare August 20, 2015 09:30
@@ -9,18 +12,21 @@ branches:
increment: Patch
prevent-increment-of-merged-branch-version: true
track-merge-target: false
commit-message-incrementing: Enabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this to be configurable per branch?

The default should be set at the root level, you can leave it configurable per branch but it should be an override to whatever is specified in the root

@JakeGinnivan
Copy link
Contributor

Awesome!

One more thing, you are going to have to rewrite history to remove For example, "+semver: breaking" will force a major version bump. from your commit message :P we are currently building v4.

So it works :P

@TomGillen
Copy link
Contributor Author

Oh, good point ;)

While active, the version increment is determined by reading
commit/merge commit messages for markers describing the change severity.
@TomGillen TomGillen force-pushed the commit-message-strategy branch from b3d54b8 to f3947a9 Compare August 20, 2015 10:32
@JakeGinnivan
Copy link
Contributor

I am heading out now. I have just put up a PR myself. This will be in 3.1 which should be released tomorrow/saturday

@JakeGinnivan
Copy link
Contributor

Thanks for this, its an awesome addition!

JakeGinnivan added a commit that referenced this pull request Aug 20, 2015
Add support for indicating change severity in commit messages
@JakeGinnivan JakeGinnivan merged commit e9da07f into GitTools:master Aug 20, 2015
@TomGillen TomGillen deleted the commit-message-strategy branch August 21, 2015 10:11
@JakeGinnivan
Copy link
Contributor

Really should have added docs for this.. didn't think about that at the time, now I am doing the release thinking we should have.. I might put initial docs in then move the tag :P

After I have put the initial docs in if you want to review/add anything feel welcome to

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

Successfully merging this pull request may close these issues.

3 participants