v3.4 regressed version height calculation involving path filters

[marcominerva]

Hi!

I have a project with this version.json file:

{
	"$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/master/src/NerdBank.GitVersioning/version.schema.json",
	"version": "1.2",
	"publicReleaseRefSpec": [
		"^refs/heads/master$" // we release out of master
	],
	"nugetPackageVersion": {
		"semVer": 2
	},
	"pathFilters": [ "." ]
}

With this configuration, If I use Nerdbank.GitVersioning version 3.3.37, everything works like a charm. However, as soon as I update it to the v3.4.194, the git versioning isn't calculated anymore (it is always set to 0).

[AArnott]

Thanks for reporting. We have had a few regressions in 3.4 due to switching from libgit2 to our own managed git implementation. See our release notes for how to switch 3.4 back to libgit2 as a potential workaround.

I haven't heard of the symptom you describe. Can you provide repro steps that we can use?

[marcominerva]

You can see the problem in this repository: https://github.com/marcominerva/TinyHelpers/tree/develop/src/TinyHelpers. Now the Nerdbank.GitVersioning is version 3.3.37, so when I build the project (that contains the version.json file I shown you above), the version is correctly set to 1.2.xxx. If, instead, I update the version to v3.4.194 and just retry to rebuild the project, the version returns to the 1.2.0.

[AArnott]

Great. I can repro the problem. The workaround I linked you to is effective as well. I'll continue investigating.

[AArnott]

@qmfrederik this regression is apparently because some code was never converted for the managed git implementation around path filters support:

Nerdbank.GitVersioning/src/NerdBank.GitVersioning/Managed/ManagedGitExtensions.cs

Lines 197 to 216 in 8126d95

	/*
	// If there are no include paths, or any of the include
	// paths refer to the root of the repository, then do not
	// filter the diff at all.
	var diffInclude =
	includePaths.Count == 0 || pathFilters.Any(filter => filter.IsRoot)
	? null
	: includePaths;
	// If the diff between this commit and any of its parents
	// does not touch a path that we care about, don't bump the
	// height.
	var relevantCommit =
	commit.Parents.Any()
	? commit.Parents.Any(parent => ContainsRelevantChanges(commit.GetRepository().Diff
	.Compare<TreeChanges>(parent.Tree, commit.Tree, diffInclude, DiffOptions)))
	: ContainsRelevantChanges(commit.GetRepository().Diff
	.Compare<TreeChanges>(null, commit.Tree, diffInclude, DiffOptions));
	*/

Do you recall why this wasn't done? Is it going to be something we can fill in?

[qmfrederik]

There's obviously a bug, but I think the intent was that IsRelevantCommit replaces the code which was commented out.

Since you can reproduce the issue, can you share a unit test which reproduces the behavior?

[AArnott]

Yes, I'm working on writing a unit test now.

[AArnott]

Test added in the fix587 branch. It passes on libgit2 and fails on managed git. It only repros when the test uses a path filter two folders deep in the tree (1 level deep did not cut it), and included a version bump. @qmfrederik do you have time to investigate?

[aminzar-access]

@AArnott just wondering if this has been released already? Thanks.

[AArnott]

@amin-sage It's available on our CI feed here: https://dev.azure.com/andrewarnott/OSS/_packaging?_a=package&feed=PublicCI&package=Nerdbank.GitVersioning&protocolType=NuGet&version=3.4.205&view=overview

Can you test it and give us the thumbs up that it's effective before we push to nuget.org?

[tg73]

I just got hit by this issue - I didn't notice straight away and it was very confusing when I started seeing height 0 (height zero) on builds that were previously "sensible" (given that height should never be less than 1 unless using offset). I can confirm that 3.4.205 from the CI feed gives sensible height. Thanks @qmfrederik for the fix!

[AArnott]

I'm pushing 3.4.205 to nuget.org now.

[aminzar-access]

Works fine here as well, tested with 2 different projects, thanks.