Write to log when error is shown in PR detail view

src/GitHub.App/Services/GitClient.cs

@@ -98,7 +98,7 @@ public Task Fetch(IRepository repository, string remoteName, params string[] ref
                     var remote = repository.Network.Remotes[remoteName];
                     repository.Network.Fetch(remote, refspecs, fetchOptions);
                 }
-                catch(Exception ex)
+                catch (Exception ex)
                 {
                     log.Error("Failed to fetch", ex);
 #if DEBUG
@@ -394,7 +394,7 @@ public async Task<string> GetPullRequestMergeBase(IRepository repo,
                 baseCommit = repo.Lookup<Commit>(baseSha);
                 if (baseCommit == null)
                 {
-                    return null;
+                    throw new NotFoundException($"Couldn't find {baseSha} after fetching from {baseCloneUrl}:{baseRef}.");
                 }
             }
 
@@ -405,14 +405,14 @@ public async Task<string> GetPullRequestMergeBase(IRepository repo,
                 headCommit = repo.Lookup<Commit>(headSha);
                 if (headCommit == null)
                 {
-                    return null;
+                    throw new NotFoundException($"Couldn't find {headSha} after fetching from {headCloneUrl}:{headRef}.");
                 }
             }
 
             var mergeBaseCommit = repo.ObjectDatabase.FindMergeBase(baseCommit, headCommit);
-            if(mergeBaseCommit == null)
+            if (mergeBaseCommit == null)
             {
-                return null;
+                throw new NotFoundException($"Couldn't find merge base between {baseCommit} and {headCommit}.");
             }
 
             return mergeBaseCommit.Sha;

src/GitHub.App/Services/PullRequestService.cs

@@ -308,18 +308,20 @@ public IObservable<string> ExtractFile(
                 }
                 else
                 {
-                    sha = await gitClient.GetPullRequestMergeBase(
-                        repo,
-                        pullRequest.Base.RepositoryCloneUrl,
-                        pullRequest.Head.RepositoryCloneUrl,
-                        pullRequest.Base.Sha,
-                        pullRequest.Head.Sha,
-                        pullRequest.Base.Ref,
-                        pullRequest.Head.Ref);
-
-                    if (sha == null)
+                    try
                     {
-                        throw new NotFoundException($"Couldn't find merge base between {pullRequest.Base.Sha} and {pullRequest.Head.Sha}.");
+                        sha = await gitClient.GetPullRequestMergeBase(
+                            repo,
+                            pullRequest.Base.RepositoryCloneUrl,
+                            pullRequest.Head.RepositoryCloneUrl,
+                            pullRequest.Base.Sha,
+                            pullRequest.Head.Sha,
+                            pullRequest.Base.Ref,
+                            pullRequest.Head.Ref);
+                    }
+                    catch (NotFoundException ex)
+                    {
+                        throw new NotFoundException($"The Pull Request file failed to load. Please check your network connection and click refresh to try again. If this issue persists, please let us know at support@github.com", ex);
                     }
                 }
 
@@ -350,7 +352,7 @@ static bool HasPreamble(string file, Encoding encoding)
             {
                 foreach (var b in encoding.GetPreamble())
                 {
-                    if(b != stream.ReadByte())
+                    if (b != stream.ReadByte())
                     {
                         return false;
                     }
@@ -473,7 +475,7 @@ async Task<bool> IsBranchMarkedAsPullRequest(IRepository repo, string branchName
         {
             var prConfigKey = $"branch.{branchName}.{SettingGHfVSPullRequest}";
             var value = ParseGHfVSConfigKeyValue(await gitClient.GetConfig<string>(repo, prConfigKey));
-            return value != null && 
+            return value != null &&
                 value.Item1 == pullRequest.Base.RepositoryCloneUrl.Owner &&
                 value.Item2 == pullRequest.Number;
         }
@@ -550,4 +552,4 @@ static Tuple<string, int> ParseGHfVSConfigKeyValue(string value)
             return null;
         }
     }
-}
+}

src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs

@@ -16,6 +16,7 @@
 using GitHub.UI;
 using LibGit2Sharp;
 using ReactiveUI;
+using NLog;
 
 namespace GitHub.ViewModels
 {
@@ -26,6 +27,8 @@ namespace GitHub.ViewModels
     [PartCreationPolicy(CreationPolicy.NonShared)]
     public class PullRequestDetailViewModel : PanePageViewModelBase, IPullRequestDetailViewModel
     {
+        static readonly Logger log = LogManager.GetCurrentClassLogger();
+
         readonly IModelService modelService;
         readonly IPullRequestService pullRequestsService;
         readonly IPullRequestSessionManager sessionManager;
@@ -93,7 +96,7 @@ public PullRequestDetailViewModel(
             Checkout = ReactiveCommand.CreateAsyncObservable(
                 this.WhenAnyValue(x => x.CheckoutState)
                     .Cast<CheckoutCommandState>()
-                    .Select(x => x != null && x.IsEnabled), 
+                    .Select(x => x != null && x.IsEnabled),
                 DoCheckout);
             Checkout.IsExecuting.Subscribe(x => isInCheckout = x);
             SubscribeOperationError(Checkout);
@@ -351,6 +354,7 @@ public override void Initialize(ViewWithData data)
                 .ObserveOn(RxApp.MainThreadScheduler)
                 .Catch<IPullRequestModel, Exception>(ex =>
                 {
+                    log.Error("Error observing GetPullRequest", ex);
                     ErrorMessage = ex.Message.Trim();
                     IsLoading = IsBusy = false;
                     return Observable.Empty<IPullRequestModel>();
@@ -460,6 +464,7 @@ public async Task Load(IRemoteRepositoryModel remoteRepository, IPullRequestMode
             }
             catch (Exception ex)
             {
+                log.Error("Error loading PullRequestModel", ex);
                 ErrorMessage = ex.Message.Trim();
             }
             finally

src/GitHub.Exports.Reactive/Services/IGitClient.cs

@@ -197,6 +197,7 @@ public interface IGitClient
         /// <returns>
         /// 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);
 
         /// Checks whether the current head is pushed to its remote tracking branch.

src/GitHub.InlineReviews/Services/PullRequestSessionService.cs

@@ -116,24 +116,26 @@ public async Task<string> GetPullRequestMergeBase(ILocalRepositoryModel reposito
             var key = new Tuple<string, string>(baseSha, headSha);
 
             string mergeBase;
-            if(mergeBaseCache.TryGetValue(key, out mergeBase))
+            if (mergeBaseCache.TryGetValue(key, out mergeBase))
             {
                 return mergeBase;
             }
 
             var repo = await GetRepository(repository);
             var baseUrl = pullRequest.Base.RepositoryCloneUrl;
             var headUrl = pullRequest.Head.RepositoryCloneUrl;
-            var headCloneUrl = pullRequest.Head.RepositoryCloneUrl;
             var baseRef = pullRequest.Base.Ref;
             var headRef = pullRequest.Head.Ref;
-            mergeBase = await gitClient.GetPullRequestMergeBase(repo, baseUrl, headUrl, baseSha, headSha, baseRef, headRef);
-            if (mergeBase != null)
+            try
+            {
+                mergeBase = await gitClient.GetPullRequestMergeBase(repo, baseUrl, headUrl, baseSha, headSha, baseRef, headRef);
+            }
+            catch (NotFoundException ex)
             {
-                return mergeBaseCache[key] = mergeBase;
+                throw new NotFoundException("The Pull Request failed to load. Please check your network connection and click refresh to try again. If this issue persists, please let us know at support@github.com", ex);
             }
 
-            throw new FileNotFoundException($"Couldn't find merge base between {baseSha} and {headSha}.");
+            return mergeBaseCache[key] = mergeBase;
         }
 
         /// <inheritdoc/>

src/UnitTests/GitHub.App/Services/GitClientTests.cs

@@ -54,7 +54,7 @@ public async Task PushesToDefaultOrigin()
 
             await gitClient.Push(repository, "master", "origin");
 
-            repository.Network.Received().Push(origin,"HEAD", @"refs/heads/master", Arg.Any<PushOptions>());
+            repository.Network.Received().Push(origin, "HEAD", @"refs/heads/master", Arg.Any<PushOptions>());
         }
 
         [Fact]
@@ -194,7 +194,11 @@ public async Task WhenToFetch(string baseUrl, string headUrl, string baseSha, st
             repo.Network.Remotes.Add(null, null).ReturnsForAnyArgs(remote);
             var gitClient = new GitClient(Substitute.For<IGitHubCredentialProvider>());
 
-            await gitClient.GetPullRequestMergeBase(repo, baseUri, headUri, baseSha, headSha, baseRef, headRef);
+            try
+            {
+                await gitClient.GetPullRequestMergeBase(repo, baseUri, headUri, baseSha, headSha, baseRef, headRef);
+            }
+            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>());
         }
@@ -210,7 +214,11 @@ public async Task WhatToFetch(string baseSha, string headSha, string mergeBaseSh
             var headUrl = new UriString("https://github.com/owner/repo");
             var gitClient = new GitClient(Substitute.For<IGitHubCredentialProvider>());
 
-            await gitClient.GetPullRequestMergeBase(repo, baseUrl, headUrl, baseSha, headSha, baseRef, headRef);
+            try
+            {
+                await gitClient.GetPullRequestMergeBase(repo, baseUrl, headUrl, baseSha, headSha, baseRef, headRef);
+            }
+            catch (NotFoundException) { /* We're interested in calls to Fetch even if it throws */ }
 
             repo.Network.Received(1).Fetch(Arg.Any<Remote>(), Arg.Is<IEnumerable<string>>(x => x.Contains(expectRefSpec)), Arg.Any<FetchOptions>());
         }

src/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs

@@ -54,7 +54,7 @@ public async Task ExtractBase_MergeBaseAvailable_UseMergeBaseSha()
         }
 
         [Fact]
-        public async void MergeBaseNotAvailable_ThrowsFileNotFoundException()
+        public async void MergeBaseNotAvailable_ThrowsNotFoundException()
         {
             var baseFileContent = "baseFileContent";
             var headFileContent = "headFileContent";
@@ -64,11 +64,10 @@ public async void MergeBaseNotAvailable_ThrowsFileNotFoundException()
             var headSha = "headSha";
             var mergeBaseSha = null as string;
             var head = false;
-            var expectMessage = $"Couldn't find merge base between {baseSha} and {headSha}.";
+            var mergeBaseException = new NotFoundException();
 
             var ex = await Assert.ThrowsAsync<NotFoundException>(() => ExtractFile(baseSha, baseFileContent, headSha, headFileContent, mergeBaseSha, mergeBaseFileContent,
-                                fileName, head, Encoding.UTF8));
-            Assert.Equal(expectMessage, ex.Message);
+                                fileName, head, Encoding.UTF8, mergeBaseException: mergeBaseException));
         }
 
         [Fact]
@@ -150,7 +149,8 @@ static bool HasPreamble(string file, Encoding encoding)
 
         static async Task<string> ExtractFile(
             string baseSha, object baseFileContent, string headSha, object headFileContent, string mergeBaseSha, object mergeBaseFileContent,
-            string fileName, bool head, Encoding encoding, string repoDir = "repoDir", int pullNumber = 666, string baseRef = "baseRef", string headRef = "headRef")
+            string fileName, bool head, Encoding encoding, string repoDir = "repoDir", int pullNumber = 666, string baseRef = "baseRef", string headRef = "headRef",
+            Exception mergeBaseException = null)
         {
             var repositoryModel = Substitute.For<ILocalRepositoryModel>();
             repositoryModel.LocalPath.Returns(repoDir);
@@ -166,7 +166,15 @@ static async Task<string> ExtractFile(
             var gitService = serviceProvider.GetGitService();
             var service = new PullRequestService(gitClient, gitService, serviceProvider.GetOperatingSystem(), Substitute.For<IUsageTracker>());
 
-            gitClient.GetPullRequestMergeBase(Arg.Any<IRepository>(), Arg.Any<UriString>(), Arg.Any<UriString>(), baseSha, headSha, baseRef, headRef).ReturnsForAnyArgs(Task.FromResult(mergeBaseSha));
+            if (mergeBaseException == null)
+            {
+                gitClient.GetPullRequestMergeBase(Arg.Any<IRepository>(), Arg.Any<UriString>(), Arg.Any<UriString>(), baseSha, headSha, baseRef, headRef).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.ExtractFile(Arg.Any<IRepository>(), mergeBaseSha, fileName).Returns(GetFileTask(mergeBaseFileContent));
             gitClient.ExtractFile(Arg.Any<IRepository>(), baseSha, fileName).Returns(GetFileTask(baseFileContent));
             gitClient.ExtractFile(Arg.Any<IRepository>(), headSha, fileName).Returns(GetFileTask(headFileContent));
@@ -561,7 +569,7 @@ static IGitService MockGitService()
         var branches = MockBranches("pr/123-foo1", "pr/123-foo1-2");
         repository.Branches.Returns(branches);
 
-        var result = Substitute.For<IGitService>();        
+        var result = Substitute.For<IGitService>();
         result.GetRepository(Arg.Any<string>()).Returns(repository);
         return result;
     }