Skip to content

Commit

Permalink
Merge pull request #471 from dotnet/fixSO
Browse files Browse the repository at this point in the history 
Fix StackOverflowException that can break a build
  • Loading branch information
@AArnott
AArnott committed Apr 28, 2020
2 parents 2819578 + ddb9510 commit 7f4b4d9
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 56 deletions.
17 changes: 14 additions & 3 deletions src/NerdBank.GitVersioning.Tests/GitExtensionsTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,17 @@ public void GetVersionHeight_ProjectDirectoryIsMoved()
Assert.Equal(1, this.Repo.GetVersionHeight("new-project-dir"));
}

[Fact(Skip = "Slow test")]
public void GetVersionHeight_VeryLongHistory()
{
this.WriteVersionFile();

// Make a *lot* of commits
this.AddCommits(2000);

this.Repo.GetVersionHeight();
}

[Fact]
public void GetCommitsFromVersion_WithPathFilters()
{
Expand Down Expand Up @@ -775,16 +786,16 @@ public void GetIdAsVersion_MigrationFromVersionTxtToJson()
[SkippableFact] // Skippable, only run test on specific machine
public void TestBiggerRepo()
{
var testBiggerRepoPath = @"C:\Users\andrew\git\NerdBank.GitVersioning";
var testBiggerRepoPath = @"D:\git\NerdBank.GitVersioning";
Skip.If(!Directory.Exists(testBiggerRepoPath));

using (this.Repo = new Repository(testBiggerRepoPath))
{
foreach (var commit in this.Repo.Head.Commits)
{
var version = commit.GetIdAsVersion();
var version = commit.GetIdAsVersion("src");
this.Logger.WriteLine($"commit {commit.Id} got version {version}");
var backAgain = this.Repo.GetCommitFromVersion(version);
var backAgain = this.Repo.GetCommitFromVersion(version, "src");
Assert.Equal(commit, backAgain);
}
}
Expand Down
139 changes: 86 additions & 53 deletions src/NerdBank.GitVersioning/GitExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -708,84 +708,117 @@ private static string EncodeAsHex(byte[] buffer)
/// Gets the number of commits in the longest single path between
/// the specified branch's head and the most distant ancestor (inclusive).
/// </summary>
/// <param name="commit">The commit to measure the height of.</param>
/// <param name="startingCommit">The commit to measure the height of.</param>
/// <param name="tracker">The caching tracker for storing or fetching version information per commit.</param>
/// <param name="continueStepping">
/// A function that returns <c>false</c> when we reach a commit that
/// should not be included in the height calculation.
/// May be null to count the height to the original commit.
/// </param>
/// <returns>The height of the branch.</returns>
private static int GetCommitHeight(Commit commit, GitWalkTracker tracker, Func<Commit, bool> continueStepping)
private static int GetCommitHeight(Commit startingCommit, GitWalkTracker tracker, Func<Commit, bool> continueStepping)
{
Requires.NotNull(commit, nameof(commit));
Requires.NotNull(startingCommit, nameof(startingCommit));
Requires.NotNull(tracker, nameof(tracker));

if (!tracker.TryGetVersionHeight(commit, out int height))
if (continueStepping is object && !continueStepping(startingCommit))
{
return 0;
}

var commitsToEvaluate = new Stack<Commit>();
bool TryCalculateHeight(Commit commit)
{
if (continueStepping == null || continueStepping(commit))
// Get max height among all parents, or schedule all missing parents for their own evaluation and return false.
int maxHeightAmongParents = 0;
bool parentMissing = false;
foreach (Commit parent in commit.Parents)
{
if (!tracker.TryGetVersionHeight(parent, out int parentHeight))
{
if (continueStepping is object && !continueStepping(parent))
{
// This parent isn't supposed to contribute to height.
continue;
}

commitsToEvaluate.Push(parent);
parentMissing = true;
}
else
{
maxHeightAmongParents = Math.Max(maxHeightAmongParents, parentHeight);
}
}

if (parentMissing)
{
var versionOptions = tracker.GetVersion(commit);
var pathFilters = versionOptions?.PathFilters;
return false;
}

var includePaths =
pathFilters
?.Where(filter => !filter.IsExclude)
.Select(filter => filter.RepoRelativePath)
.ToList();
var versionOptions = tracker.GetVersion(commit);
var pathFilters = versionOptions?.PathFilters;

var excludePaths = pathFilters?.Where(filter => filter.IsExclude).ToList();
var includePaths =
pathFilters
?.Where(filter => !filter.IsExclude)
.Select(filter => filter.RepoRelativePath)
.ToList();

var ignoreCase = commit.GetRepository().Config.Get<bool>("core.ignorecase")?.Value ?? false;
bool ContainsRelevantChanges(IEnumerable<TreeEntryChanges> changes) =>
excludePaths.Count == 0
? changes.Any()
// If there is a single change that isn't excluded,
// then this commit is relevant.
: changes.Any(change => !excludePaths.Any(exclude => exclude.Excludes(change.Path, ignoreCase)));
var excludePaths = pathFilters?.Where(filter => filter.IsExclude).ToList();

height = 1;
var ignoreCase = commit.GetRepository().Config.Get<bool>("core.ignorecase")?.Value ?? false;
bool ContainsRelevantChanges(IEnumerable<TreeEntryChanges> changes) =>
excludePaths.Count == 0
? changes.Any()
// If there is a single change that isn't excluded,
// then this commit is relevant.
: changes.Any(change => !excludePaths.Any(exclude => exclude.Excludes(change.Path, ignoreCase)));

if (includePaths != null)
{
// 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)))
: ContainsRelevantChanges(commit.GetRepository().Diff
.Compare<TreeChanges>(null, commit.Tree, diffInclude));

if (!relevantCommit)
{
height = 0;
}
}
int height = 1;

if (commit.Parents.Any())
if (includePaths != null)
{
// 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)))
: ContainsRelevantChanges(commit.GetRepository().Diff
.Compare<TreeChanges>(null, commit.Tree, diffInclude));

if (!relevantCommit)
{
height += commit.Parents.Max(p => GetCommitHeight(p, tracker, continueStepping));
height = 0;
}
}
else

tracker.RecordHeight(commit, height + maxHeightAmongParents);
return true;
}

commitsToEvaluate.Push(startingCommit);
while (commitsToEvaluate.Count > 0)
{
Commit commit = commitsToEvaluate.Peek();
if (tracker.TryGetVersionHeight(commit, out _) || TryCalculateHeight(commit))
{
height = 0;
commitsToEvaluate.Pop();
}

tracker.RecordHeight(commit, height);
}

return height;
Assumes.True(tracker.TryGetVersionHeight(startingCommit, out int result));
return result;
}

/// <summary>
Expand Down

0 comments on commit 7f4b4d9

Please sign in to comment.