[Bug] /overrideconfig breaks configuration

[Inok]

When I specify /overrideconfig, GitVersion breaks with the following error:

System.Collections.Generic.KeyNotFoundException: The given key 'develop' was not present in the dictionary.
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at GitVersion.RepositoryMetadataProvider.GetChosenBranch(Config configuration) in D:\a\GitVersion\GitVersion\src\GitVersionCore\Core\RepositoryMetadataProvider.cs:line 208
   at GitVersion.Configuration.BranchConfigurationCalculator.InheritBranchConfiguration(Branch targetBranch, BranchConfig branchConfiguration, Commit currentCommit, Config configuration, IList`1 exclud
edInheritBranches) in D:\a\GitVersion\GitVersion\src\GitVersionCore\Configuration\BranchConfigurationCalculator.cs:line 124
...

Looks like the reason is ConfigExtensions.ApplyOverridesTo() implementation, which was changed at #2200, and since that PR it just overwrites everything instead of overriding explicitly specified configs.

Expected Behavior

/overrideconfig must not overwrite all configuration, it should override specified configs (we use it with 'tag-prefix').

Your Environment

Affected versions: 5.3.0+ (5.2.4 works great)

[asbjornu]

Since you've figured out where the problem is, would you be up for submitting a pull request with a test that provokes the problem and some code that also fixes it? 😃 🙏

[Inok]

I'll be glad to help, but could you please tell me what to do with all that stuff?

GitVersion/src/GitVersionCore/Configuration/ConfigExtensions.cs

Lines 177 to 188 in 12a2baa

	public static void ApplyOverridesTo(this Config config, Config overrideConfig)
	{
	config.Branches.Clear();
	config.Ignore = overrideConfig.Ignore;
	config.Branches = overrideConfig.Branches;
	config.Increment = overrideConfig.Increment;
	config.NextVersion = overrideConfig.NextVersion;
	config.VersioningMode = overrideConfig.VersioningMode;
	config.AssemblyFileVersioningFormat = overrideConfig.AssemblyFileVersioningFormat;
	config.TagPrefix = string.IsNullOrWhiteSpace(overrideConfig.TagPrefix) ? config.TagPrefix : overrideConfig.TagPrefix;
	config.TagPreReleaseWeight = overrideConfig.TagPreReleaseWeight;
	}

I guess it's acceptable to just get back to previous implementation and only override "tag prefix". It was the last behavior that worked properly, so it won't be a breaking change.
I could try to support overriding for another configs, but my opinion is that it should be another PR.

[asbjornu]

Do you have any thoughts or opinions about this, @arturcic?

[arturcic]

To be honest not really have knowledge of the code logic on that part unfortunatelly

[Inok]

So here is a repro: #2294
Unfortunately, not only executable file uses overrideConfig, but also tests. Tests override not only tag-prefix, and, which is worse, depend on a fact that not specified configs will be set to null, so the fix in the PR breaks a lot of tests. As a result, we can't just delete these lines.

I'll keep trying to fix that, and I would really appreciate any advice.

[asbjornu]

Thanks for taking a jab at this, @Inok! In GitVersion 6, we want all config properties to be overridable from the command line with the following (proposed) syntax:

gitversion calculate --override:tag-prefix="pr" --override:next-version="1.2.3"

Any change in that direction is welcomed. Tests that have the wrong expectation around this should be adjusted accordingly.

[Inok]

In GitVersion 6, we want all config properties to be overridable from the command line with the following (proposed) syntax:

It would be awesome, looking forward to it )

Tests that have the wrong expectation around this should be adjusted accordingly.

Unfortunately, there are a lot of tests with these expectations. The only simple solution I found that fixes the issue and also keeps test's behavior the same is the following:

1. Turn Config into an abstract class, add abstract void MergeTo (Config config) method to the Config. It allows us to specify merge strategy by choosing an appropriate derived class.
2. Inherit SoftMergeConfig from Config and use it in non-tests projects. It will merge configs softly (i.e. it will overwrite original value only if the overriding value is not null, as it was in 5.2.4 for TagPrefix)
3. Inherit OverwritingConfig from Config and use it for tests based on the current behavior (almost all tests).

What do you think about that? I could add the corresponding changes to the PR for a review, if that approach sound acceptable.

Of course, it would be better to not create various Config types and instead introduce IConfigMergeStrategy, but it looks like a far more complex change.

[asbjornu]

I think your proposed change is fine, but I would probably do it a bit differently:

1. Delete the ConfigExtensions class and move all methods from it into Config.
2. Rename ApplyOverridesTo to Merge and make it virtual.
3. Create a TestableConfig class used in the tests that performs an override of Merge(), implementing whatever is necessary to keep the current test behavior.

Does that sound like a viable solution?

[Inok]

I thought about that, and my only concern was that that solution doesn't force us to choose the appropriate type of a config.
But since you've told that

Tests that have the wrong expectation around this should be adjusted accordingly.

I think that your solution is definitely viable and is better than mine. So I'll try to do that.

Should I also mark TestableConfig with "Obsolete" attribute to avoid its future use and to force new tests to use default config or override it explicitly?