From 96be5aae2f99f1ff7b123948d67a2635f04988ca Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Tue, 21 Sep 2021 09:53:32 -0600 Subject: [PATCH] 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 ContainsRelevantChanges(IEnumerable changes) => : 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; + } } }