Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Partial performance improvement for #114 #134

Merged
merged 2 commits into from Jun 19, 2017
Merged

Partial performance improvement for #114 #134

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f92d363
Minimize calls to git when calculating VersionOracle
robmen Jun 18, 2017
9edf81a
Correctly handle case where version file is deleted after existing
robmen Jun 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/NerdBank.GitVersioning.Tests/VersionOracleTests.cs
View file
Open in desktop
@@ -1,4 +1,5 @@
using System.IO;
using System;
using System.IO;
using System.Linq;
using LibGit2Sharp;
using Nerdbank.GitVersioning;
Expand All @@ -15,6 +16,14 @@ public VersionOracleTests(ITestOutputHelper logger)

private string CommitIdShort => this.Repo.Head.Commits.First().Id.Sha.Substring(0, 10);

[Fact]
public void NotRepo()
{
// Seems safe to assume the system directory is not a repository.
var oracle = VersionOracle.Create(Environment.SystemDirectory);
Assert.Equal(0, oracle.VersionHeight);
}

[Fact(Skip = "Unstable test. See issue #125")]
public void Submodule_RecognizedWithCorrectVersion()
{
Expand Down
47 changes: 27 additions & 20 deletions src/NerdBank.GitVersioning/GitExtensions.cs
View file
Open in desktop
Expand Up @@ -33,13 +33,18 @@ public static class GitExtensions
/// </summary>
/// <param name="commit">The commit to measure the height of.</param>
/// <param name="repoRelativeProjectDirectory">The repo-relative project directory for which to calculate the version.</param>
/// <param name="baseVersion">Optional base version to calculate the height. If not specified, the base version will be calculated by scanning the repository.</param>
/// <returns>The height of the commit. Always a positive integer.</returns>
public static int GetVersionHeight(this Commit commit, string repoRelativeProjectDirectory = null)
public static int GetVersionHeight(this Commit commit, string repoRelativeProjectDirectory = null, Version baseVersion = null)
{
Requires.NotNull(commit, nameof(commit));
Requires.Argument(repoRelativeProjectDirectory == null || !Path.IsPathRooted(repoRelativeProjectDirectory), nameof(repoRelativeProjectDirectory), "Path should be relative to repo root.");

var baseVersion = VersionFile.GetVersion(commit, repoRelativeProjectDirectory)?.Version?.Version ?? Version0;
if (baseVersion == null)
{
baseVersion = VersionFile.GetVersion(commit, repoRelativeProjectDirectory)?.Version?.Version ?? Version0;
}

int height = commit.GetHeight(c => CommitMatchesMajorMinorVersion(c, baseVersion, repoRelativeProjectDirectory));
return height;
}
Expand Down Expand Up @@ -170,7 +175,7 @@ public static Commit GetCommitFromTruncatedIdInteger(this Repository repo, int t
/// <param name="commit">The commit whose ID and position in history is to be encoded.</param>
/// <param name="repoRelativeProjectDirectory">The repo-relative project directory for which to calculate the version.</param>
/// <param name="versionHeight">
/// The version height, previously calculated by a call to <see cref="GetVersionHeight(Commit, string)"/>
/// The version height, previously calculated by a call to <see cref="GetVersionHeight(Commit, string, Version)"/>
/// with the same value for <paramref name="repoRelativeProjectDirectory"/>.
/// </param>
/// <returns>
Expand All @@ -188,7 +193,13 @@ public static Version GetIdAsVersion(this Commit commit, string repoRelativeProj
Requires.Argument(repoRelativeProjectDirectory == null || !Path.IsPathRooted(repoRelativeProjectDirectory), nameof(repoRelativeProjectDirectory), "Path should be relative to repo root.");

var versionOptions = VersionFile.GetVersion(commit, repoRelativeProjectDirectory);
return GetIdAsVersionHelper(commit, versionOptions, repoRelativeProjectDirectory, versionHeight);

if (!versionHeight.HasValue)
{
versionHeight = GetVersionHeight(commit, repoRelativeProjectDirectory);
}

return GetIdAsVersionHelper(commit, versionOptions, versionHeight.Value);
}

/// <summary>
Expand All @@ -198,7 +209,7 @@ public static Version GetIdAsVersion(this Commit commit, string repoRelativeProj
/// <param name="repo">The repo whose ID and position in history is to be encoded.</param>
/// <param name="repoRelativeProjectDirectory">The repo-relative project directory for which to calculate the version.</param>
/// <param name="versionHeight">
/// The version height, previously calculated by a call to <see cref="GetVersionHeight(Commit, string)"/>
/// The version height, previously calculated by a call to <see cref="GetVersionHeight(Commit, string, Version)"/>
/// with the same value for <paramref name="repoRelativeProjectDirectory"/>.
/// </param>
/// <returns>
Expand All @@ -219,7 +230,13 @@ public static Version GetIdAsVersion(this Repository repo, string repoRelativePr
if (IsVersionFileChangedInWorkingCopy(repo, repoRelativeProjectDirectory, out committedVersionOptions, out workingCopyVersionOptions))
{
// Apply ordinary logic, but to the working copy version info.
Version result = GetIdAsVersionHelper(headCommit, workingCopyVersionOptions, repoRelativeProjectDirectory, versionHeight);
if (!versionHeight.HasValue)
{
var baseVersion = workingCopyVersionOptions?.Version?.Version;
versionHeight = GetVersionHeight(headCommit, repoRelativeProjectDirectory, baseVersion);
}

Version result = GetIdAsVersionHelper(headCommit, workingCopyVersionOptions, versionHeight.Value);
return result;
}

Expand Down Expand Up @@ -367,7 +384,7 @@ public static string FindLibGit2NativeBinaries(string basePath)
/// <param name="expectedVersion">The version to test for in the commit</param>
/// <param name="repoRelativeProjectDirectory">The repo-relative directory from which <paramref name="expectedVersion"/> was originally calculated.</param>
/// <returns><c>true</c> if the <paramref name="commit"/> matches the major and minor components of <paramref name="expectedVersion"/>.</returns>
private static bool CommitMatchesMajorMinorVersion(Commit commit, Version expectedVersion, string repoRelativeProjectDirectory)
internal static bool CommitMatchesMajorMinorVersion(this Commit commit, Version expectedVersion, string repoRelativeProjectDirectory)
{
Requires.NotNull(commit, nameof(commit));
Requires.NotNull(expectedVersion, nameof(expectedVersion));
Expand Down Expand Up @@ -494,11 +511,7 @@ private static void AddReachableCommitsFrom(Commit startingCommit, HashSet<Commi
/// </summary>
/// <param name="commit">The commit whose ID and position in history is to be encoded.</param>
/// <param name="versionOptions">The version options applicable at this point (either from commit or working copy).</param>
/// <param name="repoRelativeProjectDirectory">The repo-relative project directory for which to calculate the version.</param>
/// <param name="versionHeight">
/// The version height, previously calculated by a call to <see cref="GetVersionHeight(Commit, string)"/>
/// with the same value for <paramref name="repoRelativeProjectDirectory"/>.
/// </param>
/// <param name="versionHeight">The version height, previously calculated by a call to <see cref="GetVersionHeight(Commit, string, Version)"/>.</param>
/// <returns>
/// A version whose <see cref="Version.Build"/> and
/// <see cref="Version.Revision"/> components are calculated based on the commit.
Expand All @@ -509,7 +522,7 @@ private static void AddReachableCommitsFrom(Commit startingCommit, HashSet<Commi
/// component is the first four bytes of the git commit id (forced to be a positive integer).
/// </remarks>
/// <returns></returns>
private static Version GetIdAsVersionHelper(Commit commit, VersionOptions versionOptions, string repoRelativeProjectDirectory, int? versionHeight)
internal static Version GetIdAsVersionHelper(this Commit commit, VersionOptions versionOptions, int versionHeight)
{
var baseVersion = versionOptions?.Version?.Version ?? Version0;
int buildNumber = baseVersion.Build;
Expand All @@ -522,14 +535,8 @@ private static Version GetIdAsVersionHelper(Commit commit, VersionOptions versio
// The build number is set to the git height. This helps ensure that
// within a major.minor release, each patch has an incrementing integer.
// The revision is set to the first two bytes of the git commit ID.
if (!versionHeight.HasValue)
{
versionHeight = commit != null
? commit.GetHeight(c => CommitMatchesMajorMinorVersion(c, baseVersion, repoRelativeProjectDirectory))
: 0;
}

int adjustedVersionHeight = versionHeight.Value == 0 ? 0 : versionHeight.Value + (versionOptions?.BuildNumberOffset ?? 0);
int adjustedVersionHeight = versionHeight == 0 ? 0 : versionHeight + (versionOptions?.BuildNumberOffset ?? 0);
Verify.Operation(adjustedVersionHeight <= MaximumBuildNumberOrRevisionComponent, "Git height is {0}, which is greater than the maximum allowed {0}.", adjustedVersionHeight, MaximumBuildNumberOrRevisionComponent);

if (buildNumber < 0)
Expand Down
67 changes: 55 additions & 12 deletions src/NerdBank.GitVersioning/VersionOracle.cs
View file
Open in desktop
Expand Up @@ -5,9 +5,7 @@
using System.Globalization;
using System.IO;
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;
using System.Threading.Tasks;
using Validation;

/// <summary>
Expand All @@ -21,9 +19,9 @@ public class VersionOracle
private static readonly Regex NumericIdentifierRegex = new Regex(@"(?<![\w-])(\d+)(?![\w-])");

/// <summary>
/// The cloud build suppport, if any.
/// The 0.0 version.
/// </summary>
private readonly ICloudBuild cloudBuild;
private static readonly Version Version0 = new Version(0, 0);

/// <summary>
/// Initializes a new instance of the <see cref="VersionOracle"/> class.
Expand Down Expand Up @@ -51,24 +49,33 @@ public static VersionOracle Create(string projectDirectory, string gitRepoDirect
/// </summary>
public VersionOracle(string projectDirectory, LibGit2Sharp.Repository repo, ICloudBuild cloudBuild)
{
this.cloudBuild = cloudBuild;
this.VersionOptions =
VersionFile.GetVersion(repo, projectDirectory) ??
VersionFile.GetVersion(projectDirectory);

var repoRoot = repo?.Info?.WorkingDirectory?.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);
var relativeRepoProjectDirectory = !string.IsNullOrWhiteSpace(repoRoot)
? projectDirectory.Substring(repoRoot.Length).TrimStart(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)
: null;

var commit = repo?.Head.Commits.FirstOrDefault();

var committedVersion = VersionFile.GetVersion(commit, relativeRepoProjectDirectory);

var workingVersion = VersionFile.GetVersion(projectDirectory);

this.VersionOptions = committedVersion ?? workingVersion;

this.GitCommitId = commit?.Id.Sha ?? cloudBuild?.GitCommitId ?? null;
this.VersionHeight = repo?.GetVersionHeight(relativeRepoProjectDirectory) ?? 0;
this.VersionHeight = CalculateVersionHeight(relativeRepoProjectDirectory, commit, committedVersion, workingVersion);
this.BuildingRef = cloudBuild?.BuildingTag ?? cloudBuild?.BuildingBranch ?? repo?.Head.CanonicalName;

// Override the typedVersion with the special build number and revision components, when available.
this.Version = repo?.GetIdAsVersion(relativeRepoProjectDirectory, this.VersionHeight) ?? this.VersionOptions?.Version.Version;
this.Version = this.Version ?? new Version(0, 0);
if (repo != null)
{
this.Version = GetIdAsVersion(commit, committedVersion, workingVersion, this.VersionHeight);
}
else
{
this.Version = this.VersionOptions?.Version.Version ?? Version0;
}

this.VersionHeightOffset = this.VersionOptions?.BuildNumberOffset ?? 0;

this.PrereleaseVersion = ReplaceMacros(this.VersionOptions?.Version.Prerelease ?? string.Empty);
Expand Down Expand Up @@ -391,5 +398,41 @@ private static string MakePrereleaseSemVer1Compliant(string prerelease, int padd

return semver1;
}

private static int CalculateVersionHeight(string relativeRepoProjectDirectory, LibGit2Sharp.Commit headCommit, VersionOptions committedVersion, VersionOptions workingVersion)
{
var headCommitVersion = committedVersion?.Version?.Version ?? Version0;

if (IsVersionFileChangedInWorkingTree(committedVersion, workingVersion))
{
var workingCopyVersion = workingVersion?.Version?.Version;

if (workingCopyVersion == null || !workingCopyVersion.Equals(headCommitVersion))
{
// The working copy has changed the major.minor version.
// So by definition the version height is 0, since no commit represents it yet.
return 0;
}
}

return headCommit?.GetHeight(c => c.CommitMatchesMajorMinorVersion(headCommitVersion, relativeRepoProjectDirectory)) ?? 0;
}

private static Version GetIdAsVersion(LibGit2Sharp.Commit headCommit, VersionOptions committedVersion, VersionOptions workingVersion, int versionHeight)
{
var version = IsVersionFileChangedInWorkingTree(committedVersion, workingVersion) ? workingVersion : committedVersion;

return headCommit.GetIdAsVersionHelper(version, versionHeight);
}

private static bool IsVersionFileChangedInWorkingTree(VersionOptions committedVersion, VersionOptions workingVersion)
{
if (workingVersion != null)
{
return !EqualityComparer<VersionOptions>.Default.Equals(workingVersion, committedVersion);
}

return false; // a missing working version is allowed and not a change.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that if I delete my version.json file, I'll still get the same version and height calculation? Why is that? In the version of this method that is in GitExtensions.cs, we consider a missing version file in the working copy to be a change. We only return false from that method if it's a "bare" repo, which doesn't seem to be considered here. Was that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I was trying to match the logic you describe. But none of the tests covered the case and this ended up giving the right answer within the set of things that I understood. For example, I removed the bare repo check because it failed tests (but everything passed without it). However, I will quickly admit that I may have missed a use case that doesn't have a test. 😄

Let me play with the code with this new information and see if I get the code to match your explanation and pass the tests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be missing a test for bare repos. I can't think of anyone that actually needs this and I can't remember why I supported it to begin with. So I'll accept this PR as-is, or whenever you're satisfied. Just let me know.

Copy link
Contributor Author

@robmen robmen Jun 18, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, at this point, you know better than I. This is working for all my test cases right now and what is in the unit tests. If an issue comes up in the future, a test could be created for the failure and everything fixed... then we'd know for sure.

Really your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I had some time and just stared at the code a little longer. I think I understand the deleted work tree case better and I think the bare repo case devolves to it. I'll have one last change right now.

}
}
}