-
Notifications
You must be signed in to change notification settings - Fork 654
Added test for issue #1046 & fix proposition #1136
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
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was problem with very long version calculating after my change, so I optimized this fix. Now calculation time is similiar to beta009 version. |
This all looks pretty reasonable to me, nice investigation and work! |
Investigating build failure..
and
0_o |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi,
I tried to fix issue #1046. I added test for it and I did some investigation.
Here's image describing structure of commits in this test:
I did fix for this, but after my changes two tests was failing:
ReleaseBranchScenarios.WhenReleaseBranchIsMergedIntoMasterHighestVersionIsTakenWithIt
andDevelopScenarios.InheritVersionFromReleaseBranch
.I changed asserts in failing tests, because looks like this tests had same issue as described in #1046. I don't know if it is proper solution. I didn't find this in documentation. Looks like corner cases.
Could someone check this is ok?
Short description of problem:
This line returned
release 2
commit for arguments:release after merge
anddevelop after merge
.Then calculated parent for
develop after merge
wasMerge branch 'release-2.0.0' into develop
. This commits have same merge base - returnedmergeBase
was alsorelease 2
(look at image).Because
findMergeBase
equalsmergeBase
, then loop stopped executing. It shouldn't stop becausemergeBaseWasForwardMerge
condition was still met.Solution
commitToFindCommonBase
should be moved up until there is no other merge bases withcommit
(current branch tip).