Feature branch does not pick up version #1004

maxraab · 2016-08-11T12:00:44Z

based on a bug discovered in #988

maxraab · 2016-08-11T12:11:14Z

/cc @JakeGinnivan

asbjornu · 2016-08-11T12:12:14Z

@maxraab Could you rebase this PR on the latest master, please? 😄

maxraab · 2016-08-11T12:24:14Z

@asbjornu done 😄

JakeGinnivan · 2016-08-11T15:19:06Z

I wonder if we should rename is-develop to tracks-release-branches which is more clear about what it is doing?

maxraab · 2016-08-11T19:04:27Z

Sounds good, should I rename all occurences?

JakeGinnivan · 2016-08-11T23:35:17Z

Yeah, also you will need to add validation in the configuration provider to tell people it has renamed.

maxraab · 2016-08-12T08:21:15Z

@JakeGinnivan I've rebased to the current master and renamed is-develop to tracks-release-branches, done some validation, add tests and updated the documenation.

asbjornu · 2016-08-12T09:04:23Z

src/GitVersionCore/Configuration/BranchConfig.cs

            IsReleaseBranch = branchConfiguration.IsReleaseBranch;
            IsMainline = branchConfiguration.IsMainline;
        }

        [YamlMember(Alias = "mode")]
-        public VersioningMode? VersioningMode { get; set; }
+        public VersioningMode? VersioningMode


Can you please not reformat code that is not a part of your PR? 😃

I've restored the original format but we should improve that in another PR.

@maxraab If the current ReSharper settings made the properties multiline, we should indeed fix that in its own PR.

maxraab · 2016-08-12T11:41:49Z

@asbjornu @JakeGinnivan For some reason, the AppVeyor build fails.. How should I handle it?

asbjornu · 2016-08-12T12:08:31Z

@maxraab It's the Create-Release-Notes task that fails. I think @JakeGinnivan has a theory that it might be due to a missing GitHub API key, making the calls to GitHub throttled and thus failing. Not sure, but just giving it a bit of time and then rerunning the test should do.

DanielRose · 2016-08-12T13:04:08Z

I've been thinking about this a bit, and I'm not so sure if this PR (while fixing the stated issue) actually makes sense.

The way I understand GitVersion logically/conceptionally, i.e. not the actual implementation (to simplify, I'll ignore next-version and when/how the obtained version number is later incremented):

First it checks in the current branch for a version number (ex. via tag, branch name, merged branch name; stated in the documentation as Version Sources)
- branch is release/1.1 => 1.1
- commit with tag 1.2 => 1.2
- branch release/1.3 was merged into current branch => 1.3
If no version number could be found, it checks the parent branch for its version number at the point where it branched
- branch feature/special branched from release/1.1 => no version number in feature/special => asks release/1.1 => 1.1

The change with is-develop was supposed to be: For a branch marked with it, creating a branch off it (with a version number) also picks up the version-number. So in step 1:

branch is develop, branch release/1.4 was created from it => 1.4

This works. But the bug is that step 2 does not work properly:

situation as above, then branch feature/awesome is branched from develop => 0.1 (simple case, no tags, merges, etc.); should be 1.4

The bug is that in step 2, the develop branch, when asked for its version number, acts as if is-develop was not set, and thus returns 0.1.

Now the OP noticed that if the branch feature/awesome is marked as is-develop, then it suddenly works. So the solution is basically to mark every branch from develop with that setting (and give it a different name).

I think this solution tries to fix the bug at the wrong place. It essentially means that all branch configurations (except perhaps master) need to be marked that way. Also logically it makes no sense: Why do I need to mark a branch with is-develop, so that when asking the parent branch, the parent branch acts as if is-develop was set there? Shouldn't that be a setting of the parent branch?

I see two more sensible solutions:

Add "Version of child branch name" as an additional Version Source, which always applies. In that case, is-develop can be dropped.
- Solution: replace all checks for is-develop in the code with true, and drop dead code.
Fix the bug, without needing to apply is-develop everywhere (and perhaps give it a better name).
- My guess at what actually happens in the code, and why this bug occurs: Step 2 is not implemented as ask the parent branch what its version is at the branch point (i.e. use the config of the parent branch), but rather as go into parent branch and continue version calculation (i.e. use the config of the current branch). However, the config for version calculation can be different between the current branch and the parent branch. By continuing the use the original config, the bug occurs.
- Solution: When going into a parent branch as part of version calculation, change to use the config of that parent branch.

Note that if my guess is correct, then there should be more bugs in the version calculation when version-calculation-relevant configs are different between two branches, such as increment or track-merge-target.

JakeGinnivan · 2016-08-12T13:27:56Z

src/GitVersionCore/BranchConfigurationCalculator.cs

@@ -18,7 +16,7 @@ public class BranchConfigurationCalculator
            if (matchingBranches.Length == 0)
            {
                var branchConfig = new BranchConfig();
-                ConfigurationProvider.ApplyBranchDefaults(config, branchConfig);
+                ConfigurationProvider.ApplyBranchDefaults(config, branchConfig, tracksReleaseBranches: true);


Not sure this is right actually as @DanielRose says. The default shouldn't be to track release branches because there could be feature branches off support branches or any number of other scenarios.

JakeGinnivan · 2016-08-12T13:43:24Z

is-develop and is-release-branch work together. Basically if a branch is marked is-develop then gitversion will try and find any release branches and use them as a base version. Track merge target I found really confusing and it kept having random bugs, this was quite simple.

It is an odd one, because by branching from the develop branch it automatically bumps. DevelopVersionStrategy should also be renamed to TrackReleaseBranch and instead of just checking context.Configuration.IsCurrentBranchDevelop that should be renamed to context.Configuration.DoesCurrentBranchInheritFromDevelop and we should check if any of the parent branches are develop. @DanielRose does that sound closer to fixing the actual issue?

maxraab · 2016-08-14T09:32:48Z

@JakeGinnivan Looks good to me. It would be handy if all branches inherit settings of their parent branches (except the branch has specific settings).

Anyway.. this bug prevents us using GitVersion. I hope we could fix it soon :)

DanielRose · 2016-08-16T09:46:20Z

I created a number of tests, as #1009. When using "normally named" feature branches, ex. feature/test, it works as expected (both with a normal git flow and branching directly off master).

However, when using these "misnamed" feature branches, i.e. those without a config (as in this issue), it does not work. The reason seems to be the value of the increment strategy: The default value (in ConfigurationProvider) is IncrementStrategy.Patch, while for feature-branches it is IncrementStrategy.Inherit. With Inherit, the version numbers get picked up correctly.

The documentation of increment is very short: The part of the SemVer to increment when GitVersion detects it needs to be (i.e commit after a tag).

In the code, Inherit is additionally documented as Uses the increment strategy from the branch the current branch was branched from. But looking at the test results, and a quick look at the code in BranchConfigurationCalculator.GetBranchConfiguration(), Inherit actually means Use the version number as calculated by the parent branch at the branch-point (which can be overridden by using tags, etc.). For all other values, the parent branch config is apparently not used.

So what I previously described as step 2 is actually only done when the branch uses the Inherit strategy. So either the description or the code should be updated. Also, it would be useful to have a way for the user to set the default config values (i.e. those which are used when no branch config exists).

Aside:

In the tests, when an increment != Inherit is used, the calculated version number in the feature branch is different when a release branch is only created off develop/master, or the release branch is merged back.

In the first case, no version number is found => uses 0.1.0 as base value.

In the second case, the merged number (1.0) in the parent branch is then incremented according to the strategy of the feature branch (=> 2.0, 1.1, 1.0.1, ...), ignoring how it was incremented in the parent branch.

DanielRose · 2016-08-25T09:48:47Z

@JakeGinnivan Can you comment on what should be done?

JakeGinnivan · 2016-08-26T05:28:55Z

I am not sure right now. Have DDD Perth tomorrow and this week has been pretty busy. Will try to catch up with this thread on Sunday

JakeGinnivan · 2016-08-29T13:12:51Z

@DanielRose is it just that the default should be inherit rather than patch? Or is there something else?

DanielRose · 2016-08-29T16:04:01Z

@JakeGinnivan Well, the bug here would (more or less - the commit counts might be unexpected) be fixed if the default was inherit.

However, this bug exposes that the behaviour of "increment" is different than what is documented (and expected - at least to me), as well as the behaviour when switching branches.

Expected:
major, minor, patch => Increase that part of the version number
inherit => Use the value of the parent branch
All these values are unique per branch, according to that branch's config. So during version calculation, when changing to the parent branch, that branch's config is then (always) used subsequently.

Actual:
major, minor, patch => Increase that part of the version number
inherit => Use the config and calculation of the parent branch
These values belong to the current branch. So during version calculation, when changing to the parent branch, that branch's config is only used if inherit was specified.
More precisely: Take the graph along the commits (including following any parent branches). Then act as if they were all on one branch, and use the single value of increment to determine how increases in the version number are to be handled.

An example (numbers don't make sense, just to illustrate):
master branch, increment major, tagged 1.0.0, some more commits
child_minor branch, increment minor, later tagged 3.2.0, some more commits
child_inherit branch, increment inherit, later tagged 5.0.0, some more commits

Point in time	expected	actual
master (after tag)	2.0.0	2.0.0
child_minor (before tag)	2.0.0	*1.1.0*
child_minor (after tag)	3.3.0	3.3.0
child_inherit (before tag)	2.0.0	2.0.0
child_inherit (after tag)	6.0.0	6.0.0

JakeGinnivan · 2016-08-30T00:48:22Z

More precisely: Take the graph along the commits (including following any parent branches). Then act as if they were all on one branch, and use the single value of increment to determine how increases in the version number are to be handled.

In reality does this end up with a different version than us figuring out parent, calculating version then taking that version instead? I know the description doesn't exactly match what we end up doing, but if we switched the default over to inherit would that fix the incorrect versions. Or do we need to change the way we calculate the version when inheriting?

JakeGinnivan · 2016-08-30T00:49:23Z

I am happy for you to propose fixes, either to the docs or to the code so they line up. Also happy for default to change to inherit in v4.

DanielRose · 2016-08-30T06:30:47Z

More precisely: Take the graph along the commits (including following any parent branches). Then act as if they were all on one branch, and use the single value of increment to determine how increases in the version number are to be handled.

In reality does this end up with a different version than us figuring out parent, calculating version then taking that version instead? I know the description doesn't exactly match what we end up doing, but if we switched the default over to inherit would that fix the incorrect versions. Or do we need to change the way we calculate the version when inheriting?

You can see the difference in the table I posted. On master, we are on version 2.0. Then branch to "child_minor", and suddenly the version is 1.1. Changing the default to inherit wouldn't help, since it has an explicit value "minor".

I am happy for you to propose fixes, either to the docs or to the code so they line up. Also happy for default to change to inherit in v4.

Ok, I'll try to find some time for that.

maxraab · 2016-09-12T17:32:29Z

@JakeGinnivan @DanielRose: What is the current state of play?

DanielRose · 2016-09-20T06:39:41Z

@maxraab @JakeGinnivan I was on vacation. I'll take a closer look at the code in the next couple of days.

DanielRose · 2016-09-29T07:21:00Z

I was able to take a closer look at the code and started looking how to fix this. Mainly so far I've added many comments to better understand what is being done.

The main changes need to be done in the version calculators themselves. Those that work with the commits (ex. MergeMessageBaseVersionStrategy) use the LibGit Branch.Commits directly. However, a branch (and especially its commits) are defined by LibGit to include the parent branches as well. So I'll need to change this to have a property with the commits only of the current branch (i.e. only up to the point where the branch was created).

So that is my next step, which I wasn't able to do so far.

maxraab · 2016-11-03T12:13:31Z

@DanielRose Are there any news concerning the version calculation?

DanielRose · 2016-11-03T16:57:54Z

@maxraab I was able to get a list of branch configs and the commits belonging to them. Next I'll need to modify the version calculators to use that list instead. I'll try to find some time to continue working on it.

DanielRose · 2016-11-15T16:30:34Z

@maxraab @JakeGinnivan I tried a bunch of approaches, but my idea fundamentally does not seem to work with Git. The problem is that Git has no concept of a "parent branch", since in its heart is a DAG with labels (see also http://stackoverflow.com/questions/3161204/find-the-parent-branch-of-a-git-branch).

Lets say I have the following repository:

     A1 --- A2 (branch A)
    /
---X
    \
     B1 --- B2 (branch B)

What branch does commit X belong to? The correct answer (in Git) is: A and B.

So when running GitVersion at commit B2, should I use the config of branch B for the commits B2, B1, X? Or should I use the config of branch B for the commits B2, B1, and then the config of branch A for the commit X? I could use some heuristics and/or additional config (ex. "master" always wins), but then this will get complicated quickly.

BTW, the reason this was not a problem until now, is that looking for a "parent" branch only happened when the configuration said to inherit from the parent branch. That way we know there is a parent branch (or at least there is supposed to be one), and in the previous case it is clear that the commit X belongs to branch A.

So how can I salvage at least part of what I did? My suggestion would be:

Keep the additional comments I put in the code.
Clarify the documentation of the inherit increment.
Have the default value for branches (i.e. those without any config) be inherit.

JakeGinnivan · 2016-11-26T09:25:02Z

Thanks for your patience @maxraab. I have rebased this.

@DanielRose I don't think this and your branch are incompatible, I like the rename of is-develop to tracks-release-branch so want to get that part in. Going to pull your inherit changes in next

maxraab force-pushed the feature-branch-does-not-pick-up-version branch from b85a3ed to 7ce9d46 Compare August 11, 2016 12:13

maxraab added 2 commits August 12, 2016 09:07

Add test to verify the behaviour

9756252

Mark default branch config with isDevelop

31de062

maxraab force-pushed the feature-branch-does-not-pick-up-version branch from 7ce9d46 to f42feae Compare August 12, 2016 08:20

maxraab force-pushed the feature-branch-does-not-pick-up-version branch from f42feae to 50f45ea Compare August 12, 2016 08:33

asbjornu reviewed Aug 12, 2016
View reviewed changes

maxraab added 2 commits August 12, 2016 11:20

Rename isDevelop to tracksReleaseBranches

2e21cb6

Add test to validate obsolete config settings

b76bca9

maxraab force-pushed the feature-branch-does-not-pick-up-version branch 5 times, most recently from f5ca9c3 to 895140b Compare August 12, 2016 11:24

Warn on obsolete branch configuration settings

6f327b9

maxraab force-pushed the feature-branch-does-not-pick-up-version branch from 895140b to 6f327b9 Compare August 12, 2016 11:30

JakeGinnivan reviewed Aug 12, 2016
View reviewed changes

DanielRose mentioned this pull request Aug 16, 2016

Add various tests for inheriting version numbers in feature branches. #1009

Closed

qetza mentioned this pull request Aug 19, 2016

Rename is-develop to track-releases-branches #1013

Closed

JakeGinnivan added this to the 4.0.0 milestone Oct 2, 2016

DanielRose mentioned this pull request Nov 17, 2016

Use the Inherit strategy by default #1085

Closed

JakeGinnivan mentioned this pull request Nov 26, 2016

Feature branch does not pick up version #1102

Merged

JakeGinnivan closed this Nov 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature branch does not pick up version #1004

Feature branch does not pick up version #1004

maxraab commented Aug 11, 2016

maxraab commented Aug 11, 2016

asbjornu commented Aug 11, 2016

maxraab commented Aug 11, 2016

JakeGinnivan commented Aug 11, 2016

maxraab commented Aug 11, 2016

JakeGinnivan commented Aug 11, 2016

maxraab commented Aug 12, 2016 •

edited

Loading

asbjornu Aug 12, 2016

maxraab Aug 12, 2016

asbjornu Aug 12, 2016

maxraab commented Aug 12, 2016 •

edited

Loading

asbjornu commented Aug 12, 2016

DanielRose commented Aug 12, 2016

JakeGinnivan Aug 12, 2016

JakeGinnivan commented Aug 12, 2016

maxraab commented Aug 14, 2016

DanielRose commented Aug 16, 2016

DanielRose commented Aug 25, 2016

JakeGinnivan commented Aug 26, 2016

JakeGinnivan commented Aug 29, 2016

DanielRose commented Aug 29, 2016 •

edited

Loading

JakeGinnivan commented Aug 30, 2016

JakeGinnivan commented Aug 30, 2016

DanielRose commented Aug 30, 2016

maxraab commented Sep 12, 2016

DanielRose commented Sep 20, 2016

DanielRose commented Sep 29, 2016

maxraab commented Nov 3, 2016

DanielRose commented Nov 3, 2016

DanielRose commented Nov 15, 2016

JakeGinnivan commented Nov 26, 2016

Feature branch does not pick up version #1004

Feature branch does not pick up version #1004

Conversation

maxraab commented Aug 11, 2016

maxraab commented Aug 11, 2016

asbjornu commented Aug 11, 2016

maxraab commented Aug 11, 2016

JakeGinnivan commented Aug 11, 2016

maxraab commented Aug 11, 2016

JakeGinnivan commented Aug 11, 2016

maxraab commented Aug 12, 2016 • edited Loading

asbjornu Aug 12, 2016

Choose a reason for hiding this comment

maxraab Aug 12, 2016

Choose a reason for hiding this comment

asbjornu Aug 12, 2016

Choose a reason for hiding this comment

maxraab commented Aug 12, 2016 • edited Loading

asbjornu commented Aug 12, 2016

DanielRose commented Aug 12, 2016

JakeGinnivan Aug 12, 2016

Choose a reason for hiding this comment

JakeGinnivan commented Aug 12, 2016

maxraab commented Aug 14, 2016

DanielRose commented Aug 16, 2016

DanielRose commented Aug 25, 2016

JakeGinnivan commented Aug 26, 2016

JakeGinnivan commented Aug 29, 2016

DanielRose commented Aug 29, 2016 • edited Loading

JakeGinnivan commented Aug 30, 2016

JakeGinnivan commented Aug 30, 2016

DanielRose commented Aug 30, 2016

maxraab commented Sep 12, 2016

DanielRose commented Sep 20, 2016

DanielRose commented Sep 29, 2016

maxraab commented Nov 3, 2016

DanielRose commented Nov 3, 2016

DanielRose commented Nov 15, 2016

JakeGinnivan commented Nov 26, 2016

maxraab commented Aug 12, 2016 •

edited

Loading

maxraab commented Aug 12, 2016 •

edited

Loading

DanielRose commented Aug 29, 2016 •

edited

Loading