[IMPROVEMENT]: Topological commit sort causes issues when master is merged into a feature branch

[dauthleikr]

Prerequisites

• I have written a descriptive issue title
• I have searched issues to ensure it has not already been reported

GitVersion package

GitVersion.Tool

GitVersion version

6.0.2+Branch.main.Sha.30211316bc16e481dc440baae39ff904c4fa4966

Operating system

Windows

What are you seeing?

Consider the following commit graph, where green is a feature branch, and pink is master.

• The bottom-most pink square on the image is a version source (commit is tagged with a version, 1.0.0)
• The green commit below the blue (selected) line contains a bump message

Configuration:
The idea of this configuration is as follows:

1. Developers can not commit on master
2. Merges into master trigger a pipeline, which builds, tests, and publishes a build, and creates a tag on the commit, which acts as a version source
3. Feature branches are created from master
4. Bump messages are created on the feature branch by the developer developing the feature
5. PR to master

assembly-versioning-scheme: MajorMinorPatch
assembly-file-versioning-scheme: MajorMinorPatch
tag-prefix: '(?i)REDACTEDPRODUCTNAME\/'
version-in-branch-pattern: '\s?REDACTEDPRODUCTNAME:?\s?(?<version>[vV]?\d+(\.\d+)?(\.\d+)?).*'
major-version-bump-message: '\+(sem)?ver\s?REDACTEDPRODUCTNAME[:\s_\/]*(breaking|major)'
minor-version-bump-message: '\+(sem)?ver\s?REDACTEDPRODUCTNAME[:\s_\/]*(feature|minor)'
patch-version-bump-message: '\+(sem)?ver\s?REDACTEDPRODUCTNAME[:\s_\/]*(fix|patch)'
no-bump-message: '\+semver\s?REDACTEDPRODUCTNAME:\s?(none|skip)'
tag-pre-release-weight: 60000
commit-date-format: yyyy-MM-dd
merge-message-formats: {}
update-build-number: true
semantic-version-format: Strict
strategies:
  - Fallback
  - ConfiguredNextVersion
  - MergeMessage
  - TaggedCommit
branches:
  master:
    mode: ManualDeployment
    label: ""
    increment: None
    prevent-increment:
      of-merged-branch: true
    track-merge-target: false
    track-merge-message: true
    regex: ^(?<BranchName>.+)
    source-branches: []
    is-source-branch-for: []
    tracks-release-branches: false
    is-release-branch: true
    is-main-branch: true
    pre-release-weight: 55000
  feature:
    mode: ContinuousDelivery
    label: "{BranchName}"
    increment: None
    prevent-increment:
      when-current-commit-tagged: false
    track-merge-message: false
    regex: ^(?<BranchName>.+)
    source-branches: []
    is-source-branch-for: []
    is-main-branch: false
    pre-release-weight: 30000
  pull-request:
    mode: ContinuousDelivery
    label: PullRequest
    increment: None
    prevent-increment:
      of-merged-branch: true
      when-current-commit-tagged: false
    label-number-pattern: '[/-](?<number>\d+)'
    track-merge-message: true
    regex: ^(pull|pull\-requests|pr)[/-]
    source-branches:
      - master
      - feature
    is-source-branch-for: []
    pre-release-weight: 30000
  unknown:
    mode: ManualDeployment
    label: "{BranchName}unk"
    increment: Inherit
    prevent-increment:
      when-current-commit-tagged: true
    regex: (?<BranchName>.+)
    source-branches:
      - master
      - feature
    is-source-branch-for: []
    is-main-branch: false
ignore:
  sha: []
mode: ManualDeployment
label: "{BranchName}"
increment: Inherit
prevent-increment:
  of-merged-branch: false
  when-branch-merged: false
  when-current-commit-tagged: true
track-merge-target: false
track-merge-message: true
commit-message-incrementing: Enabled
regex: ""
source-branches: []
is-source-branch-for: []
tracks-release-branches: false
is-release-branch: false
is-main-branch: false
workflow: '' # Do not use a default template

Even though the feature branch contains a bump message, it is not respected, because a merge from master into the feature branch happened, after the bump message was created. Without the merge, everything is fine.
We want merges from master into feature branches to be possible, without the bump message getting lost.
Example:

• I create a feature branch based on 1.0.0
• I develop my feature, create a bump message for "feature" -> 1.1.0
• My team merges a new major feature into master -> master is 2.0.0
• I merge master in my feature branch. I expect: 2.1.0. I get: 2.0.0
• I need to create my bump message again <- I do not want to do this

What is expected?

Somehow consider the bump messages from before the merge of master into my feature branch. The bump version commit is not discoverable from the head of master, but it is discoverable from the head of my feature branch, so I feel like it should be possible to implement.

The issue can also be "fixed" by changing the filter in GetCommitsReacheableFromHead to CommitSortStrategies.Topological | CommitSortStrategies.Time | CommitSortStrategies.Reverse, or CommitSortStrategies.Time | CommitSortStrategies.Reverse (which seems to be the default for git log), but is not optimal for obvious reasons.

Steps to Reproduce

Please recreate the commit graph shown above. Sorry for the heavy redaction :(

RepositoryFixture Test

No response

Output log or link to your CI build (if appropriate).

[HHobeck]

Please provide steps to reproduce (not only a graphic) by creating:

• native git commands or
• a integration test or
• an example repository

Thank you very much.

[dauthleikr]

@HHobeck Thanks for your response.
Please consider the following git repository:
https://github.com/dauthleikr/gitversion-versionbump-repro

Checkout branch "feature". git log:

We expect 1.2.0, because there was a tag for 1.1.0 (version source), and a bump message +semver PRODUCTNAME: minor.
We get 1.1.0 ... until we create another bump message. This is because of the merge and the topological sort used in GetCommitsReacheableFromHead (see my previous message)

I used the following command to calculate the version: dotnet gitversion /config .\Example.yaml /output json /nocache

[HHobeck]

Okay thanks for creating the repository. If I see your configuration file I think it is not really necessary to overwhelm the contributors with irrelevant information. I would like to have a minimal example based on the given supported workflows like GitFlow/v1. For example you can use the following configuration for your use case to illustrate the behavior (even the custom tag definition or bump message regular expressions are not really necessary and can be simplified):

workflow: GitFlow/v1
tag-prefix: '(?i)PRODUCTNAME[:\s_\/vV]*'
minor-version-bump-message: '\+(sem)?ver\s?PRODUCTNAME[:\s_\/]*(feature|minor)'

[HHobeck]

I see your use case and think it might be a good idea for improvement. I think the code which needs to be changed is located in the following class:

GitVersion/src/GitVersion.Core/VersionCalculation/IncrementStrategyFinder.cs

Line 73 in c63c1a3

IEnumerable<ICommit> commits = GetCommitHistory(

I'm not sure if it will break other scenarios. Thus I would recommend you to create a prototype first and see if integration tests are failing. If we know this we can make a decision whether or not a new configuration property needs to be introduced.

[dauthleikr]

@HHobeck
Regarding the config: Thanks for the more minimal configuration, I was getting very frustrated with all the implicit behavior, so I started one from scratch ...

I have implemented a prototype which works for the described scenario. Only two tests are still failing, one of which is a performance test, and the other one interacts with a more complex configuration, which maybe a repository maintainer could take a look at?
Prototype: https://github.com/dauthleikr/GitVersion/tree/bump-message-before-merge-proto

Let me know if you want a Pull Request, integration test, etc., but I would greatly appreciate a repository maintainer completing this :)

[HHobeck]

I have implemented a prototype which works for the described scenario. Only two tests are still failing, one of which is a performance test, and the other one interacts with a more complex configuration, which maybe a repository maintainer could take a look at?
Prototype: https://github.com/dauthleikr/GitVersion/tree/bump-message-before-merge-proto

@dauthleikr: Great work. Thank you very much. I think you can use the following function to determine the graph history:

var intermediateCommits = this.repositoryStore.GetCommitLog(
    baseVersionSource: baseVersionSource,
    currentCommit: currentCommit,
    ignore: ignore
);

[HHobeck]

Let me know if you want a Pull Request, integration test, etc., but I would greatly appreciate a repository maintainer completing this :)

Integration test:
The failing test ensures exactly your scenario. Unfortunately the conditions are not well defined. In my opinion it should look as following:

  [TestCase(false, "2.0.0-alpha.3")]
  [TestCase(true, "3.0.0-alpha.2")]
  public void EnsureVersionAfterMainIsMergedBackToDevelopIsCorrectForGitFlow(bool applyTag, string semanticVersion)
  {
      var configuration = GitFlowConfigurationBuilder.New.Build();

      using var fixture = new EmptyRepositoryFixture();

      fixture.MakeACommit("A");
      fixture.ApplyTag("1.0.0");
      fixture.BranchTo("develop");
      fixture.MakeACommit("B +semver: major");

      // ✅ succeeds as expected
      fixture.AssertFullSemver("2.0.0-alpha.1", configuration);

      fixture.Checkout("main");
      fixture.MakeACommit("C");
      if (applyTag) fixture.ApplyTag("2.0.0");
      fixture.Checkout("develop");
      fixture.MergeNoFF("main");

      // ✅ succeeds as expected
      fixture.AssertFullSemver(semanticVersion, configuration);
  }

Performance test:
I'm uncertain how to fix the performance test. Your change could be problematic if you have a repository with a huge number of tags.

@asbjornu: What do you think? The time increased from under 5 seconds to 18 seconds for a repository with 500 tags.

[asbjornu]

I think we should avoid decreasing the performance of GitVersion, as we already have complaints about it being too slow in many scenarios. In this particular scenario, why is master merged into the PR branches instead of the PR branches being rebased onto HEAD of master?

[HHobeck]

I think we should avoid decreasing the performance of GitVersion, as we already have complaints about it being too slow in many scenarios. In this particular scenario, why is master merged into the PR branches instead of the PR branches being rebased onto HEAD of master?

Of course we need to address the performance issue. That's why we are talking about. From the business perspective it feels not right that the commit B +semver: Major will be ignored:

In my opinion we cannot force the user to always do a rebase. If you think about the risk when rewriting the history of the develop branch. This is normally a no-go and not recommended. I see the scenario of the author realistic.

Three actions coming to my mind to mediate the performance problem:

1. We could introduce a branch property with name CommitHistoryTopology to control this behavior (this will not really solve the performance issue)
2. We could restrict the tags in TaggedCommitVersionStrategy to only iterate to the highest possible version increment. For instance if you have two tags (tag 3.0.0 and a tag 1.0.0) you know for sure that the second tag will never be higher then the first tag. We can skip the second iteration.
3. Maybe with caching and/or parallel execution we can increase the performance

[dauthleikr]

Yes, sadly we cannot enforce a rebase-only workflow.

I briefly had time to profile this from my work machine - I found that most of the performance cost comes from repeated repository accesses, all the implemented commit caches always miss. I refactored one of the methods to not only cache its own result, but also cache all the children that come with it. I was able to achieve a 10x performance gain on the performance integration test with this experiment. Unfortunately I have to head out and do not have the time to run the tests, I believe they will take another half hour to run on this machine. I might get back to it tomorrow. Feel free to experiment with the following changed method from IncrementStrategyFinder:

private ICommit[] GetHeadCommits(ICommit? headCommit, IIgnoreConfiguration ignore)
{
    var key = headCommit?.Sha ?? "NULL";
    if (!this.headCommitsCache.TryGetValue(key, out var commits))
    {
        commits = [.. this.repositoryStore.GetCommitsReacheableFromHead(headCommit, ignore)];
        this.headCommitsCache[key] = commits;

        for (var i = commits.Length - 2; i >= 0; i--)
        {
            var prevKey = commits[i]?.Sha ?? "NULL";
            this.headCommitsCache.TryAdd(prevKey, commits[..(i + 1)]);
        }
    }

    return commits;
}