Merge branch 'master' into refactor/pr-sessions

Author: jcansdale

docs/contributing/images/add-comment.png

@@ -26,10 +26,26 @@ If the pull request is from a fork then a remote will be added to the forked rep
 
 > Note that you cannot check out a pull request branch when your working directory has uncommitted changes. First commit or stash your changes and then refresh the Pull Request view.
 
-## Comparing files
+## Viewing Changes
 
-To compare the contents of a file in the pull request with its content on the target branch, double click a file in the **Changed Files** tree. This will open the Visual Studio diff viewer. If the pull request has been checked out, the right hand pane will be editable.
+To view the changes in the pull request for a file, double click a file in the **Changed Files** tree. This will open the Visual Studio diff viewer.
 
 ![Diff of two files in the Visual Studio diff viewer](images/pr-diff-files.png)
 
-If the pull request is checked out, right clicking on a file on the **Changed Files** tree and selecting **Open File** will open the file for editing in Visual Studio.
+You can also right-click on a file in the changed files tree to get more options:
+
+- **View Changes**: This is the default option that is also triggered when the file is double-clicked. It shows the changes to the file that are introduced by the pull request.
+- **View File**: This opens a read-only editor showing the contents of the file in the pull request.
+- **View Changes in Solution**: This menu item is only available when the pull request branch is checked out. It shows the changes in the pull request, but the right hand side of the diff is the file in the working directory. This view allows you to use Visual Studio navigation commands such as **Go to Definition (F12)**.
+- **Open File in Solution**: This menu item opens the working directory file in an editor.
+
+## Leaving Comments
+
+You can add comments to a pull request directly from Visual Studio. When a file is [open in the diff viewer](#viewing-changes) you can click the **Add Comment** icon in the margin to add a comment on a line.
+
+![Hover over margin to see add comment icon](images/hover-to-add-comment.png)
+
+Then click the icon on the desired line and leave a comment.
+![Add a comment](images/add-comment.png)
+
+Existing comments left by you or other reviewers will also show up in this margin. Click the icon to open an inline conversation view from which you can review and reply to comments:

docs/contributing/images/hover-to-add-comment.png

@@ -2,5 +2,5 @@
 
 @set path=%cd%\tools\VsixUtil;%path%
 
-vsixutil /install "%cd%\build\%Configuration%\GitHub.VisualStudio.vsix" /s Enterprise;Professional;Community
+vsixutil /install "%cd%\build\%Configuration%\GitHub.VisualStudio.vsix"
 @echo Installed %Configuration% build of GitHub for Visual Studio

docs/contributing/reviewing-a-pull-request-in-visual-studio.md

@@ -34,7 +34,7 @@ public PullRequestListViewModelDesigner()
                 Assignee = new AccountDesigner { Login = "haacked", IsUser = true },
             });
             prs.Add(new PullRequestModel(409, "Fix publish button style and a really, really long name for this thing... OMG look how long this name is yusssss",
-                new AccountDesigner { Login = "shana", IsUser = true },                
+                new AccountDesigner { Login = "shana", IsUser = true },
                 DateTimeOffset.Now - TimeSpan.FromHours(5))
             {
                 CommentCount = 27,
@@ -76,5 +76,6 @@ public PullRequestListViewModelDesigner()
 
         public ReactiveCommand<object> OpenPullRequest { get; }
         public ReactiveCommand<object> CreatePullRequest { get; }
+        public ReactiveCommand<object> OpenPullRequestOnGitHub { get; }
     }
 }

documentation/manifest.md

@@ -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}.");
                 }
             }
 

install.cmd

@@ -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)
                     {

src/GitHub.App/SampleData/PullRequestListViewModelDesigner.cs

@@ -2,7 +2,6 @@
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
 using System.ComponentModel.Composition;
-using System.Diagnostics;
 using System.Linq;
 using System.Reactive.Linq;
 using System.Reactive.Subjects;
@@ -32,6 +31,7 @@ public class PullRequestListViewModel : PanePageViewModelBase, IPullRequestListV
         readonly TrackingCollection<IAccount> trackingAuthors;
         readonly TrackingCollection<IAccount> trackingAssignees;
         readonly IPackageSettings settings;
+        readonly IVisualStudioBrowser visualStudioBrowser;
         readonly PullRequestListUIState listSettings;
         readonly bool constructing;
         IRemoteRepositoryModel remoteRepository;
@@ -40,8 +40,9 @@ public class PullRequestListViewModel : PanePageViewModelBase, IPullRequestListV
         PullRequestListViewModel(
             IConnectionRepositoryHostMap connectionRepositoryHostMap,
             ITeamExplorerServiceHolder teservice,
-            IPackageSettings settings)
-            : this(connectionRepositoryHostMap.CurrentRepositoryHost, teservice.ActiveRepo, settings)
+            IPackageSettings settings,
+            IVisualStudioBrowser visualStudioBrowser)
+            : this(connectionRepositoryHostMap.CurrentRepositoryHost, teservice.ActiveRepo, settings, visualStudioBrowser)
         {
             Guard.ArgumentNotNull(connectionRepositoryHostMap, nameof(connectionRepositoryHostMap));
             Guard.ArgumentNotNull(teservice, nameof(teservice));
@@ -51,16 +52,19 @@ public class PullRequestListViewModel : PanePageViewModelBase, IPullRequestListV
         public PullRequestListViewModel(
             IRepositoryHost repositoryHost,
             ILocalRepositoryModel repository,
-            IPackageSettings settings)
+            IPackageSettings settings,
+            IVisualStudioBrowser visualStudioBrowser)
         {
             Guard.ArgumentNotNull(repositoryHost, nameof(repositoryHost));
             Guard.ArgumentNotNull(repository, nameof(repository));
             Guard.ArgumentNotNull(settings, nameof(settings));
+            Guard.ArgumentNotNull(visualStudioBrowser, nameof(visualStudioBrowser));
 
             constructing = true;
             this.repositoryHost = repositoryHost;
             this.localRepository = repository;
             this.settings = settings;
+            this.visualStudioBrowser = visualStudioBrowser;
 
             Title = Resources.PullRequestsNavigationItemText;
 
@@ -108,6 +112,9 @@ public PullRequestListViewModel(
             CreatePullRequest = ReactiveCommand.Create();
             CreatePullRequest.Subscribe(_ => DoCreatePullRequest());
 
+            OpenPullRequestOnGitHub = ReactiveCommand.Create();
+            OpenPullRequestOnGitHub.Subscribe(x => DoOpenPullRequestOnGitHub((int)x));
+
             constructing = false;
         }
 
@@ -269,6 +276,8 @@ public IAccount EmptyUser
         public ReactiveCommand<object> OpenPullRequest { get; }
         public ReactiveCommand<object> CreatePullRequest { get; }
 
+        public ReactiveCommand<object> OpenPullRequestOnGitHub { get; }
+
         bool disposed;
         protected void Dispose(bool disposing)
         {
@@ -334,5 +343,12 @@ void DoCreatePullRequest()
             var d = new ViewWithData(UIControllerFlow.PullRequestCreation);
             navigate.OnNext(d);
         }
+
+        void DoOpenPullRequestOnGitHub(int pullRequest)
+        {
+            var repoUrl = SelectedRepository.CloneUrl.ToRepositoryUrl();
+            var url = repoUrl.Append("pull/" + pullRequest);
+            visualStudioBrowser.OpenUrl(url);
+        }
     }
 }

src/GitHub.App/Services/GitClient.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>