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

Commit 24dec7d

Browse files
grokysgrokys
committed
Reuse existing diff tabs.
This requires that when we extract a file to a temp file, that the same file at the same commit is always extracted to the same location. To do this, we use a hash of the file's relative path in the repository and the encoding as the temp directory name, and place the commit sha in the temp file's filename.
1 parent 1a867f8 commit 24dec7d

File tree

3 files changed

+139
-32
lines changed

3 files changed

+139
-32
lines changed

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

Lines changed: 58 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System.Reactive.Linq;
77
using System.Reactive.Threading.Tasks;
88
using System.Threading.Tasks;
9+
using EnvDTE;
910
using GitHub.Commands;
1011
using GitHub.Extensions;
1112
using GitHub.Models;
@@ -16,6 +17,7 @@
1617
using Microsoft.VisualStudio.Shell;
1718
using Microsoft.VisualStudio.Shell.Interop;
1819
using Microsoft.VisualStudio.Text;
20+
using Microsoft.VisualStudio.Text.Differencing;
1921
using Microsoft.VisualStudio.Text.Editor;
2022
using Microsoft.VisualStudio.Text.Projection;
2123
using Microsoft.VisualStudio.TextManager.Interop;
@@ -128,8 +130,6 @@ public async Task OpenDiff(IPullRequestSession session, string relativePath, str
128130
var file = await session.GetFile(relativePath, headSha ?? "HEAD");
129131
var mergeBase = await pullRequestService.GetMergeBase(session.LocalRepository, session.PullRequest);
130132
var encoding = pullRequestService.GetEncoding(session.LocalRepository, file.RelativePath);
131-
var rightPath = file.RelativePath;
132-
var leftPath = await GetBaseFileName(session, file);
133133
var rightFile = workingDirectory ?
134134
Path.Combine(session.LocalRepository.LocalPath, relativePath) :
135135
await pullRequestService.ExtractToTempFile(
@@ -138,12 +138,20 @@ await pullRequestService.ExtractToTempFile(
138138
relativePath,
139139
file.CommitSha,
140140
encoding);
141+
142+
if (FocusExistingDiffViewer(session, mergeBase, rightFile))
143+
{
144+
return;
145+
}
146+
141147
var leftFile = await pullRequestService.ExtractToTempFile(
142148
session.LocalRepository,
143149
session.PullRequest,
144150
relativePath,
145151
mergeBase,
146152
encoding);
153+
var leftPath = await GetBaseFileName(session, file);
154+
var rightPath = file.RelativePath;
147155
var leftLabel = $"{leftPath};{session.GetBaseBranchDisplay()}";
148156
var rightLabel = workingDirectory ? rightPath : $"{rightPath};PR {session.PullRequest.Number}";
149157
var caption = $"Diff - {Path.GetFileName(file.RelativePath)}";
@@ -173,9 +181,7 @@ await pullRequestService.ExtractToTempFile(
173181
(uint)options);
174182
}
175183

176-
object docView;
177-
frame.GetProperty((int)__VSFPROPID.VSFPROPID_DocView, out docView);
178-
var diffViewer = ((IVsDifferenceCodeWindow)docView).DifferenceViewer;
184+
var diffViewer = GetDiffViewer(frame);
179185

180186
AddBufferTag(diffViewer.LeftView.TextBuffer, session, leftPath, mergeBase, DiffSide.Left);
181187

@@ -384,6 +390,45 @@ IVsTextView OpenDocument(string fullPath)
384390
return view;
385391
}
386392

393+
bool FocusExistingDiffViewer(
394+
IPullRequestSession session,
395+
string mergeBase,
396+
string rightPath)
397+
{
398+
IVsUIHierarchy uiHierarchy;
399+
uint itemID;
400+
IVsWindowFrame windowFrame;
401+
402+
// Diff documents are indexed by the path on the right hand side of the comparison.
403+
if (VsShellUtilities.IsDocumentOpen(
404+
serviceProvider,
405+
rightPath,
406+
Guid.Empty,
407+
out uiHierarchy,
408+
out itemID,
409+
out windowFrame))
410+
{
411+
var diffViewer = GetDiffViewer(windowFrame);
412+
413+
if (diffViewer != null)
414+
{
415+
PullRequestTextBufferInfo leftBufferInfo;
416+
417+
if (diffViewer.LeftView.TextBuffer.Properties.TryGetProperty(
418+
typeof(PullRequestTextBufferInfo),
419+
out leftBufferInfo) &&
420+
leftBufferInfo.Session.PullRequest.Number == session.PullRequest.Number &&
421+
leftBufferInfo.CommitSha == mergeBase)
422+
{
423+
windowFrame.Show();
424+
return true;
425+
}
426+
}
427+
}
428+
429+
return false;
430+
}
431+
387432
void ShowErrorInStatusBar(string message)
388433
{
389434
statusBar.ShowMessage(message);
@@ -416,7 +461,7 @@ void AddBufferTag(
416461
}
417462
}
418463

419-
void EnableNavigateToEditor(IWpfTextView textView, IPullRequestSession session, IPullRequestSessionFile file)
464+
void EnableNavigateToEditor(ITextView textView, IPullRequestSession session, IPullRequestSessionFile file)
420465
{
421466
var view = vsEditorAdaptersFactory.GetViewAdapter(textView);
422467
EnableNavigateToEditor(view, session, file);
@@ -515,6 +560,13 @@ static string GetAbsolutePath(IPullRequestSession session, IPullRequestSessionFi
515560
return Path.Combine(session.LocalRepository.LocalPath, file.RelativePath);
516561
}
517562

563+
static IDifferenceViewer GetDiffViewer(IVsWindowFrame frame)
564+
{
565+
object docView;
566+
frame.GetProperty((int)__VSFPROPID.VSFPROPID_DocView, out docView);
567+
return (docView as IVsDifferenceCodeWindow)?.DifferenceViewer;
568+
}
569+
518570
static IDisposable OpenInProvisionalTab()
519571
{
520572
return new NewDocumentStateScope(

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

Lines changed: 56 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,17 @@
1616
using System.Collections.Generic;
1717
using LibGit2Sharp;
1818
using GitHub.Logging;
19+
using System.Security.Cryptography;
20+
using GitHub.Extensions;
21+
using Serilog;
1922

2023
namespace GitHub.Services
2124
{
2225
[Export(typeof(IPullRequestService))]
2326
[PartCreationPolicy(CreationPolicy.Shared)]
2427
public class PullRequestService : IPullRequestService
2528
{
29+
static readonly ILogger log = LogManager.ForContext<PullRequestService>();
2630
const string SettingCreatedByGHfVS = "created-by-ghfvs";
2731
const string SettingGHfVSPullRequest = "ghfvs-pr-owner-number";
2832

@@ -409,7 +413,7 @@ public IObservable<Unit> SwitchToBranch(ILocalRepositoryModel repository, IPullR
409413
{
410414
var branchName = GetLocalBranchesInternal(repository, repo, pullRequest).FirstOrDefault();
411415

412-
Log.Assert(branchName != null, "PullRequestService.SwitchToBranch called but no local branch found");
416+
log.Assert(branchName != null, "PullRequestService.SwitchToBranch called but no local branch found");
413417

414418
if (branchName != null)
415419
{
@@ -467,11 +471,18 @@ public async Task<string> ExtractToTempFile(
467471
string commitSha,
468472
Encoding encoding)
469473
{
470-
using (var repo = gitService.GetRepository(repository.LocalPath))
474+
var tempFilePath = CalculateTempFileName(relativePath, commitSha, encoding);
475+
476+
if (!File.Exists(tempFilePath))
471477
{
472-
var remote = await gitClient.GetHttpRemote(repo, "origin");
473-
return await ExtractToTempFile(repo, pullRequest.Number, commitSha, relativePath, encoding);
478+
using (var repo = gitService.GetRepository(repository.LocalPath))
479+
{
480+
var remote = await gitClient.GetHttpRemote(repo, "origin");
481+
await ExtractToTempFile(repo, pullRequest.Number, commitSha, relativePath, encoding, tempFilePath);
482+
}
474483
}
484+
485+
return tempFilePath;
475486
}
476487

477488
public Encoding GetEncoding(ILocalRepositoryModel repository, string relativePath)
@@ -562,39 +573,30 @@ string CreateUniqueRemoteName(IRepository repo, string name)
562573
return uniqueName;
563574
}
564575

565-
async Task<string> ExtractToTempFile(
576+
async Task ExtractToTempFile(
566577
IRepository repo,
567578
int pullRequestNumber,
568579
string commitSha,
569-
string fileName,
570-
Encoding encoding)
580+
string relativePath,
581+
Encoding encoding,
582+
string tempFilePath)
571583
{
572584
string contents;
573-
585+
574586
try
575587
{
576-
contents = await gitClient.ExtractFile(repo, commitSha, fileName) ?? string.Empty;
588+
contents = await gitClient.ExtractFile(repo, commitSha, relativePath) ?? string.Empty;
577589
}
578590
catch (FileNotFoundException)
579591
{
580592
var pullHeadRef = $"refs/pull/{pullRequestNumber}/head";
581593
var remote = await gitClient.GetHttpRemote(repo, "origin");
582594
await gitClient.Fetch(repo, remote.Name, commitSha, pullHeadRef);
583-
contents = await gitClient.ExtractFile(repo, commitSha, fileName) ?? string.Empty;
595+
contents = await gitClient.ExtractFile(repo, commitSha, relativePath) ?? string.Empty;
584596
}
585597

586-
return CreateTempFile(fileName, commitSha, contents, encoding);
587-
}
588-
589-
static string CreateTempFile(string fileName, string commitSha, string contents, Encoding encoding)
590-
{
591-
var tempDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
592-
var tempFileName = $"{Path.GetFileNameWithoutExtension(fileName)}@{commitSha}{Path.GetExtension(fileName)}";
593-
var tempFile = Path.Combine(tempDir, tempFileName);
594-
595-
Directory.CreateDirectory(tempDir);
596-
File.WriteAllText(tempFile, contents, encoding);
597-
return tempFile;
598+
Directory.CreateDirectory(Path.GetDirectoryName(tempFilePath));
599+
File.WriteAllText(tempFilePath, contents, encoding);
598600
}
599601

600602
IEnumerable<string> GetLocalBranchesInternal(
@@ -674,6 +676,17 @@ static string GetSafeBranchName(string name)
674676
}
675677
}
676678

679+
static string CalculateTempFileName(string relativePath, string commitSha, Encoding encoding)
680+
{
681+
// The combination of relative path, commit SHA and encoding should be sufficient to uniquely identify a file.
682+
var relativeDir = Path.GetDirectoryName(relativePath) ?? string.Empty;
683+
var key = relativeDir + '|' + encoding.WebName;
684+
var relativePathHash = GetSha256Hash(key);
685+
var tempDir = Path.Combine(Path.GetTempPath(), "GitHubVisualStudio", "FileContents", relativePathHash);
686+
var tempFileName = $"{Path.GetFileNameWithoutExtension(relativePath)}@{commitSha}{Path.GetExtension(relativePath)}";
687+
return Path.Combine(tempDir, tempFileName);
688+
}
689+
677690
static string BuildGHfVSConfigKeyValue(IPullRequestModel pullRequest)
678691
{
679692
return pullRequest.Base.RepositoryCloneUrl.Owner + '#' +
@@ -700,5 +713,26 @@ static Tuple<string, int> ParseGHfVSConfigKeyValue(string value)
700713

701714
return null;
702715
}
716+
717+
static string GetSha256Hash(string input)
718+
{
719+
Guard.ArgumentNotNull(input, nameof(input));
720+
721+
try
722+
{
723+
using (var sha256 = SHA256.Create())
724+
{
725+
var bytes = Encoding.UTF8.GetBytes(input);
726+
var hash = sha256.ComputeHash(bytes);
727+
728+
return string.Join("", hash.Select(b => b.ToString("x2", CultureInfo.InvariantCulture)));
729+
}
730+
}
731+
catch (Exception e)
732+
{
733+
log.Error(e, "IMPOSSIBLE! Generating Sha256 hash caused an exception");
734+
return null;
735+
}
736+
}
703737
}
704738
}

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

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -593,7 +593,14 @@ public async Task ExtractsExistingFile()
593593
gitClient.ExtractFile(Arg.Any<IRepository>(), "123", "filename").Returns(GetFileTask(fileContent));
594594
var file = await target.ExtractToTempFile(repository, pr, "filename", "123", Encoding.UTF8);
595595

596-
Assert.That(File.ReadAllText(file), Is.EqualTo(fileContent));
596+
try
597+
{
598+
Assert.That(File.ReadAllText(file), Is.EqualTo(fileContent));
599+
}
600+
finally
601+
{
602+
File.Delete(file);
603+
}
597604
}
598605

599606
[Test]
@@ -607,7 +614,14 @@ public async Task CreatesEmptyFileForNonExistentFile()
607614
gitClient.ExtractFile(Arg.Any<IRepository>(), "123", "filename").Returns(GetFileTask(null));
608615
var file = await target.ExtractToTempFile(repository, pr, "filename", "123", Encoding.UTF8);
609616

610-
Assert.That(File.ReadAllText(file), Is.EqualTo(string.Empty));
617+
try
618+
{
619+
Assert.That(File.ReadAllText(file), Is.EqualTo(string.Empty));
620+
}
621+
finally
622+
{
623+
File.Delete(file);
624+
}
611625
}
612626

613627
// https://github.com/github/VisualStudio/issues/1010
@@ -631,8 +645,15 @@ public async Task CanChangeEncoding(string encodingName)
631645
gitClient.ExtractFile(Arg.Any<IRepository>(), "123", "filename").Returns(GetFileTask(fileContent));
632646
var file = await target.ExtractToTempFile(repository, pr, "filename", "123", encoding);
633647

634-
Assert.That(File.ReadAllText(expectedPath), Is.EqualTo(File.ReadAllText(file)));
635-
Assert.That(File.ReadAllBytes(expectedPath), Is.EqualTo(File.ReadAllBytes(file)));
648+
try
649+
{
650+
Assert.That(File.ReadAllText(expectedPath), Is.EqualTo(File.ReadAllText(file)));
651+
Assert.That(File.ReadAllBytes(expectedPath), Is.EqualTo(File.ReadAllBytes(file)));
652+
}
653+
finally
654+
{
655+
File.Delete(file);
656+
}
636657
}
637658

638659
static IPullRequestModel CreatePullRequest()

0 commit comments

Comments
 (0)