From 3866cd6c40195d015b7a567436779471e7cea5db Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Tue, 21 Sep 2021 09:53:32 -0600 Subject: [PATCH 1/2] Add test for version height with path filters and merge conflict This demonstrates the repro for #658 --- .../VersionOracleTests.cs | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/NerdBank.GitVersioning.Tests/VersionOracleTests.cs b/src/NerdBank.GitVersioning.Tests/VersionOracleTests.cs index 4f10abf1..9ae579d6 100644 --- a/src/NerdBank.GitVersioning.Tests/VersionOracleTests.cs +++ b/src/NerdBank.GitVersioning.Tests/VersionOracleTests.cs @@ -751,6 +751,38 @@ public void GetVersion_PathFilterInTwoDeepSubDirAndVersionBump() Assert.Equal(1, this.GetVersionHeight(relativeDirectory)); } + [Fact] + public void GetVersion_PathFilterPlusMerge() + { + this.InitializeSourceControl(withInitialCommit: false); + this.WriteVersionFile(new VersionOptions + { + Version = new SemanticVersion("1.0"), + PathFilters = new FilterPath[] { new FilterPath(".", string.Empty) }, + }); + + string conflictedFilePath = Path.Combine(this.RepoPath, "foo.txt"); + + File.WriteAllText(conflictedFilePath, "foo"); + Commands.Stage(this.LibGit2Repository, conflictedFilePath); + this.LibGit2Repository.Commit("Add foo.txt with foo content.", this.Signer, this.Signer); + Branch originalBranch = this.LibGit2Repository.Head; + + Branch topicBranch = this.LibGit2Repository.Branches.Add("topic", "HEAD~1"); + Commands.Checkout(this.LibGit2Repository, topicBranch); + File.WriteAllText(conflictedFilePath, "bar"); + Commands.Stage(this.LibGit2Repository, conflictedFilePath); + this.LibGit2Repository.Commit("Add foo.txt with bar content.", this.Signer, this.Signer); + + Commands.Checkout(this.LibGit2Repository, originalBranch); + MergeResult result = this.LibGit2Repository.Merge(topicBranch, this.Signer, new MergeOptions { FileConflictStrategy = CheckoutFileConflictStrategy.Ours }); + Assert.Equal(MergeStatus.Conflicts, result.Status); + Commands.Stage(this.LibGit2Repository, conflictedFilePath); + this.LibGit2Repository.Commit("Merge two branches", this.Signer, this.Signer); + + Assert.Equal(3, this.GetVersionHeight()); + } + [Fact] public void GetVersionHeight_ProjectDirectoryDifferentToVersionJsonDirectory() { From 96be5aae2f99f1ff7b123948d67a2635f04988ca Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Tue, 21 Sep 2021 09:53:32 -0600 Subject: [PATCH 2/2] Count merge commits in path filtered height if any parent changed it The managed git implementation had it correctly implemented, according to the code comments in the libgit2 implementation. But those code comments did not match the *code* in the libgit2 implementation. As a result, the managed git code did not match. In this commit I correct the libgit2 impl code comments to describe what the code *actually* does. Then I fix the identical code comment and code in the managed git implementation to match. Fixes #658 --- .../LibGit2/LibGit2GitExtensions.cs | 4 +- .../Managed/ManagedGitExtensions.cs | 56 +++++++++++-------- 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/src/NerdBank.GitVersioning/LibGit2/LibGit2GitExtensions.cs b/src/NerdBank.GitVersioning/LibGit2/LibGit2GitExtensions.cs index 045c647a..b2ff2d35 100644 --- a/src/NerdBank.GitVersioning/LibGit2/LibGit2GitExtensions.cs +++ b/src/NerdBank.GitVersioning/LibGit2/LibGit2GitExtensions.cs @@ -433,8 +433,8 @@ bool TryCalculateHeight(Commit commit) : 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. + // touches a path that we care about, bump the height. + // A no-parent commit is relevant if it introduces anything in the filtered path. var relevantCommit = commit.Parents.Any() ? commit.Parents.Any(parent => ContainsRelevantChanges(commit.GetRepository().Diff diff --git a/src/NerdBank.GitVersioning/Managed/ManagedGitExtensions.cs b/src/NerdBank.GitVersioning/Managed/ManagedGitExtensions.cs index 2f05103e..e51bf4ea 100644 --- a/src/NerdBank.GitVersioning/Managed/ManagedGitExtensions.cs +++ b/src/NerdBank.GitVersioning/Managed/ManagedGitExtensions.cs @@ -169,22 +169,27 @@ bool TryCalculateHeight(GitCommit commit) if (pathFilters is not null) { - var relevantCommit = true; - - foreach (var parentId in commit.Parents) + // If the diff between this commit and any of its parents + // touches a path that we care about, bump the height. + bool relevantCommit = false, anyParents = false; + foreach (GitObjectId parentId in commit.Parents) { - var parent = repository.GetCommit(parentId); - relevantCommit = IsRelevantCommit(repository, commit, parent, pathFilters); - - // 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. - if (!relevantCommit) + anyParents = true; + GitCommit parent = repository.GetCommit(parentId); + if (IsRelevantCommit(repository, commit, parent, pathFilters)) { + // No need to scan further, as a positive match will never turn negative. + relevantCommit = true; break; } } + if (!anyParents) + { + // A no-parent commit is relevant if it introduces anything in the filtered path. + relevantCommit = IsRelevantCommit(repository, commit, parent: default(GitCommit), pathFilters); + } + if (!relevantCommit) { height = 0; @@ -214,12 +219,12 @@ private static bool IsRelevantCommit(GitRepository repository, GitCommit commit, return IsRelevantCommit( repository, repository.GetTree(commit.Tree), - repository.GetTree(parent.Tree), + parent != default ? repository.GetTree(parent.Tree) : null, relativePath: string.Empty, filters); } - private static bool IsRelevantCommit(GitRepository repository, GitTree tree, GitTree parent, string relativePath, IReadOnlyList filters) + private static bool IsRelevantCommit(GitRepository repository, GitTree tree, GitTree? parent, string relativePath, IReadOnlyList filters) { // Walk over all child nodes in the current tree. If a child node was found in the parent, // remove it, so that after the iteration the parent contains all nodes which have been @@ -231,8 +236,9 @@ private static bool IsRelevantCommit(GitRepository repository, GitTree tree, Git // If the entry is not present in the parent commit, it was added; // if the Sha does not match, it was modified. - if (!parent.Children.TryGetValue(child.Key, out parentEntry) - || parentEntry.Sha != child.Value.Sha) + if (parent is null || + !parent.Children.TryGetValue(child.Key, out parentEntry) || + parentEntry.Sha != child.Value.Sha) { // Determine whether the change was relevant. var fullPath = $"{relativePath}{entry.Name}"; @@ -264,23 +270,27 @@ private static bool IsRelevantCommit(GitRepository repository, GitTree tree, Git if (parentEntry is not null) { + Assumes.NotNull(parent); parent.Children.Remove(child.Key); } } // Inspect removed entries (i.e. present in parent but not in the current tree) - foreach (var child in parent.Children) + if (parent is not null) { - // Determine whether the change was relevant. - var fullPath = Path.Combine(relativePath, child.Key); + foreach (var child in parent.Children) + { + // Determine whether the change was relevant. + var fullPath = Path.Combine(relativePath, child.Key); - bool isRelevant = - filters.Any(f => f.Includes(fullPath, repository.IgnoreCase)) - && !filters.Any(f => f.Excludes(fullPath, repository.IgnoreCase)); + bool isRelevant = + filters.Any(f => f.Includes(fullPath, repository.IgnoreCase)) + && !filters.Any(f => f.Excludes(fullPath, repository.IgnoreCase)); - if (isRelevant) - { - return true; + if (isRelevant) + { + return true; + } } }