GitVersion icon indicating copy to clipboard operation
GitVersion copied to clipboard

GitVersion should only consider tags matching current branch config (was: GitVersion fails to return correct version when tag exists on commit)

Open DocZoid opened this issue 5 years ago • 22 comments

Describe the bug Tags have a too strong weight on the version evaluation which I feel is a bug. I am quite new to versioning and GitVersion, so I might be on the wrong track without knowing. I have read in some other issue that fast-forward merges for master are generally discouraged (forbidden?), and this is a part of my issue (#2). The proposed fix would as a side effect allow using fast-forward merges in my opinion.

There are two scenarios where the tag should not be used to evaluate the version:

  1. When I build a snapshot from the develop branch, tag the branch with the created version (e.g. 0.1.2-snapshot.2), and then branch this commit to a new release (release/0.1.2)
  2. When I build a release candidate from a release branch, tag the release branch with the new created version (e.g. 0.1.2-rc.1), and then make a fast-forward merge to master, essentially having master point to that tagged commit

Expected Behavior

  1. The resulting version should be 0.1.2-rc.1 (with the release configuration "tag: rc")
  2. The resulting version should be 0.1.2

Actual Behavior

  1. The resulting version is 0.1.2-snapshot.2 based on the tag
  2. The resulting version is 0.1.2-rc.1 based on the tag

Possible Fix

Multiple branches point to the same commit. This is confusing GitVersion, which takes tags from that commit, but it does not respect the branch configuration of the current branch when analyzing the tags, e.g. when I am working on a release branch, it should only look for previous version tags which are formatted like its own tag configuration in the release branch ("with "rc" in it, so tags 0.1.2-rc.1 should be considered, but not 0.1.2-snapshot.2, because of the configuration "tag: rc")

Steps to Reproduce

  1. assume or create config: "develop -> tag: snapshot", "release -> tag: rc"
  2. tag a commit in "develop" with a version tag, e.g. 0.1.2-snapshot.2
  3. branch that commit to release/0.1.2
  4. using release/0.1.2 as current branch, run GitVersion. Calculated version should be 0.1.2-rc.1 instead of 0.1.2-snapshot.2

Context

We have commit hooks checking every commit message. Therefor, it seems like an easy option to use fast-forward merges for master to avoid commits, but GitVersion is missing the mentioned configuration option to be able to handle fast-forward merges. Also, I do not know how GitVersion is intended to be used when creating a release branch without adding an empty commit on that branch before building it, if the commit already has a tag. Deleting all tags locally would be a workaround, but it has a smell of nastiness.

Your Environment

  • Version Used: 5.3.3+Branch.master.Sha.08a43ede0b292c8ce2178b3d305587f151a2bdf4
  • Operating System and version (Windows 10, Ubuntu 18.04): Windows 10
  • Link to your project: -
  • Link to your CI build (if appropriate): -

DocZoid avatar Jun 24 '20 14:06 DocZoid

The workflow you are describing is not supported by GitVersion. The closest match you have may be Mainline mode, but tags are going to have the highest weight when figuring out a version number no matter what you do. This is by design and is not going to change.

asbjornu avatar Jun 25 '20 10:06 asbjornu

Can you elaborate more on why the workflow is not supported? I use GitFlow, with develop creating snapshots (instead of alpha). Mainline mode does not suite our development needs.

I know that it is a design decision to build versions preferably by using the commit's tag, and I am not suggesting to remove it, but rather to improve it. Of course this can also be a configurable option for each branch. The option could work like that: (in the branch configuration) consider-tags-of-other-branches: <true/false>, or more general, but probably over-the-top: filter-tag-regex: .*. Setting the first option to false yields the current behaviour. True would create a regex <SemVer-regex>-<tag-param of branch>.* to filter the found tags before actually analyzing them (or only <SemVer-regex> if tag is set to '').

If that improvement is not an option (apart from possible minor bugs in my suggested implementation), please explain why, and what I could change to build a release (candidate) from an already tagged commit.

DocZoid avatar Jun 25 '20 11:06 DocZoid

Perhaps I'm not properly understanding your use-case. Please submit a PR with a RepositoryFixture that implements the behaviour you'd like to see.

asbjornu avatar Jul 15 '20 10:07 asbjornu

I'd like to, but the way of working for such a change and the GitVersion internals are all new to me. But I'll try to get through it.

I just did a code analysis to find a place where I would assume the change to take place.

In TaggedCommitVersionStrategy.cs I see a comment /// Version is extracted from all tags on the branch which are valid, and not newer than the current commit.. I want to point out that the comment (and the following code examples) all state that the tags have to refer to the current branch, not the current commit. When using FF merges or when branching (and not committing after branching), commits (and their tags) may belong to multiple branches. This cannot be distinghuished in GIT, but it can in GitVersion, based on the configuration.

My intention is to have GitVersion focus more on the current branch and disregard tags on the same commit, but belonging to another branch (again, using the configuration to tell to which branch a tag belongs).

The essential code seems to be in GetTaggedVersions:

         var tagsOnBranch = currentBranch
               .Commits
               .SelectMany(commit => { return allTags.Where(t => IsValidTag(t.Item1, commit)); })
               .Select(t =>
               {
                   if (t.Item1.PeeledTarget() is Commit)
                       return new VersionTaggedCommit(t.Item1.PeeledTarget() as Commit, t.Item2, t.Item1.FriendlyName);

                   return null;
               })
               .Where(a => a != null)
               .Take(5)
               .ToList();

where t.Item1.FriendlyName refers to the tag name. I would want to filter that to have a prefix that matches the configured tag in the GitVersion branch configuration (e.g. "release: tag: rc" means if the current branch is release, all tags returned here should have the prefix "rc", meaning they actually belong to the current branch. Do not consider tags with other prefixes, like "snapshot.32", since we are on a release branch and do not want to build a snapshot, but a release candidate).

Is there a function somewhere that can tell me if a Git-tag matches a specific configured branch tag? How would I get the configured branch tag? Also, two special cases have to be considered: if the configured branch tag is empty (e.g. master branch), the match must yield true if the Git-tag has no prefix, and if the configured branch tag is "useBranchName", this has to be resolved.

DocZoid avatar Jul 16 '20 08:07 DocZoid

Adding a test shouldn't require deep knowledge about the GitVersion codebase. They are pretty straight forward:

https://github.com/GitTools/GitVersion/blob/e8102abf08381bcc4d7514df1bd9f57db6bc29fd/src/GitVersionCore.Tests/IntegrationTests/VersionInTagScenarios.cs#L33-L52

Start with writing a test that provokes the behaviour you're experiencing, then you can start deep-diving into the code and change things to see whether it fixes your problem or not. Without seeing a reproduction in a test, it's very hard for me to help you figure out where or how to fix your problem, so please start there. You can submit a draft pull request to allow feedback and discussion before the problem is solved and the PR is good for a final review and merge.

My intention is to have GitVersion focus more on the current branch and disregard tags on the same commit, but belonging to another branch (again, using the configuration to tell to which branch a tag belongs).

This sounds impossible. Git doesn't have a link between branch and tag references. To Git, they are basically the same: a reference to a commit. So as long as you have a tag reference to the same commit that is checked out, which branch was checked out when the commit was made, is irrelevant. You would need to add another commit to the current branch to deviate from the "tagged branch" so to say.

asbjornu avatar Jul 16 '20 10:07 asbjornu

I'll try to get quite close to it for one example at least, based on some other tests. Apologies for likely mistakes.


        [Test]
        public void BetaBranchCreatedButStillOnTaggedAlphaCommitShouldCreateBetaVersion()
        {
            // Arrange
            var config = new Config
            {
                Branches =
                {
                    { "release", new BranchConfig { Tag = "beta" } },
                    { "develop", new BranchConfig { Tag = "alpha" } }
                }
            };

            // Act
            using var fixture = new BaseGitFlowRepositoryFixture("1.0.0");
            //not sure if needed
            //fixture.Checkout("master");
            //fixture.MergeNoFF("develop");
            fixture.Checkout("develop");
            fixture.MakeACommit("Feature commit 1");
            fixture.ApplyTag("1.1.0-alpha.1"); // assuming this has been build as 1.1.0-alpha.1
            fixture.BranchTo("release/1.1.0"); // about to be released
            //fixture.MakeACommit("Release commit 1"); // no additional empty commit in this scenario!
            fixture.Checkout("release/1.1.0"); // still on the same commit, but another branch, choosing to build same code as beta
            fixture.AssertFullSemver("1.1.0-beta.1", config); //will be 1.1.0-alpha.1, should be 1.1.0-beta.1. Tag is an "alpha" tag, only "beta" tags should count when on release branch. If no beta tag found, build new beta version on release branch.
            fixture.Checkout("develop"); // back to develop
            fixture.AssertFullSemver("1.1.0-alpha.1", config); //will be 1.1.0-alpha.1 based on tag (as before)

            var version = fixture.GetVersion(config);

            // Assert
            // done in-line
        }

DocZoid avatar Jul 17 '20 13:07 DocZoid

Sorry, but writing code in a comment is not the same as submitting a draft pull request, @DocZoid.

asbjornu avatar Jul 18 '20 23:07 asbjornu

Sorry, but you are not being helpful either, @asbjornu . I'm just getting as far as I can with the time I have. It might take some time until I have a pull request out, and I was hoping for some constructive feedback.

DocZoid avatar Jul 20 '20 05:07 DocZoid

Sorry, but the constructive feedback is very hard to give on an island of code like that. If I had a proper draft pull request to look at, I would for instance know whether the code even compiled and the result of the test would be available to me. As presented, I would have to open my editor, paste in your code, compile it, execute tests, etc. That requires me to actually block out time to even know what I'm looking at here. If you are unable to allot the required time and energy to submit a draft pull request, I'm not going to be able to allot the required time and energy to give you feedback. Sorry.

asbjornu avatar Jul 20 '20 09:07 asbjornu

I never said that I won't make a pull request, it's just small steps with a fail-fast mindset (if you tell me "that won't work because you have not considered <mutant unicorns>" I might as well give it up altogether).

Well, I have checked out the code and added a (hopefully improved) test. I have no access to push the commit - is it something I have to look up in GitHub or do you need to give me repo rights? I didn't use GitHub for contributing yet and the permissions doc page I found didn't help.

Edit: I believe I need collaborator rights

DocZoid avatar Jul 21 '20 07:07 DocZoid

@DocZoid

Well, I have checked out the code and added a (hopefully improved) test. I have no access to push the commit - is it something I have to look up in GitHub or do you need to give me repo rights? I didn't use GitHub for contributing yet and the permissions doc page I found didn't help.

We don't give permissions to directly commit the changes to the repository. But all those that want to contribute usually fork the repository, make the adjustments in their fork then they create a pull request against the original repository.

arturcic avatar Jul 21 '20 07:07 arturcic

You can read about how to fork a repository and then how to submit a pull request from a fork.

asbjornu avatar Jul 21 '20 08:07 asbjornu

Getting there... do I work on master then, or do I still start a new branch?

DocZoid avatar Jul 24 '20 11:07 DocZoid

A separate branch would be best, as a pull request is not frozen in time, so whenever commits are added to the branch you create the pull request from, they will be added to the pull request. As master is a moving target, a separate branch gives more control and stability.

asbjornu avatar Jul 24 '20 11:07 asbjornu

@asbjornu I finally have some time again to get back on this topic. Does this pull request match your expectations or am I missing something? What is your opinion on the shown behavior?

DocZoid avatar Oct 05 '20 10:10 DocZoid

Thanks for the PR and test, @DocZoid. It is a great way to understand your use-case. From what I understand, you would like release branches to always yield the pre-release label (aka. "tag") beta even though the currently built commit is tagged with alpha?

I don't think we can support that, at least not without configuration. Tags always take presedence when they are available on the current commit and I'm not sure we want to change this, even with configuration. It's also a bit strange to create a release branch directly from a tagged commit without any commits in it. In Git Flow (which is one of a few flows GitVersion supports), release branches should be branched out from develop.

Which flow are you using?

asbjornu avatar Oct 05 '20 11:10 asbjornu

Exactly, that is scenario 1 from my initial comment. I'll try to add a test for scenario 2 as well.

It has to be configurable for sure. I wrote

I know that it is a design decision to build versions preferably by using the commit's tag, and I am not suggesting to remove it, but rather to improve it. Of course this can also be a configurable option for each branch. The option could work like that: (in the branch configuration) consider-tags-of-other-branches: <true/false>, or more general, but probably over-the-top: filter-tag-regex: .*.

For example, the release branch configuration for the test would be filter-tag-regex: .*-beta.*. GitVersion knows that the current branch which is checked out is a release branch, therefor it can fetch the filter-tag-regex configuration of the current branch and apply the filter .*-beta.*to all known tags of the current commit. Only the tags remaining will be used for further version number calculation. The default setting would be filter-tag-regex: .*, which yields the current behavior, that all tags found on the current commit are used for version number calculation.

It's also a bit strange to create a release branch directly from a tagged commit without any commits in it. In Git Flow (which is one of a few flows GitVersion supports), release branches should be branched out from develop. Which flow are you using?

Actually we are using Git Flow, but only as a branching model, not using the commands. When creating a release branch, we do not create empty commits to branch out of develop because this would be a step which would have to be done automatically by our CI/CD build pipeline, and we would not want to have it commit automatically. This is also not supported by the repository permissions and commit hook configuration. Just using a regular branch (initiated by jira in our case) is a neat way to support Git Flow but not using the command line tool.

DocZoid avatar Oct 06 '20 05:10 DocZoid

This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jan 06 '21 12:01 stale[bot]

Sorry for not answering this before, @DocZoid.

Actually we are using Git Flow, but only as a branching model, not using the commands.

I too use Git Flow "manually" so to speak, because I often don't agree with everything the automatic tooling does and prefer doing the individual steps myself. But by the sound of your workflow, it does not match entirely what Git Flow prescribes.

Just using a regular branch (initiated by jira in our case) is a neat way to support Git Flow but not using the command line tool.

If you're not adding any commits to the release branch, is it really a branch? Why not use a tag instead?

asbjornu avatar Feb 11 '22 20:02 asbjornu

Hello there ! We're encountering almost the same issue. Here is a description of our way of working : All developers work on a branch named main At each commit, we compute the semver and use it to tag the docker image (and embed the version in assemblies). We are working in mainline mode and on main every commit is tagged (with pre-release tag alpha). Every commit is then deployed on a dev environment.

When we're feature ready, we create a release branch in order to stabilize and let the external QA happen. Commits on this branch are also built and tagged (without pre-release tag). Then once built, they go to the QA environment then production.

We would like to end up with something like this : image

However, as you can notice and as it is the case in this thread, when we create the release branch on the commit "second feature", it is already tagged 0.2.0-alpha.0. gitversion is then unable to produce a 0.2.0 but instead (with our configuration I can post if you want) output 0.2.0-alpha.

We've debugged and narrowed down to this line : https://github.com/GitTools/GitVersion/blob/5c21ee61e344c142c7d18342d0a8ac4422636569/src/GitVersion.Core/VersionCalculation/NextVersionCalculator.cs#L90

taggedSemanticVersion is 0.2.0-alpha.0 (which is correct), semver is 0.2.0 (which is correct) but the comparison don't take in account the prerelease while comparing (the false parameter in CompareTo). Therefore it considers that 0.2.0-alpha.0 == 0.2.0 (which is false). If we switch the parameter to true, it works great. I can make a PR with such a change however, I'm unsure about consequences. What do you think ?

Addendum: I've just tested, this change breaks 5 tests

NatMarchand avatar Feb 15 '22 18:02 NatMarchand

@NatMarchand, thanks for the detailed explanation. Is the following pseudocode a fair description of what's happening?

var semver = new SemVer("0.2.0");
var taggedSemanticVersion = new SemVer("0.2.0-alpha.0");

if (semver.CompareTo(taggedSemanticVersion, false) > 0)
{
    // "0.2.0-alpha.0" is considered to be equal to "0.2.0",
    // therefore, the `taggedSemanticVersion` is set to `null`
    // and `semver` is used instead.
}

I can't say exactly why the prerelease label would not be considered in this case. Can you name the tests that are failing when changing includePrerelease from false to true?

asbjornu avatar Mar 07 '22 22:03 asbjornu

It seems like this may be a duplicate of #2241.

asbjornu avatar Mar 08 '22 20:03 asbjornu

@DocZoid Could you please take a look to the following bugs: https://github.com/GitTools/GitVersion/issues/3438 and https://github.com/GitTools/GitVersion/issues/3436.

HHobeck avatar Mar 16 '23 15:03 HHobeck

An integration test in #3441 gives the follwoting result:

[Test]
public void __Just_A_Test__()
{
    var configuration = GitFlowConfigurationBuilder.New
        .WithBranch("develop", builder => builder.WithLabel("snapshot"))
        .WithBranch("release", builder => builder.WithLabel("rc"))
        .Build();

    using var fixture = new EmptyRepositoryFixture();

    fixture.MakeACommit();
    fixture.BranchTo("develop");
    fixture.MakeACommit();
    fixture.MakeATaggedCommit("0.1.2-snapshot.2");
    fixture.BranchTo("release/0.1.2");

    // ✅ succeeds as expected
    fixture.AssertFullSemver("0.1.2-rc.1+0", configuration);
}

HHobeck avatar Mar 17 '23 14:03 HHobeck

:tada: This issue has been resolved in version 6.0.0-beta.2 :tada: The release is available on:

Your GitReleaseManager bot :package::rocket:

arturcic avatar Apr 06 '23 19:04 arturcic