Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Commit 19bf7aa

Browse files
jcansdalejcansdale
authored
Merge pull request #1199 from github/jcansdale/add-more-logging
Write to log when error is shown in PR detail view
2 parents bd76f92 + 582d7a4 commit 19bf7aa

File tree

7 files changed

+62
-36
lines changed

7 files changed

+62
-36
lines changed

‎src/GitHub.App/Services/GitClient.cs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ public Task Fetch(IRepository repository, string remoteName, params string[] ref
9898
var remote = repository.Network.Remotes[remoteName];
9999
repository.Network.Fetch(remote, refspecs, fetchOptions);
100100
}
101-
catch(Exception ex)
101+
catch (Exception ex)
102102
{
103103
log.Error("Failed to fetch", ex);
104104
#if DEBUG
@@ -394,7 +394,7 @@ public async Task<string> GetPullRequestMergeBase(IRepository repo,
394394
baseCommit = repo.Lookup<Commit>(baseSha);
395395
if (baseCommit == null)
396396
{
397-
return null;
397+
throw new NotFoundException($"Couldn't find {baseSha} after fetching from {baseCloneUrl}:{baseRef}.");
398398
}
399399
}
400400

@@ -405,14 +405,14 @@ public async Task<string> GetPullRequestMergeBase(IRepository repo,
405405
headCommit = repo.Lookup<Commit>(headSha);
406406
if (headCommit == null)
407407
{
408-
return null;
408+
throw new NotFoundException($"Couldn't find {headSha} after fetching from {headCloneUrl}:{headRef}.");
409409
}
410410
}
411411

412412
var mergeBaseCommit = repo.ObjectDatabase.FindMergeBase(baseCommit, headCommit);
413-
if(mergeBaseCommit == null)
413+
if (mergeBaseCommit == null)
414414
{
415-
return null;
415+
throw new NotFoundException($"Couldn't find merge base between {baseCommit} and {headCommit}.");
416416
}
417417

418418
return mergeBaseCommit.Sha;

‎src/GitHub.App/Services/PullRequestService.cs‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -308,18 +308,20 @@ public IObservable<string> ExtractFile(
308308
}
309309
else
310310
{
311-
sha = await gitClient.GetPullRequestMergeBase(
312-
repo,
313-
pullRequest.Base.RepositoryCloneUrl,
314-
pullRequest.Head.RepositoryCloneUrl,
315-
pullRequest.Base.Sha,
316-
pullRequest.Head.Sha,
317-
pullRequest.Base.Ref,
318-
pullRequest.Head.Ref);
319-
320-
if (sha == null)
311+
try
321312
{
322-
throw new NotFoundException($"Couldn't find merge base between {pullRequest.Base.Sha} and {pullRequest.Head.Sha}.");
313+
sha = await gitClient.GetPullRequestMergeBase(
314+
repo,
315+
pullRequest.Base.RepositoryCloneUrl,
316+
pullRequest.Head.RepositoryCloneUrl,
317+
pullRequest.Base.Sha,
318+
pullRequest.Head.Sha,
319+
pullRequest.Base.Ref,
320+
pullRequest.Head.Ref);
321+
}
322+
catch (NotFoundException ex)
323+
{
324+
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 [email protected]", ex);
323325
}
324326
}
325327

@@ -350,7 +352,7 @@ static bool HasPreamble(string file, Encoding encoding)
350352
{
351353
foreach (var b in encoding.GetPreamble())
352354
{
353-
if(b != stream.ReadByte())
355+
if (b != stream.ReadByte())
354356
{
355357
return false;
356358
}
@@ -473,7 +475,7 @@ async Task<bool> IsBranchMarkedAsPullRequest(IRepository repo, string branchName
473475
{
474476
var prConfigKey = $"branch.{branchName}.{SettingGHfVSPullRequest}";
475477
var value = ParseGHfVSConfigKeyValue(await gitClient.GetConfig<string>(repo, prConfigKey));
476-
return value != null &&
478+
return value != null &&
477479
value.Item1 == pullRequest.Base.RepositoryCloneUrl.Owner &&
478480
value.Item2 == pullRequest.Number;
479481
}
@@ -550,4 +552,4 @@ static Tuple<string, int> ParseGHfVSConfigKeyValue(string value)
550552
return null;
551553
}
552554
}
553-
}
555+
}

‎src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
using GitHub.UI;
1717
using LibGit2Sharp;
1818
using ReactiveUI;
19+
using NLog;
1920

2021
namespace GitHub.ViewModels
2122
{
@@ -26,6 +27,8 @@ namespace GitHub.ViewModels
2627
[PartCreationPolicy(CreationPolicy.NonShared)]
2728
public class PullRequestDetailViewModel : PanePageViewModelBase, IPullRequestDetailViewModel
2829
{
30+
static readonly Logger log = LogManager.GetCurrentClassLogger();
31+
2932
readonly IModelService modelService;
3033
readonly IPullRequestService pullRequestsService;
3134
readonly IPullRequestSessionManager sessionManager;
@@ -93,7 +96,7 @@ public PullRequestDetailViewModel(
9396
Checkout = ReactiveCommand.CreateAsyncObservable(
9497
this.WhenAnyValue(x => x.CheckoutState)
9598
.Cast<CheckoutCommandState>()
96-
.Select(x => x != null && x.IsEnabled),
99+
.Select(x => x != null && x.IsEnabled),
97100
DoCheckout);
98101
Checkout.IsExecuting.Subscribe(x => isInCheckout = x);
99102
SubscribeOperationError(Checkout);
@@ -351,6 +354,7 @@ public override void Initialize(ViewWithData data)
351354
.ObserveOn(RxApp.MainThreadScheduler)
352355
.Catch<IPullRequestModel, Exception>(ex =>
353356
{
357+
log.Error("Error observing GetPullRequest", ex);
354358
ErrorMessage = ex.Message.Trim();
355359
IsLoading = IsBusy = false;
356360
return Observable.Empty<IPullRequestModel>();
@@ -460,6 +464,7 @@ public async Task Load(IRemoteRepositoryModel remoteRepository, IPullRequestMode
460464
}
461465
catch (Exception ex)
462466
{
467+
log.Error("Error loading PullRequestModel", ex);
463468
ErrorMessage = ex.Message.Trim();
464469
}
465470
finally

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,7 @@ public interface IGitClient
197197
/// <returns>
198198
/// The merge base SHA or null.
199199
/// </returns>
200+
/// <exception cref="LibGit2Sharp.NotFoundException">Thrown when the merge base can't be found.</exception>
200201
Task<string> GetPullRequestMergeBase(IRepository repo, UriString baseCloneUrl, UriString headCloneUrl, string baseSha, string headSha, string baseRef, string headRef);
201202

202203
/// Checks whether the current head is pushed to its remote tracking branch.

‎src/GitHub.InlineReviews/Services/PullRequestSessionService.cs‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -116,24 +116,26 @@ public async Task<string> GetPullRequestMergeBase(ILocalRepositoryModel reposito
116116
var key = new Tuple<string, string>(baseSha, headSha);
117117

118118
string mergeBase;
119-
if(mergeBaseCache.TryGetValue(key, out mergeBase))
119+
if (mergeBaseCache.TryGetValue(key, out mergeBase))
120120
{
121121
return mergeBase;
122122
}
123123

124124
var repo = await GetRepository(repository);
125125
var baseUrl = pullRequest.Base.RepositoryCloneUrl;
126126
var headUrl = pullRequest.Head.RepositoryCloneUrl;
127-
var headCloneUrl = pullRequest.Head.RepositoryCloneUrl;
128127
var baseRef = pullRequest.Base.Ref;
129128
var headRef = pullRequest.Head.Ref;
130-
mergeBase = await gitClient.GetPullRequestMergeBase(repo, baseUrl, headUrl, baseSha, headSha, baseRef, headRef);
131-
if (mergeBase != null)
129+
try
130+
{
131+
mergeBase = await gitClient.GetPullRequestMergeBase(repo, baseUrl, headUrl, baseSha, headSha, baseRef, headRef);
132+
}
133+
catch (NotFoundException ex)
132134
{
133-
return mergeBaseCache[key] = mergeBase;
135+
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 [email protected]", ex);
134136
}
135137

136-
throw new FileNotFoundException($"Couldn't find merge base between {baseSha} and {headSha}.");
138+
return mergeBaseCache[key] = mergeBase;
137139
}
138140

139141
/// <inheritdoc/>

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

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ public async Task PushesToDefaultOrigin()
5454

5555
await gitClient.Push(repository, "master", "origin");
5656

57-
repository.Network.Received().Push(origin,"HEAD", @"refs/heads/master", Arg.Any<PushOptions>());
57+
repository.Network.Received().Push(origin, "HEAD", @"refs/heads/master", Arg.Any<PushOptions>());
5858
}
5959

6060
[Fact]
@@ -194,7 +194,11 @@ public async Task WhenToFetch(string baseUrl, string headUrl, string baseSha, st
194194
repo.Network.Remotes.Add(null, null).ReturnsForAnyArgs(remote);
195195
var gitClient = new GitClient(Substitute.For<IGitHubCredentialProvider>());
196196

197-
await gitClient.GetPullRequestMergeBase(repo, baseUri, headUri, baseSha, headSha, baseRef, headRef);
197+
try
198+
{
199+
await gitClient.GetPullRequestMergeBase(repo, baseUri, headUri, baseSha, headSha, baseRef, headRef);
200+
}
201+
catch (NotFoundException) { /* We're interested in calls to Fetch even if it throws */ }
198202

199203
repo.Network.Received(receivedFetch).Fetch(Arg.Any<Remote>(), Arg.Any<string[]>(), Arg.Any<FetchOptions>());
200204
}
@@ -210,7 +214,11 @@ public async Task WhatToFetch(string baseSha, string headSha, string mergeBaseSh
210214
var headUrl = new UriString("https://github.com/owner/repo");
211215
var gitClient = new GitClient(Substitute.For<IGitHubCredentialProvider>());
212216

213-
await gitClient.GetPullRequestMergeBase(repo, baseUrl, headUrl, baseSha, headSha, baseRef, headRef);
217+
try
218+
{
219+
await gitClient.GetPullRequestMergeBase(repo, baseUrl, headUrl, baseSha, headSha, baseRef, headRef);
220+
}
221+
catch (NotFoundException) { /* We're interested in calls to Fetch even if it throws */ }
214222

215223
repo.Network.Received(1).Fetch(Arg.Any<Remote>(), Arg.Is<IEnumerable<string>>(x => x.Contains(expectRefSpec)), Arg.Any<FetchOptions>());
216224
}

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

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ public async Task ExtractBase_MergeBaseAvailable_UseMergeBaseSha()
5454
}
5555

5656
[Fact]
57-
public async void MergeBaseNotAvailable_ThrowsFileNotFoundException()
57+
public async void MergeBaseNotAvailable_ThrowsNotFoundException()
5858
{
5959
var baseFileContent = "baseFileContent";
6060
var headFileContent = "headFileContent";
@@ -64,11 +64,10 @@ public async void MergeBaseNotAvailable_ThrowsFileNotFoundException()
6464
var headSha = "headSha";
6565
var mergeBaseSha = null as string;
6666
var head = false;
67-
var expectMessage = $"Couldn't find merge base between {baseSha} and {headSha}.";
67+
var mergeBaseException = new NotFoundException();
6868

6969
var ex = await Assert.ThrowsAsync<NotFoundException>(() => ExtractFile(baseSha, baseFileContent, headSha, headFileContent, mergeBaseSha, mergeBaseFileContent,
70-
fileName, head, Encoding.UTF8));
71-
Assert.Equal(expectMessage, ex.Message);
70+
fileName, head, Encoding.UTF8, mergeBaseException: mergeBaseException));
7271
}
7372

7473
[Fact]
@@ -150,7 +149,8 @@ static bool HasPreamble(string file, Encoding encoding)
150149

151150
static async Task<string> ExtractFile(
152151
string baseSha, object baseFileContent, string headSha, object headFileContent, string mergeBaseSha, object mergeBaseFileContent,
153-
string fileName, bool head, Encoding encoding, string repoDir = "repoDir", int pullNumber = 666, string baseRef = "baseRef", string headRef = "headRef")
152+
string fileName, bool head, Encoding encoding, string repoDir = "repoDir", int pullNumber = 666, string baseRef = "baseRef", string headRef = "headRef",
153+
Exception mergeBaseException = null)
154154
{
155155
var repositoryModel = Substitute.For<ILocalRepositoryModel>();
156156
repositoryModel.LocalPath.Returns(repoDir);
@@ -166,7 +166,15 @@ static async Task<string> ExtractFile(
166166
var gitService = serviceProvider.GetGitService();
167167
var service = new PullRequestService(gitClient, gitService, serviceProvider.GetOperatingSystem(), Substitute.For<IUsageTracker>());
168168

169-
gitClient.GetPullRequestMergeBase(Arg.Any<IRepository>(), Arg.Any<UriString>(), Arg.Any<UriString>(), baseSha, headSha, baseRef, headRef).ReturnsForAnyArgs(Task.FromResult(mergeBaseSha));
169+
if (mergeBaseException == null)
170+
{
171+
gitClient.GetPullRequestMergeBase(Arg.Any<IRepository>(), Arg.Any<UriString>(), Arg.Any<UriString>(), baseSha, headSha, baseRef, headRef).ReturnsForAnyArgs(Task.FromResult(mergeBaseSha));
172+
}
173+
else
174+
{
175+
gitClient.GetPullRequestMergeBase(Arg.Any<IRepository>(), Arg.Any<UriString>(), Arg.Any<UriString>(), baseSha, headSha, baseRef, headRef).ReturnsForAnyArgs(Task.FromException<string>(mergeBaseException));
176+
}
177+
170178
gitClient.ExtractFile(Arg.Any<IRepository>(), mergeBaseSha, fileName).Returns(GetFileTask(mergeBaseFileContent));
171179
gitClient.ExtractFile(Arg.Any<IRepository>(), baseSha, fileName).Returns(GetFileTask(baseFileContent));
172180
gitClient.ExtractFile(Arg.Any<IRepository>(), headSha, fileName).Returns(GetFileTask(headFileContent));
@@ -561,7 +569,7 @@ static IGitService MockGitService()
561569
var branches = MockBranches("pr/123-foo1", "pr/123-foo1-2");
562570
repository.Branches.Returns(branches);
563571

564-
var result = Substitute.For<IGitService>();
572+
var result = Substitute.For<IGitService>();
565573
result.GetRepository(Arg.Any<string>()).Returns(repository);
566574
return result;
567575
}

0 commit comments

Comments
 (0)