From bb3a976900a95fe2c4e242dba6112ba978ba7dac Mon Sep 17 00:00:00 2001 From: Tom Glastonbury <1101693+tg73@users.noreply.github.com> Date: Mon, 27 Sep 2021 12:05:24 +0100 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 (cherry picked from commit 96be5aae2f99f1ff7b123948d67a2635f04988ca) --- .../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 3d17c5f6..ee665f18 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 c1f16041..e57e6b68 100644 --- a/src/NerdBank.GitVersioning/Managed/ManagedGitExtensions.cs +++ b/src/NerdBank.GitVersioning/Managed/ManagedGitExtensions.cs @@ -171,22 +171,27 @@ bool TryCalculateHeight(GitCommit commit) if (pathFilters != 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; @@ -216,12 +221,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 @@ -233,8 +238,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}"; @@ -266,23 +272,27 @@ private static bool IsRelevantCommit(GitRepository repository, GitTree tree, Git if (parentEntry != 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; + } } }