Initial change to type-safe Diff.Compare #1180

Therzok · 2015-08-23T22:38:41Z

Therzok · 2015-08-23T22:54:11Z

This patch leaves a bit of a sour taste. In exchange for safety, we're exposing the public constructor of the class we have to construct the patch from.

I've left the DiffSafeHandleProxy (whose name I still want to discuss) empty for now.

Therzok · 2015-08-23T22:55:42Z

We can fallback to Activator.CreateInstance in exchange for the new constraint, and we also keep the API simple.

Therzok · 2015-08-23T22:58:29Z

After further investigation, Activator.CreateInstance (whichever variant) requires a public parameterless constructor.

nulltoken · 2015-08-24T08:38:37Z

@Therzok How about this?

diff --git a/LibGit2Sharp.Tests/DiffTreeToTreeFixture.cs b/LibGit2Sharp.Tests/DiffTreeToTreeFixture.cs
index bebbdde..0e8ef90 100644
--- a/LibGit2Sharp.Tests/DiffTreeToTreeFixture.cs
+++ b/LibGit2Sharp.Tests/DiffTreeToTreeFixture.cs
@@ -1124,6 +1124,17 @@ public void RetrievingDiffChangesMustAlwaysBeCaseSensitive()
         }

         [Fact]
+        public void CallingCompareWithAnUnsupportedGenericParamThrows()
+        {
+            var path = SandboxStandardTestRepoGitDir();
+            using (var repo = new Repository(path))
+            {
+                Assert.Throws<LibGit2SharpException>(() => repo.Diff.Compare<string>(default(Tree), default(Tree)));
+                Assert.Throws<LibGit2SharpException>(() => repo.Diff.Compare<string>());
+            }
+        }
+
+        [Fact]
         public void UsingPatienceAlgorithmCompareOptionProducesPatienceDiff()
         {
             string repoPath = InitNewRepository();
diff --git a/LibGit2Sharp.Tests/MetaFixture.cs b/LibGit2Sharp.Tests/MetaFixture.cs
index 7353e13..a9b6b06 100644
--- a/LibGit2Sharp.Tests/MetaFixture.cs
+++ b/LibGit2Sharp.Tests/MetaFixture.cs
@@ -17,7 +17,7 @@ public class MetaFixture
     {
         private static readonly HashSet<Type> explicitOnlyInterfaces = new HashSet<Type>
         {
-            typeof(IBelongToARepository), typeof(IDiffResult<Patch>), typeof(IDiffResult<TreeChanges>), typeof(IDiffResult<PatchStats>),
+            typeof(IBelongToARepository),
         };

         [Fact]
diff --git a/LibGit2Sharp/Diff.cs b/LibGit2Sharp/Diff.cs
index d412a14..4341528 100644
--- a/LibGit2Sharp/Diff.cs
+++ b/LibGit2Sharp/Diff.cs
@@ -95,6 +95,29 @@ internal Diff(Repository repo)
                        };
         }

+        private static readonly IDictionary<Type, Func<DiffSafeHandle, object>> ChangesBuilders = new Dictionary<Type, Func<DiffSafeHandle, object>>
+        {
+            { typeof(Patch), diff => new Patch(diff) },
+            { typeof(TreeChanges), diff => new TreeChanges(diff) },
+            { typeof(PatchStats), diff => new PatchStats(diff) },
+        };
+
+
+        private static T BuildDiffResult<T>(DiffSafeHandle diff) where T : class, IDiffResult
+        {
+            Func<DiffSafeHandle, object> builder;
+
+            if (!ChangesBuilders.TryGetValue(typeof(T), out builder))
+            {
+                throw new LibGit2SharpException(CultureInfo.InvariantCulture,
+                    "Unexpected type '{0}' passed to Compare. Supported values are: {1}",
+                    typeof(T),
+                    string.Join(", ", ChangesBuilders.Keys.Select(x => x.Name)));
+            }
+
+            return (T)builder(diff);
+        }
+
         /// <summary>
         /// Show changes between two <see cref="Blob"/>s.
         /// </summary>
@@ -127,7 +150,7 @@ public virtual ContentChanges Compare(Blob oldBlob, Blob newBlob, CompareOptions
         /// <param name="oldTree">The <see cref="Tree"/> you want to compare from.</param>
         /// <param name="newTree">The <see cref="Tree"/> you want to compare to.</param>
         /// <returns>A <see cref="TreeChanges"/> containing the changes between the <paramref name="oldTree"/> and the <paramref name="newTree"/>.</returns>
-        public virtual T Compare<T>(Tree oldTree, Tree newTree) where T : class, IDiffResult<T>, new()
+        public virtual T Compare<T>(Tree oldTree, Tree newTree) where T : class, IDiffResult
         {
             return Compare<T>(oldTree, newTree, null, null, null);
         }
@@ -139,7 +162,7 @@ public virtual T Compare<T>(Tree oldTree, Tree newTree) where T : class, IDiffRe
         /// <param name="newTree">The <see cref="Tree"/> you want to compare to.</param>
         /// <param name="paths">The list of paths (either files or directories) that should be compared.</param>
         /// <returns>A <see cref="TreeChanges"/> containing the changes between the <paramref name="oldTree"/> and the <paramref name="newTree"/>.</returns>
-        public virtual T Compare<T>(Tree oldTree, Tree newTree, IEnumerable<string> paths) where T : class, IDiffResult<T>, new()
+        public virtual T Compare<T>(Tree oldTree, Tree newTree, IEnumerable<string> paths) where T : class, IDiffResult
         {
             return Compare<T>(oldTree, newTree, paths, null, null);
         }
@@ -156,7 +179,7 @@ public virtual T Compare<T>(Tree oldTree, Tree newTree, IEnumerable<string> path
         /// </param>
         /// <returns>A <see cref="TreeChanges"/> containing the changes between the <paramref name="oldTree"/> and the <paramref name="newTree"/>.</returns>
         public virtual T Compare<T>(Tree oldTree, Tree newTree, IEnumerable<string> paths,
-            ExplicitPathsOptions explicitPathsOptions) where T : class, IDiffResult<T>, new()
+            ExplicitPathsOptions explicitPathsOptions) where T : class, IDiffResult
         {
             return Compare<T>(oldTree, newTree, paths, explicitPathsOptions, null);
         }
@@ -169,7 +192,7 @@ public virtual T Compare<T>(Tree oldTree, Tree newTree, IEnumerable<string> path
         /// <param name="paths">The list of paths (either files or directories) that should be compared.</param>
         /// <param name="compareOptions">Additional options to define patch generation behavior.</param>
         /// <returns>A <see cref="TreeChanges"/> containing the changes between the <paramref name="oldTree"/> and the <paramref name="newTree"/>.</returns>
-        public virtual T Compare<T>(Tree oldTree, Tree newTree, IEnumerable<string> paths, CompareOptions compareOptions) where T : class, IDiffResult<T>, new()
+        public virtual T Compare<T>(Tree oldTree, Tree newTree, IEnumerable<string> paths, CompareOptions compareOptions) where T : class, IDiffResult
         {
             return Compare<T>(oldTree, newTree, paths, null, compareOptions);
         }
@@ -181,7 +204,7 @@ public virtual T Compare<T>(Tree oldTree, Tree newTree, IEnumerable<string> path
         /// <param name="newTree">The <see cref="Tree"/> you want to compare to.</param>
         /// <param name="compareOptions">Additional options to define patch generation behavior.</param>
         /// <returns>A <see cref="TreeChanges"/> containing the changes between the <paramref name="oldTree"/> and the <paramref name="newTree"/>.</returns>
-        public virtual T Compare<T>(Tree oldTree, Tree newTree, CompareOptions compareOptions) where T : class, IDiffResult<T>, new()
+        public virtual T Compare<T>(Tree oldTree, Tree newTree, CompareOptions compareOptions) where T : class, IDiffResult
         {
             return Compare<T>(oldTree, newTree, null, null, compareOptions);
         }
@@ -199,9 +222,8 @@ public virtual T Compare<T>(Tree oldTree, Tree newTree, CompareOptions compareOp
         /// <param name="compareOptions">Additional options to define patch generation behavior.</param>
         /// <returns>A <see cref="TreeChanges"/> containing the changes between the <paramref name="oldTree"/> and the <paramref name="newTree"/>.</returns>
         public virtual T Compare<T>(Tree oldTree, Tree newTree, IEnumerable<string> paths, ExplicitPathsOptions explicitPathsOptions,
-                               CompareOptions compareOptions) where T : class, IDiffResult<T>, new()
+                               CompareOptions compareOptions) where T : class, IDiffResult
         {
-
             var comparer = TreeToTree(repo);
             ObjectId oldTreeId = oldTree != null ? oldTree.Id : null;
             ObjectId newTreeId = newTree != null ? newTree.Id : null;
@@ -219,10 +241,7 @@ public virtual T Compare<T>(Tree oldTree, Tree newTree, CompareOptions compareOp

             using (DiffSafeHandle diff = BuildDiffList(oldTreeId, newTreeId, comparer, diffOptions, paths, explicitPathsOptions, compareOptions))
             {
-                using (var proxy = new DiffSafeHandleProxy(diff))
-                {
-                    return new T().FromNative(proxy);
-                }
+                return BuildDiffResult<T>(diff);
             }
         }

@@ -238,7 +257,7 @@ public virtual T Compare<T>(Tree oldTree, Tree newTree, CompareOptions compareOp
         /// <typeparam name="T">Can be either a <see cref="TreeChanges"/> if you are only interested in the list of files modified, added, ..., or
         /// a <see cref="Patch"/> if you want the actual patch content for the whole diff and for individual files.</typeparam>
         /// <returns>A <typeparamref name="T"/> containing the changes between the <see cref="Tree"/> and the selected target.</returns>
-        public virtual T Compare<T>(Tree oldTree, DiffTargets diffTargets) where T : class, IDiffResult<T>, new()
+        public virtual T Compare<T>(Tree oldTree, DiffTargets diffTargets) where T : class, IDiffResult
         {
             return Compare<T>(oldTree, diffTargets, null, null, null);
         }
@@ -256,7 +275,7 @@ public virtual T Compare<T>(Tree oldTree, DiffTargets diffTargets) where T : cla
         /// <typeparam name="T">Can be either a <see cref="TreeChanges"/> if you are only interested in the list of files modified, added, ..., or
         /// a <see cref="Patch"/> if you want the actual patch content for the whole diff and for individual files.</typeparam>
         /// <returns>A <typeparamref name="T"/> containing the changes between the <see cref="Tree"/> and the selected target.</returns>
-        public virtual T Compare<T>(Tree oldTree, DiffTargets diffTargets, IEnumerable<string> paths) where T : class, IDiffResult<T>, new()
+        public virtual T Compare<T>(Tree oldTree, DiffTargets diffTargets, IEnumerable<string> paths) where T : class, IDiffResult
         {
             return Compare<T>(oldTree, diffTargets, paths, null, null);
         }
@@ -279,7 +298,7 @@ public virtual T Compare<T>(Tree oldTree, DiffTargets diffTargets, IEnumerable<s
         /// a <see cref="Patch"/> if you want the actual patch content for the whole diff and for individual files.</typeparam>
         /// <returns>A <typeparamref name="T"/> containing the changes between the <see cref="Tree"/> and the selected target.</returns>
         public virtual T Compare<T>(Tree oldTree, DiffTargets diffTargets, IEnumerable<string> paths,
-            ExplicitPathsOptions explicitPathsOptions) where T : class, IDiffResult<T>, new()
+            ExplicitPathsOptions explicitPathsOptions) where T : class, IDiffResult
         {
             return Compare<T>(oldTree, diffTargets, paths, explicitPathsOptions, null);
         }
@@ -303,7 +322,7 @@ public virtual T Compare<T>(Tree oldTree, DiffTargets diffTargets, IEnumerable<s
         /// a <see cref="Patch"/> if you want the actual patch content for the whole diff and for individual files.</typeparam>
         /// <returns>A <typeparamref name="T"/> containing the changes between the <see cref="Tree"/> and the selected target.</returns>
         public virtual T Compare<T>(Tree oldTree, DiffTargets diffTargets, IEnumerable<string> paths,
-            ExplicitPathsOptions explicitPathsOptions, CompareOptions compareOptions) where T : class, IDiffResult<T>, new()
+            ExplicitPathsOptions explicitPathsOptions, CompareOptions compareOptions) where T : class, IDiffResult
         {
             var comparer = HandleRetrieverDispatcher[diffTargets](repo);
             ObjectId oldTreeId = oldTree != null ? oldTree.Id : null;
@@ -324,10 +343,7 @@ public virtual T Compare<T>(Tree oldTree, DiffTargets diffTargets, IEnumerable<s

             using (DiffSafeHandle diff = BuildDiffList(oldTreeId, null, comparer, diffOptions, paths, explicitPathsOptions, compareOptions))
             {
-                using (var proxy = new DiffSafeHandleProxy(diff))
-                {
-                    return new T().FromNative(proxy);
-                }
+                return BuildDiffResult<T>(diff);
             }
         }

@@ -341,7 +357,7 @@ public virtual T Compare<T>(Tree oldTree, DiffTargets diffTargets, IEnumerable<s
         /// <typeparam name="T">Can be either a <see cref="TreeChanges"/> if you are only interested in the list of files modified, added, ..., or
         /// a <see cref="Patch"/> if you want the actual patch content for the whole diff and for individual files.</typeparam>
         /// <returns>A <typeparamref name="T"/> containing the changes between the working directory and the index.</returns>
-        public virtual T Compare<T>() where T : class, IDiffResult<T>, new()
+        public virtual T Compare<T>() where T : class, IDiffResult
         {
             return Compare<T>(DiffModifiers.None);
         }
@@ -357,7 +373,7 @@ public virtual T Compare<T>() where T : class, IDiffResult<T>, new()
         /// <typeparam name="T">Can be either a <see cref="TreeChanges"/> if you are only interested in the list of files modified, added, ..., or
         /// a <see cref="Patch"/> if you want the actual patch content for the whole diff and for individual files.</typeparam>
         /// <returns>A <typeparamref name="T"/> containing the changes between the working directory and the index.</returns>
-        public virtual T Compare<T>(IEnumerable<string> paths) where T : class, IDiffResult<T>, new()
+        public virtual T Compare<T>(IEnumerable<string> paths) where T : class, IDiffResult
         {
             return Compare<T>(DiffModifiers.None, paths);
         }
@@ -374,7 +390,7 @@ public virtual T Compare<T>(IEnumerable<string> paths) where T : class, IDiffRes
         /// <typeparam name="T">Can be either a <see cref="TreeChanges"/> if you are only interested in the list of files modified, added, ..., or
         /// a <see cref="Patch"/> if you want the actual patch content for the whole diff and for individual files.</typeparam>
         /// <returns>A <typeparamref name="T"/> containing the changes between the working directory and the index.</returns>
-        public virtual T Compare<T>(IEnumerable<string> paths, bool includeUntracked) where T : class, IDiffResult<T>, new()
+        public virtual T Compare<T>(IEnumerable<string> paths, bool includeUntracked) where T : class, IDiffResult
         {
             return Compare<T>(includeUntracked ? DiffModifiers.IncludeUntracked : DiffModifiers.None, paths);
         }
@@ -395,7 +411,7 @@ public virtual T Compare<T>(IEnumerable<string> paths, bool includeUntracked) wh
         /// <typeparam name="T">Can be either a <see cref="TreeChanges"/> if you are only interested in the list of files modified, added, ..., or
         /// a <see cref="Patch"/> if you want the actual patch content for the whole diff and for individual files.</typeparam>
         /// <returns>A <typeparamref name="T"/> containing the changes between the working directory and the index.</returns>
-        public virtual T Compare<T>(IEnumerable<string> paths, bool includeUntracked, ExplicitPathsOptions explicitPathsOptions) where T : class, IDiffResult<T>, new()
+        public virtual T Compare<T>(IEnumerable<string> paths, bool includeUntracked, ExplicitPathsOptions explicitPathsOptions) where T : class, IDiffResult
         {
             return Compare<T>(includeUntracked ? DiffModifiers.IncludeUntracked : DiffModifiers.None, paths, explicitPathsOptions);
         }
@@ -421,7 +437,7 @@ public virtual T Compare<T>(IEnumerable<string> paths, bool includeUntracked, Ex
             IEnumerable<string> paths,
             bool includeUntracked,
             ExplicitPathsOptions explicitPathsOptions,
-            CompareOptions compareOptions) where T : class, IDiffResult<T>, new()
+            CompareOptions compareOptions) where T : class, IDiffResult
         {
             return Compare<T>(includeUntracked ? DiffModifiers.IncludeUntracked : DiffModifiers.None, paths, explicitPathsOptions, compareOptions);
         }
@@ -430,7 +446,7 @@ public virtual T Compare<T>(IEnumerable<string> paths, bool includeUntracked, Ex
             DiffModifiers diffOptions,
             IEnumerable<string> paths = null,
             ExplicitPathsOptions explicitPathsOptions = null,
-            CompareOptions compareOptions = null) where T : class, IDiffResult<T>, new()
+            CompareOptions compareOptions = null) where T : class, IDiffResult
         {
             var comparer = WorkdirToIndex(repo);

@@ -446,10 +462,7 @@ public virtual T Compare<T>(IEnumerable<string> paths, bool includeUntracked, Ex

             using (DiffSafeHandle diff = BuildDiffList(null, null, comparer, diffOptions, paths, explicitPathsOptions, compareOptions))
             {
-                using (var proxy = new DiffSafeHandleProxy(diff))
-                {
-                    return new T().FromNative(proxy);
-                }
+                return BuildDiffResult<T>(diff);
             }
         }

diff --git a/LibGit2Sharp/DiffSafeHandleProxy.cs b/LibGit2Sharp/DiffSafeHandleProxy.cs
deleted file mode 100644
index cc6d249..0000000
--- a/LibGit2Sharp/DiffSafeHandleProxy.cs
+++ /dev/null
@@ -1,25 +0,0 @@
-using System;
-using LibGit2Sharp.Core.Handles;
-
-namespace LibGit2Sharp
-{
-    public class DiffSafeHandleProxy : IDisposable
-    {
-        internal readonly DiffSafeHandle nativeHandle;
-
-        internal DiffSafeHandleProxy(DiffSafeHandle handle)
-        {
-            nativeHandle = handle;
-        }
-
-        #region IDisposable implementation
-
-        void IDisposable.Dispose()
-        {
-            nativeHandle.Dispose();
-        }
-
-        #endregion
-    }
-}
-
diff --git a/LibGit2Sharp/IDiffResult.cs b/LibGit2Sharp/IDiffResult.cs
index 85e013d..77aa481 100644
--- a/LibGit2Sharp/IDiffResult.cs
+++ b/LibGit2Sharp/IDiffResult.cs
@@ -1,10 +1,6 @@
-using System;
-
-namespace LibGit2Sharp
+namespace LibGit2Sharp
 {
-    public interface IDiffResult<T> where T:class
-    {
-        T FromNative(DiffSafeHandleProxy diff);
-    }
+    public interface IDiffResult
+    { }
 }

diff --git a/LibGit2Sharp/LibGit2Sharp.csproj b/LibGit2Sharp/LibGit2Sharp.csproj
index 6bed081..561ad76 100644
--- a/LibGit2Sharp/LibGit2Sharp.csproj
+++ b/LibGit2Sharp/LibGit2Sharp.csproj
@@ -381,7 +381,6 @@
     <Compile Include="Core\GitCertificateSshType.cs" />
     <Compile Include="CertificateSsh.cs" />
     <Compile Include="IDiffResult.cs" />
-    <Compile Include="DiffSafeHandleProxy.cs" />
   </ItemGroup>
   <ItemGroup>
     <CodeAnalysisDictionary Include="CustomDictionary.xml" />
diff --git a/LibGit2Sharp/Patch.cs b/LibGit2Sharp/Patch.cs
index c0aa868..fce96e1 100644
--- a/LibGit2Sharp/Patch.cs
+++ b/LibGit2Sharp/Patch.cs
@@ -16,7 +16,7 @@ namespace LibGit2Sharp
     /// deleted, modified, ..., then consider using a simpler <see cref="TreeChanges"/>.</para>
     /// </summary>
     [DebuggerDisplay("{DebuggerDisplay,nq}")]
-    public class Patch : IEnumerable<PatchEntryChanges>, IDiffResult<Patch>
+    public class Patch : IEnumerable<PatchEntryChanges>, IDiffResult
     {
         private readonly StringBuilder fullPatchBuilder = new StringBuilder();

@@ -27,9 +27,23 @@ public class Patch : IEnumerable<PatchEntryChanges>, IDiffResult<Patch>
         /// <summary>
         /// Needed for mocking purposes.
         /// </summary>
-        public Patch()
+        protected Patch()
         { }

+        internal Patch(DiffSafeHandle diff)
+        {
+            int count = Proxy.git_diff_num_deltas(diff);
+            for (int i = 0; i < count; i++)
+            {
+                using (var patch = Proxy.git_patch_from_diff(diff, i))
+                {
+                    var delta = Proxy.git_diff_get_delta(diff, i);
+                    AddFileChange(delta);
+                    Proxy.git_patch_print(patch, PrintCallBack);
+                }
+            }
+        }
+
         private void AddFileChange(GitDiffDelta delta)
         {
             var treeEntryChanges = new TreeEntryChanges(delta);
@@ -100,25 +114,6 @@ IEnumerator IEnumerable.GetEnumerator()

         #endregion

-        #region IDiffResult implementation
-
-        Patch IDiffResult<Patch>.FromNative(DiffSafeHandleProxy diff)
-        {
-            int count = Proxy.git_diff_num_deltas(diff.nativeHandle);
-            for (int i = 0; i < count; i++)
-            {
-                using (var patch = Proxy.git_patch_from_diff(diff.nativeHandle, i))
-                {
-                    var delta = Proxy.git_diff_get_delta(diff.nativeHandle, i);
-                    AddFileChange(delta);
-                    Proxy.git_patch_print(patch, PrintCallBack);
-                }
-            }
-            return this;
-        }
-
-        #endregion
-
         /// <summary>
         /// Gets the <see cref="ContentChanges"/> corresponding to the specified <paramref name="path"/>.
         /// </summary>
diff --git a/LibGit2Sharp/PatchStats.cs b/LibGit2Sharp/PatchStats.cs
index ebefb7c..fc16d30 100644
--- a/LibGit2Sharp/PatchStats.cs
+++ b/LibGit2Sharp/PatchStats.cs
@@ -13,18 +13,40 @@ namespace LibGit2Sharp
     /// <para>The individual patches for each file can be accessed through the indexer of this class.</para>
     /// </summary>
     [DebuggerDisplay("{DebuggerDisplay,nq}")]
-    public class PatchStats : IEnumerable<ContentChangeStats>, IDiffResult<PatchStats>
+    public class PatchStats : IEnumerable<ContentChangeStats>, IDiffResult
     {
         private readonly IDictionary<FilePath, ContentChangeStats> changes = new Dictionary<FilePath, ContentChangeStats>();
-        private int totalLinesAdded;
-        private int totalLinesDeleted;
+        private readonly int totalLinesAdded;
+        private readonly int totalLinesDeleted;

         /// <summary>
         /// For mocking.
         /// </summary>
-        public PatchStats()
+        protected PatchStats()
         { }

+        internal PatchStats(DiffSafeHandle diff)
+        {
+            int count = Proxy.git_diff_num_deltas(diff);
+            for (int i = 0; i < count; i++)
+            {
+                using (var patch = Proxy.git_patch_from_diff(diff, i))
+                {
+                    var delta = Proxy.git_diff_get_delta(diff, i);
+                    var pathPtr = delta.NewFile.Path != IntPtr.Zero ? delta.NewFile.Path : delta.OldFile.Path;
+                    var newFilePath = LaxFilePathMarshaler.FromNative(pathPtr);
+
+                    var stats = Proxy.git_patch_line_stats(patch);
+                    int added = stats.Item1;
+                    int deleted = stats.Item2;
+                    changes.Add(newFilePath, new ContentChangeStats(added, deleted));
+                    totalLinesAdded += added;
+                    totalLinesDeleted += deleted;
+                }
+
+            }
+        }
+
         #region IEnumerable<ContentChanges> Members

         /// <summary>
@@ -47,32 +69,6 @@ IEnumerator IEnumerable.GetEnumerator()

         #endregion

-        #region IDiffResult implementation
-
-        PatchStats IDiffResult<PatchStats>.FromNative(DiffSafeHandleProxy diff)
-        {
-            int count = Proxy.git_diff_num_deltas(diff.nativeHandle);
-            for (int i = 0; i < count; i++)
-            {
-                using (var patch = Proxy.git_patch_from_diff(diff.nativeHandle, i))
-                {
-                    var delta = Proxy.git_diff_get_delta(diff.nativeHandle, i);
-                    var pathPtr = delta.NewFile.Path != IntPtr.Zero ? delta.NewFile.Path : delta.OldFile.Path;
-                    var newFilePath = LaxFilePathMarshaler.FromNative(pathPtr);
-
-                    var stats = Proxy.git_patch_line_stats(patch);
-                    int added = stats.Item1;
-                    int deleted = stats.Item2;
-                    changes.Add(newFilePath, new ContentChangeStats(added, deleted));
-                    totalLinesAdded += added;
-                    totalLinesDeleted += deleted;
-                }
-            }
-            return this;
-        }
-
-        #endregion
-
         /// <summary>
         /// Gets the <see cref="ContentChangeStats"/> corresponding to the specified <paramref name="path"/>.
         /// </summary>
diff --git a/LibGit2Sharp/TreeChanges.cs b/LibGit2Sharp/TreeChanges.cs
index 4e012dc..dcf7885 100644
--- a/LibGit2Sharp/TreeChanges.cs
+++ b/LibGit2Sharp/TreeChanges.cs
@@ -15,7 +15,7 @@ namespace LibGit2Sharp
     /// <para>To obtain the actual patch of the diff, use the <see cref="Patch"/> class when calling Compare.</para>.
     /// </summary>
     [DebuggerDisplay("{DebuggerDisplay,nq}")]
-    public class TreeChanges : IEnumerable<TreeEntryChanges>, IDiffResult<TreeChanges>
+    public class TreeChanges : IEnumerable<TreeEntryChanges>, IDiffResult
     {
         private readonly List<TreeEntryChanges> changes = new List<TreeEntryChanges>();
         private readonly List<TreeEntryChanges> added = new List<TreeEntryChanges>();
@@ -47,9 +47,14 @@ public class TreeChanges : IEnumerable<TreeEntryChanges>, IDiffResult<TreeChange
         /// <summary>
         /// Needed for mocking purposes.
         /// </summary>
-        public TreeChanges()
+        protected TreeChanges()
         { }

+        internal TreeChanges(DiffSafeHandle diff)
+        {
+            Proxy.git_diff_foreach(diff, FileCallback, null, null);
+        }
+
         private int FileCallback(GitDiffDelta delta, float progress, IntPtr payload)
         {
             AddFileChange(delta);
@@ -86,16 +91,6 @@ IEnumerator IEnumerable.GetEnumerator()

         #endregion

-        #region IDiffResult implementation
-
-        TreeChanges IDiffResult<TreeChanges>.FromNative(DiffSafeHandleProxy diff)
-        {
-            Proxy.git_diff_foreach(diff.nativeHandle, FileCallback, null, null);
-            return this;
-        }
-
-        #endregion
-
         /// <summary>
         /// List of <see cref="TreeEntryChanges"/> that have been been added.
         /// </summary>
--

Therzok · 2015-08-24T12:27:54Z

Okay, so, with your thing, we're not getting rid of the type constructor dictionary and the runtime error. What I wanted is for the code not to compile if it wasn't a good type.

nulltoken · 2015-08-24T13:25:46Z

Okay, so, with your thing, we're not getting rid of the type constructor dictionary and the runtime error.

I didn't say it was perfect 😉 However,

There's compile type checking (T is expected to be a IDiffResult).
The exception should only be thrown when a LibGit2Sharp core contributor created a new public IDiffResult derived type but forgot to add it to the builder dictionary.
The exception message content is dynamically built from the builder dictionary.

What I wanted is for the code not to compile if it wasn't a good type.

It should work. Diff.Compare() shouldn't compile. Or did I miss something?

Therzok · 2015-08-24T14:10:01Z

@nulltoken Your suggestion is better if we do some other extra things:

Add a unit test that checks whether all IDiffResult implementing types exist in the ChangesBuilder.
Reword the message so it says that user-provided types are not supported.

Therzok · 2015-08-24T18:24:01Z

Up for review again.

nulltoken · 2015-08-24T18:42:59Z

LibGit2Sharp.Tests/MetaFixture.cs

@@ -17,7 +17,7 @@ public class MetaFixture
    {
        private static readonly HashSet<Type> explicitOnlyInterfaces = new HashSet<Type>
        {
-            typeof(IBelongToARepository),
+            typeof(IBelongToARepository), typeof(IDiffResult),


Do we need this for an empty interface?

Yes. Otherwise the test will check whether all the public api of the class is the public API contract defined by the interface. Which is empty. So every method will end up being 'extra' and the test will fail.

Therzok · 2015-08-24T19:32:30Z

LibGit2Sharp.Tests/DiffTreeToTreeFixture.cs

@@ -1124,17 +1124,6 @@ public void RetrievingDiffChangesMustAlwaysBeCaseSensitive()
        }

        [Fact]
-        public void CallingCompareWithAnUnsupportedGenericParamThrows()


This test fails to compile, to removed. :)

Indeed. That's a win! 😉

nulltoken · 2015-08-24T20:51:06Z

Besides the tiny nitpick in Diff.cs, this looks pretty neat to me!

Once fixed, can you squash so this gets merged?

Therzok · 2015-08-24T21:04:06Z

It's one commit anyway.

nulltoken · 2015-08-24T21:07:02Z

It's one commit anyway.

I can still the first proposal in the commit list with the DiffResult<T>. Can you meld the two commits together?

This allows for a typesafe Diff.Compare model. This commit introduces a new IDiffResult interface which marks classes which can be passed to Diff.Compare<T> so we get rid of the runtime exception. Fixes #1176.

Therzok · 2015-08-24T21:38:39Z

I am blind. Done.

Initial change to type-safe Diff.Compare

nulltoken · 2015-08-24T22:04:51Z

💥

Therzok force-pushed the typesafe-compare branch 2 times, most recently from f07006d to 16e2dcc Compare August 23, 2015 22:53

Therzok force-pushed the typesafe-compare branch from 4b6f2c6 to 5242d4d Compare August 24, 2015 18:23

Therzok force-pushed the typesafe-compare branch from 5242d4d to f235851 Compare August 24, 2015 18:24

nulltoken reviewed Aug 24, 2015
View reviewed changes

Therzok force-pushed the typesafe-compare branch 2 times, most recently from 2146645 to dbf9ca3 Compare August 24, 2015 19:32

Therzok reviewed Aug 24, 2015
View reviewed changes

Therzok force-pushed the typesafe-compare branch from dbf9ca3 to c5aa469 Compare August 24, 2015 21:02

Decorate Diff.Compare generics with interface

8a673bb

This allows for a typesafe Diff.Compare model. This commit introduces a new IDiffResult interface which marks classes which can be passed to Diff.Compare<T> so we get rid of the runtime exception. Fixes #1176.

Therzok force-pushed the typesafe-compare branch from c5aa469 to 8a673bb Compare August 24, 2015 21:38

nulltoken added a commit that referenced this pull request Aug 24, 2015

Merge pull request #1180 from libgit2/typesafe-compare

e734df5

Initial change to type-safe Diff.Compare

nulltoken merged commit e734df5 into vNext Aug 24, 2015

nulltoken deleted the typesafe-compare branch August 24, 2015 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial change to type-safe Diff.Compare #1180

Initial change to type-safe Diff.Compare #1180

Therzok commented Aug 23, 2015

Therzok commented Aug 23, 2015

Therzok commented Aug 23, 2015

Therzok commented Aug 23, 2015

nulltoken commented Aug 24, 2015

Therzok commented Aug 24, 2015

nulltoken commented Aug 24, 2015

Therzok commented Aug 24, 2015

Therzok commented Aug 24, 2015

nulltoken Aug 24, 2015

Therzok Aug 24, 2015

nulltoken Aug 24, 2015

Therzok Aug 24, 2015

nulltoken Aug 24, 2015

nulltoken commented Aug 24, 2015

Therzok commented Aug 24, 2015

nulltoken commented Aug 24, 2015

Therzok commented Aug 24, 2015

nulltoken commented Aug 24, 2015

Initial change to type-safe Diff.Compare #1180

Initial change to type-safe Diff.Compare #1180

Conversation

Therzok commented Aug 23, 2015

Therzok commented Aug 23, 2015

Therzok commented Aug 23, 2015

Therzok commented Aug 23, 2015

nulltoken commented Aug 24, 2015

Therzok commented Aug 24, 2015

nulltoken commented Aug 24, 2015

Therzok commented Aug 24, 2015

Therzok commented Aug 24, 2015

nulltoken Aug 24, 2015

Choose a reason for hiding this comment

Therzok Aug 24, 2015

Choose a reason for hiding this comment

nulltoken Aug 24, 2015

Choose a reason for hiding this comment

Therzok Aug 24, 2015

Choose a reason for hiding this comment

nulltoken Aug 24, 2015

Choose a reason for hiding this comment

nulltoken commented Aug 24, 2015

Therzok commented Aug 24, 2015

nulltoken commented Aug 24, 2015

Therzok commented Aug 24, 2015

nulltoken commented Aug 24, 2015