Merge pull request #638 from dotnet/fix637

Always generate commit ID component of version as big endian

Author: AArnott

…itVersioning.Tests/GitExtensionsTests.cs …oning.Tests/LibGit2GitExtensionsTests.cssrc/NerdBank.GitVersioning.Tests/GitExtensionsTests.cs renamed to src/NerdBank.GitVersioning.Tests/LibGit2GitExtensionsTests.cs src/NerdBank.GitVersioning.Tests/GitExtensionsTests.cs renamed to src/NerdBank.GitVersioning.Tests/LibGit2GitExtensionsTests.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Buffers.Binary;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.IO;
@@ -11,9 +12,9 @@
 using Xunit.Abstractions;
 using Version = System.Version;
 
-public partial class GitExtensionsTests : RepoTestBase
+public class LibGit2GitExtensionsTests : RepoTestBase
 {
-    public GitExtensionsTests(ITestOutputHelper Logger)
+    public LibGit2GitExtensionsTests(ITestOutputHelper Logger)
         : base(Logger)
     {
         this.InitializeSourceControl();
@@ -166,22 +167,6 @@ public void GetVersionHeight_ProgressAndReset(string version1, string version2,
         Assert.Equal(!versionHeightReset, height2 > height1);
     }
 
-    [Fact]
-    public void GetTruncatedCommitIdAsInteger_Roundtrip()
-    {
-        var firstCommit = this.LibGit2Repository.Commit("First", this.Signer, this.Signer, new CommitOptions { AllowEmptyCommit = true });
-        var secondCommit = this.LibGit2Repository.Commit("Second", this.Signer, this.Signer, new CommitOptions { AllowEmptyCommit = true });
-
-        int id1 = firstCommit.GetTruncatedCommitIdAsInt32();
-        int id2 = secondCommit.GetTruncatedCommitIdAsInt32();
-
-        this.Logger.WriteLine($"Commit {firstCommit.Id.Sha.Substring(0, 8)} as int: {id1}");
-        this.Logger.WriteLine($"Commit {secondCommit.Id.Sha.Substring(0, 8)} as int: {id2}");
-
-        Assert.Equal(firstCommit, this.LibGit2Repository.GetCommitFromTruncatedIdInteger(id1));
-        Assert.Equal(secondCommit, this.LibGit2Repository.GetCommitFromTruncatedIdInteger(id2));
-    }
-
     [Fact]
     public void GetIdAsVersion_ReadsMajorMinorFromVersionTxt()
     {
@@ -384,6 +369,19 @@ public void GetIdAsVersion_Roundtrip_UnstableOffset(int startingOffset, int offs
         }
     }
 
+    [Fact]
+    public void GetCommitsFromVersion_MatchesOnEitherEndian()
+    {
+        this.InitializeSourceControl();
+        Commit commit = this.WriteVersionFile(new VersionOptions { Version = SemanticVersion.Parse("1.2"), GitCommitIdShortAutoMinimum = 4 });
+
+        Version originalVersion = new VersionOracle(this.Context).Version;
+        Version swappedEndian = new Version(originalVersion.Major, originalVersion.Minor, originalVersion.Build, BinaryPrimitives.ReverseEndianness((ushort)originalVersion.Revision));
+        ushort twoBytesFromCommitId = checked((ushort)originalVersion.Revision);
+        Assert.Contains(commit, LibGit2GitExtensions.GetCommitsFromVersion(this.Context, originalVersion));
+        Assert.Contains(commit, LibGit2GitExtensions.GetCommitsFromVersion(this.Context, swappedEndian));
+    }
+
     [Fact]
     public void GetIdAsVersion_Roundtrip_WithSubdirectoryVersionFiles()
     {

src/NerdBank.GitVersioning.Tests/ManagedGit/GitObjectIdTests.cs

@@ -93,7 +93,7 @@ public void AsUInt16Test()
         {
             // The hash code is the int32 representation of the first 4 bytes
             var objectId = GitObjectId.ParseHex(this.shaAsHexAsciiByteArray);
-            Assert.Equal(0x914e, objectId.AsUInt16());
+            Assert.Equal(0x4e91, objectId.AsUInt16());
             Assert.Equal(0, GitObjectId.Empty.GetHashCode());
         }
 

src/NerdBank.GitVersioning.Tests/TestUtilities.cs

@@ -79,6 +79,10 @@ internal static ExpandedRepo ExtractRepoArchive(string repoArchiveName)
         }
     }
 
+    internal static string ToHex(ushort number) => number.ToString("X");
+
+    internal static ushort FromHex(string hex) => ushort.Parse(hex, System.Globalization.NumberStyles.HexNumber);
+
     internal class ExpandedRepo : IDisposable
     {
         internal ExpandedRepo(string repoPath)

src/NerdBank.GitVersioning.Tests/VersionOracleTests.cs

@@ -845,6 +845,23 @@ public void GitCommitIdShort()
         }
     }
 
+    [Fact]
+    public void GitCommidIdLeading16BitsDecodedWithBigEndian()
+    {
+        this.WriteVersionFile(new VersionOptions { Version = SemanticVersion.Parse("1.2"), GitCommitIdShortAutoMinimum = 4 });
+        this.InitializeSourceControl();
+        this.AddCommits(1);
+        var oracle = new VersionOracle(this.Context);
+
+        string leadingFourChars = this.Context.GitCommitId.Substring(0, 4);
+        ushort expectedNumber = TestUtilities.FromHex(leadingFourChars);
+        ushort actualNumber = checked((ushort)oracle.Version.Revision);
+        this.Logger.WriteLine("First two characters from commit ID in hex is {0}", leadingFourChars);
+        this.Logger.WriteLine("First two characters, converted to a number is {0}", expectedNumber);
+        this.Logger.WriteLine("Generated 16-bit ushort from commit ID is {0}, whose hex representation is {1}", actualNumber, TestUtilities.ToHex(actualNumber));
+        Assert.Equal(expectedNumber, actualNumber);
+    }
+
     [Fact(Skip = "Slow test")]
     public void GetVersionHeight_VeryLongHistory()
     {

src/NerdBank.GitVersioning/LibGit2/LibGit2GitExtensions.cs

@@ -3,8 +3,8 @@
 namespace Nerdbank.GitVersioning.LibGit2
 {
     using System;
+    using System.Buffers.Binary;
     using System.Collections.Generic;
-    using System.Globalization;
     using System.IO;
     using System.Linq;
     using System.Runtime.InteropServices;
@@ -97,18 +97,6 @@ public static int GetHeight(LibGit2Context context, Func<Commit, bool>? continue
             return GetCommitHeight(context.Commit, tracker, continueStepping);
         }
 
-        /// <summary>
-        /// Takes the first 4 bytes of a commit ID (i.e. first 8 characters of its hex-encoded SHA)
-        /// and returns them as an integer.
-        /// </summary>
-        /// <param name="commit">The commit to identify with an integer.</param>
-        /// <returns>The integer which identifies a commit.</returns>
-        public static int GetTruncatedCommitIdAsInt32(this Commit commit)
-        {
-            Requires.NotNull(commit, nameof(commit));
-            return BitConverter.ToInt32(commit.Id.RawId, 0);
-        }
-
         /// <summary>
         /// Takes the first 2 bytes of a commit ID (i.e. first 4 characters of its hex-encoded SHA)
         /// and returns them as an 16-bit unsigned integer.
@@ -118,21 +106,7 @@ public static int GetTruncatedCommitIdAsInt32(this Commit commit)
         public static ushort GetTruncatedCommitIdAsUInt16(this Commit commit)
         {
             Requires.NotNull(commit, nameof(commit));
-            return BitConverter.ToUInt16(commit.Id.RawId, 0);
-        }
-
-        /// <summary>
-        /// Looks up a commit by an integer that captures the first for bytes of its ID.
-        /// </summary>
-        /// <param name="repo">The repo to search for a matching commit.</param>
-        /// <param name="truncatedId">The value returned from <see cref="GetTruncatedCommitIdAsInt32(Commit)"/>.</param>
-        /// <returns>A matching commit.</returns>
-        public static Commit GetCommitFromTruncatedIdInteger(this Repository repo, int truncatedId)
-        {
-            Requires.NotNull(repo, nameof(repo));
-
-            byte[] rawId = BitConverter.GetBytes(truncatedId);
-            return repo.Lookup<Commit>(EncodeAsHex(rawId));
+            return BinaryPrimitives.ReadUInt16BigEndian(commit.Id.RawId);
         }
 
         /// <summary>
@@ -310,7 +284,11 @@ private static bool IsCommitIdMismatch(Version version, VersionOptions versionOp
                     ushort objectIdLeadingValue = (ushort)expectedCommitIdLeadingValue;
                     ushort objectIdMask = (ushort)(objectIdLeadingValue == MaximumBuildNumberOrRevisionComponent ? 0xfffe : 0xffff);
 
-                    return !commit.Id.StartsWith(objectIdLeadingValue, objectIdMask);
+                    // Accept a big endian match or a little endian match.
+                    // Nerdbank.GitVersioning up to v3.4 would produce versions based on the endianness of the CPU it ran on (typically little endian).
+                    // Starting with v3.5, it deterministically used big endian. In order for `nbgv get-commits` to match on versions computed before and after the change,
+                    // we match on either endian setting.
+                    return !(commit.Id.StartsWith(objectIdLeadingValue, bigEndian: true, objectIdMask) || commit.Id.StartsWith(objectIdLeadingValue, bigEndian: false, objectIdMask));
                 }
             }
 
@@ -324,10 +302,11 @@ private static bool IsCommitIdMismatch(Version version, VersionOptions versionOp
         /// <param name="object">The object whose ID is to be tested.</param>
         /// <param name="leadingBytes">The leading 16-bits to be tested.</param>
         /// <param name="bitMask">The mask that indicates which bits should be compared.</param>
+        /// <param name="bigEndian"><see langword="true"/> to read the first two bytes as big endian (v3.5+ behavior); <see langword="false"/> to use little endian (v3.4 and earlier behavior).</param>
         /// <returns><c>True</c> if the object's ID starts with <paramref name="leadingBytes"/> after applying the <paramref name="bitMask"/>.</returns>
-        private static bool StartsWith(this ObjectId @object, ushort leadingBytes, ushort bitMask = 0xffff)
+        private static bool StartsWith(this ObjectId @object, ushort leadingBytes, bool bigEndian, ushort bitMask = 0xffff)
         {
-            ushort truncatedObjectId = BitConverter.ToUInt16(@object.RawId, 0);
+            ushort truncatedObjectId = bigEndian ? BinaryPrimitives.ReadUInt16BigEndian(@object.RawId) : BinaryPrimitives.ReadUInt16LittleEndian(@object.RawId);
             return (truncatedObjectId & bitMask) == leadingBytes;
         }
 

src/NerdBank.GitVersioning/ManagedGit/GitObjectId.cs

@@ -167,7 +167,7 @@ public override bool Equals(object? obj)
         /// <returns>
         /// A <see cref="ushort"/> which represents the first two bytes of this <see cref="GitObjectId"/>.
         /// </returns>
-        public ushort AsUInt16() => BinaryPrimitives.ReadUInt16LittleEndian(this.Value.Slice(0, 2));
+        public ushort AsUInt16() => BinaryPrimitives.ReadUInt16BigEndian(this.Value.Slice(0, 2));
 
         /// <summary>
         /// Returns the SHA1 hash of this object.