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

[Bug] track-merge-changes produces unexpected result when combining hotfix and support branches #3052

Closed
Crown0815 opened this issue Mar 17, 2022 · 27 comments
Labels
Milestone

Comments

@Crown0815
Copy link

Describe the bug

It appears that track-merge-changes does not work when combining hotfix and support branches. I have the following test

[Test]
public void MyTest()
{
    var config = new Config();
    config.Branches.Add("hotfix", new BranchConfig
    {
        TrackMergeTarget = true,
    });

    using var fixture = new EmptyRepositoryFixture();
    fixture.Repository.MakeACommit();
    fixture.Checkout(MainBranch);
    fixture.Repository.ApplyTag("1.7.0");
    
    fixture.BranchTo("hotfix/1.7");
    fixture.Repository.MakeCommits(1);
    fixture.Repository.ApplyTag("1.7.1-beta.1");

    fixture.Checkout(MainBranch);
    fixture.BranchTo("support/1.7");
    fixture.Repository.Merge(fixture.Repository.Branches["hotfix/1.7"], new Signature("a", "b", DateTimeOffset.Now-TimeSpan.FromMinutes(55)), new MergeOptions()
    {
        FastForwardStrategy = FastForwardStrategy.NoFastForward,
    });

    fixture.Repository.ApplyTag("1.7.1");

    fixture.Checkout("hotfix/1.7");
    fixture.Repository.MakeACommit("semver: bug");

    fixture.AssertFullSemver("1.7.2-beta.1", config);
}

that produces the following repository:

* d9e96de 54 minutes ago  (HEAD -> hotfix/1.7)
|   
| *  57c51f7 55 minutes ago  (tag: 1.7.1, support/1.7)
|/|   
* | 79a0af5 56 minutes ago  (tag: 1.7.1-beta.1)
|/  
* 2d1d766 58 minutes ago  (tag: 1.7.0, main)

Expected Behavior

hotfix is set to track-merge-targets = true; hence, I would expect its next version to be 1.7.2-beta.1

Actual Behavior

Running the test produces:

should be 1.7.2-beta.1
but was 1.7.1-beta.2+2

Possible Fix

I suspect the merge target tracking is incomplete, or I am not configuring GitVersion correctly.

Steps to Reproduce

See test above

Context

I am attempting to merge hotfix branches into support branches and have the version of the hotfix branches bumped without requiring a merge back from support into hotfix.

Your Environment

Standard git repository

  • Version Used: 5.3.6
  • Operating System and version (Windows 10):
  • Link to your project: N/A
  • Link to your CI build (if appropriate): N/A
@Crown0815 Crown0815 added the bug label Mar 17, 2022
@Crown0815 Crown0815 changed the title [Bug] [Bug] track-merge-changes produces unexpected result when combining hotfix and support branches Mar 17, 2022
@asbjornu
Copy link
Member

asbjornu commented Mar 18, 2022

I'm not sure, but I think the problem may be that the tag 1.7.1 is applied to the support/1.7 branch and not to main. I don't think support branches are considered merge targets for hotfix by default. If you apply the 1.7.1 tag to main, I think you should get the expected version 1.7.2-beta.1.

@Crown0815
Copy link
Author

@asbjornu yes, but main is not on version 1.7.1, it is still on version 1.7.0 in our case, so that is not an option. Can merge targets be configured?

@asbjornu
Copy link
Member

You can configure source-branches as a sort of opposite relationship to "merge target".

@HHobeck
Copy link
Contributor

HHobeck commented Apr 30, 2022

I'm not sure what your branching and merging strategy is exactly but it seems to be that the gitflow strategy maybe a good solution for you (see gitflow example here). Anyway if you ask me you should on the one hand create a dedicated hotfix branch from the source branch you want to place the bug fix and on the other hand create a hotfix branch with the target semantic version number (e.g. hotfix/1.7.1). At this moment you have tested your hotfix and ensure the stability of the release candidate (RC), you are going to merge the hotfix to the support branch (support/1.7) and delete the hotfix branch. That’s mean you declare this fix as resolved and released it to manufacturing (RTM) (e.g. tagging this commit with 1.7.1). Now if you have another bug (which needs to be hot fixed) you create an additional branch from the support branch with the name hotfix/1.7.2. In my opinion is this the same procedure like you would fix bugs in the main branch.

@HHobeck
Copy link
Contributor

HHobeck commented Apr 30, 2022

I have taken a look in to the source code of GitVersion in version 5.10.1 and cannot find any usage of the TrackMergeTarget settings. In my opinion this property is not used and should be marked as deprecated.

@asbjornu: Do you know if I have missed something?

@asbjornu
Copy link
Member

Hm, good question, @HHobeck. I'm not using that feature myself, did not implement it, and am not sure exactly how it's supposed to work. It may be that we didn't have tests covering that feature and the affected code has just been refactored away. Perhaps the best thing to do is to deprecate it entirely in v6.

@HHobeck
Copy link
Contributor

HHobeck commented Sep 9, 2022

I have found the following logic changed by Jake Ginnivan in 2015 (see commit [1]):

    public class GitFlowDevelopBranchBaseVersionStrategy : HighestTagBaseVersionStrategy
    {
        protected override bool IsValidTag(GitVersionContext context, string branchName, Tag tag, Commit commit)
        {
            if (!string.IsNullOrWhiteSpace(branchName))
            {
                if (context.Configuration.TrackMergeTarget)
                {
                    return IsDirectMergeFromCommit(tag, commit);
                }
            }

            return base.IsValidTag(context, branchName, tag, commit);
        }

        static bool IsDirectMergeFromCommit(Tag tag, Commit commit)
        {
            var targetCommit = tag.Target as Commit;
            if (targetCommit != null)
            {
                var parents = targetCommit.Parents;
                if (parents != null)
                {
                    return parents
                        .Where(parent => parent != null)
                        .Any(parent => string.Equals(parent.Id.Sha, commit.Id.Sha, StringComparison.OrdinalIgnoreCase));
                }
            }

            return false;
        }
    }

The configuration properties TrackMergeTarget was used to determine base versions in the scenario where a branch has been merged into the current branch or vice versa. In the latest code base I cannot find any usage.

commit [1]:

Jake Ginnivan
Commit 1d1b0c4
Author: Jake Ginnivan jake@ginnivan.net
Date: Sunday, 15 March 2015 12:43
Parent: 1ee64bf

Switched to configuration option rather than hardcoding branch name

@HHobeck
Copy link
Contributor

HHobeck commented Sep 9, 2022

I would like to understand the intention of the TrackMergeTarget property. @asbjornu May be you can give me your opinion about the following scenario:

    [Test]
    public void __Just_A_Test__()
    {
        var configuration = new Config()
        {
            VersioningMode = VersioningMode.ContinuousDeployment,
            Branches = new Dictionary<string, BranchConfig>()
            {
                {
                    "develop", new BranchConfig() {
                        TrackMergeTarget = true
                    }
                }
            }
        };

        using var fixture = new EmptyRepositoryFixture();
        fixture.MakeACommit();
        fixture.AssertFullSemver("0.1.0-ci.0", configuration);
        fixture.BranchTo("develop");
        fixture.AssertFullSemver("0.1.0-alpha.0", configuration);
        fixture.MakeACommit();
        fixture.AssertFullSemver("0.1.0-alpha.1", configuration);
        fixture.BranchTo("release/1.0.0");
        fixture.AssertFullSemver("1.0.0-beta.0", configuration);
        fixture.MakeACommit();
        fixture.AssertFullSemver("1.0.0-beta.1", configuration);
        fixture.MakeACommit();
        fixture.AssertFullSemver("1.0.0-beta.2", configuration);
        fixture.Checkout("develop");
        fixture.AssertFullSemver("0.1.0-alpha.1", configuration); // 1.1.0 expected
        fixture.MakeACommit();
        fixture.AssertFullSemver("1.1.0-alpha.1", configuration);
        fixture.MergeNoFF("release/1.0.0");
        fixture.AssertFullSemver("1.1.0-alpha.4", configuration);
        fixture.Repository.Branches.Remove("release/1.0.0");
        fixture.AssertFullSemver("1.1.0-alpha.4", configuration); // okay because TrackMergeTarget=true ??
        configuration.Branches["develop"].TrackMergeTarget = false;
        fixture.AssertFullSemver("1.1.0-alpha.4", configuration); // 0.1.0 expected because TrackMergeTarget=false ??
    }

From the release management point of view this means the following: You create a release 1.0.0 branch and want to stabilize the release candidate to bring this one to production. So what happened if your product manager says to cancel the release and go back to the develop phase? The release candidate 1.0.0 has not been merged and tagged on master. But it will be properly merged to develop and deleted. Thus the next version on the develop branch is still the next version of the previous version (that means in my example the version 0.1.0).

Maybe it is a good idea to reuse this property and make the following change:

/// <summary>
/// Version is extracted from older commits' merge messages.
/// BaseVersionSource is the commit where the message was found.
/// Increments if PreventIncrementForMergedBranchVersion (from the branch config) is false.
/// </summary>
public class MergeMessageVersionStrategy : VersionStrategyBase
{
    private readonly ILog log;

    public MergeMessageVersionStrategy(ILog log, Lazy<GitVersionContext> versionContext) : base(versionContext) => this.log = log.NotNull();

    public override IEnumerable<BaseVersion> GetVersions()
    {
        if (Context.CurrentBranch.Commits == null || Context.CurrentCommit == null)
            return Enumerable.Empty<BaseVersion>();

        // new
        if (!Context.Configuration.TrackMergeTarget)
            return Enumerable.Empty<BaseVersion>();
        ...

@HHobeck
Copy link
Contributor

HHobeck commented Sep 9, 2022

I'm also thinking about it in which circumstances the MergeMessageVersionStrategy is used in the gitflow worflow. Cannot find any scenario. Maybe it is good for the github workflow with continues delivery?

@asbjornu
Copy link
Member

The release candidate 1.0.0 has not been merged and tagged on master. But it will be properly merged to develop and deleted.

Why would a release/* branch be merged to develop but not to master? Shouldn't those merges be done at the same time and either both or none of the merges happen? I know there's no way to enforce that but socially, but still – that's how I understand the workings of Git Flow's release/* branches.

@asbjornu
Copy link
Member

I'm also thinking about it in which circumstances the MergeMessageVersionStrategy is used in the gitflow worflow. Cannot find any scenario. Maybe it is good for the github workflow with continues delivery?

I would recommend against its use for any other workflow than trunk based development aka mode: Mainline.

@HHobeck
Copy link
Contributor

HHobeck commented Sep 12, 2022

Why would a release/* branch be merged to develop but not to master? Shouldn't those merges be done at the same time and either both or none of the merges happen? I know there's no way to enforce that but socially, but still – that's how I understand the workings of Git Flow's release/* branches.

I think it is quite common to merge from release to develop and not to master (at least not at the same time). Think about it if you finalize or stabilize a beta version and find issues. You will fix it on release and merge it back to the develop branch.

Another scenario could be if you cancel the hole release or you decide to go back to the alpha phase. Please see my comment here

@HHobeck
Copy link
Contributor

HHobeck commented Sep 12, 2022

I would recommend against its use for any other workflow than trunk based development aka mode: Mainline.

Okay and why is the TrackMergeTarget property be true per default in the develop branch configuration? (Please see ConfigurationBuilder.cs)

@asbjornu
Copy link
Member

asbjornu commented Sep 12, 2022

Okay and why is the TrackMergeTarget property be true per default in the develop branch configuration? (Please see ConfigurationBuilder.cs)

I don't see how MergeMessageVersionStrategy is related to TrackMergeTarget?

@asbjornu
Copy link
Member

I think it is quite common to merge from release to develop and not to master (at least not at the same time). Think about it if you finalize or stabilize a beta version and find issues. You will fix it on release and merge it back to the develop branch.

I suppose that's a valid use-case, but it's going to be strange selective merges because I wouldn't want everything in a release/* to be merged into develop until the release is finalized, such as the changelog, version number increments, etc. How would you resolve this?

Another scenario could be if you cancel the hole release or you decide to go back to the alpha phase.

As long as the release/* branch is not merged to main, I would just delete the release/* branch entirely. I'm not a fan of having protected release/* branches.

@HHobeck
Copy link
Contributor

HHobeck commented Sep 12, 2022

I think it is quite common to merge from release to develop and not to master (at least not at the same time). Think about it if you finalize or stabilize a beta version and find issues. You will fix it on release and merge it back to the develop branch.

I suppose that's a valid use-case, but it's going to be strange selective merges because I wouldn't want everything in a release/* to be merged into develop until the release is finalized, such as the changelog, version number increments, etc. How would you resolve this?

What do you mean with the changelogs, version number increments, etc.? I cannot see any problems with that. A merge from release to develop is just a normal commit:

    [Test]
    public void __Just_A_Test__()
    {
        var config = new Config() { NextVersion = "1.0.0" };
        using EmptyRepositoryFixture fixture = new("develop"); // starting with develop branch

        fixture.MakeACommit();
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.0.0-alpha.1");
        fixture.BranchTo("release/1.0.0");
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.0.0-beta.1+0");
        fixture.MakeACommit(); // <<-- first
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.0.0-beta.1+1");
        fixture.MakeACommit(); // <<-- second
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.0.0-beta.1+2");
        fixture.Checkout("develop");
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.1.0-alpha.0");
        fixture.MakeACommit();
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.1.0-alpha.1");
        fixture.MergeNoFF("release/1.0.0"); // <<-- third
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.1.0-alpha.4"); // +3 one merge and two commits from release
    }

@HHobeck
Copy link
Contributor

HHobeck commented Sep 12, 2022

Another scenario could be if you cancel the hole release or you decide to go back to the alpha phase.

As long as the release/* branch is not merged to main, I would just delete the release/* branch entirely. I'm not a fan of having protected release/* branches.

Yep I agree with you. But when this scenario happens GitVersion should also know how to handle this.

@HHobeck
Copy link
Contributor

HHobeck commented Sep 12, 2022

Okay and why is the TrackMergeTarget property be true per default in the develop branch configuration? (Please see ConfigurationBuilder.cs)

I don't see how MergeMessageVersionStrategy is related to TrackMergeTarget?

Yep that's the big question actually. ;) If you ask me MergeMessageVersionStrategy is the logic which should be called only when the TrackMergeTarget is set to true and this is only good for the main line mode.

@HHobeck
Copy link
Contributor

HHobeck commented Sep 12, 2022

Okay and why is the TrackMergeTarget property be true per default in the develop branch configuration? (Please see ConfigurationBuilder.cs)

I don't see how MergeMessageVersionStrategy is related to TrackMergeTarget?

Yep that's the big question actually. ;) If you ask me MergeMessageVersionStrategy is the logic which should be called only when the TrackMergeTarget is set to true and this is only good for the main line mode.

Here is an example which illustrates the behavior:

    [Test]
    public void __Just_A_Test__()
    {
        var config = new Config() { NextVersion = "1.0.0" };
        var configWithoutTrackMergeTarget = new Config()
        {
            NextVersion = "1.0.0",
            Branches = new Dictionary<string, BranchConfig>()
            {
                { "develop", new BranchConfig() { TrackMergeTarget = false } }
            }
        };

        using EmptyRepositoryFixture fixture = new("develop"); // starting with develop

        fixture.MakeACommit();
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.0.0-alpha.1");
        fixture.BranchTo("release/1.0.0");
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.0.0-beta.1+0");
        fixture.MakeACommit();
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.0.0-beta.1+1");
        fixture.MakeACommit();
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.0.0-beta.1+2");
        fixture.Checkout("develop");
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.1.0-alpha.0");
        fixture.MakeACommit();
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.1.0-alpha.1");
        fixture.MergeNoFF("release/1.0.0");

        fixture.Repository.Branches.Remove("release/1.0.0");

        fixture.GetVersion(config).FullSemVer.ShouldBe("1.1.0-alpha.4");
        fixture.GetVersion(configWithoutTrackMergeTarget).FullSemVer.ShouldBe("1.0.0-alpha.5");

    }

@HHobeck
Copy link
Contributor

HHobeck commented Sep 12, 2022

Here is a better example:

    [Test]
    public void __Just_A_Test__()
    {
        var config = new Config()
        {
            VersioningMode = VersioningMode.ContinuousDelivery,
            Branches = new Dictionary<string, BranchConfig>()
            {
                { "main", new BranchConfig() { TrackMergeTarget = true } }
            }
        };

        var configWithoutTrackMergeTarget = new Config()
        {
            VersioningMode = VersioningMode.ContinuousDelivery,
            Branches = new Dictionary<string, BranchConfig>()
            {
                { "main", new BranchConfig() { TrackMergeTarget = false } }
            }
        };

        using EmptyRepositoryFixture fixture = new("main"); // starting with main

        fixture.MakeACommit();
        fixture.ApplyTag("1.0.0");
        fixture.GetVersion(config).FullSemVer.ShouldBe("1.0.0");
        fixture.BranchTo("release/2.0.0");
        fixture.GetVersion(config).FullSemVer.ShouldBe("2.0.0-beta.1+0");
        fixture.MakeACommit();
        fixture.GetVersion(config).FullSemVer.ShouldBe("2.0.0-beta.1+1");
        fixture.MakeACommit();
        fixture.GetVersion(config).FullSemVer.ShouldBe("2.0.0-beta.1+2");
        fixture.Checkout("main");
        fixture.MergeNoFF("release/2.0.0");
        fixture.GetVersion(config).FullSemVer.ShouldBe("2.0.0+0");
        fixture.Repository.Branches.Remove("release/2.0.0");

        fixture.GetVersion(config).FullSemVer.ShouldBe("2.0.0+3");
        // until you are not tagging it this is a hotfix
        fixture.GetVersion(configWithoutTrackMergeTarget).FullSemVer.ShouldBe("1.0.1+3");
    }

@asbjornu
Copy link
Member

What do you mean with the changelogs, version number increments, etc.? I cannot see any problems with that. A merge from release to develop is just a normal commit:

What I mean is that in a support/* branch, you will often alter changelogs, bump version numbers in package.json files and such to match that of the release number. All of those versioned artifacts does not belong in develop before the release is complete (and in production), imho.

Yep that's the big question actually. ;) If you ask me MergeMessageVersionStrategy is the logic which should be called only when the TrackMergeTarget is set to true and this is only good for the main line mode.

I see. Yes, that's certainly a valid way to look at it. I don't think that expectation holds in GitVersion today, though. I believe MergeMessageVersionStrategy is executed in more cases than just for branches with TrackMergeTarget set to true.

@HHobeck
Copy link
Contributor

HHobeck commented Sep 14, 2022

What I mean is that in a support/* branch, you will often alter changelogs, bump version numbers in package.json files and such to match that of the release number. All of those versioned artifacts does not belong in develop before the release is complete (and in production), imho.

Good point. I agree with you that this is even more complicated as expected. But I think still that GitVersion can also track or even not track the bump message when it comes from a merged branch. I mean GitVersion generates a minor or major version change on master but it is not valid correct when it is not tagged in the git flow workflow (or at least it dependents on how your deployment process is (ContinuousDelivery vs. ContinuousDeployment??)). Thus this feature is worth to implement in GitVersion because it pays in the support of git flow and other workflows. At this point I'm not sure if it is a new feature or if it is a part of the track-merge-target feature. Because the intention is maybe the same.

I see. Yes, that's certainly a valid way to look at it. I don't think that expectation holds in GitVersion today, though. I believe MergeMessageVersionStrategy is executed in more cases than just for branches with TrackMergeTarget set to true.

One thing may be an idea to separate this and make it configurable. This would be easier to implement and better for the understanding. Concrete my proposal is to separate this two features and introducing a new configuration property like track-merge-message.

Last but not least we need to either remove the feature track-merge-target or re-implement it in this way like it was build in the origin intention.

@HHobeck
Copy link
Contributor

HHobeck commented Sep 14, 2022

But still could some one explain me for which scenario the feature track-merge-target is good for or give me a business use case please?

track-merge-target
Strategy which will look for tagged merge commits directly off the current branch. For example develop → release/1.0.0 → merge into master and tag 1.0.0. The tag is not on develop, but develop should be version 1.0.0 now.

@Crown0815 Your example is not clear for me what you are trying to do. Which workflow applies in your example?

@Crown0815
Copy link
Author

@HHobeck we are using a custom workflow where we try to reduce dependencies between different branches and prevent "back-merges". It looks like this:

0452afbe-efad-4b48-b509-babb6a0ae15b

You can see that the support branch lives on its own and receives all the hotfixes. Those never reach master.

Simply put, master always represents versions X.Y.0, so the new features, while support always represents the X.Y.Z (with Z > 0) versions.

All I want is to be able to merge a hotfix branch into the support branch and have the bugfix version number on the hotfix branch bumped. To support this workflow, I thought that I could just configure the hotfix branches' merge target to be the support branches.

@HHobeck
Copy link
Contributor

HHobeck commented Oct 2, 2022

Okay thank you very much for outlining your branching and merging strategy. If I compare your custom workflow with the gitflow workflow then the difference is only how you treat the main, hotfix and support branch right!? The main branch represents the initial release (tagged as RTM) and the support branch contains the hotfix releases (also tagged as RTM of the same lifecycle). I don't understand exactly what your motivation behind it is and what advantages you have compared to the gitflow workflow.

But anyway I don't want to challenge your motivation behind it. If I read the documentation about the track-merge-changes feature then I agree this might help you detecting the version in the hotfix branch which have been done in the support branch. But I'm not sure if it is the ideal way to detect the version in all circumstances because this feature is only restricted on the current branch.

@HHobeck
Copy link
Contributor

HHobeck commented Oct 2, 2022

In my opinion this issue here is related to the issue #1789 and vice versa and both can be solved with the track-merge-targets feature. But like I have in the other discussion already pointed out the problem might be located in the TrackReleaseBranchesVersionStrategy that this class not detecting the tagged version on branches which are marked as IsMainlin=true. The irony of my suggestion is that not only the track-merge-target feature is broken the feature detecting the tagged version on mainline branches is broken as well. Another point is that the track-merge-target feature is from the performance perspective maybe not so optimal.

TLDR: You should maybe define the support branch with IsMainline=true and implement the fix for TrackReleaseBranchesVersionStrategy detecting not only the hardcoded branch with the name master or main.

@arturcic
Copy link
Member

arturcic commented Mar 2, 2023

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

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants