diff --git a/src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs b/src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs index 387bfeda66..a4bd06b8a0 100644 --- a/src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs +++ b/src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs @@ -125,11 +125,6 @@ public PullRequestDetailViewModelDesigner() public Task InitializeAsync(ILocalRepositoryModel localRepository, IConnection connection, string owner, string repo, int number) => Task.CompletedTask; - public Task ExtractFile(IPullRequestFileNode file, bool head) - { - return null; - } - public string GetLocalFilePath(IPullRequestFileNode file) { return null; diff --git a/src/GitHub.App/Services/PullRequestEditorService.cs b/src/GitHub.App/Services/PullRequestEditorService.cs index 563b176f58..df1b49fb6f 100644 --- a/src/GitHub.App/Services/PullRequestEditorService.cs +++ b/src/GitHub.App/Services/PullRequestEditorService.cs @@ -71,9 +71,26 @@ public async Task OpenFile( try { - var file = await session.GetFile(relativePath); - var fullPath = GetAbsolutePath(session, file); - var fileName = workingDirectory ? fullPath : await ExtractFile(session, file, true); + var fullPath = Path.Combine(session.LocalRepository.LocalPath, relativePath); + string fileName; + string commitSha; + + if (workingDirectory) + { + fileName = fullPath; + commitSha = null; + } + else + { + var file = await session.GetFile(relativePath); + fileName = await pullRequestService.ExtractToTempFile( + session.LocalRepository, + session.PullRequest, + file.RelativePath, + file.CommitSha, + pullRequestService.GetEncoding(session.LocalRepository, file.RelativePath)); + commitSha = file.CommitSha; + } using (workingDirectory ? null : OpenInProvisionalTab()) { @@ -84,7 +101,7 @@ public async Task OpenFile( if (!workingDirectory) { - AddBufferTag(buffer, session, fullPath, null); + AddBufferTag(buffer, session, fullPath, commitSha, null); } } @@ -100,21 +117,33 @@ public async Task OpenFile( } /// - public async Task OpenDiff( - IPullRequestSession session, - string relativePath, - bool workingDirectory) + public async Task OpenDiff(IPullRequestSession session, string relativePath, string headSha) { Guard.ArgumentNotNull(session, nameof(session)); Guard.ArgumentNotEmptyString(relativePath, nameof(relativePath)); try { - var file = await session.GetFile(relativePath); + var workingDirectory = headSha == null; + var file = await session.GetFile(relativePath, headSha ?? "HEAD"); + var mergeBase = await pullRequestService.GetMergeBase(session.LocalRepository, session.PullRequest); + var encoding = pullRequestService.GetEncoding(session.LocalRepository, file.RelativePath); var rightPath = file.RelativePath; var leftPath = await GetBaseFileName(session, file); - var rightFile = workingDirectory ? GetAbsolutePath(session, file) : await ExtractFile(session, file, true); - var leftFile = await ExtractFile(session, file, false); + var rightFile = workingDirectory ? + Path.Combine(session.LocalRepository.LocalPath, relativePath) : + await pullRequestService.ExtractToTempFile( + session.LocalRepository, + session.PullRequest, + relativePath, + file.CommitSha, + encoding); + var leftFile = await pullRequestService.ExtractToTempFile( + session.LocalRepository, + session.PullRequest, + relativePath, + mergeBase, + encoding); var leftLabel = $"{leftPath};{session.GetBaseBranchDisplay()}"; var rightLabel = workingDirectory ? rightPath : $"{rightPath};PR {session.PullRequest.Number}"; var caption = $"Diff - {Path.GetFileName(file.RelativePath)}"; @@ -148,14 +177,11 @@ public async Task OpenDiff( frame.GetProperty((int)__VSFPROPID.VSFPROPID_DocView, out docView); var diffViewer = ((IVsDifferenceCodeWindow)docView).DifferenceViewer; - AddBufferTag(diffViewer.LeftView.TextBuffer, session, leftPath, DiffSide.Left); + AddBufferTag(diffViewer.LeftView.TextBuffer, session, leftPath, mergeBase, DiffSide.Left); if (!workingDirectory) { - AddBufferTag(diffViewer.RightView.TextBuffer, session, rightPath, DiffSide.Right); - EnableNavigateToEditor(diffViewer.LeftView, session, file); - EnableNavigateToEditor(diffViewer.RightView, session, file); - EnableNavigateToEditor(diffViewer.InlineView, session, file); + AddBufferTag(diffViewer.RightView.TextBuffer, session, rightPath, file.CommitSha, DiffSide.Right); } if (workingDirectory) @@ -179,7 +205,7 @@ public async Task OpenDiff( Guard.ArgumentNotEmptyString(relativePath, nameof(relativePath)); Guard.ArgumentNotNull(thread, nameof(thread)); - await OpenDiff(session, relativePath, false); + await OpenDiff(session, relativePath, thread.CommitSha); // HACK: We need to wait here for the diff view to set itself up and move its cursor // to the first changed line. There must be a better way of doing this. @@ -365,11 +391,16 @@ void ShowErrorInStatusBar(string message, Exception e) statusBar.ShowMessage(message + ": " + e.Message); } - void AddBufferTag(ITextBuffer buffer, IPullRequestSession session, string path, DiffSide? side) + void AddBufferTag( + ITextBuffer buffer, + IPullRequestSession session, + string path, + string commitSha, + DiffSide? side) { buffer.Properties.GetOrCreateSingletonProperty( typeof(PullRequestTextBufferInfo), - () => new PullRequestTextBufferInfo(session, path, side)); + () => new PullRequestTextBufferInfo(session, path, commitSha, side)); var projection = buffer as IProjectionBuffer; @@ -377,7 +408,7 @@ void AddBufferTag(ITextBuffer buffer, IPullRequestSession session, string path, { foreach (var source in projection.SourceBuffers) { - AddBufferTag(source, session, path, side); + AddBufferTag(source, session, path, commitSha, side); } } } @@ -430,19 +461,6 @@ async Task DoNavigateToEditor(IPullRequestSession session, IPullRequestSessionFi } } - async Task ExtractFile(IPullRequestSession session, IPullRequestSessionFile file, bool head) - { - var encoding = pullRequestService.GetEncoding(session.LocalRepository, file.RelativePath); - var relativePath = head ? file.RelativePath : await GetBaseFileName(session, file); - - return await pullRequestService.ExtractFile( - session.LocalRepository, - session.PullRequest, - relativePath, - head, - encoding).ToTask(); - } - ITextBuffer GetBufferAt(string filePath) { IVsUIHierarchy uiHierarchy; diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs index ce921a03e6..b0edd9bc26 100644 --- a/src/GitHub.App/Services/PullRequestService.cs +++ b/src/GitHub.App/Services/PullRequestService.cs @@ -326,6 +326,20 @@ public IObservable CalculateHistoryDivergence(ILocalRepos }); } + public async Task GetMergeBase(ILocalRepositoryModel repository, IPullRequestModel pullRequest) + { + using (var repo = gitService.GetRepository(repository.LocalPath)) + { + return await gitClient.GetPullRequestMergeBase( + repo, + pullRequest.Base.RepositoryCloneUrl, + pullRequest.Base.Sha, + pullRequest.Head.Sha, + pullRequest.Base.Ref, + pullRequest.Number); + } + } + public IObservable GetTreeChanges(ILocalRepositoryModel repository, IPullRequestModel pullRequest) { return Observable.Defer(async () => @@ -446,46 +460,18 @@ public IObservable> GetPullRequestForCurrentBranch(ILocalRepo }); } - public IObservable ExtractFile( + public async Task ExtractToTempFile( ILocalRepositoryModel repository, IPullRequestModel pullRequest, - string fileName, - bool head, + string relativePath, + string commitSha, Encoding encoding) { - return Observable.Defer(async () => + using (var repo = gitService.GetRepository(repository.LocalPath)) { - using (var repo = gitService.GetRepository(repository.LocalPath)) - { - var remote = await gitClient.GetHttpRemote(repo, "origin"); - string sha; - - if (head) - { - sha = pullRequest.Head.Sha; - } - else - { - try - { - sha = await gitClient.GetPullRequestMergeBase( - repo, - pullRequest.Base.RepositoryCloneUrl, - pullRequest.Base.Sha, - pullRequest.Head.Sha, - pullRequest.Base.Ref, - pullRequest.Number); - } - 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); - } - } - - var file = await ExtractToTempFile(repo, pullRequest.Number, sha, fileName, encoding); - return Observable.Return(file); - } - }); + var remote = await gitClient.GetHttpRemote(repo, "origin"); + return await ExtractToTempFile(repo, pullRequest.Number, commitSha, relativePath, encoding); + } } public Encoding GetEncoding(ILocalRepositoryModel repository, string relativePath) diff --git a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs index 6144bd920f..9387aaf1d6 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs @@ -486,27 +486,6 @@ public override async Task Refresh() } } - /// - /// Gets a file as it appears in the pull request. - /// - /// The changed file. - /// - /// If true, gets the file at the PR head, otherwise gets the file at the PR merge base. - /// - /// The path to a temporary file. - public Task ExtractFile(IPullRequestFileNode file, bool head) - { - var path = file.RelativePath; - var encoding = pullRequestsService.GetEncoding(LocalRepository, path); - - if (!head && file.OldPath != null) - { - path = file.OldPath; - } - - return pullRequestsService.ExtractFile(LocalRepository, model, path, head, encoding).ToTask(); - } - /// /// Gets the full path to a file in the working directory. /// diff --git a/src/GitHub.App/ViewModels/GitHubPane/PullRequestFilesViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/PullRequestFilesViewModel.cs index bad6947451..6448d906b4 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/PullRequestFilesViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/PullRequestFilesViewModel.cs @@ -44,12 +44,12 @@ public PullRequestFilesViewModel( this.service = service; DiffFile = ReactiveCommand.CreateAsyncTask(x => - editorService.OpenDiff(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, false)); + editorService.OpenDiff(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, "HEAD")); ViewFile = ReactiveCommand.CreateAsyncTask(x => editorService.OpenFile(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, false)); DiffFileWithWorkingDirectory = ReactiveCommand.CreateAsyncTask( isBranchCheckedOut, - x => editorService.OpenDiff(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, true)); + x => editorService.OpenDiff(pullRequestSession, ((IPullRequestFileNode)x).RelativePath)); OpenFileInWorkingDirectory = ReactiveCommand.CreateAsyncTask( isBranchCheckedOut, x => editorService.OpenFile(pullRequestSession, ((IPullRequestFileNode)x).RelativePath, true)); diff --git a/src/GitHub.App/ViewModels/GitHubPane/PullRequestReviewFileCommentViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/PullRequestReviewFileCommentViewModel.cs index bc6b706455..010a2d480c 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/PullRequestReviewFileCommentViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/PullRequestReviewFileCommentViewModel.cs @@ -1,6 +1,7 @@ using System; -using System.Diagnostics.CodeAnalysis; +using System.Linq; using System.Reactive; +using System.Threading.Tasks; using GitHub.Extensions; using GitHub.Models; using GitHub.Services; @@ -13,11 +14,10 @@ namespace GitHub.ViewModels.GitHubPane /// public class PullRequestReviewFileCommentViewModel : IPullRequestReviewFileCommentViewModel { - [SuppressMessage("Microsoft.Performance", "CA1823:AvoidUnusedPrivateFields", Justification = "This will be used in a later PR")] readonly IPullRequestEditorService editorService; - [SuppressMessage("Microsoft.Performance", "CA1823:AvoidUnusedPrivateFields", Justification = "This will be used in a later PR")] readonly IPullRequestSession session; readonly IPullRequestReviewCommentModel model; + IInlineCommentThreadModel thread; public PullRequestReviewFileCommentViewModel( IPullRequestEditorService editorService, @@ -31,6 +31,8 @@ public PullRequestReviewFileCommentViewModel( this.editorService = editorService; this.session = session; this.model = model; + + Open = ReactiveCommand.CreateAsyncTask(DoOpen); } /// @@ -41,5 +43,27 @@ public PullRequestReviewFileCommentViewModel( /// public ReactiveCommand Open { get; } + + async Task DoOpen(object o) + { + try + { + if (thread == null) + { + var commit = model.Position.HasValue ? model.CommitId : model.OriginalCommitId; + var file = await session.GetFile(RelativePath, commit); + thread = file.InlineCommentThreads.FirstOrDefault(t => t.Comments.Any(c => c.Id == model.Id)); + } + + if (thread != null && thread.LineNumber != -1) + { + await editorService.OpenDiff(session, RelativePath, thread); + } + } + catch (Exception) + { + // TODO: Show error. + } + } } } \ No newline at end of file diff --git a/src/GitHub.Exports.Reactive/Models/IInlineCommentThreadModel.cs b/src/GitHub.Exports.Reactive/Models/IInlineCommentThreadModel.cs index fcd2621dbc..1036eae959 100644 --- a/src/GitHub.Exports.Reactive/Models/IInlineCommentThreadModel.cs +++ b/src/GitHub.Exports.Reactive/Models/IInlineCommentThreadModel.cs @@ -36,19 +36,14 @@ public interface IInlineCommentThreadModel bool IsStale { get; set; } /// - /// Gets or sets the 0-based line number of the comment. + /// Gets or sets the 0-based line number of the comment, or -1 of the thread is outdated. /// int LineNumber { get; set; } /// - /// Gets the SHA of the commit that the thread was left con. + /// Gets the SHA of the commit that the thread appears on. /// - string OriginalCommitSha { get; } - - /// - /// Gets the 1-based line number in the original diff that the thread was left on. - /// - int OriginalPosition { get; } + string CommitSha { get; } /// /// Gets the relative path to the file that the thread is on. diff --git a/src/GitHub.Exports.Reactive/Models/IPullRequestSessionFile.cs b/src/GitHub.Exports.Reactive/Models/IPullRequestSessionFile.cs index 34bc72ccfe..18492e7ed4 100644 --- a/src/GitHub.Exports.Reactive/Models/IPullRequestSessionFile.cs +++ b/src/GitHub.Exports.Reactive/Models/IPullRequestSessionFile.cs @@ -33,6 +33,12 @@ public interface IPullRequestSessionFile : INotifyPropertyChanged /// string CommitSha { get; } + /// + /// Gets a value indicating whether is tracking the related pull + /// request HEAD or whether it is pinned at a particular commit. + /// + bool IsTrackingHead { get; } + /// /// Gets the path to the file relative to the repository. /// diff --git a/src/GitHub.Exports.Reactive/Models/PullRequestTextBufferInfo.cs b/src/GitHub.Exports.Reactive/Models/PullRequestTextBufferInfo.cs index e4bab6b4c9..704002064c 100644 --- a/src/GitHub.Exports.Reactive/Models/PullRequestTextBufferInfo.cs +++ b/src/GitHub.Exports.Reactive/Models/PullRequestTextBufferInfo.cs @@ -1,12 +1,13 @@ using System; +using GitHub.Extensions; using GitHub.Services; namespace GitHub.Models { /// /// When attached as a property to a Visual Studio ITextBuffer, informs the inline comment - /// tagger that the buffer represents a buffer opened from a pull request at the HEAD commit - /// of a pull request. + /// tagger that the buffer represents a buffer opened from a pull request at the specified + /// commit of a pull request. /// public class PullRequestTextBufferInfo { @@ -15,14 +16,21 @@ public class PullRequestTextBufferInfo /// /// The pull request session. /// The relative path to the file in the repository. + /// The SHA of the commit. /// Which side of a diff comparision the buffer represents. public PullRequestTextBufferInfo( IPullRequestSession session, string relativePath, + string commitSha, DiffSide? side) { + Guard.ArgumentNotNull(session, nameof(session)); + Guard.ArgumentNotEmptyString(relativePath, nameof(relativePath)); + Guard.ArgumentNotEmptyString(commitSha, nameof(commitSha)); + Session = session; RelativePath = relativePath; + CommitSha = commitSha; Side = side; } @@ -36,6 +44,11 @@ public PullRequestTextBufferInfo( /// public string RelativePath { get; } + /// + /// Gets the SHA of the commit. + /// + public string CommitSha { get; } + /// /// Gets a value indicating which side of a diff comparision the buffer represents. /// diff --git a/src/GitHub.Exports.Reactive/Services/IPullRequestEditorService.cs b/src/GitHub.Exports.Reactive/Services/IPullRequestEditorService.cs index b0371575af..5b859bd16f 100644 --- a/src/GitHub.Exports.Reactive/Services/IPullRequestEditorService.cs +++ b/src/GitHub.Exports.Reactive/Services/IPullRequestEditorService.cs @@ -26,12 +26,12 @@ public interface IPullRequestEditorService /// /// The pull request session. /// The path to the file, relative to the repository. - /// - /// If true the right hand side of the diff will be the current state of the file in the - /// working directory, if false it will be the HEAD commit of the pull request. + /// + /// The commit SHA of the right hand side of the diff. Pass null to compare with the + /// working directory, or "HEAD" to compare with the HEAD commit of the pull request. /// /// A task tracking the operation. - Task OpenDiff(IPullRequestSession session, string relativePath, bool workingDirectory); + Task OpenDiff(IPullRequestSession session, string relativePath, string headSha = null); /// /// Opens an diff viewer for a file in a pull request with the specified inline comment diff --git a/src/GitHub.Exports.Reactive/Services/IPullRequestService.cs b/src/GitHub.Exports.Reactive/Services/IPullRequestService.cs index 4d8ab5e19f..7ad69d7866 100644 --- a/src/GitHub.Exports.Reactive/Services/IPullRequestService.cs +++ b/src/GitHub.Exports.Reactive/Services/IPullRequestService.cs @@ -5,8 +5,6 @@ using System.Threading.Tasks; using GitHub.Models; using LibGit2Sharp; -using Octokit; -using IConnection = GitHub.Models.IConnection; namespace GitHub.Services { @@ -115,6 +113,14 @@ IObservable CreatePullRequest(IModelService modelService, /// IObservable CalculateHistoryDivergence(ILocalRepositoryModel repository, int pullRequestNumber); + /// + /// Gets the SHA of the merge base for a pull request. + /// + /// The repository. + /// The pull request details. + /// + Task GetMergeBase(ILocalRepositoryModel repository, IPullRequestModel pullRequest); + /// /// Gets the changes between the pull request base and head. /// @@ -148,19 +154,19 @@ IObservable CreatePullRequest(IModelService modelService, Encoding GetEncoding(ILocalRepositoryModel repository, string relativePath); /// - /// Gets a file as it appears in a pull request. + /// Extracts a file at the specified commit to a temporary file. /// /// The repository. /// The pull request details. - /// The filename relative to the repository root. - /// If true, gets the file at the PR head, otherwise gets the file at the PR base. + /// The path to the file, relative to the repository root. + /// The SHA of the commit. /// The encoding to use. - /// The paths of the left and right files for the diff. - IObservable ExtractFile( + /// The path to the temporary file. + Task ExtractToTempFile( ILocalRepositoryModel repository, IPullRequestModel pullRequest, - string fileName, - bool head, + string relativePath, + string commitSha, Encoding encoding); /// diff --git a/src/GitHub.Exports.Reactive/Services/IPullRequestSession.cs b/src/GitHub.Exports.Reactive/Services/IPullRequestSession.cs index a2ee33578c..eea9601f81 100644 --- a/src/GitHub.Exports.Reactive/Services/IPullRequestSession.cs +++ b/src/GitHub.Exports.Reactive/Services/IPullRequestSession.cs @@ -62,11 +62,14 @@ public interface IPullRequestSession /// Gets a file touched by the pull request. /// /// The relative path to the file. + /// + /// The commit at which to get the file contents, or "HEAD" to track the pull request head. + /// /// /// A object or null if the file was not touched by /// the pull request. /// - Task GetFile(string relativePath); + Task GetFile(string relativePath, string commitSha = "HEAD"); /// /// Gets the merge base SHA for the pull request. diff --git a/src/GitHub.Exports.Reactive/ViewModels/GitHubPane/IPullRequestDetailViewModel.cs b/src/GitHub.Exports.Reactive/ViewModels/GitHubPane/IPullRequestDetailViewModel.cs index fa091605d7..41e20eb3f3 100644 --- a/src/GitHub.Exports.Reactive/ViewModels/GitHubPane/IPullRequestDetailViewModel.cs +++ b/src/GitHub.Exports.Reactive/ViewModels/GitHubPane/IPullRequestDetailViewModel.cs @@ -190,16 +190,6 @@ Task InitializeAsync( string repo, int number); - /// - /// Gets a file as it appears in the pull request. - /// - /// The changed file. - /// - /// If true, gets the file at the PR head, otherwise gets the file at the PR merge base. - /// - /// The path to a temporary file. - Task ExtractFile(IPullRequestFileNode file, bool head); - /// /// Gets the full path to a file in the working directory. /// diff --git a/src/GitHub.InlineReviews/Models/InlineCommentThreadModel.cs b/src/GitHub.InlineReviews/Models/InlineCommentThreadModel.cs index 4a6fac84e4..4619d3c545 100644 --- a/src/GitHub.InlineReviews/Models/InlineCommentThreadModel.cs +++ b/src/GitHub.InlineReviews/Models/InlineCommentThreadModel.cs @@ -19,7 +19,7 @@ class InlineCommentThreadModel : ReactiveObject, IInlineCommentThreadModel /// Initializes a new instance of the class. /// /// The relative path to the file that the thread is on. - /// The SHA of the commit that the thread was left con. + /// The SHA of the commit that the thread appears on. /// /// The 1-based line number in the original diff that the thread was left on. /// @@ -28,20 +28,18 @@ class InlineCommentThreadModel : ReactiveObject, IInlineCommentThreadModel /// public InlineCommentThreadModel( string relativePath, - string originalCommitSha, - int originalPosition, + string commitSha, IList diffMatch, IEnumerable comments) { Guard.ArgumentNotNull(relativePath, nameof(relativePath)); - Guard.ArgumentNotNull(originalCommitSha, nameof(originalCommitSha)); + Guard.ArgumentNotNull(commitSha, nameof(commitSha)); Guard.ArgumentNotNull(diffMatch, nameof(diffMatch)); Comments = comments.ToList(); DiffMatch = diffMatch; DiffLineType = diffMatch[0].Type; - OriginalCommitSha = originalCommitSha; - OriginalPosition = originalPosition; + CommitSha = commitSha; RelativePath = relativePath; } @@ -69,10 +67,7 @@ public int LineNumber } /// - public string OriginalCommitSha { get; } - - /// - public int OriginalPosition { get; } + public string CommitSha { get; } /// public string RelativePath { get; } diff --git a/src/GitHub.InlineReviews/Models/PullRequestSessionFile.cs b/src/GitHub.InlineReviews/Models/PullRequestSessionFile.cs index d625d931f6..a38b0d22c7 100644 --- a/src/GitHub.InlineReviews/Models/PullRequestSessionFile.cs +++ b/src/GitHub.InlineReviews/Models/PullRequestSessionFile.cs @@ -18,7 +18,7 @@ namespace GitHub.InlineReviews.Models /// /// [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1001:TypesThatOwnDisposableFieldsShouldBeDisposable", - Justification = "linesChanged is sharred and shouldn't be disposed")] + Justification = "linesChanged is shared and shouldn't be disposed")] public class PullRequestSessionFile : ReactiveObject, IPullRequestSessionFile { readonly Subject>> linesChanged = new Subject>>(); @@ -32,9 +32,14 @@ public class PullRequestSessionFile : ReactiveObject, IPullRequestSessionFile /// /// The relative path to the file in the repository. /// - public PullRequestSessionFile(string relativePath) + /// + /// The commit to pin the file to, or "HEAD" to follow the pull request head. + /// + public PullRequestSessionFile(string relativePath, string commitSha = "HEAD") { RelativePath = relativePath; + this.commitSha = commitSha; + IsTrackingHead = commitSha == "HEAD"; } /// @@ -54,9 +59,24 @@ public IReadOnlyList Diff public string CommitSha { get { return commitSha; } - internal set { this.RaiseAndSetIfChanged(ref commitSha, value); } + internal set + { + if (value != commitSha) + { + if (!IsTrackingHead) + { + throw new GitHubLogicException( + "Cannot change the CommitSha of a PullRequestSessionFile that is not tracking HEAD."); + } + + this.RaiseAndSetIfChanged(ref commitSha, value); + } + } } + /// + public bool IsTrackingHead { get; } + /// public IReadOnlyList InlineCommentThreads { @@ -69,7 +89,9 @@ public IReadOnlyList InlineCommentThreads .Where(x => x.Item1 >= 0) .Distinct() .ToList(); + this.RaisePropertyChanging(); inlineCommentThreads = value; + this.RaisePropertyChanged(); NotifyLinesChanged(lines); } } diff --git a/src/GitHub.InlineReviews/Services/IPullRequestSessionService.cs b/src/GitHub.InlineReviews/Services/IPullRequestSessionService.cs index 5f82830fd9..5cd8228756 100644 --- a/src/GitHub.InlineReviews/Services/IPullRequestSessionService.cs +++ b/src/GitHub.InlineReviews/Services/IPullRequestSessionService.cs @@ -48,13 +48,15 @@ Task> Diff( /// The pull request session. /// The relative path to the file. /// The diff. + /// The SHA of the HEAD. /// /// A collection of objects with updated line numbers. /// IReadOnlyList BuildCommentThreads( IPullRequestModel pullRequest, string relativePath, - IReadOnlyList diff); + IReadOnlyList diff, + string headSha); /// /// Updates a set of comment thread models for a file based on a new diff. diff --git a/src/GitHub.InlineReviews/Services/PullRequestSession.cs b/src/GitHub.InlineReviews/Services/PullRequestSession.cs index 29680ffe15..ed9873a5b0 100644 --- a/src/GitHub.InlineReviews/Services/PullRequestSession.cs +++ b/src/GitHub.InlineReviews/Services/PullRequestSession.cs @@ -67,21 +67,23 @@ public async Task> GetAllFiles() } /// - public async Task GetFile(string relativePath) + public async Task GetFile( + string relativePath, + string commitSha = "HEAD") { await getFilesLock.WaitAsync(); try { PullRequestSessionFile file; + var normalizedPath = relativePath.Replace("\\", "/"); + var key = normalizedPath + '@' + commitSha; - relativePath = relativePath.Replace("\\", "/"); - - if (!fileIndex.TryGetValue(relativePath, out file)) + if (!fileIndex.TryGetValue(key, out file)) { - file = new PullRequestSessionFile(relativePath); + file = new PullRequestSessionFile(normalizedPath, commitSha); await UpdateFile(file); - fileIndex.Add(relativePath, file); + fileIndex.Add(key, file); } return file; @@ -174,9 +176,9 @@ async Task UpdateFile(PullRequestSessionFile file) { var mergeBaseSha = await GetMergeBase(); file.BaseSha = PullRequest.Base.Sha; - file.CommitSha = PullRequest.Head.Sha; + file.CommitSha = file.IsTrackingHead ? PullRequest.Head.Sha : file.CommitSha; file.Diff = await service.Diff(LocalRepository, mergeBaseSha, file.CommitSha, file.RelativePath); - file.InlineCommentThreads = service.BuildCommentThreads(PullRequest, file.RelativePath, file.Diff); + file.InlineCommentThreads = service.BuildCommentThreads(PullRequest, file.RelativePath, file.Diff, file.CommitSha); } async Task> CreateAllFiles() diff --git a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs index 13d2577956..50e5b0dbba 100644 --- a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs +++ b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs @@ -301,7 +301,8 @@ async Task UpdateLiveFile(PullRequestSessionLiveFile file, bool rebuildThreads) file.InlineCommentThreads = sessionService.BuildCommentThreads( session.PullRequest, file.RelativePath, - file.Diff); + file.Diff, + session.PullRequest.Head.Sha); } else { diff --git a/src/GitHub.InlineReviews/Services/PullRequestSessionService.cs b/src/GitHub.InlineReviews/Services/PullRequestSessionService.cs index 8165159d01..8b4f61cd65 100644 --- a/src/GitHub.InlineReviews/Services/PullRequestSessionService.cs +++ b/src/GitHub.InlineReviews/Services/PullRequestSessionService.cs @@ -76,7 +76,8 @@ public virtual async Task> Diff(ILocalRepositoryModel r public IReadOnlyList BuildCommentThreads( IPullRequestModel pullRequest, string relativePath, - IReadOnlyList diff) + IReadOnlyList diff, + string headSha) { relativePath = relativePath.Replace("\\", "/"); @@ -101,8 +102,7 @@ public IReadOnlyList BuildCommentThreads( var thread = new InlineCommentThreadModel( relativePath, - comments.Key.Item1, - comments.Key.Item2, + headSha, diffLines, comments); threads.Add(thread); diff --git a/src/GitHub.InlineReviews/Tags/InlineCommentTagger.cs b/src/GitHub.InlineReviews/Tags/InlineCommentTagger.cs index 41b93e82c1..4ea56eb4de 100644 --- a/src/GitHub.InlineReviews/Tags/InlineCommentTagger.cs +++ b/src/GitHub.InlineReviews/Tags/InlineCommentTagger.cs @@ -136,9 +136,10 @@ async Task Initialize() if (bufferInfo != null) { + var commitSha = bufferInfo.Side == DiffSide.Left ? "HEAD" : bufferInfo.CommitSha; session = bufferInfo.Session; relativePath = bufferInfo.RelativePath; - file = await session.GetFile(relativePath); + file = await session.GetFile(relativePath, commitSha); fileSubscription = file.LinesChanged.Subscribe(LinesChanged); side = bufferInfo.Side ?? DiffSide.Right; NotifyTagsChanged(); diff --git a/src/GitHub.InlineReviews/ViewModels/InlineCommentPeekViewModel.cs b/src/GitHub.InlineReviews/ViewModels/InlineCommentPeekViewModel.cs index 25a560f668..fdfa2f2e95 100644 --- a/src/GitHub.InlineReviews/ViewModels/InlineCommentPeekViewModel.cs +++ b/src/GitHub.InlineReviews/ViewModels/InlineCommentPeekViewModel.cs @@ -114,9 +114,10 @@ public async Task Initialize() if (info != null) { + var commitSha = info.Side == DiffSide.Left ? "HEAD" : info.CommitSha; relativePath = info.RelativePath; side = info.Side ?? DiffSide.Right; - file = await info.Session.GetFile(relativePath); + file = await info.Session.GetFile(relativePath, commitSha); session = info.Session; await UpdateThread(); } diff --git a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs index 56771a4d49..37f896fc12 100644 --- a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs @@ -19,6 +19,7 @@ using NSubstitute; using NUnit.Framework; using System.ComponentModel; +using GitHub.Api; namespace GitHub.InlineReviews.UnitTests.Services { @@ -260,7 +261,8 @@ public async Task InlineCommentThreadsIsSet() sessionService.BuildCommentThreads( target.CurrentSession.PullRequest, FilePath, - Arg.Any>()) + Arg.Any>(), + Arg.Any()) .Returns(threads); var file = await target.GetLiveFile(FilePath, textView, textView.TextBuffer); @@ -284,7 +286,8 @@ public async Task CreatesTrackingPointsForThreads() sessionService.BuildCommentThreads( target.CurrentSession.PullRequest, FilePath, - Arg.Any>()) + Arg.Any>(), + Arg.Any()) .Returns(threads); var file = (PullRequestSessionLiveFile)await target.GetLiveFile(FilePath, textView, textView.TextBuffer); @@ -307,7 +310,8 @@ public async Task MovingToNoRepositoryShouldNullOutProperties() sessionService.BuildCommentThreads( target.CurrentSession.PullRequest, FilePath, - Arg.Any>()) + Arg.Any>(), + Arg.Any()) .Returns(threads); var file = (PullRequestSessionLiveFile)await target.GetLiveFile(FilePath, textView, textView.TextBuffer); @@ -346,7 +350,8 @@ public async Task ModifyingBufferMarksThreadsAsStaleAndSignalsRebuild() sessionService.BuildCommentThreads( target.CurrentSession.PullRequest, FilePath, - Arg.Any>()) + Arg.Any>(), + Arg.Any()) .Returns(threads); var file = (PullRequestSessionLiveFile)await target.GetLiveFile(FilePath, textView, textView.TextBuffer); @@ -635,7 +640,7 @@ public async Task UpdatingCurrentSessionPullRequestTriggersLinesChanged() CreateInlineCommentThreadModel(expectedLineNumber), }; - sessionService.BuildCommentThreads(null, null, null).ReturnsForAnyArgs(threads); + sessionService.BuildCommentThreads(null, null, null, null).ReturnsForAnyArgs(threads); var target = CreateTarget(sessionService: sessionService); var file = await target.GetLiveFile(FilePath, textView, textView.TextBuffer); diff --git a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionServiceTests.cs b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionServiceTests.cs index 355c7e74ea..59c711fbdb 100644 --- a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionServiceTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionServiceTests.cs @@ -1,6 +1,7 @@ using System; using System.Linq; using System.Threading.Tasks; +using GitHub.Api; using GitHub.Factories; using GitHub.InlineReviews.Services; using GitHub.InlineReviews.UnitTests.TestDoubles; @@ -46,7 +47,8 @@ Line 2 var result = target.BuildCommentThreads( pullRequest, FilePath, - diff); + diff, + "HEAD_SHA"); var thread = result.Single(); Assert.That(2, Is.EqualTo(thread.LineNumber)); @@ -70,7 +72,8 @@ public async Task IgnoreCommentsWithNoDiffLineContext() var result = target.BuildCommentThreads( pullRequest, FilePath, - diff); + diff, + "HEAD_SHA"); Assert.That(result, Is.Empty); } @@ -105,7 +108,8 @@ Line 2 var result = target.BuildCommentThreads( pullRequest, FilePath, - diff); + diff, + "HEAD_SHA"); var thread = result.Single(); Assert.That(4, Is.EqualTo(thread.LineNumber)); @@ -145,7 +149,8 @@ Line 2 var result = target.BuildCommentThreads( pullRequest, FilePath, - diff); + diff, + "HEAD_SHA"); Assert.That(2, Is.EqualTo(result.Count)); Assert.That(-1, Is.EqualTo(result[1].LineNumber)); @@ -184,7 +189,8 @@ Line 2 var result = target.BuildCommentThreads( pullRequest, winFilePath, - diff); + diff, + "HEAD_SHA"); var thread = result.First(); Assert.That(4, Is.EqualTo(thread.LineNumber)); @@ -226,7 +232,8 @@ Line 2 var threads = target.BuildCommentThreads( pullRequest, FilePath, - diff); + diff, + "HEAD_SHA"); Assert.That(2, Is.EqualTo(threads[0].LineNumber)); @@ -271,7 +278,8 @@ Line 2 var threads = target.BuildCommentThreads( pullRequest, FilePath, - diff); + diff, + "HEAD_SHA"); threads[0].IsStale = true; var changedLines = target.UpdateCommentThreads(threads, diff); diff --git a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionTests.cs b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionTests.cs index 82be2cf01e..1794de8695 100644 --- a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionTests.cs @@ -40,7 +40,7 @@ public async Task BaseShaIsSet() } [Test] - public async Task CommitShaIsSet() + public async Task HeadCommitShaIsSet() { var target = new PullRequestSession( CreateSessionService(), @@ -52,6 +52,23 @@ public async Task CommitShaIsSet() var file = await target.GetFile(FilePath); Assert.That("HEAD_SHA", Is.SameAs(file.CommitSha)); + Assert.That(file.IsTrackingHead, Is.True); + } + + [Test] + public async Task PinnedCommitShaIsSet() + { + var target = new PullRequestSession( + CreateSessionService(), + Substitute.For(), + CreatePullRequest(), + Substitute.For(), + "owner", + true); + var file = await target.GetFile(FilePath, "123"); + + Assert.That("123", Is.SameAs(file.CommitSha)); + Assert.That(file.IsTrackingHead, Is.False); } [Test] @@ -117,6 +134,38 @@ Line 2 Assert.That(2, Is.EqualTo(thread.LineNumber)); } } + + [Test] + public async Task SameNonHeadCommitShasReturnSameFiles() + { + var target = new PullRequestSession( + CreateSessionService(), + Substitute.For(), + CreatePullRequest(), + Substitute.For(), + "owner", + true); + var file1 = await target.GetFile(FilePath, "123"); + var file2 = await target.GetFile(FilePath, "123"); + + Assert.That(file1, Is.SameAs(file2)); + } + + [Test] + public async Task DifferentCommitShasReturnDifferentFiles() + { + var target = new PullRequestSession( + CreateSessionService(), + Substitute.For(), + CreatePullRequest(), + Substitute.For(), + "owner", + true); + var file1 = await target.GetFile(FilePath, "123"); + var file2 = await target.GetFile(FilePath, "456"); + + Assert.That(file1, Is.Not.SameAs(file2)); + } } public class ThePostReviewCommentMethod @@ -201,7 +250,7 @@ public async Task UpdatesThePullRequestModel() } [Test] - public async Task AddsNewReviewCommentToThread() + public async Task AddsNewReviewCommentToThreadOnHeadFile() { var baseContents = @"Line 1 Line 2 @@ -211,7 +260,6 @@ Line 3 Line 2 Line 3 with comment Line 4"; - var comment1 = CreateComment(@"@@ -1,4 +1,4 @@ Line 1 Line 2 @@ -239,14 +287,68 @@ Line 2 "owner", true); - var file = await target.GetFile(FilePath); + var file = await target.GetFile(FilePath, "HEAD"); + + Assert.That(file.InlineCommentThreads[0].Comments, Has.Count.EqualTo(1)); + Assert.That(file.InlineCommentThreads[0].LineNumber, Is.EqualTo(2)); + + pullRequest = CreatePullRequest(comment1, comment2); + await target.Update(pullRequest); + + Assert.That(file.InlineCommentThreads[0].Comments, Has.Count.EqualTo(2)); + Assert.That(file.InlineCommentThreads[0].LineNumber, Is.EqualTo(2)); + } + } + + [Test] + public async Task AddsNewReviewCommentToThreadNonHeadFile() + { + var baseContents = @"Line 1 +Line 2 +Line 3 +Line 4"; + var headContents = @"Line 1 +Line 2 +Line 3 with comment +Line 4"; + + var comment1 = CreateComment(@"@@ -1,4 +1,4 @@ + Line 1 + Line 2 +-Line 3 ++Line 3 with comment", "Comment1"); + var comment2 = CreateComment(@"@@ -1,4 +1,4 @@ + Line 1 + Line 2 +-Line 3 ++Line 3 with comment", "Comment2"); + + using (var diffService = new FakeDiffService()) + { + var pullRequest = CreatePullRequest(comment1); + var service = CreateSessionService(diffService); + + diffService.AddFile(FilePath, baseContents, "MERGE_BASE"); + diffService.AddFile(FilePath, headContents, "123"); + + var target = new PullRequestSession( + service, + Substitute.For(), + pullRequest, + Substitute.For(), + "owner", + true); + + var file = await target.GetFile(FilePath, "123"); - Assert.That(1, Is.EqualTo(file.InlineCommentThreads[0].Comments.Count)); + Assert.That(file.InlineCommentThreads[0].Comments, Has.Count.EqualTo(1)); + Assert.That(file.InlineCommentThreads[0].LineNumber, Is.EqualTo(2)); pullRequest = CreatePullRequest(comment1, comment2); await target.Update(pullRequest); - Assert.That(2, Is.EqualTo(file.InlineCommentThreads[0].Comments.Count)); + Assert.That(file.InlineCommentThreads[0].Comments, Has.Count.EqualTo(2)); + Assert.That(file.InlineCommentThreads[0].LineNumber, Is.EqualTo(2)); } } diff --git a/test/GitHub.InlineReviews.UnitTests/Tags/InlineCommentTaggerTests.cs b/test/GitHub.InlineReviews.UnitTests/Tags/InlineCommentTaggerTests.cs index 617c9bf129..99d16f3528 100644 --- a/test/GitHub.InlineReviews.UnitTests/Tags/InlineCommentTaggerTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Tags/InlineCommentTaggerTests.cs @@ -140,6 +140,48 @@ public void ShouldRaiseTagsChangedOnFileLinesChanged() Assert.True(raised); } + [Test] + public void ShouldCallSessionGetFileWithCorrectCommitSha() + { + var sessionManager = CreateSessionManager( + CreateSessionFile(), + DiffSide.Right, + "123"); + var session = sessionManager.CurrentSession; + var target = new InlineCommentTagger( + Substitute.For(), + Substitute.For(), + sessionManager); + + // Line 11 has an add diff entry. + var span = CreateSpan(11); + var firstPass = target.GetTags(span); + var result = target.GetTags(span).ToList(); + + session.Received(1).GetFile("file.cs", "123"); + } + + [Test] + public void ShouldAlwaysCallSessionGetFileWithHeadCommitShaForLeftHandSide() + { + var sessionManager = CreateSessionManager( + CreateSessionFile(), + DiffSide.Left, + "123"); + var session = sessionManager.CurrentSession; + var target = new InlineCommentTagger( + Substitute.For(), + Substitute.For(), + sessionManager); + + // Line 11 has an add diff entry. + var span = CreateSpan(11); + var firstPass = target.GetTags(span); + var result = target.GetTags(span).ToList(); + + session.Received(1).GetFile("file.cs", "HEAD"); + } + static IPullRequestSessionFile CreateSessionFile() { var diffChunk = new DiffChunk @@ -182,13 +224,15 @@ static IPullRequestSessionManager CreateSessionManager(DiffSide side) static IPullRequestSessionManager CreateSessionManager( IPullRequestSessionFile file, - DiffSide side) + DiffSide side, + string bufferInfoCommitSha = "HEAD") { var session = Substitute.For(); - session.GetFile("file.cs").Returns(file); + session.GetFile("file.cs", bufferInfoCommitSha).Returns(file); - var info = new PullRequestTextBufferInfo(session, "file.cs", side); + var info = new PullRequestTextBufferInfo(session, "file.cs", bufferInfoCommitSha, side); var result = Substitute.For(); + result.CurrentSession.Returns(session); result.GetTextBufferInfo(null).ReturnsForAnyArgs(info); return result; } diff --git a/test/GitHub.InlineReviews.UnitTests/ViewModels/InlineCommentPeekViewModelTests.cs b/test/GitHub.InlineReviews.UnitTests/ViewModels/InlineCommentPeekViewModelTests.cs index 1f9e259067..22055a2dff 100644 --- a/test/GitHub.InlineReviews.UnitTests/ViewModels/InlineCommentPeekViewModelTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/ViewModels/InlineCommentPeekViewModelTests.cs @@ -67,6 +67,34 @@ public async Task ThreadIsCreatedForNewComment() Assert.That(CommentEditState.Editing, Is.EqualTo(target.Thread.Comments[0].EditState)); } + [Test] + public async Task ShouldGetRelativePathFromTextBufferInfoIfPresent() + { + var session = CreateSession(); + var bufferInfo = new PullRequestTextBufferInfo(session, RelativePath, "123", DiffSide.Right); + var sessionManager = CreateSessionManager( + relativePath: "ShouldNotUseThis", + session: session, + textBufferInfo: bufferInfo); + + // There is an existing comment thread at line 10. + var target = new InlineCommentPeekViewModel( + CreatePeekService(lineNumber: 10), + CreatePeekSession(), + sessionManager, + Substitute.For(), + Substitute.For()); + + await target.Initialize(); + + // There should be an existing comment and a reply placeholder. + Assert.That(target.Thread, Is.InstanceOf(typeof(InlineCommentThreadViewModel))); + Assert.That(2, Is.EqualTo(target.Thread.Comments.Count)); + Assert.That("Existing comment", Is.EqualTo(target.Thread.Comments[0].Body)); + Assert.That(string.Empty, Is.EqualTo(target.Thread.Comments[1].Body)); + Assert.That(CommentEditState.Placeholder, Is.EqualTo(target.Thread.Comments[1].EditState)); + } + [Test] public async Task SwitchesFromNewThreadToExistingThreadWhenCommentPosted() { @@ -259,7 +287,18 @@ IPeekSession CreatePeekSession() return result; } - IPullRequestSessionManager CreateSessionManager(string commitSha = "COMMIT") + IPullRequestSession CreateSession() + { + var result = Substitute.For(); + result.LocalRepository.CloneUrl.Returns(new UriString("https://foo.bar")); + return result; + } + + IPullRequestSessionManager CreateSessionManager( + string commitSha = "COMMIT", + string relativePath = RelativePath, + IPullRequestSession session = null, + PullRequestTextBufferInfo textBufferInfo = null) { var thread = CreateThread(10, "Existing comment"); @@ -286,13 +325,18 @@ IPullRequestSessionManager CreateSessionManager(string commitSha = "COMMIT") file.InlineCommentThreads.Returns(new[] { thread }); file.LinesChanged.Returns(new Subject>>()); - var session = Substitute.For(); - session.LocalRepository.CloneUrl.Returns(new UriString("https://foo.bar")); + session = session ?? CreateSession(); + + if (textBufferInfo != null) + { + session.GetFile(textBufferInfo.RelativePath, textBufferInfo.CommitSha).Returns(file); + } var result = Substitute.For(); result.CurrentSession.Returns(session); - result.GetLiveFile(RelativePath, Arg.Any(), Arg.Any()).Returns(file); - result.GetRelativePath(Arg.Any()).Returns(RelativePath); + result.GetLiveFile(relativePath, Arg.Any(), Arg.Any()).Returns(file); + result.GetRelativePath(Arg.Any()).Returns(relativePath); + result.GetTextBufferInfo(Arg.Any()).Returns(textBufferInfo); return result; } diff --git a/test/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs b/test/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs index 63dc45e80c..dcb7535d8c 100644 --- a/test/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs +++ b/test/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs @@ -579,203 +579,66 @@ static ILocalRepositoryModel CreateLocalRepositoryModel(Repository repo) static Signature Author => new Signature("foo", "foo@bar.com", DateTimeOffset.Now); - public class TheExtractFileMethod + public class TheExtractToTempFileMethod { [Test] - public async Task ExtractHead() + public async Task ExtractsExistingFile() { - var baseFileContent = "baseFileContent"; - var headFileContent = "headFileContent"; - var fileName = "fileName"; - var baseSha = "baseSha"; - var headSha = "headSha"; - var head = true; - - var file = await ExtractFile(baseSha, baseFileContent, headSha, headFileContent, baseSha, baseFileContent, - fileName, head, Encoding.UTF8); + var gitClient = MockGitClient(); + var target = CreateTarget(gitClient); + var repository = Substitute.For(); + var fileContent = "file content"; + var pr = CreatePullRequest(); - Assert.That(headFileContent, Is.EqualTo(File.ReadAllText(file))); - } + gitClient.ExtractFile(Arg.Any(), "123", "filename").Returns(GetFileTask(fileContent)); + var file = await target.ExtractToTempFile(repository, pr, "filename", "123", Encoding.UTF8); - [Test] - public async Task ExtractBase_MergeBaseAvailable_UseMergeBaseSha() - { - var baseFileContent = "baseFileContent"; - var headFileContent = "headFileContent"; - var mergeBaseFileContent = "mergeBaseFileContent"; - var fileName = "fileName"; - var baseSha = "baseSha"; - var headSha = "headSha"; - var mergeBaseSha = "mergeBaseSha"; - var head = false; - - var file = await ExtractFile(baseSha, baseFileContent, headSha, headFileContent, mergeBaseSha, mergeBaseFileContent, - fileName, head, Encoding.UTF8); - - Assert.That(mergeBaseFileContent, Is.EqualTo(File.ReadAllText(file))); - } - - [Test] - public void MergeBaseNotAvailable_ThrowsNotFoundException() - { - var baseFileContent = "baseFileContent"; - var headFileContent = "headFileContent"; - var mergeBaseFileContent = null as string; - var fileName = "fileName"; - var baseSha = "baseSha"; - var headSha = "headSha"; - var mergeBaseSha = null as string; - var head = false; - var mergeBaseException = new NotFoundException(); - - var ex = Assert.ThrowsAsync(() => ExtractFile(baseSha, baseFileContent, headSha, headFileContent, mergeBaseSha, mergeBaseFileContent, - fileName, head, Encoding.UTF8, mergeBaseException: mergeBaseException)); + Assert.That(File.ReadAllText(file), Is.EqualTo(fileContent)); } [Test] - public async Task FileAdded_BaseFileEmpty() + public async Task CreatesEmptyFileForNonExistentFile() { - var baseFileContent = null as string; - var headFileContent = "headFileContent"; - var fileName = "fileName"; - var baseSha = "baseSha"; - var headSha = "headSha"; - var head = false; - - var file = await ExtractFile(baseSha, baseFileContent, headSha, headFileContent, baseSha, baseFileContent, - fileName, head, Encoding.UTF8); + var gitClient = MockGitClient(); + var target = CreateTarget(gitClient); + var repository = Substitute.For(); + var pr = CreatePullRequest(); - Assert.That(string.Empty, Is.EqualTo(File.ReadAllText(file))); - } + gitClient.ExtractFile(Arg.Any(), "123", "filename").Returns(GetFileTask(null)); + var file = await target.ExtractToTempFile(repository, pr, "filename", "123", Encoding.UTF8); - [Test] - public async Task FileDeleted_HeadFileEmpty() - { - var baseFileContent = "baseFileContent"; - var headFileContent = null as string; - var fileName = "fileName"; - var baseSha = "baseSha"; - var headSha = "headSha"; - var baseRef = new GitReferenceModel("ref", "label", baseSha, "uri"); - var headRef = new GitReferenceModel("ref", "label", headSha, "uri"); - var head = true; - - var file = await ExtractFile(baseSha, baseFileContent, headSha, headFileContent, baseSha, baseFileContent, - fileName, head, Encoding.UTF8); - - Assert.That(string.Empty, Is.EqualTo(File.ReadAllText(file))); + Assert.That(File.ReadAllText(file), Is.EqualTo(string.Empty)); } // https://github.com/github/VisualStudio/issues/1010 [TestCase("utf-8")] // Unicode (UTF-8) [TestCase("Windows-1252")] // Western European (Windows) - public async Task ChangeEncoding(string encodingName) + public async Task CanChangeEncoding(string encodingName) { var encoding = Encoding.GetEncoding(encodingName); var repoDir = Path.GetTempPath(); - var baseFileContent = "baseFileContent"; - var headFileContent = null as string; var fileName = "fileName.txt"; - var baseSha = "baseSha"; - var headSha = "headSha"; - var baseRef = new GitReferenceModel("ref", "label", baseSha, "uri"); - var head = false; - - var file = await ExtractFile(baseSha, baseFileContent, headSha, headFileContent, - baseSha, baseFileContent, fileName, head, encoding, repoDir); + var fileContent = "file content"; + var gitClient = MockGitClient(); + var target = CreateTarget(gitClient); + var repository = Substitute.For(); + var pr = CreatePullRequest(); var expectedPath = Path.Combine(repoDir, fileName); - var expectedContent = baseFileContent; + var expectedContent = fileContent; File.WriteAllText(expectedPath, expectedContent, encoding); + gitClient.ExtractFile(Arg.Any(), "123", "filename").Returns(GetFileTask(fileContent)); + var file = await target.ExtractToTempFile(repository, pr, "filename", "123", encoding); + Assert.That(File.ReadAllText(expectedPath), Is.EqualTo(File.ReadAllText(file))); Assert.That(File.ReadAllBytes(expectedPath), Is.EqualTo(File.ReadAllBytes(file))); } - static bool HasPreamble(string file, Encoding encoding) - { - using (var stream = File.OpenRead(file)) - { - foreach (var b in encoding.GetPreamble()) - { - if (b != stream.ReadByte()) - { - return false; - } - } - } - - return true; - } - - static async Task 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", - Exception mergeBaseException = null) + static IPullRequestModel CreatePullRequest() { - var repositoryModel = Substitute.For(); - repositoryModel.LocalPath.Returns(repoDir); - - var pullRequest = Substitute.For(); - pullRequest.Number.Returns(1); - - pullRequest.Base.Returns(new GitReferenceModel(baseRef, "label", baseSha, "uri")); - pullRequest.Head.Returns(new GitReferenceModel("ref", "label", headSha, "uri")); - - var serviceProvider = Substitutes.ServiceProvider; - var gitClient = MockGitClient(); - var gitService = serviceProvider.GetGitService(); - var service = new PullRequestService(gitClient, gitService, serviceProvider.GetOperatingSystem(), Substitute.For()); - - if (mergeBaseException == null) - { - gitClient.GetPullRequestMergeBase(Arg.Any(), Arg.Any(), baseSha, headSha, baseRef, pullNumber).ReturnsForAnyArgs(Task.FromResult(mergeBaseSha)); - } - else - { - gitClient.GetPullRequestMergeBase(Arg.Any(), Arg.Any(), baseSha, headSha, baseRef, pullNumber).ReturnsForAnyArgs(Task.FromException(mergeBaseException)); - } - - gitClient.ExtractFile(Arg.Any(), mergeBaseSha, fileName).Returns(GetFileTask(mergeBaseFileContent)); - gitClient.ExtractFile(Arg.Any(), baseSha, fileName).Returns(GetFileTask(baseFileContent)); - gitClient.ExtractFile(Arg.Any(), headSha, fileName).Returns(GetFileTask(headFileContent)); - - return await service.ExtractFile(repositoryModel, pullRequest, fileName, head, encoding); - } - - static IObservable GetFileObservable(object fileOrException) - { - if (fileOrException is string) - { - return Observable.Return((string)fileOrException); - } - - if (fileOrException is Exception) - { - return Observable.Throw((Exception)fileOrException); - } - - return Observable.Throw(new FileNotFoundException()); - } - - static Task GetFileTask(object content) - { - if (content is string) - { - return Task.FromResult((string)content); - } - - if (content is Exception) - { - return Task.FromException((Exception)content); - } - - if (content == null) - { - return Task.FromResult(null); - } - - throw new ArgumentException("Unsupported content type: " + content); + var result = Substitute.For(); + return result; } } @@ -1105,6 +968,24 @@ public async Task ShouldRemoveUnusedRemote() } } + static PullRequestService CreateTarget( + IGitClient gitClient = null, + IGitService gitService = null, + IOperatingSystem os = null, + IUsageTracker usageTracker = null) + { + gitClient = gitClient ?? Substitute.For(); + gitService = gitService ?? Substitute.For(); + os = os ?? Substitute.For(); + usageTracker = usageTracker ?? Substitute.For(); + + return new PullRequestService( + gitClient, + gitService, + os, + usageTracker); + } + static BranchCollection MockBranches(params string[] names) { var result = Substitute.For(); @@ -1138,4 +1019,24 @@ static IGitService MockGitService() result.GetRepository(Arg.Any()).Returns(repository); return result; } + + static Task GetFileTask(object content) + { + if (content is string) + { + return Task.FromResult((string)content); + } + + if (content is Exception) + { + return Task.FromException((Exception)content); + } + + if (content == null) + { + return Task.FromResult(null); + } + + throw new ArgumentException("Unsupported content type: " + content); + } }