Merge pull request #1971 from github/fixes/1707-single-call-to-ReachableFrom

Fix slowness and non-determinism of GetLatestPushedSha (GitHub > Open on GitHub)

Author: grokys

src/GitHub.App/SampleData/GitServiceDesigner.cs

@@ -7,7 +7,7 @@ namespace GitHub.SampleData
 {
     class GitServiceDesigner : IGitService
     {
-        public Task<string> GetLatestPushedSha(string path) => Task.FromResult<string>(null);
+        public Task<string> GetLatestPushedSha(string path, string remote = "origin") => Task.FromResult<string>(null);
         public UriString GetRemoteUri(IRepository repo, string remote = "origin") => null;
         public IRepository GetRepository(string path) => null;
         public UriString GetUri(string path, string remote = "origin") => null;

src/GitHub.Exports/Services/GitService.cs

@@ -13,6 +13,14 @@ namespace GitHub.Services
     [PartCreationPolicy(CreationPolicy.NonShared)]
     public class GitService : IGitService
     {
+        readonly IRepositoryFacade repositoryFacade;
+
+        [ImportingConstructor]
+        public GitService(IRepositoryFacade repositoryFacade)
+        {
+            this.repositoryFacade = repositoryFacade;
+        }
+
         /// <summary>
         /// Returns the URL of the remote for the specified <see cref="repository"/>. If the repository
         /// is null or no remote named origin exists, this method returns null
@@ -56,8 +64,8 @@ public UriString GetUri(string path, string remote = "origin")
         /// <returns>An instance of <see cref="IRepositoryModel"/> or null</returns>
         public IRepository GetRepository(string path)
         {
-            var repoPath = Repository.Discover(path);
-            return repoPath == null ? null : new Repository(repoPath);
+            var repoPath = repositoryFacade.Discover(path);
+            return repoPath == null ? null : repositoryFacade.NewRepository(repoPath);
         }
 
         /// <summary>
@@ -80,42 +88,70 @@ public UriString GetRemoteUri(IRepository repo, string remote = "origin")
         /// <remarks>
         /// This is equivalent to creating it via MEF with <see cref="CreationPolicy.NonShared"/>
         /// </remarks>
-        public static IGitService GitServiceHelper => new GitService();
+        public static IGitService GitServiceHelper => new GitService(new RepositoryFacade());
 
         /// <summary>
         /// Finds the latest pushed commit of a file and returns the sha of that commit. Returns null when no commits have 
         /// been found in any remote branches or the current local branch. 
         /// </summary>
         /// <param name="path">The local path of a repository or a file inside a repository. This cannot be null.</param>
+        /// <param name="remote">The remote name to look for</param>
         /// <returns></returns>
-        public Task<string> GetLatestPushedSha(string path)
+        public Task<string> GetLatestPushedSha(string path, string remote = "origin")
         {
             Guard.ArgumentNotNull(path, nameof(path));
 
-            return Task.Factory.StartNew(() =>
+            return Task.Run(() =>
             {
                 using (var repo = GetRepository(path))
                 {
                     if (repo != null)
                     {
-                        if (repo.Head.IsTracking && repo.Head.Tip.Sha == repo.Head.TrackedBranch.Tip.Sha)
+                        // This is the common case where HEAD is tracking a remote branch
+                        var commonAncestor = repo.Head.TrackingDetails.CommonAncestor;
+                        if (commonAncestor != null)
                         {
-                            return repo.Head.Tip.Sha;
+                            return commonAncestor.Sha;
                         }
 
-                        var remoteHeads = repo.Refs.Where(r => r.IsRemoteTrackingBranch).ToList();
-                        foreach (var c in repo.Commits)
+                        // This is the common case where a branch was forked from a local branch.
+                        // Use CommonAncestor because we don't want to search for a commit that only exists
+                        // locally or that has been added to the remote tracking branch since the fork.
+                        var commonAncestorShas = repo.Branches
+                            .Where(b => b.IsTracking)
+                            .Select(b => b.TrackingDetails.CommonAncestor?.Sha)
+                            .Where(s => s != null)
+                            .ToArray();
+
+                        var sortByTopological = new CommitFilter { SortBy = CommitSortStrategies.Topological };
+                        foreach (var commit in repo.Commits.QueryBy(sortByTopological))
                         {
-                            if (repo.Refs.ReachableFrom(remoteHeads, new[] { c }).Any())
+                            if (commonAncestorShas.Contains(commit.Sha))
                             {
-                                return c.Sha;
+                                return commit.Sha;
                             }
                         }
+
+                        // This is a less common case where a branch was forked from a branch
+                        // which has since had new commits added to it.
+                        var nearestCommonAncestor = repo.Branches
+                            .Where(b => b.IsRemote)
+                            .Select(b => b.Tip)
+                            .Distinct()
+                            .Select(c => repo.ObjectDatabase.CalculateHistoryDivergence(c, repo.Head.Tip))
+                            .Where(hd => hd.AheadBy != null)
+                            .OrderBy(hd => hd.BehindBy)
+                            .Select(hd => hd.CommonAncestor)
+                            .FirstOrDefault();
+                        if (nearestCommonAncestor != null)
+                        {
+                            return nearestCommonAncestor.Sha;
+                        }
                     }
 
                     return null;
                 }
             });
         }
     }
-}
+}

src/GitHub.Exports/Services/IGitService.cs

@@ -28,7 +28,7 @@ public interface IGitService
         /// <param name="remote">The remote name to look for</param>
         /// <returns>A <see cref="UriString"/> representing the origin or null if none found.</returns>
         UriString GetUri(string path, string remote = "origin");
-        
+
         /// <summary>
         /// Probes for a git repository and if one is found, returns a <see cref="IRepositoryModel"/> instance for the
         /// repository.
@@ -54,7 +54,8 @@ public interface IGitService
         /// been found in any remote branches or the current local branch. 
         /// </summary>
         /// <param name="path">The local path of a repository or a file inside a repository. This cannot be null.</param>
+        /// <param name="remote">The remote name to look for</param>
         /// <returns></returns>
-        Task<string> GetLatestPushedSha(string path);
+        Task<string> GetLatestPushedSha(string path, string remote = "origin");
     }
 }

src/GitHub.Exports/Services/IRepositoryFacade.cs

@@ -0,0 +1,13 @@
+using LibGit2Sharp;
+
+namespace GitHub.Services
+{
+    /// <summary>
+    /// Facade for <see cref="LibGit2Sharp.Repository"/> static methods.
+    /// </summary>
+    public interface IRepositoryFacade
+    {
+        IRepository NewRepository(string path);
+        string Discover(string startingPath);
+    }
+}

src/GitHub.Exports/Services/RepositoryFacade.cs

@@ -0,0 +1,22 @@
+using System.ComponentModel.Composition;
+using LibGit2Sharp;
+
+namespace GitHub.Services
+{
+    /// <summary>
+    /// Facade for <see cref="LibGit2Sharp.Repository"/> static methods.
+    /// </summary>
+    [Export(typeof(IRepositoryFacade))]
+    public class RepositoryFacade : IRepositoryFacade
+    {
+        public IRepository NewRepository(string path)
+        {
+            return new Repository(path);
+        }
+
+        public string Discover(string startingPath)
+        {
+            return Repository.Discover(startingPath);
+        }
+    }
+}