GitVersion icon indicating copy to clipboard operation
GitVersion copied to clipboard

Failing test for merge of remote main with local main in mainline mode

Open gautelo opened this issue 3 years ago • 9 comments

Added test asserting that merging changes on remote main with change in local main should increment semver for all commits in both remote and local main.

Related Issue

#2739

Motivation and Context

In order to avoid several commits creating the same semver.

How Has This Been Tested?

It is a failing thest for a bug report.

Checklist:

  • [x] My code follows the code style of this project.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [x] I have added tests to cover my changes.
  • [ ] All new and existing tests passed.

gautelo avatar Jun 29 '21 14:06 gautelo

I should mention that I attempted to use the RemoteRepositoryFixture, but that I don't believe it's well suited to the test in question. When attempting to commit both to the remote fixture and to the fixture.LocalRepositoryFixture the latter generated completely incorrect versions. Not exactly sure why. By instead using a clone to represent the local repository I got what I wanted. So, since this gave me what I wanted, I didn't dig further into the RemoteRepositoryFixture.

gautelo avatar Jun 29 '21 14:06 gautelo

Is there something further I need to do for this to progress? I could try to make changes necessary for the test to pass, but honestly I would need some guidance on where to start.

gautelo avatar Jul 11 '21 20:07 gautelo

I'm not sure this is a bug, to be honest. The local branch has two commits: l.1 and the merge commit from the remote main branch. The name of the remote branch won't and can't be considered, since branch names are ephemeral. If a feature/* branch was merged, would you still expect the version number to be 1.0.5?

asbjornu avatar Jul 12 '21 11:07 asbjornu

I would expect a merged feature/* branch into the main branch to increment differently, but I expect merging remote feature/* into local feature/* will generate the same kind of issue.

The problem here is that there are two main branches that are merged, and in this case a version number is being reused for different commits. In my opinion two different commits should never generate the same semver. (Unless you force it through a tag or commit message, of course.)

Not sure what the solution is, but, as I describe in the connected issue the problem that arises is that the build system will generate the same semver for different commits causing a NuGet package conflict in the build caused by the merge commit.

The way I see it, when there is a merge with same named branch it's reasonable to assume all commits are counted rather than only one of the branches.

gautelo avatar Jul 12 '21 12:07 gautelo

The way I see it, when there is a merge with same named branch it's reasonable to assume all commits are counted rather than only one of the branches.

The problem with this is that GitVersion needs to be able to generate the same version number based on the Git history today (when the merged branch possibly still exists) and 3 years from now (when the merged branch is deleted).

Since branches are ephemeral, their name can't be used for anything useful other than for the version number generated for the branch itself (such as release/* branches that usually contain a version number). I've created #2750 to clarify this in the documentation.

asbjornu avatar Jul 12 '21 13:07 asbjornu

That documentation change does not seem correct.

However, when the release/1.2.3 branch is merged into main, the fact that the merged commits came from a branch named release/1.2.3 vanishes with the branch which will be deleted. The name of the merged release branch can therefore not be considered for version calculation in the target branch of the merge.

When you merge the 'release/1.2.3' branch the version of the current branch is bumped according to 1.2.3, but when you merge a 'feature/something' branch it merges according to different rules. I actually think you can merge a 'release/some-alias-for-the-release' and have a different version bump than when you merge a 'feature/something'. That means that indeed the merge name does not disappear. Because it is encoded in the merge commit message and is taken into account.

gautelo avatar Jul 13 '21 09:07 gautelo

When you merge the 'release/1.2.3' branch the version of the current branch is bumped according to 1.2.3

Hm. It may be that MergeMessageVersionStrategy somehow deals with this, but we don't have any tests that explicitly cover this. Here are the merge message formats GitVersion supports:

https://github.com/GitTools/GitVersion/blob/54a3eb3056403ae5243c19d7656cb9687120e03a/src/GitVersion.Core/Model/MergeMessage.cs#L13-L18

We have tests in ReleaseBranchScenarios that verify that the calculated version number is taken from the merged release branch, but those that remove the release branch also adds a git tag first. Example:

https://github.com/GitTools/GitVersion/blob/54a3eb3056403ae5243c19d7656cb9687120e03a/src/GitVersion.Core.Tests/IntegrationTests/ReleaseBranchScenarios.cs#L16-L31

The safe way to ensure the version number 1.2.3 stays for the future is to tag the merge-commit of release/1.2.3 into main with 1.2.3.

but when you merge a 'feature/something' branch it merges according to different rules. I actually think you can merge a 'release/some-alias-for-the-release' and have a different version bump than when you merge a 'feature/something'.

Yes, that's because release/* branches use a different versioning strategy than feature/* branches. This is explicit all the way from the configuration of is-release-branch for release/* branches and throughout the codebase.

Because it is encoded in the merge commit message and is taken into account.

It may be, but as the feature is currently implemented, it almost seems accidental. We should add more tests to make this support explicit and more obvious.

asbjornu avatar Jul 13 '21 13:07 asbjornu

Mhm. I'll try to find some time to write further tests asserting the behavior discussed in another PR once I'm back from vacation. Then I'll come back to this PR afterwards and we can discuss further.

gautelo avatar Jul 13 '21 15:07 gautelo

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 Mar 02 '22 08:03 stale[bot]