Merge pull request #1212 from github/fixes/1190-find-merge-base

Meaghan Lewis · web-flow · commit 91e349dd2a87 · 2017-09-07T07:49:39.000-07:00
Find merge base when external PR branch has been deleted
diff --git a/src/GitHub.App/Services/GitClient.cs b/src/GitHub.App/Services/GitClient.cs
@@ -380,32 +380,34 @@ public Task<bool> IsModified(IRepository repository, string path, byte[] content
         }
 
         public async Task<string> GetPullRequestMergeBase(IRepository repo,
-            UriString baseCloneUrl, UriString headCloneUrl, string baseSha, string headSha, string baseRef, string headRef)
+            UriString targetCloneUrl, string baseSha, string headSha, string baseRef, int pullNumber)
         {
             Guard.ArgumentNotNull(repo, nameof(repo));
-            Guard.ArgumentNotNull(baseCloneUrl, nameof(baseCloneUrl));
-            Guard.ArgumentNotNull(headCloneUrl, nameof(headCloneUrl));
+            Guard.ArgumentNotNull(targetCloneUrl, nameof(targetCloneUrl));
             Guard.ArgumentNotEmptyString(baseRef, nameof(baseRef));
 
-            var baseCommit = repo.Lookup<Commit>(baseSha);
-            if (baseCommit == null)
+            var headCommit = repo.Lookup<Commit>(headSha);
+            if (headCommit == null)
             {
-                await Fetch(repo, baseCloneUrl, baseRef);
-                baseCommit = repo.Lookup<Commit>(baseSha);
-                if (baseCommit == null)
+                // The PR base branch might no longer exist, so we fetch using `refs/pull/<PR>/head` first.
+                // This will often fetch the base commits, even when the base branch no longer exists.
+                var headRef = $"refs/pull/{pullNumber}/head";
+                await Fetch(repo, targetCloneUrl, headRef);
+                headCommit = repo.Lookup<Commit>(headSha);
+                if (headCommit == null)
                 {
-                    throw new NotFoundException($"Couldn't find {baseSha} after fetching from {baseCloneUrl}:{baseRef}.");
+                    throw new NotFoundException($"Couldn't find {headSha} after fetching from {targetCloneUrl}:{headRef}.");
                 }
             }
 
-            var headCommit = repo.Lookup<Commit>(headSha);
-            if (headCommit == null)
+            var baseCommit = repo.Lookup<Commit>(baseSha);
+            if (baseCommit == null)
             {
-                await Fetch(repo, headCloneUrl, headRef);
-                headCommit = repo.Lookup<Commit>(headSha);
-                if (headCommit == null)
+                await Fetch(repo, targetCloneUrl, baseRef);
+                baseCommit = repo.Lookup<Commit>(baseSha);
+                if (baseCommit == null)
                 {
-                    throw new NotFoundException($"Couldn't find {headSha} after fetching from {headCloneUrl}:{headRef}.");
+                    throw new NotFoundException($"Couldn't find {baseSha} after fetching from {targetCloneUrl}:{baseRef}.");
                 }
             }
 
diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs
@@ -313,11 +313,10 @@ public IObservable<string> ExtractFile(
                         sha = await gitClient.GetPullRequestMergeBase(
                             repo,
                             pullRequest.Base.RepositoryCloneUrl,
-                            pullRequest.Head.RepositoryCloneUrl,
                             pullRequest.Base.Sha,
                             pullRequest.Head.Sha,
                             pullRequest.Base.Ref,
-                            pullRequest.Head.Ref);
+                            pullRequest.Number);
                     }
                     catch (NotFoundException ex)
                     {
diff --git a/src/GitHub.Exports.Reactive/Services/IGitClient.cs b/src/GitHub.Exports.Reactive/Services/IGitClient.cs
@@ -188,8 +188,7 @@ public interface IGitClient
         /// Find the merge base SHA between two commits.
         /// </summary>
         /// <param name="repository">The repository.</param>
-        /// <param name="baseCloneUrl">The clone url of the PR base.</param>
-        /// <param name="headCloneUrl">The clone url of the PR head.</param>
+        /// <param name="targetCloneUrl">The clone url of the PR target repo.</param>
         /// <param name="baseSha">The PR base SHA.</param>
         /// <param name="headSha">The PR head SHA.</param>
         /// <param name="baseRef">The PR base ref (e.g. 'master').</param>
@@ -198,7 +197,7 @@ public interface IGitClient
         /// The merge base SHA or null.
         /// </returns>
         /// <exception cref="LibGit2Sharp.NotFoundException">Thrown when the merge base can't be found.</exception>
-        Task<string> GetPullRequestMergeBase(IRepository repo, UriString baseCloneUrl, UriString headCloneUrl, string baseSha, string headSha, string baseRef, string headRef);
+        Task<string> GetPullRequestMergeBase(IRepository repo, UriString targetCloneUrl, string baseSha, string headSha, string baseRef, int pullNumber);
 
         /// Checks whether the current head is pushed to its remote tracking branch.
         /// </summary>
diff --git a/src/GitHub.InlineReviews/Services/PullRequestSessionService.cs b/src/GitHub.InlineReviews/Services/PullRequestSessionService.cs
@@ -122,13 +122,13 @@ public async Task<string> GetPullRequestMergeBase(ILocalRepositoryModel reposito
             }
 
             var repo = await GetRepository(repository);
-            var baseUrl = pullRequest.Base.RepositoryCloneUrl;
+            var targetUrl = pullRequest.Base.RepositoryCloneUrl;
             var headUrl = pullRequest.Head.RepositoryCloneUrl;
             var baseRef = pullRequest.Base.Ref;
-            var headRef = pullRequest.Head.Ref;
+            var pullNumber = pullRequest.Number;
             try
             {
-                mergeBase = await gitClient.GetPullRequestMergeBase(repo, baseUrl, headUrl, baseSha, headSha, baseRef, headRef);
+                mergeBase = await gitClient.GetPullRequestMergeBase(repo, targetUrl, baseSha, headSha, baseRef, pullNumber);
             }
             catch (NotFoundException ex)
             {
diff --git a/src/UnitTests/GitHub.App/Services/GitClientTests.cs b/src/UnitTests/GitHub.App/Services/GitClientTests.cs
@@ -162,61 +162,61 @@ public class TheGetPullRequestMergeBaseMethod : TestBaseClass
         [Fact]
         public async Task LocalBaseHeadAndMergeBase_DontFetch()
         {
-            var baseUrl = new UriString("https://github.com/owner/repo");
-            var headUrl = new UriString("https://github.com/owner/repo");
+            var targetCloneUrl = new UriString("https://github.com/owner/repo");
             var baseSha = "baseSha";
             var headSha = "headSha";
             var expectMergeBaseSha = "mergeBaseSha";
             var baseRef = "master";
-            var headRef = "headRef";
+            var pullNumber = 0;
             var repo = MockRepo(baseSha, headSha, expectMergeBaseSha);
             var gitClient = new GitClient(Substitute.For<IGitHubCredentialProvider>());
 
-            var mergeBaseSha = await gitClient.GetPullRequestMergeBase(repo, baseUrl, headUrl, baseSha, headSha, baseRef, headRef);
+            var mergeBaseSha = await gitClient.GetPullRequestMergeBase(repo, targetCloneUrl, baseSha, headSha, baseRef, pullNumber);
 
             repo.Network.DidNotReceiveWithAnyArgs().Fetch(null as Remote, null, null as FetchOptions);
             Assert.Equal(expectMergeBaseSha, mergeBaseSha);
         }
 
         [Theory]
-        [InlineData("https://github.com/owner/repo", "https://github.com/owner/repo", "baseSha", "headSha", "mergeBaseSha", 0)]
-        [InlineData("https://github.com/owner/repo", "https://github.com/owner/repo", null, "headSha", "mergeBaseSha", 1)]
-        [InlineData("https://github.com/owner/repo", "https://github.com/owner/repo", "baseSha", null, "mergeBaseSha", 1)]
-        [InlineData("https://github.com/owner/repo", "https://github.com/owner/repo", "baseSha", "headSha", null, 0)]
-        public async Task WhenToFetch(string baseUrl, string headUrl, string baseSha, string headSha, string mergeBaseSha, int receivedFetch)
+        [InlineData("https://github.com/owner/repo", "baseSha", "headSha", "mergeBaseSha", 0)]
+        [InlineData("https://github.com/owner/repo", null, "headSha", "mergeBaseSha", 1)]
+        [InlineData("https://github.com/owner/repo", "baseSha", null, "mergeBaseSha", 1)]
+        [InlineData("https://github.com/owner/repo", "baseSha", "headSha", null, 0)]
+        public async Task WhenToFetch(string targetCloneUrl, string baseSha, string headSha, string mergeBaseSha, int receivedFetch)
         {
-            var baseUri = new UriString(baseUrl);
-            var headUri = new UriString(headUrl);
+            var targetCloneUri = new UriString("https://github.com/owner/repo");
             var baseRef = "master";
-            var headRef = "headRef";
+            var pullNumber = 0;
             var repo = MockRepo(baseSha, headSha, mergeBaseSha);
             var remote = Substitute.For<Remote>();
             repo.Network.Remotes.Add(null, null).ReturnsForAnyArgs(remote);
             var gitClient = new GitClient(Substitute.For<IGitHubCredentialProvider>());
 
             try
             {
-                await gitClient.GetPullRequestMergeBase(repo, baseUri, headUri, baseSha, headSha, baseRef, headRef);
+                await gitClient.GetPullRequestMergeBase(repo, targetCloneUri, baseSha, headSha, baseRef, pullNumber);
             }
             catch (NotFoundException) { /* We're interested in calls to Fetch even if it throws */ }
 
             repo.Network.Received(receivedFetch).Fetch(Arg.Any<Remote>(), Arg.Any<string[]>(), Arg.Any<FetchOptions>());
         }
 
         [Theory]
-        [InlineData("baseSha", null, "mergeBaseSha", "baseRef", "headRef", "headRef")]
-        [InlineData(null, "headSha", "mergeBaseSha", "baseRef", "headRef", "baseRef")]
-        public async Task WhatToFetch(string baseSha, string headSha, string mergeBaseSha, string baseRef, string headRef,
+        [InlineData("baseSha", null, "mergeBaseSha", "baseRef", 777, "refs/pull/777/head")]
+        [InlineData(null, "headSha", "mergeBaseSha", "baseRef", 777, "baseRef")]
+
+        // PR base might not exist, so we must fetch `refs/pull/<PR>/head` first.
+        [InlineData(null, null, "mergeBaseSha", "baseRef", 777, "refs/pull/777/head")]
+        public async Task WhatToFetch(string baseSha, string headSha, string mergeBaseSha, string baseRef, int pullNumber,
             string expectRefSpec)
         {
             var repo = MockRepo(baseSha, headSha, mergeBaseSha);
-            var baseUrl = new UriString("https://github.com/owner/repo");
-            var headUrl = new UriString("https://github.com/owner/repo");
+            var targetCloneUri = new UriString("https://github.com/owner/repo");
             var gitClient = new GitClient(Substitute.For<IGitHubCredentialProvider>());
 
             try
             {
-                await gitClient.GetPullRequestMergeBase(repo, baseUrl, headUrl, baseSha, headSha, baseRef, headRef);
+                await gitClient.GetPullRequestMergeBase(repo, targetCloneUri, baseSha, headSha, baseRef, pullNumber);
             }
             catch (NotFoundException) { /* We're interested in calls to Fetch even if it throws */ }
 
@@ -233,12 +233,12 @@ static IRepository MockRepo(string baseSha, string headSha, string mergeBaseSha)
 
             if (baseSha != null)
             {
-                repo.Lookup<Commit>(baseSha).Returns(baseCommit);
+                repo.Lookup<Commit>(baseSha).Returns(baseSha != null ? baseCommit : null);
             }
 
             if (headSha != null)
             {
-                repo.Lookup<Commit>(headSha).Returns(headCommit);
+                repo.Lookup<Commit>(headSha).Returns(headSha != null ? headCommit : null);
             }
 
             repo.ObjectDatabase.FindMergeBase(baseCommit, headCommit).Returns(mergeBaseCommit);
diff --git a/src/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs b/src/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs
@@ -168,11 +168,11 @@ static async Task<string> ExtractFile(
 
             if (mergeBaseException == null)
             {
-                gitClient.GetPullRequestMergeBase(Arg.Any<IRepository>(), Arg.Any<UriString>(), Arg.Any<UriString>(), baseSha, headSha, baseRef, headRef).ReturnsForAnyArgs(Task.FromResult(mergeBaseSha));
+                gitClient.GetPullRequestMergeBase(Arg.Any<IRepository>(), Arg.Any<UriString>(), baseSha, headSha, baseRef, pullNumber).ReturnsForAnyArgs(Task.FromResult(mergeBaseSha));
             }
             else
             {
-                gitClient.GetPullRequestMergeBase(Arg.Any<IRepository>(), Arg.Any<UriString>(), Arg.Any<UriString>(), baseSha, headSha, baseRef, headRef).ReturnsForAnyArgs(Task.FromException<string>(mergeBaseException));
+                gitClient.GetPullRequestMergeBase(Arg.Any<IRepository>(), Arg.Any<UriString>(), baseSha, headSha, baseRef, pullNumber).ReturnsForAnyArgs(Task.FromException<string>(mergeBaseException));
             }
 
             gitClient.ExtractFile(Arg.Any<IRepository>(), mergeBaseSha, fileName).Returns(GetFileTask(mergeBaseFileContent));

Original file line number	Diff line number	Diff line change
`@@ -380,32 +380,34 @@ public Task<bool> IsModified(IRepository repository, string path, byte[] content`
`380`	`380`	`}`
`381`	`381`
`382`	`382`	`public async Task<string> GetPullRequestMergeBase(IRepository repo,`
`383`		`- UriString baseCloneUrl, UriString headCloneUrl, string baseSha, string headSha, string baseRef, string headRef)`
	`383`	`+ UriString targetCloneUrl, string baseSha, string headSha, string baseRef, int pullNumber)`
`384`	`384`	`{`
`385`	`385`	`Guard.ArgumentNotNull(repo, nameof(repo));`
`386`		`- Guard.ArgumentNotNull(baseCloneUrl, nameof(baseCloneUrl));`
`387`		`- Guard.ArgumentNotNull(headCloneUrl, nameof(headCloneUrl));`
	`386`	`+ Guard.ArgumentNotNull(targetCloneUrl, nameof(targetCloneUrl));`
`388`	`387`	`Guard.ArgumentNotEmptyString(baseRef, nameof(baseRef));`
`389`	`388`
`390`		`- var baseCommit = repo.Lookup<Commit>(baseSha);`
`391`		`- if (baseCommit == null)`
	`389`	`+ var headCommit = repo.Lookup<Commit>(headSha);`
	`390`	`+ if (headCommit == null)`
`392`	`391`	`{`
`393`		`- await Fetch(repo, baseCloneUrl, baseRef);`
`394`		`- baseCommit = repo.Lookup<Commit>(baseSha);`
`395`		`- if (baseCommit == null)`
	`392`	+ // The PR base branch might no longer exist, so we fetch using `refs/pull/<PR>/head` first.
	`393`	`+ // This will often fetch the base commits, even when the base branch no longer exists.`
	`394`	`+ var headRef = $"refs/pull/{pullNumber}/head";`
	`395`	`+ await Fetch(repo, targetCloneUrl, headRef);`
	`396`	`+ headCommit = repo.Lookup<Commit>(headSha);`
	`397`	`+ if (headCommit == null)`
`396`	`398`	`{`
`397`		`- throw new NotFoundException($"Couldn't find {baseSha} after fetching from {baseCloneUrl}:{baseRef}.");`
	`399`	`+ throw new NotFoundException($"Couldn't find {headSha} after fetching from {targetCloneUrl}:{headRef}.");`
`398`	`400`	`}`
`399`	`401`	`}`
`400`	`402`
`401`		`- var headCommit = repo.Lookup<Commit>(headSha);`
`402`		`- if (headCommit == null)`
	`403`	`+ var baseCommit = repo.Lookup<Commit>(baseSha);`
	`404`	`+ if (baseCommit == null)`
`403`	`405`	`{`
`404`		`- await Fetch(repo, headCloneUrl, headRef);`
`405`		`- headCommit = repo.Lookup<Commit>(headSha);`
`406`		`- if (headCommit == null)`
	`406`	`+ await Fetch(repo, targetCloneUrl, baseRef);`
	`407`	`+ baseCommit = repo.Lookup<Commit>(baseSha);`
	`408`	`+ if (baseCommit == null)`
`407`	`409`	`{`
`408`		`- throw new NotFoundException($"Couldn't find {headSha} after fetching from {headCloneUrl}:{headRef}.");`
	`410`	`+ throw new NotFoundException($"Couldn't find {baseSha} after fetching from {targetCloneUrl}:{baseRef}.");`
`409`	`411`	`}`
`410`	`412`	`}`
`411`	`413`
Original file line number	Diff line number	Diff line change
`@@ -313,11 +313,10 @@ public IObservable<string> ExtractFile(`
`313`	`313`	`sha = await gitClient.GetPullRequestMergeBase(`
`314`	`314`	`repo,`
`315`	`315`	`pullRequest.Base.RepositoryCloneUrl,`
`316`		`- pullRequest.Head.RepositoryCloneUrl,`
`317`	`316`	`pullRequest.Base.Sha,`
`318`	`317`	`pullRequest.Head.Sha,`
`319`	`318`	`pullRequest.Base.Ref,`
`320`		`- pullRequest.Head.Ref);`
	`319`	`+ pullRequest.Number);`
`321`	`320`	`}`
`322`	`321`	`catch (NotFoundException ex)`
`323`	`322`	`{`
Original file line number	Diff line number	Diff line change
`@@ -122,13 +122,13 @@ public async Task<string> GetPullRequestMergeBase(ILocalRepositoryModel reposito`
`122`	`122`	`}`
`123`	`123`
`124`	`124`	`var repo = await GetRepository(repository);`
`125`		`- var baseUrl = pullRequest.Base.RepositoryCloneUrl;`
	`125`	`+ var targetUrl = pullRequest.Base.RepositoryCloneUrl;`
`126`	`126`	`var headUrl = pullRequest.Head.RepositoryCloneUrl;`
`127`	`127`	`var baseRef = pullRequest.Base.Ref;`
`128`		`- var headRef = pullRequest.Head.Ref;`
	`128`	`+ var pullNumber = pullRequest.Number;`
`129`	`129`	`try`
`130`	`130`	`{`
`131`		`- mergeBase = await gitClient.GetPullRequestMergeBase(repo, baseUrl, headUrl, baseSha, headSha, baseRef, headRef);`
	`131`	`+ mergeBase = await gitClient.GetPullRequestMergeBase(repo, targetUrl, baseSha, headSha, baseRef, pullNumber);`
`132`	`132`	`}`
`133`	`133`	`catch (NotFoundException ex)`
`134`	`134`	`{`
Original file line number	Diff line number	Diff line change
`@@ -168,11 +168,11 @@ static async Task<string> ExtractFile(`
`168`	`168`
`169`	`169`	`if (mergeBaseException == null)`
`170`	`170`	`{`
`171`		`- gitClient.GetPullRequestMergeBase(Arg.Any<IRepository>(), Arg.Any<UriString>(), Arg.Any<UriString>(), baseSha, headSha, baseRef, headRef).ReturnsForAnyArgs(Task.FromResult(mergeBaseSha));`
	`171`	`+ gitClient.GetPullRequestMergeBase(Arg.Any<IRepository>(), Arg.Any<UriString>(), baseSha, headSha, baseRef, pullNumber).ReturnsForAnyArgs(Task.FromResult(mergeBaseSha));`
`172`	`172`	`}`
`173`	`173`	`else`
`174`	`174`	`{`
`175`		`- gitClient.GetPullRequestMergeBase(Arg.Any<IRepository>(), Arg.Any<UriString>(), Arg.Any<UriString>(), baseSha, headSha, baseRef, headRef).ReturnsForAnyArgs(Task.FromException<string>(mergeBaseException));`
	`175`	`+ gitClient.GetPullRequestMergeBase(Arg.Any<IRepository>(), Arg.Any<UriString>(), baseSha, headSha, baseRef, pullNumber).ReturnsForAnyArgs(Task.FromException<string>(mergeBaseException));`
`176`	`176`	`}`
`177`	`177`
`178`	`178`	`gitClient.ExtractFile(Arg.Any<IRepository>(), mergeBaseSha, fileName).Returns(GetFileTask(mergeBaseFileContent));`