Return commit SHA from ResolveBlob

jcansdale · jcansdale · commit 19be6724d6c7 · 2018-07-10T10:23:41.000+01:00
The commit SHA is required when opening the specific version of blob
with `AnnotateFile`.
diff --git a/src/GitHub.App/Services/GitHubContextService.cs b/src/GitHub.App/Services/GitHubContextService.cs
@@ -249,7 +249,7 @@ public bool TryOpenFile(string repositoryDir, GitHubContext context)
             return true;
         }
 
-        public (string commitish, string path, bool isSha) ResolveBlob(string repositoryDir, GitHubContext context, string remoteName = "origin")
+        public (string commitish, string path, string commitSha) ResolveBlob(string repositoryDir, GitHubContext context, string remoteName = "origin")
         {
             Guard.ArgumentNotNull(repositoryDir, nameof(repositoryDir));
             Guard.ArgumentNotNull(context, nameof(context));
@@ -259,13 +259,13 @@ public bool TryOpenFile(string repositoryDir, GitHubContext context)
                 if (context.TreeishPath == null)
                 {
                     // Blobs without a TreeishPath aren't currently supported
-                    return (null, null, false);
+                    return (null, null, null);
                 }
 
                 if (context.BlobName == null)
                 {
                     // Not a blob
-                    return (null, null, false);
+                    return (null, null, null);
                 }
 
                 var objectishPath = $"{context.TreeishPath}/{context.BlobName}";
@@ -275,28 +275,30 @@ public bool TryOpenFile(string repositoryDir, GitHubContext context)
                 {
                     if (repository.Lookup($"{commitSha}:{pathSha}") != null)
                     {
-                        return (commitSha, pathSha, true);
+                        return (commitSha, pathSha, commitSha);
                     }
                 }
 
                 foreach (var (commitish, path) in objectish)
                 {
                     var remoteRef = $"refs/remotes/{remoteName}/{commitish}";
-                    if (repository.Lookup(remoteRef) == null)
+                    var commit = repository.Lookup(remoteRef);
+                    if (commit == null)
                     {
                         continue;
                     }
 
-                    if (repository.Lookup($"{remoteRef}:{path}") == null)
+                    var blob = repository.Lookup($"{remoteRef}:{path}");
+                    if (blob == null)
                     {
                         // Resolved commitish but not path
-                        return (remoteRef, null, false);
+                        return (remoteRef, null, commit.Sha);
                     }
 
-                    return (remoteRef, path, false);
+                    return (remoteRef, path, commit.Sha);
                 }
 
-                return (null, null, false);
+                return (null, null, null);
             }
 
             IEnumerable<(string commitish, string path)> ToObjectish(string treeishPath)
@@ -324,13 +326,13 @@ public bool HasChangesInWorkingDirectory(string repositoryDir, string commitish,
 
         public async Task<bool> TryAnnotateFile(string repositoryDir, string currentBranch, GitHubContext context)
         {
-            var (commitish, path, isSha) = ResolveBlob(repositoryDir, context);
+            var (commitish, path, commitSha) = ResolveBlob(repositoryDir, context);
             if (path == null)
             {
                 return false;
             }
 
-            if (!AnnotateFile(repositoryDir, currentBranch, path, commitish))
+            if (!AnnotateFile(repositoryDir, currentBranch, path, commitSha))
             {
                 return false;
             }
diff --git a/src/GitHub.Exports/Services/IGitHubContextService.cs b/src/GitHub.Exports/Services/IGitHubContextService.cs
@@ -14,7 +14,7 @@ public interface IGitHubContextService
         Uri ToRepositoryUrl(GitHubContext context);
         bool TryOpenFile(string repositoryDir, GitHubContext context);
         Task<bool> TryAnnotateFile(string repositoryDir, string currentBranch, GitHubContext context);
-        (string commitish, string path, bool isSha) ResolveBlob(string repositoryDir, GitHubContext context, string remoteName = "origin");
+        (string commitish, string path, string commitSha) ResolveBlob(string repositoryDir, GitHubContext context, string remoteName = "origin");
         bool HasChangesInWorkingDirectory(string repositoryDir, string commitish, string path);
     }
 }
diff --git a/test/GitHub.App.UnitTests/Services/GitHubContextServiceTests.cs b/test/GitHub.App.UnitTests/Services/GitHubContextServiceTests.cs
@@ -325,19 +325,22 @@ public void RepositoryHome(string windowTitle, string expectOwner, string expect
 
     public class TheResolveBlobMethod
     {
-        [TestCase("https://github.com/github/VisualStudio/blob/master/foo.cs", "refs/remotes/origin/master", "refs/remotes/origin/master:foo.cs", "refs/remotes/origin/master", "foo.cs")]
-        [TestCase("https://github.com/github/VisualStudio/blob/master/src/foo.cs", "refs/remotes/origin/master", "refs/remotes/origin/master:src/foo.cs", "refs/remotes/origin/master", "src/foo.cs")]
-        [TestCase("https://github.com/github/VisualStudio/blob/branch-name/src/foo.cs", "refs/remotes/origin/branch-name", "refs/remotes/origin/branch-name:src/foo.cs", "refs/remotes/origin/branch-name", "src/foo.cs")]
-        [TestCase("https://github.com/github/VisualStudio/blob/fixes/666-bug/src/foo.cs", "refs/remotes/origin/fixes/666-bug", "refs/remotes/origin/fixes/666-bug:src/foo.cs", "refs/remotes/origin/fixes/666-bug", "src/foo.cs")]
-        [TestCase("https://github.com/github/VisualStudio/blob/fixes/666-bug/A/B/foo.cs", "refs/remotes/origin/fixes/666-bug", "refs/remotes/origin/fixes/666-bug:A/B/foo.cs", "refs/remotes/origin/fixes/666-bug", "A/B/foo.cs")]
-        [TestCase("https://github.com/github/VisualStudio/blob/master/foo.cs", "refs/remotes/origin/master", null, "refs/remotes/origin/master", null, Description = "Resolve commit only")]
-        [TestCase("https://github.com/github/VisualStudio/blob/36d6b0bb6e319337180d523281c42d9611744e66/src/code.cs", "36d6b0bb6e319337180d523281c42d9611744e66", "36d6b0bb6e319337180d523281c42d9611744e66:src/code.cs", "36d6b0bb6e319337180d523281c42d9611744e66", "src/code.cs", true, Description = "Resolve commit only")]
-        [TestCase("https://github.com/github/VisualStudio/commit/8cf9a268c497adb4fc0a14572253165e179dd11e", "8cf9a268c497adb4fc0a14572253165e179dd11e", null, null, null)]
-        public void ResolveBlob(string url, string commitish, string objectish, string expectCommitish, string expectPath, bool expectIsSha = false)
+        const string CommitSha = "36d6b0bb6e319337180d523281c42d9611744e66";
+
+        [TestCase("https://github.com/github/VisualStudio/blob/master/foo.cs", "refs/remotes/origin/master", "refs/remotes/origin/master:foo.cs", "refs/remotes/origin/master", "foo.cs", CommitSha)]
+        [TestCase("https://github.com/github/VisualStudio/blob/master/src/foo.cs", "refs/remotes/origin/master", "refs/remotes/origin/master:src/foo.cs", "refs/remotes/origin/master", "src/foo.cs", CommitSha)]
+        [TestCase("https://github.com/github/VisualStudio/blob/branch-name/src/foo.cs", "refs/remotes/origin/branch-name", "refs/remotes/origin/branch-name:src/foo.cs", "refs/remotes/origin/branch-name", "src/foo.cs", CommitSha)]
+        [TestCase("https://github.com/github/VisualStudio/blob/fixes/666-bug/src/foo.cs", "refs/remotes/origin/fixes/666-bug", "refs/remotes/origin/fixes/666-bug:src/foo.cs", "refs/remotes/origin/fixes/666-bug", "src/foo.cs", CommitSha)]
+        [TestCase("https://github.com/github/VisualStudio/blob/fixes/666-bug/A/B/foo.cs", "refs/remotes/origin/fixes/666-bug", "refs/remotes/origin/fixes/666-bug:A/B/foo.cs", "refs/remotes/origin/fixes/666-bug", "A/B/foo.cs", CommitSha)]
+        [TestCase("https://github.com/github/VisualStudio/blob/master/foo.cs", "refs/remotes/origin/master", null, "refs/remotes/origin/master", null, CommitSha, Description = "Resolve commit only")]
+        [TestCase("https://github.com/github/VisualStudio/blob/36d6b0bb6e319337180d523281c42d9611744e66/src/code.cs", CommitSha, CommitSha + ":src/code.cs", CommitSha, "src/code.cs", CommitSha, Description = "Resolve commit only")]
+        [TestCase("https://github.com/github/VisualStudio/commit/8cf9a268c497adb4fc0a14572253165e179dd11e", "8cf9a268c497adb4fc0a14572253165e179dd11e", null, null, null, null)]
+        public void ResolveBlob(string url, string commitish, string objectish, string expectCommitish, string expectPath, string expectCommitSha)
         {
             var repositoryDir = "repositoryDir";
             var repository = Substitute.For<IRepository>();
             var commit = Substitute.For<Commit>();
+            commit.Sha.Returns(expectCommitSha);
             var blob = Substitute.For<Blob>();
             repository.Lookup(commitish).Returns(commit);
             repository.Lookup(objectish).Returns(blob);
@@ -349,11 +352,11 @@ public void ResolveBlob(string url, string commitish, string objectish, string e
             var target = CreateGitHubContextService(repositoryDir, repository);
             var context = target.FindContextFromUrl(url);
 
-            var (resolvedCommitish, resolvedPath, isSha) = target.ResolveBlob(repositoryDir, context);
+            var (resolvedCommitish, resolvedPath, commitSha) = target.ResolveBlob(repositoryDir, context);
 
             Assert.That(resolvedCommitish, Is.EqualTo(expectCommitish));
             Assert.That(resolvedPath, Is.EqualTo(expectPath));
-            Assert.That(isSha, Is.EqualTo(expectIsSha));
+            Assert.That(commitSha, Is.EqualTo(expectCommitSha));
         }
     }
 
diff --git a/test/GitHub.VisualStudio.UnitTests/Commands/OpenFromClipboardCommandTests.cs b/test/GitHub.VisualStudio.UnitTests/Commands/OpenFromClipboardCommandTests.cs
@@ -42,7 +42,7 @@ public async Task DifferentLocalRepository()
             var activeRepositoryName = "activeRepositoryName";
             var activeRepositoryDir = "activeRepositoryDir";
             var context = new GitHubContext { RepositoryName = targetRepositoryName };
-            (string, string, bool)? resolveBlobResult = null;
+            (string, string, string)? resolveBlobResult = null;
             var vsServices = Substitute.For<IVSServices>();
             var target = CreateOpenFromClipboardCommand(vsServices: vsServices,
                 contextFromClipboard: context, repositoryDir: activeRepositoryDir, repositoryName: activeRepositoryName, resolveBlobResult: resolveBlobResult);
@@ -57,7 +57,7 @@ public async Task CouldNotResolve()
         {
             var context = new GitHubContext();
             var repositoryDir = "repositoryDir";
-            (string, string, bool)? resolveBlobResult = null;
+            (string, string, string)? resolveBlobResult = null;
             var vsServices = Substitute.For<IVSServices>();
             var target = CreateOpenFromClipboardCommand(vsServices: vsServices,
                 contextFromClipboard: context, repositoryDir: repositoryDir, resolveBlobResult: resolveBlobResult);
@@ -72,7 +72,7 @@ public async Task CouldResolve()
         {
             var context = new GitHubContext();
             var repositoryDir = "repositoryDir";
-            var resolveBlobResult = ("master", "foo.cs", false);
+            var resolveBlobResult = ("master", "foo.cs", "");
             var vsServices = Substitute.For<IVSServices>();
             var target = CreateOpenFromClipboardCommand(vsServices: vsServices,
                 contextFromClipboard: context, repositoryDir: repositoryDir, resolveBlobResult: resolveBlobResult);
@@ -88,7 +88,7 @@ public async Task NoChangesInWorkingDirectory()
             var gitHubContextService = Substitute.For<IGitHubContextService>();
             var context = new GitHubContext();
             var repositoryDir = "repositoryDir";
-            var resolveBlobResult = ("master", "foo.cs", false);
+            var resolveBlobResult = ("master", "foo.cs", "");
             var vsServices = Substitute.For<IVSServices>();
             var target = CreateOpenFromClipboardCommand(gitHubContextService: gitHubContextService, vsServices: vsServices,
                 contextFromClipboard: context, repositoryDir: repositoryDir, resolveBlobResult: resolveBlobResult, hasChanges: false);
@@ -109,7 +109,7 @@ public async Task HasChangesInWorkingDirectory(bool annotateFileSupported, strin
             var context = new GitHubContext();
             var repositoryDir = "repositoryDir";
             var currentBranch = "currentBranch";
-            var resolveBlobResult = ("master", "foo.cs", false);
+            var resolveBlobResult = ("master", "foo.cs", "");
             var vsServices = Substitute.For<IVSServices>();
             var target = CreateOpenFromClipboardCommand(gitHubContextService: gitHubContextService, vsServices: vsServices,
                 contextFromClipboard: context, repositoryDir: repositoryDir, currentBranch: currentBranch, resolveBlobResult: resolveBlobResult, hasChanges: true);
@@ -137,7 +137,7 @@ static OpenFromClipboardCommand CreateOpenFromClipboardCommand(
             string repositoryDir = null,
             string repositoryName = null,
             string currentBranch = null,
-            (string, string, bool)? resolveBlobResult = null,
+            (string, string, string)? resolveBlobResult = null,
             bool? hasChanges = null)
         {
             var sp = Substitute.For<IServiceProvider>();

Original file line number	Diff line number	Diff line change
`@@ -249,7 +249,7 @@ public bool TryOpenFile(string repositoryDir, GitHubContext context)`
`249`	`249`	`return true;`
`250`	`250`	`}`
`251`	`251`
`252`		`- public (string commitish, string path, bool isSha) ResolveBlob(string repositoryDir, GitHubContext context, string remoteName = "origin")`
	`252`	`+ public (string commitish, string path, string commitSha) ResolveBlob(string repositoryDir, GitHubContext context, string remoteName = "origin")`
`253`	`253`	`{`
`254`	`254`	`Guard.ArgumentNotNull(repositoryDir, nameof(repositoryDir));`
`255`	`255`	`Guard.ArgumentNotNull(context, nameof(context));`
`@@ -259,13 +259,13 @@ public bool TryOpenFile(string repositoryDir, GitHubContext context)`
`259`	`259`	`if (context.TreeishPath == null)`
`260`	`260`	`{`
`261`	`261`	`// Blobs without a TreeishPath aren't currently supported`
`262`		`- return (null, null, false);`
	`262`	`+ return (null, null, null);`
`263`	`263`	`}`
`264`	`264`
`265`	`265`	`if (context.BlobName == null)`
`266`	`266`	`{`
`267`	`267`	`// Not a blob`
`268`		`- return (null, null, false);`
	`268`	`+ return (null, null, null);`
`269`	`269`	`}`
`270`	`270`
`271`	`271`	`var objectishPath = $"{context.TreeishPath}/{context.BlobName}";`
`@@ -275,28 +275,30 @@ public bool TryOpenFile(string repositoryDir, GitHubContext context)`
`275`	`275`	`{`
`276`	`276`	`if (repository.Lookup($"{commitSha}:{pathSha}") != null)`
`277`	`277`	`{`
`278`		`- return (commitSha, pathSha, true);`
	`278`	`+ return (commitSha, pathSha, commitSha);`
`279`	`279`	`}`
`280`	`280`	`}`
`281`	`281`
`282`	`282`	`foreach (var (commitish, path) in objectish)`
`283`	`283`	`{`
`284`	`284`	`var remoteRef = $"refs/remotes/{remoteName}/{commitish}";`
`285`		`- if (repository.Lookup(remoteRef) == null)`
	`285`	`+ var commit = repository.Lookup(remoteRef);`
	`286`	`+ if (commit == null)`
`286`	`287`	`{`
`287`	`288`	`continue;`
`288`	`289`	`}`
`289`	`290`
`290`		`- if (repository.Lookup($"{remoteRef}:{path}") == null)`
	`291`	`+ var blob = repository.Lookup($"{remoteRef}:{path}");`
	`292`	`+ if (blob == null)`
`291`	`293`	`{`
`292`	`294`	`// Resolved commitish but not path`
`293`		`- return (remoteRef, null, false);`
	`295`	`+ return (remoteRef, null, commit.Sha);`
`294`	`296`	`}`
`295`	`297`
`296`		`- return (remoteRef, path, false);`
	`298`	`+ return (remoteRef, path, commit.Sha);`
`297`	`299`	`}`
`298`	`300`
`299`		`- return (null, null, false);`
	`301`	`+ return (null, null, null);`
`300`	`302`	`}`
`301`	`303`
`302`	`304`	`IEnumerable<(string commitish, string path)> ToObjectish(string treeishPath)`
`@@ -324,13 +326,13 @@ public bool HasChangesInWorkingDirectory(string repositoryDir, string commitish,`
`324`	`326`
`325`	`327`	`public async Task<bool> TryAnnotateFile(string repositoryDir, string currentBranch, GitHubContext context)`
`326`	`328`	`{`
`327`		`- var (commitish, path, isSha) = ResolveBlob(repositoryDir, context);`
	`329`	`+ var (commitish, path, commitSha) = ResolveBlob(repositoryDir, context);`
`328`	`330`	`if (path == null)`
`329`	`331`	`{`
`330`	`332`	`return false;`
`331`	`333`	`}`
`332`	`334`
`333`		`- if (!AnnotateFile(repositoryDir, currentBranch, path, commitish))`
	`335`	`+ if (!AnnotateFile(repositoryDir, currentBranch, path, commitSha))`
`334`	`336`	`{`
`335`	`337`	`return false;`
`336`	`338`	`}`
Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,7 @@ public interface IGitHubContextService`
`14`	`14`	`Uri ToRepositoryUrl(GitHubContext context);`
`15`	`15`	`bool TryOpenFile(string repositoryDir, GitHubContext context);`
`16`	`16`	`Task<bool> TryAnnotateFile(string repositoryDir, string currentBranch, GitHubContext context);`
`17`		`- (string commitish, string path, bool isSha) ResolveBlob(string repositoryDir, GitHubContext context, string remoteName = "origin");`
	`17`	`+ (string commitish, string path, string commitSha) ResolveBlob(string repositoryDir, GitHubContext context, string remoteName = "origin");`
`18`	`18`	`bool HasChangesInWorkingDirectory(string repositoryDir, string commitish, string path);`
`19`	`19`	`}`
`20`	`20`	`}`