From 80abca23a146a2e92d8904d7cb64a209b1adf5da Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Oct 2018 09:30:15 +0100 Subject: [PATCH 01/20] Remove IGitService dependency from RepositoryModel Refactor the constructor that took IGitService into LocalRepositoryModel where it was used. --- .../Models/LocalRepositoryModel.cs | 15 +++++++++---- src/GitHub.Exports/Models/RepositoryModel.cs | 22 ++----------------- 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/src/GitHub.Exports/Models/LocalRepositoryModel.cs b/src/GitHub.Exports/Models/LocalRepositoryModel.cs index fe07ef5ab8..a1e26fe939 100644 --- a/src/GitHub.Exports/Models/LocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/LocalRepositoryModel.cs @@ -41,15 +41,22 @@ public LocalRepositoryModel(string name, UriString cloneUrl, string localPath, I /// /// Initializes a new instance of the class. /// - /// The repository's local path. + /// The repository's local path. /// The service used to find the repository's URL. - public LocalRepositoryModel(string path, IGitService gitService) - : base(path, gitService) + public LocalRepositoryModel(string localPath, IGitService gitService) { Guard.ArgumentNotNull(gitService, nameof(gitService)); + Guard.ArgumentNotNull(localPath, nameof(localPath)); + var dir = new DirectoryInfo(localPath); + if (!dir.Exists) + { + throw new ArgumentException("Path does not exist", nameof(localPath)); + } + CloneUrl = gitService.GetUri(localPath); + LocalPath = localPath; + Name = CloneUrl?.RepositoryName ?? dir.Name; this.gitService = gitService; - LocalPath = path; Icon = Octicon.repo; } diff --git a/src/GitHub.Exports/Models/RepositoryModel.cs b/src/GitHub.Exports/Models/RepositoryModel.cs index 3ec99829b5..f8cfc718d6 100644 --- a/src/GitHub.Exports/Models/RepositoryModel.cs +++ b/src/GitHub.Exports/Models/RepositoryModel.cs @@ -1,9 +1,7 @@ using System; using System.Diagnostics; -using System.IO; using GitHub.Extensions; using GitHub.Primitives; -using GitHub.Services; using GitHub.UI; namespace GitHub.Models @@ -33,30 +31,14 @@ public RepositoryModel( CloneUrl = cloneUrl; } - /// - /// Initializes a new instance of the class. - /// - /// - /// The path to the local repository from which repository name and clone URL will be - /// extracted. - /// - /// The service used to find the repository's and . - protected RepositoryModel(string path, IGitService gitService) + protected RepositoryModel() { - Guard.ArgumentNotNull(path, nameof(path)); - - var dir = new DirectoryInfo(path); - if (!dir.Exists) - throw new ArgumentException("Path does not exist", nameof(path)); - var uri = gitService.GetUri(path); - Name = uri?.RepositoryName ?? dir.Name; - CloneUrl = gitService.GetUri(path); } /// /// Gets the name of the repository. /// - public string Name { get; } + public string Name { get; protected set; } /// /// Gets the repository clone URL. From 1177e933de4275615f2ab5d3e2bff8d4d7c68f07 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Oct 2018 11:45:14 +0100 Subject: [PATCH 02/20] Move LocalRepositoryModel ctor into GitService 1/2 --- .../SampleData/GitServiceDesigner.cs | 2 + .../PullRequestCreationViewModelDesigner.cs | 16 +++++--- src/GitHub.Exports/Models/BranchModel.cs | 3 +- .../Models/LocalRepositoryModel.cs | 28 +++---------- src/GitHub.Exports/Models/RepositoryModel.cs | 6 +-- src/GitHub.Exports/Services/GitService.cs | 39 ++++++++++++++++--- src/GitHub.Exports/Services/IGitService.cs | 7 ++++ .../RegistryHelper.cs | 3 +- .../Services/LocalRepositoryModelFactory.cs | 2 +- .../Commands/LinkCommandBase.cs | 6 +-- 10 files changed, 70 insertions(+), 42 deletions(-) diff --git a/src/GitHub.App/SampleData/GitServiceDesigner.cs b/src/GitHub.App/SampleData/GitServiceDesigner.cs index de81803439..1c8392730b 100644 --- a/src/GitHub.App/SampleData/GitServiceDesigner.cs +++ b/src/GitHub.App/SampleData/GitServiceDesigner.cs @@ -1,4 +1,5 @@ using System.Threading.Tasks; +using GitHub.Models; using GitHub.Primitives; using GitHub.Services; using LibGit2Sharp; @@ -7,6 +8,7 @@ namespace GitHub.SampleData { class GitServiceDesigner : IGitService { + public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) => null; public Task GetLatestPushedSha(string path, string remote = "origin") => Task.FromResult(null); public UriString GetRemoteUri(IRepository repo, string remote = "origin") => null; public IRepository GetRepository(string path) => null; diff --git a/src/GitHub.App/SampleData/PullRequestCreationViewModelDesigner.cs b/src/GitHub.App/SampleData/PullRequestCreationViewModelDesigner.cs index 99f05b86f9..816e814b10 100644 --- a/src/GitHub.App/SampleData/PullRequestCreationViewModelDesigner.cs +++ b/src/GitHub.App/SampleData/PullRequestCreationViewModelDesigner.cs @@ -15,15 +15,21 @@ public class PullRequestCreationViewModelDesigner : PanePageViewModelBase, IPull { public PullRequestCreationViewModelDesigner() { + var repositoryModel = new LocalRepositoryModel + { + Name = "repo", + CloneUrl = "http://github.com/user/repo" + }; + Branches = new List { - new BranchModel("master", new LocalRepositoryModel("http://github.com/user/repo", new GitServiceDesigner())), - new BranchModel("don/stub-ui", new LocalRepositoryModel("http://github.com/user/repo", new GitServiceDesigner())), - new BranchModel("feature/pr/views", new LocalRepositoryModel("http://github.com/user/repo", new GitServiceDesigner())), - new BranchModel("release-1.0.17.0", new LocalRepositoryModel("http://github.com/user/repo", new GitServiceDesigner())), + new BranchModel("master", repositoryModel), + new BranchModel("don/stub-ui", repositoryModel), + new BranchModel("feature/pr/views", repositoryModel), + new BranchModel("release-1.0.17.0", repositoryModel), }.AsReadOnly(); - TargetBranch = new BranchModel("master", new LocalRepositoryModel("http://github.com/user/repo", new GitServiceDesigner())); + TargetBranch = new BranchModel("master", repositoryModel); SourceBranch = Branches[2]; SelectedAssignee = "Haacked (Phil Haack)"; diff --git a/src/GitHub.Exports/Models/BranchModel.cs b/src/GitHub.Exports/Models/BranchModel.cs index 9a02091871..b61bc61853 100644 --- a/src/GitHub.Exports/Models/BranchModel.cs +++ b/src/GitHub.Exports/Models/BranchModel.cs @@ -32,7 +32,8 @@ public BranchModel(LibGit2Sharp.Branch branch, IRepositoryModel repo, IGitServic Extensions.Guard.ArgumentNotNull(repo, nameof(repo)); Name = DisplayName = branch.FriendlyName; #pragma warning disable 0618 // TODO: Replace `Branch.Remote` with `Repository.Network.Remotes[branch.RemoteName]`. - Repository = branch.IsRemote ? new LocalRepositoryModel(branch.Remote.Url, gitService) : repo; + // NOTE: This method expects a localPath not a URL! + Repository = branch.IsRemote ? gitService.CreateLocalRepositoryModel(branch.Remote.Url) : repo; #pragma warning restore 0618 IsTracking = branch.IsTracking; Sha = branch.Tip?.Sha; diff --git a/src/GitHub.Exports/Models/LocalRepositoryModel.cs b/src/GitHub.Exports/Models/LocalRepositoryModel.cs index a1e26fe939..ebfedecadd 100644 --- a/src/GitHub.Exports/Models/LocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/LocalRepositoryModel.cs @@ -20,6 +20,10 @@ public class LocalRepositoryModel : RepositoryModel, ILocalRepositoryModel, IEqu { readonly IGitService gitService; + public LocalRepositoryModel() + { + } + /// /// Initializes a new instance of the class. /// @@ -38,28 +42,6 @@ public LocalRepositoryModel(string name, UriString cloneUrl, string localPath, I Icon = Octicon.repo; } - /// - /// Initializes a new instance of the class. - /// - /// The repository's local path. - /// The service used to find the repository's URL. - public LocalRepositoryModel(string localPath, IGitService gitService) - { - Guard.ArgumentNotNull(gitService, nameof(gitService)); - Guard.ArgumentNotNull(localPath, nameof(localPath)); - var dir = new DirectoryInfo(localPath); - if (!dir.Exists) - { - throw new ArgumentException("Path does not exist", nameof(localPath)); - } - - CloneUrl = gitService.GetUri(localPath); - LocalPath = localPath; - Name = CloneUrl?.RepositoryName ?? dir.Name; - this.gitService = gitService; - Icon = Octicon.repo; - } - /// /// Updates the clone URL from the local repository. /// @@ -164,7 +146,7 @@ static string GetLinkFormat(LinkType linkType) /// /// Gets the local path of the repository. /// - public string LocalPath { get; } + public string LocalPath { get; set; } /// /// Gets the head SHA of the repository. diff --git a/src/GitHub.Exports/Models/RepositoryModel.cs b/src/GitHub.Exports/Models/RepositoryModel.cs index f8cfc718d6..fe3976de98 100644 --- a/src/GitHub.Exports/Models/RepositoryModel.cs +++ b/src/GitHub.Exports/Models/RepositoryModel.cs @@ -38,7 +38,7 @@ protected RepositoryModel() /// /// Gets the name of the repository. /// - public string Name { get; protected set; } + public string Name { get; set; } /// /// Gets the repository clone URL. @@ -46,7 +46,7 @@ protected RepositoryModel() public UriString CloneUrl { get { return cloneUrl; } - protected set + set { if (cloneUrl != value) { @@ -67,7 +67,7 @@ protected set public Octicon Icon { get { return icon; } - protected set + set { if (icon != value) { diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index a02700f972..83e3f7a489 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -1,11 +1,13 @@ -using System.ComponentModel.Composition; -using GitHub.Primitives; -using LibGit2Sharp; -using System; +using System; +using System.IO; +using System.Linq; using System.Threading.Tasks; +using System.ComponentModel.Composition; +using GitHub.UI; using GitHub.Models; -using System.Linq; +using GitHub.Primitives; using GitHub.Extensions; +using LibGit2Sharp; namespace GitHub.Services { @@ -21,6 +23,33 @@ public GitService(IRepositoryFacade repositoryFacade) this.repositoryFacade = repositoryFacade; } + /// + /// Initializes a new instance of the class. + /// + /// The repository's local path. + public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) + { + Guard.ArgumentNotNull(localPath, nameof(localPath)); + + var dir = new DirectoryInfo(localPath); + if (!dir.Exists) + { + throw new ArgumentException("Path does not exist", nameof(localPath)); + } + + var cloneUrl = GetUri(localPath); + var name = cloneUrl?.RepositoryName ?? dir.Name; + var icon = Octicon.repo; + + return new LocalRepositoryModel + { + LocalPath = localPath, + CloneUrl = cloneUrl, + Name = name, + Icon = icon + }; + } + /// /// Returns the URL of the remote for the specified . If the repository /// is null or no remote named origin exists, this method returns null diff --git a/src/GitHub.Exports/Services/IGitService.cs b/src/GitHub.Exports/Services/IGitService.cs index 91453ad792..40002f939f 100644 --- a/src/GitHub.Exports/Services/IGitService.cs +++ b/src/GitHub.Exports/Services/IGitService.cs @@ -7,6 +7,13 @@ namespace GitHub.Services { public interface IGitService { + /// + /// Initializes a new instance of the class. + /// + /// The repository's local path. + /// A repository model. + ILocalRepositoryModel CreateLocalRepositoryModel(string localPath); + /// /// Returns the URL of the remote for the specified . If the repository /// is null or no remote exists, this method returns null diff --git a/src/GitHub.TeamFoundation.14/RegistryHelper.cs b/src/GitHub.TeamFoundation.14/RegistryHelper.cs index f4fa44eb31..dafba2e15f 100644 --- a/src/GitHub.TeamFoundation.14/RegistryHelper.cs +++ b/src/GitHub.TeamFoundation.14/RegistryHelper.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.ComponentModel.Composition; using System.Diagnostics; using System.Globalization; using System.IO; @@ -40,7 +41,7 @@ internal static IEnumerable PokeTheRegistryForRepositoryL { var path = subkey?.GetValue("Path") as string; if (path != null && Directory.Exists(path)) - return new LocalRepositoryModel(path, GitService.GitServiceHelper); + return GitService.GitServiceHelper.CreateLocalRepositoryModel(path); } catch (Exception) { diff --git a/src/GitHub.TeamFoundation.14/Services/LocalRepositoryModelFactory.cs b/src/GitHub.TeamFoundation.14/Services/LocalRepositoryModelFactory.cs index f6d7dc4a54..06c4bc9572 100644 --- a/src/GitHub.TeamFoundation.14/Services/LocalRepositoryModelFactory.cs +++ b/src/GitHub.TeamFoundation.14/Services/LocalRepositoryModelFactory.cs @@ -7,7 +7,7 @@ class LocalRepositoryModelFactory : ILocalRepositoryModelFactory { public ILocalRepositoryModel Create(string localPath) { - return new LocalRepositoryModel(localPath, GitService.GitServiceHelper); + return GitService.GitServiceHelper.CreateLocalRepositoryModel(localPath); } } } diff --git a/src/GitHub.VisualStudio/Commands/LinkCommandBase.cs b/src/GitHub.VisualStudio/Commands/LinkCommandBase.cs index 7326e306b8..ddc442541c 100644 --- a/src/GitHub.VisualStudio/Commands/LinkCommandBase.cs +++ b/src/GitHub.VisualStudio/Commands/LinkCommandBase.cs @@ -46,7 +46,7 @@ protected ILocalRepositoryModel GetRepositoryByPath(string path) if (!string.IsNullOrEmpty(path)) { var repo = ServiceProvider.TryGetService().GetRepository(path); - return new LocalRepositoryModel(repo.Info.WorkingDirectory.TrimEnd('\\'), GitService.GitServiceHelper); + return GitService.GitServiceHelper.CreateLocalRepositoryModel(repo.Info.WorkingDirectory.TrimEnd('\\')); } } catch (Exception ex) @@ -66,7 +66,7 @@ protected ILocalRepositoryModel GetActiveRepo() var path = ServiceProvider.TryGetService()?.GetActiveRepoPath() ?? String.Empty; try { - activeRepo = !string.IsNullOrEmpty(path) ? new LocalRepositoryModel(path, GitService.GitServiceHelper) : null; + activeRepo = !string.IsNullOrEmpty(path) ? GitService.GitServiceHelper.CreateLocalRepositoryModel(path) : null; } catch (Exception ex) { @@ -86,7 +86,7 @@ void RefreshRepo() string path = vsGitServices?.GetActiveRepoPath() ?? String.Empty; try { - ActiveRepo = !String.IsNullOrEmpty(path) ? new LocalRepositoryModel(path, GitService.GitServiceHelper) : null; + ActiveRepo = !String.IsNullOrEmpty(path) ? GitService.GitServiceHelper.CreateLocalRepositoryModel(path) : null; } catch (Exception ex) { From c6c8b849816a296aef900f548edce3ea1ba6ec5d Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Oct 2018 11:56:31 +0100 Subject: [PATCH 03/20] Move LocalRepositoryModel ctor into GitService 2/2 --- .../SampleData/GitServiceDesigner.cs | 1 + .../Models/LocalRepositoryModel.cs | 18 --------------- src/GitHub.Exports/Services/GitService.cs | 23 +++++++++++++++++++ src/GitHub.Exports/Services/IGitService.cs | 9 ++++++++ 4 files changed, 33 insertions(+), 18 deletions(-) diff --git a/src/GitHub.App/SampleData/GitServiceDesigner.cs b/src/GitHub.App/SampleData/GitServiceDesigner.cs index 1c8392730b..b2f10226d2 100644 --- a/src/GitHub.App/SampleData/GitServiceDesigner.cs +++ b/src/GitHub.App/SampleData/GitServiceDesigner.cs @@ -9,6 +9,7 @@ namespace GitHub.SampleData class GitServiceDesigner : IGitService { public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) => null; + public ILocalRepositoryModel CreateLocalRepositoryModel(string name, UriString cloneUrl, string localPath) => null; public Task GetLatestPushedSha(string path, string remote = "origin") => Task.FromResult(null); public UriString GetRemoteUri(IRepository repo, string remote = "origin") => null; public IRepository GetRepository(string path) => null; diff --git a/src/GitHub.Exports/Models/LocalRepositoryModel.cs b/src/GitHub.Exports/Models/LocalRepositoryModel.cs index ebfedecadd..7b05f77463 100644 --- a/src/GitHub.Exports/Models/LocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/LocalRepositoryModel.cs @@ -24,24 +24,6 @@ public LocalRepositoryModel() { } - /// - /// Initializes a new instance of the class. - /// - /// The repository name. - /// The repository's clone URL. - /// The repository's local path. - /// The service used to refresh the repository's URL. - public LocalRepositoryModel(string name, UriString cloneUrl, string localPath, IGitService gitService) - : base(name, cloneUrl) - { - Guard.ArgumentNotEmptyString(localPath, nameof(localPath)); - Guard.ArgumentNotNull(gitService, nameof(gitService)); - - this.gitService = gitService; - LocalPath = localPath; - Icon = Octicon.repo; - } - /// /// Updates the clone URL from the local repository. /// diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index 83e3f7a489..3c19afd813 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -27,6 +27,7 @@ public GitService(IRepositoryFacade repositoryFacade) /// Initializes a new instance of the class. /// /// The repository's local path. + /// A repository model. public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) { Guard.ArgumentNotNull(localPath, nameof(localPath)); @@ -50,6 +51,28 @@ public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) }; } + /// + /// Initializes a new instance of the class. + /// + /// The repository name. + /// The repository's clone URL. + /// The repository's local path. + /// A repository model. + public ILocalRepositoryModel CreateLocalRepositoryModel(string name, UriString cloneUrl, string localPath) + { + Guard.ArgumentNotEmptyString(name, nameof(name)); + Guard.ArgumentNotNull(cloneUrl, nameof(cloneUrl)); + Guard.ArgumentNotEmptyString(localPath, nameof(localPath)); + + return new LocalRepositoryModel + { + LocalPath = localPath, + CloneUrl = cloneUrl, + Name = name, + Icon = Octicon.repo + }; + } + /// /// Returns the URL of the remote for the specified . If the repository /// is null or no remote named origin exists, this method returns null diff --git a/src/GitHub.Exports/Services/IGitService.cs b/src/GitHub.Exports/Services/IGitService.cs index 40002f939f..9dd4348edb 100644 --- a/src/GitHub.Exports/Services/IGitService.cs +++ b/src/GitHub.Exports/Services/IGitService.cs @@ -14,6 +14,15 @@ public interface IGitService /// A repository model. ILocalRepositoryModel CreateLocalRepositoryModel(string localPath); + /// + /// Initializes a new instance of the class. + /// + /// The repository name. + /// The repository's clone URL. + /// The repository's local path. + /// A repository model. + ILocalRepositoryModel CreateLocalRepositoryModel(string name, UriString cloneUrl, string localPath); + /// /// Returns the URL of the remote for the specified . If the repository /// is null or no remote exists, this method returns null From edb823260e1621a4eee70c6d098a6efc4ecc7983 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Oct 2018 12:31:43 +0100 Subject: [PATCH 04/20] Fix tests after LocalRepositoryModel refactor --- .../Models/RepositoryModelTests.cs | 12 ++++++------ .../Services/PullRequestServiceTests.cs | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/GitHub.App.UnitTests/Models/RepositoryModelTests.cs b/test/GitHub.App.UnitTests/Models/RepositoryModelTests.cs index fc048b5c95..ef3219bdbb 100644 --- a/test/GitHub.App.UnitTests/Models/RepositoryModelTests.cs +++ b/test/GitHub.App.UnitTests/Models/RepositoryModelTests.cs @@ -20,8 +20,8 @@ public class ComparisonTests : TestBaseClass public void SameContentEqualsTrue(string name1, string url1, string path1, string name2, string url2, string path2) { var gitService = Substitute.For(); - var a = new LocalRepositoryModel(name1, new UriString(url1), path1, gitService); - var b = new LocalRepositoryModel(name2, new UriString(url2), path2, gitService); + var a = new LocalRepositoryModel { Name = name1, CloneUrl = url1, LocalPath = path1 }; + var b = new LocalRepositoryModel { Name = name2, CloneUrl = url2, LocalPath = path2 }; Assert.That(a, Is.EqualTo(b)); Assert.False(a == b); Assert.That(a.GetHashCode(), Is.EqualTo(b.GetHashCode())); @@ -58,11 +58,11 @@ public void NoRemoteUrl() { using (var temp = new TempDirectory()) { - var gitService = Substitute.For(); + var gitService = new GitService(new RepositoryFacade()); var repo = Substitute.For(); var path = temp.Directory.CreateSubdirectory("repo-name"); gitService.GetUri(path.FullName).Returns((UriString)null); - var model = new LocalRepositoryModel(path.FullName, gitService); + var model = gitService.CreateLocalRepositoryModel(path.FullName); Assert.That("repo-name", Is.EqualTo(model.Name)); } } @@ -72,11 +72,11 @@ public void WithRemoteUrl() { using (var temp = new TempDirectory()) { - var gitService = Substitute.For(); + var gitService = new GitService(new RepositoryFacade()); var repo = Substitute.For(); var path = temp.Directory.CreateSubdirectory("repo-name"); gitService.GetUri(path.FullName).Returns(new UriString("https://github.com/user/repo-name")); - var model = new LocalRepositoryModel(path.FullName, gitService); + var model = gitService.CreateLocalRepositoryModel(path.FullName); Assert.That("repo-name", Is.EqualTo(model.Name)); Assert.That("user", Is.EqualTo(model.Owner)); } diff --git a/test/GitHub.App.UnitTests/Services/PullRequestServiceTests.cs b/test/GitHub.App.UnitTests/Services/PullRequestServiceTests.cs index c8e437cb8d..2ede44920b 100644 --- a/test/GitHub.App.UnitTests/Services/PullRequestServiceTests.cs +++ b/test/GitHub.App.UnitTests/Services/PullRequestServiceTests.cs @@ -701,10 +701,10 @@ public void CreatePullRequestAllArgsMandatory() ms = Substitute.For(); Assert.Throws(() => service.CreatePullRequest(ms, sourceRepo, targetRepo, source, target, title, body)); - sourceRepo = new LocalRepositoryModel("name", new GitHub.Primitives.UriString("http://github.com/github/stuff"), "c:\\path", gitService); + sourceRepo = new LocalRepositoryModel { Name = "name", CloneUrl = "http://github.com/github/stuff", LocalPath = "c:\\path" }; Assert.Throws(() => service.CreatePullRequest(ms, sourceRepo, targetRepo, source, target, title, body)); - targetRepo = new LocalRepositoryModel("name", new GitHub.Primitives.UriString("http://github.com/github/stuff"), "c:\\path", gitService); + targetRepo = new LocalRepositoryModel { Name = "name", CloneUrl = "http://github.com/github/stuff", LocalPath = "c:\\path" }; Assert.Throws(() => service.CreatePullRequest(ms, sourceRepo, targetRepo, source, target, title, body)); title = "a title"; From 835e5617396a290777ed09d7a021d9690db98d64 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Oct 2018 14:39:45 +0100 Subject: [PATCH 05/20] Move GenerateUrl to LinkCommandBase Move GenerateUrl from LocalRepositoryModel to LinkCommandBase. --- .../Models/ILocalRepositoryModel.cs | 11 -- .../Models/LocalRepositoryModel.cs | 91 ---------------- .../Commands/BlameLinkCommand.cs | 4 +- .../Commands/CopyLinkCommand.cs | 4 +- .../Commands/LinkCommandBase.cs | 100 +++++++++++++++++- .../Commands/OpenLinkCommand.cs | 4 +- .../Commands/LinkCommandBaseTests.cs} | 33 ++---- 7 files changed, 113 insertions(+), 134 deletions(-) rename test/{GitHub.Exports.UnitTests/LocalRepositoryModelTests.cs => GitHub.VisualStudio.UnitTests/Commands/LinkCommandBaseTests.cs} (81%) diff --git a/src/GitHub.Exports/Models/ILocalRepositoryModel.cs b/src/GitHub.Exports/Models/ILocalRepositoryModel.cs index 1295599b6a..f3fa4afcd3 100644 --- a/src/GitHub.Exports/Models/ILocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/ILocalRepositoryModel.cs @@ -24,16 +24,5 @@ public interface ILocalRepositoryModel : IRepositoryModel, INotifyPropertyChange /// Updates the url information based on the local path /// void Refresh(); - - /// - /// Generates a http(s) url to the repository in the remote server, optionally - /// pointing to a specific file and specific line range in it. - /// - /// The type of repository link to create - /// The file to generate an url to. Optional. - /// A specific line, or (if specifying the as well) the start of a range - /// The end of a line range on the specified file. - /// An UriString with the generated url, or null if the repository has no remote server configured or if it can't be found locally - Task GenerateUrl(LinkType linkType, string path = null, int startLine = -1, int endLine = -1); } } diff --git a/src/GitHub.Exports/Models/LocalRepositoryModel.cs b/src/GitHub.Exports/Models/LocalRepositoryModel.cs index 7b05f77463..6e3d34e7ee 100644 --- a/src/GitHub.Exports/Models/LocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/LocalRepositoryModel.cs @@ -34,97 +34,6 @@ public void Refresh() CloneUrl = gitService.GetUri(LocalPath); } - /// - /// Generates a http(s) url to the repository in the remote server, optionally - /// pointing to a specific file and specific line range in it. - /// - /// Type of link to generate - /// The file to generate an url to. Optional. - /// A specific line, or (if specifying the as well) the start of a range - /// The end of a line range on the specified file. - /// An UriString with the generated url, or null if the repository has no remote server configured or if it can't be found locally - public async Task GenerateUrl(LinkType linkType, string path = null, int startLine = -1, int endLine = -1) - { - if (CloneUrl == null) - return null; - - var sha = await gitService.GetLatestPushedSha(path ?? LocalPath); - // this also incidentally checks whether the repo has a valid LocalPath - if (String.IsNullOrEmpty(sha)) - return CloneUrl.ToRepositoryUrl().AbsoluteUri; - - if (path != null && Path.IsPathRooted(path)) - { - // if the path root doesn't match the repository local path, then ignore it - if (!path.StartsWith(LocalPath, StringComparison.OrdinalIgnoreCase)) - { - Debug.Assert(false, String.Format(CultureInfo.CurrentCulture, "GenerateUrl: path {0} doesn't match repository {1}", path, LocalPath)); - path = null; - } - else - path = path.Substring(LocalPath.Length + 1); - } - - if (startLine > 0 && endLine > 0 && startLine > endLine) - { - // if startLine is greater than endLine and both are set, swap them - var temp = startLine; - startLine = endLine; - endLine = temp; - } - - if (startLine == endLine) - { - // if startLine is the same as endLine don't generate a range link - endLine = -1; - } - - return new UriString(GenerateUrl(linkType, CloneUrl.ToRepositoryUrl().AbsoluteUri, sha, path, startLine, endLine)); - } - - const string CommitFormat = "{0}/commit/{1}"; - const string BlobFormat = "{0}/blob/{1}/{2}"; - const string BlameFormat = "{0}/blame/{1}/{2}"; - const string StartLineFormat = "{0}#L{1}"; - const string EndLineFormat = "{0}-L{1}"; - static string GenerateUrl(LinkType linkType, string basePath, string sha, string path, int startLine = -1, int endLine = -1) - { - if (sha == null) - return basePath; - - if (String.IsNullOrEmpty(path)) - return String.Format(CultureInfo.InvariantCulture, CommitFormat, basePath, sha); - - var ret = String.Format(CultureInfo.InvariantCulture, GetLinkFormat(linkType), basePath, sha, path.Replace(@"\", "/")); - - if (startLine < 0) - return ret; - ret = String.Format(CultureInfo.InvariantCulture, StartLineFormat, ret, startLine); - if (endLine < 0) - return ret; - return String.Format(CultureInfo.InvariantCulture, EndLineFormat, ret, endLine); - } - - /// - /// Selects the proper format for the link type, defaults to the blob url when link type is not selected. - /// - /// Type of link to generate - /// The string format of the selected link type - static string GetLinkFormat(LinkType linkType) - { - switch (linkType) - { - case LinkType.Blame: - return BlameFormat; - - case LinkType.Blob: - return BlobFormat; - - default: - return BlobFormat; - } - } - /// /// Gets the local path of the repository. /// diff --git a/src/GitHub.VisualStudio/Commands/BlameLinkCommand.cs b/src/GitHub.VisualStudio/Commands/BlameLinkCommand.cs index 0d55d951be..40d0352dab 100644 --- a/src/GitHub.VisualStudio/Commands/BlameLinkCommand.cs +++ b/src/GitHub.VisualStudio/Commands/BlameLinkCommand.cs @@ -15,8 +15,8 @@ namespace GitHub.VisualStudio.Commands public class BlameLinkCommand : LinkCommandBase, IBlameLinkCommand { [ImportingConstructor] - protected BlameLinkCommand(IGitHubServiceProvider serviceProvider) - : base(CommandSet, CommandId, serviceProvider) + protected BlameLinkCommand(IGitHubServiceProvider serviceProvider, Lazy gitService) + : base(CommandSet, CommandId, serviceProvider, gitService) { } diff --git a/src/GitHub.VisualStudio/Commands/CopyLinkCommand.cs b/src/GitHub.VisualStudio/Commands/CopyLinkCommand.cs index c01aedfdfc..b195d110cf 100644 --- a/src/GitHub.VisualStudio/Commands/CopyLinkCommand.cs +++ b/src/GitHub.VisualStudio/Commands/CopyLinkCommand.cs @@ -17,8 +17,8 @@ namespace GitHub.VisualStudio.Commands public class CopyLinkCommand : LinkCommandBase, ICopyLinkCommand { [ImportingConstructor] - protected CopyLinkCommand(IGitHubServiceProvider serviceProvider) - : base(CommandSet, CommandId, serviceProvider) + protected CopyLinkCommand(IGitHubServiceProvider serviceProvider, Lazy gitService) + : base(CommandSet, CommandId, serviceProvider, gitService) { } diff --git a/src/GitHub.VisualStudio/Commands/LinkCommandBase.cs b/src/GitHub.VisualStudio/Commands/LinkCommandBase.cs index ddc442541c..d1abe4f1d4 100644 --- a/src/GitHub.VisualStudio/Commands/LinkCommandBase.cs +++ b/src/GitHub.VisualStudio/Commands/LinkCommandBase.cs @@ -1,4 +1,5 @@ using System; +using System.Globalization; using System.IO; using System.Threading.Tasks; using GitHub.Api; @@ -20,16 +21,19 @@ public abstract class LinkCommandBase : VsCommand static readonly ILogger log = LogManager.ForContext(); readonly Lazy apiFactory; readonly Lazy usageTracker; + readonly Lazy lazyGitService; protected LinkCommandBase( Guid commandSet, int commandId, - IGitHubServiceProvider serviceProvider) + IGitHubServiceProvider serviceProvider, + Lazy gitService) : base(commandSet, commandId) { ServiceProvider = serviceProvider; apiFactory = new Lazy(() => ServiceProvider.TryGetService()); usageTracker = new Lazy(() => serviceProvider.TryGetService()); + lazyGitService = gitService; } protected ILocalRepositoryModel ActiveRepo { get; private set; } @@ -134,7 +138,7 @@ protected Task GenerateLink(LinkType linkType) var repo = GetRepositoryByPath(activeDocument.Name); - return repo.GenerateUrl(linkType, activeDocument.Name, activeDocument.StartLine, activeDocument.EndLine); + return GenerateUrl(lazyGitService.Value, repo, linkType, activeDocument.Name, activeDocument.StartLine, activeDocument.EndLine); } protected override void QueryStatus() @@ -160,5 +164,97 @@ public static bool IsFileDescendantOfDirectory(string file, string directory) } return false; } + + /// + /// Generates a http(s) url to the repository in the remote server, optionally + /// pointing to a specific file and specific line range in it. + /// + /// Type of link to generate + /// The file to generate an url to. Optional. + /// A specific line, or (if specifying the as well) the start of a range + /// The end of a line range on the specified file. + /// An UriString with the generated url, or null if the repository has no remote server configured or if it can't be found locally + public static async Task GenerateUrl(IGitService gitService, + ILocalRepositoryModel repo, LinkType linkType, string path = null, int startLine = -1, int endLine = -1) + { + if (repo.CloneUrl == null) + return null; + + var sha = await gitService.GetLatestPushedSha(path ?? repo.LocalPath); + // this also incidentally checks whether the repo has a valid LocalPath + if (String.IsNullOrEmpty(sha)) + return repo.CloneUrl.ToRepositoryUrl().AbsoluteUri; + + if (path != null && Path.IsPathRooted(path)) + { + // if the path root doesn't match the repository local path, then ignore it + if (!path.StartsWith(repo.LocalPath, StringComparison.OrdinalIgnoreCase)) + { + System.Diagnostics.Debug.Assert(false, String.Format(CultureInfo.CurrentCulture, "GenerateUrl: path {0} doesn't match repository {1}", path, repo.LocalPath)); + path = null; + } + else + path = path.Substring(repo.LocalPath.Length + 1); + } + + if (startLine > 0 && endLine > 0 && startLine > endLine) + { + // if startLine is greater than endLine and both are set, swap them + var temp = startLine; + startLine = endLine; + endLine = temp; + } + + if (startLine == endLine) + { + // if startLine is the same as endLine don't generate a range link + endLine = -1; + } + + return new UriString(GenerateUrl(linkType, repo.CloneUrl.ToRepositoryUrl().AbsoluteUri, sha, path, startLine, endLine)); + } + + const string CommitFormat = "{0}/commit/{1}"; + const string BlobFormat = "{0}/blob/{1}/{2}"; + const string BlameFormat = "{0}/blame/{1}/{2}"; + const string StartLineFormat = "{0}#L{1}"; + const string EndLineFormat = "{0}-L{1}"; + static string GenerateUrl(LinkType linkType, string basePath, string sha, string path, int startLine = -1, int endLine = -1) + { + if (sha == null) + return basePath; + + if (String.IsNullOrEmpty(path)) + return String.Format(CultureInfo.InvariantCulture, CommitFormat, basePath, sha); + + var ret = String.Format(CultureInfo.InvariantCulture, GetLinkFormat(linkType), basePath, sha, path.Replace(@"\", "/")); + + if (startLine < 0) + return ret; + ret = String.Format(CultureInfo.InvariantCulture, StartLineFormat, ret, startLine); + if (endLine < 0) + return ret; + return String.Format(CultureInfo.InvariantCulture, EndLineFormat, ret, endLine); + } + + /// + /// Selects the proper format for the link type, defaults to the blob url when link type is not selected. + /// + /// Type of link to generate + /// The string format of the selected link type + static string GetLinkFormat(LinkType linkType) + { + switch (linkType) + { + case LinkType.Blame: + return BlameFormat; + + case LinkType.Blob: + return BlobFormat; + + default: + return BlobFormat; + } + } } } diff --git a/src/GitHub.VisualStudio/Commands/OpenLinkCommand.cs b/src/GitHub.VisualStudio/Commands/OpenLinkCommand.cs index 5d5ddc7142..3c00747ca2 100644 --- a/src/GitHub.VisualStudio/Commands/OpenLinkCommand.cs +++ b/src/GitHub.VisualStudio/Commands/OpenLinkCommand.cs @@ -14,8 +14,8 @@ namespace GitHub.VisualStudio.Commands public class OpenLinkCommand : LinkCommandBase, IOpenLinkCommand { [ImportingConstructor] - protected OpenLinkCommand(IGitHubServiceProvider serviceProvider) - : base(CommandSet, CommandId, serviceProvider) + protected OpenLinkCommand(IGitHubServiceProvider serviceProvider, Lazy gitService) + : base(CommandSet, CommandId, serviceProvider, gitService) { } diff --git a/test/GitHub.Exports.UnitTests/LocalRepositoryModelTests.cs b/test/GitHub.VisualStudio.UnitTests/Commands/LinkCommandBaseTests.cs similarity index 81% rename from test/GitHub.Exports.UnitTests/LocalRepositoryModelTests.cs rename to test/GitHub.VisualStudio.UnitTests/Commands/LinkCommandBaseTests.cs index cb2c163b61..8be88301a7 100644 --- a/test/GitHub.Exports.UnitTests/LocalRepositoryModelTests.cs +++ b/test/GitHub.VisualStudio.UnitTests/Commands/LinkCommandBaseTests.cs @@ -1,14 +1,13 @@ -using System.Threading.Tasks; -using System.Collections.Generic; +using System; +using System.Threading.Tasks; using GitHub.Models; using GitHub.Exports; using GitHub.Services; -using GitHub.Primitives; +using GitHub.VisualStudio.Commands; using NSubstitute; -using LibGit2Sharp; using NUnit.Framework; -public class LocalRepositoryModelTests : TestBaseClass +public class LinkCommandBaseTests : TestBaseClass { [TestCase(1, LinkType.Blob, false, "https://github.com/foo/bar", "123123", @"src\dir\file1.cs", -1, -1, "https://github.com/foo/bar/blob/123123/src/dir/file1.cs")] [TestCase(2, LinkType.Blob, false, "https://github.com/foo/bar", "123123", @"src\dir\file1.cs", 1, -1, "https://github.com/foo/bar/blob/123123/src/dir/file1.cs#L1")] @@ -48,32 +47,18 @@ public async Task GenerateUrl(int testid, LinkType linkType, bool createRootedPa if (createRootedPath && path != null) path = System.IO.Path.Combine(basePath.FullName, path); ILocalRepositoryModel model = null; - if (!string.IsNullOrEmpty(baseUrl)) - model = new LocalRepositoryModel("bar", new UriString(baseUrl), basePath.FullName, gitService); - else - model = new LocalRepositoryModel(basePath.FullName, gitService); - var result = await model.GenerateUrl(linkType, path, startLine, endLine); - Assert.That(expected, Is.EqualTo(result?.ToString())); + model = new LocalRepositoryModel { Name = "bar", CloneUrl = baseUrl, LocalPath = basePath.FullName }; + + var result = await LinkCommandBase.GenerateUrl(gitService, model, linkType, path, startLine, endLine); + + Assert.That(result?.ToString(), Is.EqualTo(expected)); } } static IGitService CreateGitService(string sha) { var gitservice = Substitute.For(); - var repo = Substitute.For(); - gitservice.GetRepository(Args.String).Returns(repo); gitservice.GetLatestPushedSha(Args.String).Returns(Task.FromResult(sha)); - if (!string.IsNullOrEmpty(sha)) - { - var refs = Substitute.For(); - var refrence = Substitute.For(); - refs.ReachableFrom(Arg.Any>(), Arg.Any>()).Returns(new Reference[] { refrence }); - repo.Refs.Returns(refs); - var commit = Substitute.For(); - commit.Sha.Returns(sha); - repo.Commits.Returns(new FakeCommitLog { commit }); - } - return gitservice; } } From fe2dcf861a71d41ab75a139b2d8a20625173c6b3 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Oct 2018 15:12:07 +0100 Subject: [PATCH 06/20] Convert LocalRepositoryModel to GitService tests Move LocalRepositoryModel construction tests to new home. --- .../Models/RepositoryModelTests.cs | 33 ------------- .../GitServiceTests.cs | 49 +++++++++++++++++-- 2 files changed, 45 insertions(+), 37 deletions(-) diff --git a/test/GitHub.App.UnitTests/Models/RepositoryModelTests.cs b/test/GitHub.App.UnitTests/Models/RepositoryModelTests.cs index ef3219bdbb..fd2b9a574c 100644 --- a/test/GitHub.App.UnitTests/Models/RepositoryModelTests.cs +++ b/test/GitHub.App.UnitTests/Models/RepositoryModelTests.cs @@ -50,39 +50,6 @@ public void DifferentContentEqualsFalse(long id1, string name1, string url1, lon } } - //[Collection("PackageServiceProvider global data tests")] - public class PathConstructorTests : TestBaseClass - { - [Test] - public void NoRemoteUrl() - { - using (var temp = new TempDirectory()) - { - var gitService = new GitService(new RepositoryFacade()); - var repo = Substitute.For(); - var path = temp.Directory.CreateSubdirectory("repo-name"); - gitService.GetUri(path.FullName).Returns((UriString)null); - var model = gitService.CreateLocalRepositoryModel(path.FullName); - Assert.That("repo-name", Is.EqualTo(model.Name)); - } - } - - [Test] - public void WithRemoteUrl() - { - using (var temp = new TempDirectory()) - { - var gitService = new GitService(new RepositoryFacade()); - var repo = Substitute.For(); - var path = temp.Directory.CreateSubdirectory("repo-name"); - gitService.GetUri(path.FullName).Returns(new UriString("https://github.com/user/repo-name")); - var model = gitService.CreateLocalRepositoryModel(path.FullName); - Assert.That("repo-name", Is.EqualTo(model.Name)); - Assert.That("user", Is.EqualTo(model.Owner)); - } - } - } - public class HostAddressTests : TestBaseClass { [TestCase("https://github.com/owner/repo")] diff --git a/test/GitHub.Exports.UnitTests/GitServiceTests.cs b/test/GitHub.Exports.UnitTests/GitServiceTests.cs index 1b2ac321ba..c98db18f97 100644 --- a/test/GitHub.Exports.UnitTests/GitServiceTests.cs +++ b/test/GitHub.Exports.UnitTests/GitServiceTests.cs @@ -1,7 +1,5 @@ using System; -using System.Collections.Generic; using System.IO; -using System.Linq; using System.Threading.Tasks; using GitHub.Services; using LibGit2Sharp; @@ -10,6 +8,41 @@ public class GitServiceTests { + public class CreateLocalRepositoryModelTests : TestBaseClass + { + [Test] + public void NoRemoteUrl() + { + using (var temp = new TempDirectory()) + { + var repositoryFacade = Substitute.For(); + var gitService = new GitService(repositoryFacade); + var path = temp.Directory.CreateSubdirectory("repo-name"); + + var model = gitService.CreateLocalRepositoryModel(path.FullName); + + Assert.That(model.Name, Is.EqualTo("repo-name")); + } + } + + [Test] + public void WithRemoteUrl() + { + using (var temp = new TempDirectory()) + { + var path = temp.Directory.CreateSubdirectory("repo-name"); + var repository = CreateRepositoryWithOrigin("https://github.com/user/repo-name"); + var repositoryFacade = CreateRepositoryFacade(path.FullName, repository); + var gitService = new GitService(repositoryFacade); + + var model = gitService.CreateLocalRepositoryModel(path.FullName); + + Assert.That(model.Name, Is.EqualTo("repo-name")); + Assert.That(model.Owner, Is.EqualTo("user")); + } + } + } + [TestCase("asdf", null)] [TestCase("", null)] [TestCase(null, null)] @@ -47,12 +80,20 @@ static IRepositoryFacade CreateRepositoryFacade(string path, IRepository repo) return repositoryFacade; } - static IRepository CreateRepository() + static IRepository CreateRepositoryWithOrigin(string originUrl) { - var repo = Substitute.For(); + var repo = CreateRepository(); + var origin = Substitute.For(); + origin.Url.Returns(originUrl); + repo.Network.Remotes["origin"].Returns(origin); return repo; } + static IRepository CreateRepository() + { + return Substitute.For(); + } + public class TheGetLatestPushedShaMethod : TestBaseClass { [Test] From 683ad50db91a8bc7e3a7d110311c26c24cd8a9e3 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Oct 2018 15:43:50 +0100 Subject: [PATCH 07/20] Move LocalReposotoryModel.Refresh to GitService --- .../SampleData/GitServiceDesigner.cs | 1 + .../Models/ILocalRepositoryModel.cs | 10 +------ src/GitHub.Exports/Models/IRepositoryModel.cs | 2 +- .../Models/LocalRepositoryModel.cs | 10 ------- src/GitHub.Exports/Services/GitService.cs | 12 ++++++++ src/GitHub.Exports/Services/IGitService.cs | 6 ++++ .../Base/TeamExplorerServiceHolder.cs | 30 ++++++++++++------- 7 files changed, 40 insertions(+), 31 deletions(-) diff --git a/src/GitHub.App/SampleData/GitServiceDesigner.cs b/src/GitHub.App/SampleData/GitServiceDesigner.cs index b2f10226d2..95bd0bc276 100644 --- a/src/GitHub.App/SampleData/GitServiceDesigner.cs +++ b/src/GitHub.App/SampleData/GitServiceDesigner.cs @@ -10,6 +10,7 @@ class GitServiceDesigner : IGitService { public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) => null; public ILocalRepositoryModel CreateLocalRepositoryModel(string name, UriString cloneUrl, string localPath) => null; + public void Refresh(ILocalRepositoryModel localRepositoryModel) { } public Task GetLatestPushedSha(string path, string remote = "origin") => Task.FromResult(null); public UriString GetRemoteUri(IRepository repo, string remote = "origin") => null; public IRepository GetRepository(string path) => null; diff --git a/src/GitHub.Exports/Models/ILocalRepositoryModel.cs b/src/GitHub.Exports/Models/ILocalRepositoryModel.cs index f3fa4afcd3..154e5a69cc 100644 --- a/src/GitHub.Exports/Models/ILocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/ILocalRepositoryModel.cs @@ -1,7 +1,4 @@ -using GitHub.Exports; -using GitHub.Primitives; -using System.ComponentModel; -using System.Threading.Tasks; +using System.ComponentModel; namespace GitHub.Models { @@ -19,10 +16,5 @@ public interface ILocalRepositoryModel : IRepositoryModel, INotifyPropertyChange /// Gets the current branch. /// IBranch CurrentBranch { get; } - - /// - /// Updates the url information based on the local path - /// - void Refresh(); } } diff --git a/src/GitHub.Exports/Models/IRepositoryModel.cs b/src/GitHub.Exports/Models/IRepositoryModel.cs index c3c1d577ae..084bf96fe4 100644 --- a/src/GitHub.Exports/Models/IRepositoryModel.cs +++ b/src/GitHub.Exports/Models/IRepositoryModel.cs @@ -17,7 +17,7 @@ public interface IRepositoryModel /// /// Gets the repository clone URL. /// - UriString CloneUrl { get; } + UriString CloneUrl { get; set; } /// /// Gets the name of the owner of the repository, taken from the clone URL. diff --git a/src/GitHub.Exports/Models/LocalRepositoryModel.cs b/src/GitHub.Exports/Models/LocalRepositoryModel.cs index 6e3d34e7ee..8b1a8d6346 100644 --- a/src/GitHub.Exports/Models/LocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/LocalRepositoryModel.cs @@ -24,16 +24,6 @@ public LocalRepositoryModel() { } - /// - /// Updates the clone URL from the local repository. - /// - public void Refresh() - { - if (LocalPath == null) - return; - CloneUrl = gitService.GetUri(LocalPath); - } - /// /// Gets the local path of the repository. /// diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index 3c19afd813..dc6fe04157 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -73,6 +73,18 @@ public ILocalRepositoryModel CreateLocalRepositoryModel(string name, UriString c }; } + /// + /// Updates the CloneUrl from the local repository. + /// + public void Refresh(ILocalRepositoryModel localRepositoryModel) + { + var localPath = localRepositoryModel.LocalPath; + if (localPath == null) + return; + + localRepositoryModel.CloneUrl = GetUri(localPath); + } + /// /// Returns the URL of the remote for the specified . If the repository /// is null or no remote named origin exists, this method returns null diff --git a/src/GitHub.Exports/Services/IGitService.cs b/src/GitHub.Exports/Services/IGitService.cs index 9dd4348edb..766d2658cb 100644 --- a/src/GitHub.Exports/Services/IGitService.cs +++ b/src/GitHub.Exports/Services/IGitService.cs @@ -23,6 +23,12 @@ public interface IGitService /// A repository model. ILocalRepositoryModel CreateLocalRepositoryModel(string name, UriString cloneUrl, string localPath); + /// + /// Updates the ClonePath information based on the local path + /// + /// The repository model to refresh. + void Refresh(ILocalRepositoryModel localRepositoryModel); + /// /// Returns the URL of the remote for the specified . If the repository /// is null or no remote exists, this method returns null diff --git a/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs b/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs index 20879c8fbe..db91cfbe9b 100644 --- a/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs +++ b/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs @@ -25,14 +25,15 @@ public class TeamExplorerServiceHolder : ITeamExplorerServiceHolder bool activeRepoNotified = false; IServiceProvider serviceProvider; - readonly IVSGitExt gitService; + readonly IVSGitExt gitExt; + readonly IGitService gitService; /// /// This class relies on IVSGitExt that provides information when VS switches repositories. /// - /// Used for monitoring the active repository. + /// Used for monitoring the active repository. [ImportingConstructor] - TeamExplorerServiceHolder(IVSGitExt gitService) : this(gitService, ThreadHelper.JoinableTaskContext) + TeamExplorerServiceHolder(IVSGitExt gitExt, IGitService gitService) : this(gitExt, gitService, ThreadHelper.JoinableTaskContext) { } @@ -41,19 +42,23 @@ public class TeamExplorerServiceHolder : ITeamExplorerServiceHolder /// /// Used for monitoring the active repository. /// Used for switching to the Main thread. - public TeamExplorerServiceHolder(IVSGitExt gitService, JoinableTaskContext joinableTaskContext) + public TeamExplorerServiceHolder(IVSGitExt gitExt, IGitService gitService, JoinableTaskContext joinableTaskContext) { JoinableTaskCollection = joinableTaskContext.CreateCollection(); JoinableTaskCollection.DisplayName = nameof(TeamExplorerServiceHolder); JoinableTaskFactory = joinableTaskContext.CreateFactory(JoinableTaskCollection); - // This might be null in Blend or SafeMode - if (gitService != null) + // HACK: This might be null in SafeMode. + // We should be throwing rather than returning a null MEF service. + if (gitExt == null) { - this.gitService = gitService; - UpdateActiveRepo(); - gitService.ActiveRepositoriesChanged += UpdateActiveRepo; + return; } + + this.gitExt = gitExt; + this.gitService = gitService; + UpdateActiveRepo(); + gitExt.ActiveRepositoriesChanged += UpdateActiveRepo; } // set by the sections when they get initialized @@ -106,7 +111,10 @@ public void Subscribe(object who, Action handler) // the repo url might have changed and we don't get notifications // for that, so this is a good place to refresh it in case that happened - repo?.Refresh(); + if (repo != null) + { + gitService.Refresh(repo); + } // if the active repo hasn't changed and there's notifications queued up, // notify the subscriber. If the repo has changed, the set above will trigger @@ -152,7 +160,7 @@ void NotifyActiveRepo() void UpdateActiveRepo() { - var repo = gitService.ActiveRepositories.FirstOrDefault(); + var repo = gitExt.ActiveRepositories.FirstOrDefault(); if (!Equals(repo, ActiveRepo)) { From 59e5652f41b3b92ed50c3a3664094bc86f50fe00 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Oct 2018 15:50:37 +0100 Subject: [PATCH 08/20] Remove LocalRepositoryModel.HeadSha property This property wasn't being used. --- src/GitHub.Exports/Models/LocalRepositoryModel.cs | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/GitHub.Exports/Models/LocalRepositoryModel.cs b/src/GitHub.Exports/Models/LocalRepositoryModel.cs index 8b1a8d6346..4f8ecde94e 100644 --- a/src/GitHub.Exports/Models/LocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/LocalRepositoryModel.cs @@ -29,20 +29,6 @@ public LocalRepositoryModel() /// public string LocalPath { get; set; } - /// - /// Gets the head SHA of the repository. - /// - public string HeadSha - { - get - { - using (var repo = gitService.GetRepository(LocalPath)) - { - return repo?.Commits.FirstOrDefault()?.Sha ?? string.Empty; - } - } - } - /// /// Gets the current branch of the repository. /// From 01d2e46c5535273e85bbf9062343275f8b19447d Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Oct 2018 18:08:50 +0100 Subject: [PATCH 09/20] Remove CreateLocalRepositoryModel overload We Now only need the CreateLocalRepositoryModel(localPath) overload. --- src/GitHub.Exports/Services/GitService.cs | 27 +++------------------- src/GitHub.Exports/Services/IGitService.cs | 9 -------- 2 files changed, 3 insertions(+), 33 deletions(-) diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index dc6fe04157..48ad2b3a3d 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -40,37 +40,16 @@ public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) var cloneUrl = GetUri(localPath); var name = cloneUrl?.RepositoryName ?? dir.Name; - var icon = Octicon.repo; - return new LocalRepositoryModel - { - LocalPath = localPath, - CloneUrl = cloneUrl, - Name = name, - Icon = icon - }; - } - - /// - /// Initializes a new instance of the class. - /// - /// The repository name. - /// The repository's clone URL. - /// The repository's local path. - /// A repository model. - public ILocalRepositoryModel CreateLocalRepositoryModel(string name, UriString cloneUrl, string localPath) - { - Guard.ArgumentNotEmptyString(name, nameof(name)); - Guard.ArgumentNotNull(cloneUrl, nameof(cloneUrl)); - Guard.ArgumentNotEmptyString(localPath, nameof(localPath)); - - return new LocalRepositoryModel + var model = new LocalRepositoryModel { LocalPath = localPath, CloneUrl = cloneUrl, Name = name, Icon = Octicon.repo }; + + return model; } /// diff --git a/src/GitHub.Exports/Services/IGitService.cs b/src/GitHub.Exports/Services/IGitService.cs index 766d2658cb..512d338b7a 100644 --- a/src/GitHub.Exports/Services/IGitService.cs +++ b/src/GitHub.Exports/Services/IGitService.cs @@ -14,15 +14,6 @@ public interface IGitService /// A repository model. ILocalRepositoryModel CreateLocalRepositoryModel(string localPath); - /// - /// Initializes a new instance of the class. - /// - /// The repository name. - /// The repository's clone URL. - /// The repository's local path. - /// A repository model. - ILocalRepositoryModel CreateLocalRepositoryModel(string name, UriString cloneUrl, string localPath); - /// /// Updates the ClonePath information based on the local path /// From 7bab4780b69807936c8385fe8b9338d85b9d0dc0 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Oct 2018 18:13:22 +0100 Subject: [PATCH 10/20] Read the current branch when model is created Previously the current branch was being read when CurrentBranch was fetched. This changes it to be read when the LocalRepositoryModel is created. --- .../Models/LocalRepositoryModel.cs | 9 +-------- src/GitHub.Exports/Services/GitService.cs | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/GitHub.Exports/Models/LocalRepositoryModel.cs b/src/GitHub.Exports/Models/LocalRepositoryModel.cs index 4f8ecde94e..024f7f2077 100644 --- a/src/GitHub.Exports/Models/LocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/LocalRepositoryModel.cs @@ -34,14 +34,7 @@ public LocalRepositoryModel() /// public IBranch CurrentBranch { - get - { - // BranchModel doesn't keep a reference to Repository - using (var repo = gitService.GetRepository(LocalPath)) - { - return new BranchModel(repo?.Head, this, gitService); - } - } + get; set; } /// diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index 48ad2b3a3d..bdac72e308 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -49,6 +49,8 @@ public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) Icon = Octicon.repo }; + RefreshCurrentBranch(model); + return model; } @@ -196,5 +198,19 @@ public Task GetLatestPushedSha(string path, string remote = "origin") } }); } + + void RefreshCurrentBranch(LocalRepositoryModel model) + { + var localPath = model.LocalPath; + using (var repo = GetRepository(localPath)) + { + var headBranch = repo?.Head; + if (headBranch != null) + { + // BranchModel doesn't keep a reference to Repository + model.CurrentBranch = new BranchModel(headBranch, model, this); + } + } + } } } \ No newline at end of file From e3f811400f73aa722664c388253b76b24cf079c9 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 23 Oct 2018 18:20:53 +0100 Subject: [PATCH 11/20] Clean up LocalRepositoryModel Remove redundant code and usings. --- .../Models/LocalRepositoryModel.cs | 29 ++++++------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/src/GitHub.Exports/Models/LocalRepositoryModel.cs b/src/GitHub.Exports/Models/LocalRepositoryModel.cs index 024f7f2077..7feae3e151 100644 --- a/src/GitHub.Exports/Models/LocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/LocalRepositoryModel.cs @@ -1,14 +1,6 @@ using System; using System.Diagnostics; using System.Globalization; -using System.IO; -using System.Linq; -using GitHub.Primitives; -using GitHub.UI; -using GitHub.Exports; -using GitHub.Services; -using GitHub.Extensions; -using System.Threading.Tasks; namespace GitHub.Models { @@ -18,16 +10,13 @@ namespace GitHub.Models [DebuggerDisplay("{DebuggerDisplay,nq}")] public class LocalRepositoryModel : RepositoryModel, ILocalRepositoryModel, IEquatable { - readonly IGitService gitService; - - public LocalRepositoryModel() - { - } - /// /// Gets the local path of the repository. /// - public string LocalPath { get; set; } + public string LocalPath + { + get; set; + } /// /// Gets the current branch of the repository. @@ -60,13 +49,13 @@ public bool Equals(LocalRepositoryModel other) if (ReferenceEquals(this, other)) return true; return other != null && - String.Equals(Name, other.Name) && - String.Equals(Owner, other.Owner) && - String.Equals(CloneUrl, other.CloneUrl) && - String.Equals(LocalPath?.TrimEnd('\\'), other.LocalPath?.TrimEnd('\\'), StringComparison.CurrentCultureIgnoreCase); + string.Equals(Name, other.Name) && + string.Equals(Owner, other.Owner) && + string.Equals(CloneUrl, other.CloneUrl) && + string.Equals(LocalPath?.TrimEnd('\\'), other.LocalPath?.TrimEnd('\\'), StringComparison.CurrentCultureIgnoreCase); } - internal string DebuggerDisplay => String.Format( + internal string DebuggerDisplay => string.Format( CultureInfo.InvariantCulture, "{4}\tOwner: {0} Name: {1} CloneUrl: {2} LocalPath: {3}", Owner, From 2f13d621cb88b1594d4247abcaeccf3d2afbcddb Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 30 Oct 2018 13:20:44 +0000 Subject: [PATCH 12/20] ClonePath -> CloneUri --- src/GitHub.Exports/Services/IGitService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/GitHub.Exports/Services/IGitService.cs b/src/GitHub.Exports/Services/IGitService.cs index 512d338b7a..1bcd0480f3 100644 --- a/src/GitHub.Exports/Services/IGitService.cs +++ b/src/GitHub.Exports/Services/IGitService.cs @@ -15,7 +15,7 @@ public interface IGitService ILocalRepositoryModel CreateLocalRepositoryModel(string localPath); /// - /// Updates the ClonePath information based on the local path + /// Updates the CloneUrl information based on the local path. /// /// The repository model to refresh. void Refresh(ILocalRepositoryModel localRepositoryModel); From c61d0f25bb48173d8e43026a4367ed3a64e620cc Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 30 Oct 2018 13:32:24 +0000 Subject: [PATCH 13/20] Make GitService.Refresh the CurrentBranch Previously CurrentBranch was created as the property was read. We now need a way to refresh it. --- src/GitHub.Exports/Models/ILocalRepositoryModel.cs | 2 +- src/GitHub.Exports/Services/GitService.cs | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/GitHub.Exports/Models/ILocalRepositoryModel.cs b/src/GitHub.Exports/Models/ILocalRepositoryModel.cs index 154e5a69cc..41c6ad508f 100644 --- a/src/GitHub.Exports/Models/ILocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/ILocalRepositoryModel.cs @@ -15,6 +15,6 @@ public interface ILocalRepositoryModel : IRepositoryModel, INotifyPropertyChange /// /// Gets the current branch. /// - IBranch CurrentBranch { get; } + IBranch CurrentBranch { get; set; } } } diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index bdac72e308..30b23849c6 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -55,7 +55,7 @@ public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) } /// - /// Updates the CloneUrl from the local repository. + /// Updates the CloneUrl and CurrentBranch from the local repository. /// public void Refresh(ILocalRepositoryModel localRepositoryModel) { @@ -64,6 +64,8 @@ public void Refresh(ILocalRepositoryModel localRepositoryModel) return; localRepositoryModel.CloneUrl = GetUri(localPath); + + RefreshCurrentBranch(localRepositoryModel); } /// @@ -199,7 +201,7 @@ public Task GetLatestPushedSha(string path, string remote = "origin") }); } - void RefreshCurrentBranch(LocalRepositoryModel model) + void RefreshCurrentBranch(ILocalRepositoryModel model) { var localPath = model.LocalPath; using (var repo = GetRepository(localPath)) From 47430fd92c1a7e17acd5dd04dc34d7628d5c6f0c Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 31 Oct 2018 09:06:08 +0000 Subject: [PATCH 14/20] Add workaround for out of date CurrentBranch issue It appears VSGitExt.ActiveRepositoriesChanged is fired before the local repository has actually changed its branch. This means we can't read the branch information immediately! --- .../ViewModels/GitHubPane/PullRequestDetailViewModel.cs | 8 +++++++- .../GitHubPane/PullRequestDetailViewModelTests.cs | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs index 32d4dbe2d7..bf7c967208 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs @@ -39,6 +39,7 @@ public sealed class PullRequestDetailViewModel : PanePageViewModelBase, IPullReq readonly ITeamExplorerContext teamExplorerContext; readonly ISyncSubmodulesCommand syncSubmodulesCommand; readonly IViewViewModelFactory viewViewModelFactory; + readonly IGitService gitService; IModelService modelService; PullRequestDetailModel model; IActorViewModel author; @@ -77,7 +78,8 @@ public PullRequestDetailViewModel( ITeamExplorerContext teamExplorerContext, IPullRequestFilesViewModel files, ISyncSubmodulesCommand syncSubmodulesCommand, - IViewViewModelFactory viewViewModelFactory) + IViewViewModelFactory viewViewModelFactory, + IGitService gitService) { Guard.ArgumentNotNull(pullRequestsService, nameof(pullRequestsService)); Guard.ArgumentNotNull(sessionManager, nameof(sessionManager)); @@ -86,6 +88,7 @@ public PullRequestDetailViewModel( Guard.ArgumentNotNull(teamExplorerContext, nameof(teamExplorerContext)); Guard.ArgumentNotNull(syncSubmodulesCommand, nameof(syncSubmodulesCommand)); Guard.ArgumentNotNull(viewViewModelFactory, nameof(viewViewModelFactory)); + Guard.ArgumentNotNull(gitService, nameof(gitService)); this.pullRequestsService = pullRequestsService; this.sessionManager = sessionManager; @@ -94,6 +97,7 @@ public PullRequestDetailViewModel( this.teamExplorerContext = teamExplorerContext; this.syncSubmodulesCommand = syncSubmodulesCommand; this.viewViewModelFactory = viewViewModelFactory; + this.gitService = gitService; Files = files; Checkout = ReactiveCommand.CreateFromObservable( @@ -401,6 +405,8 @@ public async Task Load(PullRequestDetailModel pullRequest) var localBranches = await pullRequestsService.GetLocalBranches(LocalRepository, pullRequest).ToList(); + // HACK: This is only necessary because the CurrentBranch is being read too early + gitService.Refresh(LocalRepository); IsCheckedOut = localBranches.Contains(LocalRepository.CurrentBranch); if (IsCheckedOut) diff --git a/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs b/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs index ac24f57097..1a15c43943 100644 --- a/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs +++ b/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs @@ -607,7 +607,8 @@ static Tuple CreateTargetAndSer Substitute.For(), Substitute.For(), Substitute.For(), - Substitute.For()); + Substitute.For(), + Substitute.For()); vm.InitializeAsync(repository, Substitute.For(), "owner", "repo", 1).Wait(); return Tuple.Create(vm, pullRequestService); From 713d9c40dd37738da635cb550bd0810a4e26fe7c Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 31 Oct 2018 12:38:42 +0000 Subject: [PATCH 15/20] Refactor to IGitService.CreateCurrentBranchModel Remove the ILocalRepositoryModel.CurrentBranch property and explicitly call IGitService.CreateCurrentBranchModel instead. Fix all the broken tests. --- .../SampleData/GitServiceDesigner.cs | 2 +- .../Services/TeamExplorerContext.cs | 27 ++---------- .../PullRequestCreationViewModel.cs | 13 ++++-- .../GitHubPane/PullRequestDetailViewModel.cs | 5 +-- .../Models/ILocalRepositoryModel.cs | 5 --- .../Models/LocalRepositoryModel.cs | 8 ---- src/GitHub.Exports/Services/GitService.cs | 36 ++++++++-------- src/GitHub.Exports/Services/IGitService.cs | 9 +++- .../Commands/OpenFromClipboardCommand.cs | 14 ++++--- .../Services/TeamExplorerContextTests.cs | 42 ++++--------------- .../PullRequestCreationViewModelTests.cs | 27 +++++++----- .../PullRequestDetailViewModelTests.cs | 5 ++- .../Commands/OpenFromClipboardCommandTests.cs | 22 ++++++---- 13 files changed, 90 insertions(+), 125 deletions(-) diff --git a/src/GitHub.App/SampleData/GitServiceDesigner.cs b/src/GitHub.App/SampleData/GitServiceDesigner.cs index 95bd0bc276..6d90eaacbb 100644 --- a/src/GitHub.App/SampleData/GitServiceDesigner.cs +++ b/src/GitHub.App/SampleData/GitServiceDesigner.cs @@ -9,7 +9,7 @@ namespace GitHub.SampleData class GitServiceDesigner : IGitService { public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) => null; - public ILocalRepositoryModel CreateLocalRepositoryModel(string name, UriString cloneUrl, string localPath) => null; + public IBranch CreateCurrentBranchModel(ILocalRepositoryModel model) => null; public void Refresh(ILocalRepositoryModel localRepositoryModel) { } public Task GetLatestPushedSha(string path, string remote = "origin") => Task.FromResult(null); public UriString GetRemoteUri(IRepository repo, string remote = "origin") => null; diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 6d443e4dfc..555f8962f2 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -40,9 +40,6 @@ public class TeamExplorerContext : ITeamExplorerContext string solutionPath; string repositoryPath; UriString cloneUrl; - string branchName; - string headSha; - string trackedSha; Tuple pullRequest; ILocalRepositoryModel repositoryModel; @@ -113,9 +110,6 @@ async Task RefreshAsync() { var newRepositoryPath = repo?.LocalPath; var newCloneUrl = repo?.CloneUrl; - var newBranchName = repo?.CurrentBranch?.Name; - var newHeadSha = repo?.CurrentBranch?.Sha; - var newTrackedSha = repo?.CurrentBranch?.TrackedSha; var newPullRequest = repo != null ? await pullRequestService.GetPullRequestForCurrentBranch(repo) : null; if (newRepositoryPath != repositoryPath) @@ -128,33 +122,20 @@ async Task RefreshAsync() log.Debug("ActiveRepository changed to {CloneUrl} @ {Path}", repo?.CloneUrl, newRepositoryPath); ActiveRepository = repo; } - else if (newBranchName != branchName) - { - log.Debug("Fire StatusChanged event when BranchName changes for ActiveRepository"); - StatusChanged?.Invoke(this, EventArgs.Empty); - } - else if (newHeadSha != headSha) - { - log.Debug("Fire StatusChanged event when HeadSha changes for ActiveRepository"); - StatusChanged?.Invoke(this, EventArgs.Empty); - } - else if (newTrackedSha != trackedSha) + else if (newPullRequest != pullRequest) { - log.Debug("Fire StatusChanged event when TrackedSha changes for ActiveRepository"); + log.Debug("Fire StatusChanged event when PullRequest changes for ActiveRepository"); StatusChanged?.Invoke(this, EventArgs.Empty); } - else if (newPullRequest != pullRequest) + else { - log.Debug("Fire StatusChanged event when PullRequest changes for ActiveRepository"); + log.Debug("Fire StatusChanged event when ***anything*** changes"); StatusChanged?.Invoke(this, EventArgs.Empty); } repositoryPath = newRepositoryPath; cloneUrl = newCloneUrl; - branchName = newBranchName; - headSha = newHeadSha; solutionPath = newSolutionPath; - trackedSha = newTrackedSha; pullRequest = newPullRequest; } } diff --git a/src/GitHub.App/ViewModels/GitHubPane/PullRequestCreationViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/PullRequestCreationViewModel.cs index f72bc605fe..764fc1ec10 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/PullRequestCreationViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/PullRequestCreationViewModel.cs @@ -38,6 +38,7 @@ public class PullRequestCreationViewModel : PanePageViewModelBase, IPullRequestC readonly IPullRequestService service; readonly IModelServiceFactory modelServiceFactory; readonly IMessageDraftStore draftStore; + readonly IGitService gitService; readonly IScheduler timerScheduler; readonly CompositeDisposable disposables = new CompositeDisposable(); ILocalRepositoryModel activeLocalRepo; @@ -49,8 +50,9 @@ public PullRequestCreationViewModel( IModelServiceFactory modelServiceFactory, IPullRequestService service, INotificationService notifications, - IMessageDraftStore draftStore) - : this(modelServiceFactory, service, notifications, draftStore, DefaultScheduler.Instance) + IMessageDraftStore draftStore, + IGitService gitService) + : this(modelServiceFactory, service, notifications, draftStore, gitService, DefaultScheduler.Instance) { } @@ -59,17 +61,20 @@ public PullRequestCreationViewModel( IPullRequestService service, INotificationService notifications, IMessageDraftStore draftStore, + IGitService gitService, IScheduler timerScheduler) { Guard.ArgumentNotNull(modelServiceFactory, nameof(modelServiceFactory)); Guard.ArgumentNotNull(service, nameof(service)); Guard.ArgumentNotNull(notifications, nameof(notifications)); Guard.ArgumentNotNull(draftStore, nameof(draftStore)); + Guard.ArgumentNotNull(gitService, nameof(gitService)); Guard.ArgumentNotNull(timerScheduler, nameof(timerScheduler)); this.service = service; this.modelServiceFactory = modelServiceFactory; this.draftStore = draftStore; + this.gitService = gitService; this.timerScheduler = timerScheduler; this.WhenAnyValue(x => x.Branches) @@ -137,7 +142,7 @@ public async Task InitializeAsync(ILocalRepositoryModel repository, IConnection { modelService = await modelServiceFactory.CreateAsync(connection); activeLocalRepo = repository; - SourceBranch = repository.CurrentBranch; + SourceBranch = gitService.CreateCurrentBranchModel(repository); var obs = modelService.ApiClient.GetRepository(repository.Owner, repository.Name) .Select(r => new RemoteRepositoryModel(r)) @@ -207,7 +212,7 @@ async Task LoadInitialState(string draftKey) void LoadDescriptionFromCommits() { - SourceBranch = activeLocalRepo.CurrentBranch; + SourceBranch = gitService.CreateCurrentBranchModel(activeLocalRepo); var uniqueCommits = this.WhenAnyValue( x => x.SourceBranch, diff --git a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs index bf7c967208..fc1f6dd703 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs @@ -405,9 +405,8 @@ public async Task Load(PullRequestDetailModel pullRequest) var localBranches = await pullRequestsService.GetLocalBranches(LocalRepository, pullRequest).ToList(); - // HACK: This is only necessary because the CurrentBranch is being read too early - gitService.Refresh(LocalRepository); - IsCheckedOut = localBranches.Contains(LocalRepository.CurrentBranch); + var currentBranch = gitService.CreateCurrentBranchModel(LocalRepository); + IsCheckedOut = localBranches.Contains(currentBranch); if (IsCheckedOut) { diff --git a/src/GitHub.Exports/Models/ILocalRepositoryModel.cs b/src/GitHub.Exports/Models/ILocalRepositoryModel.cs index 41c6ad508f..781180f4f2 100644 --- a/src/GitHub.Exports/Models/ILocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/ILocalRepositoryModel.cs @@ -11,10 +11,5 @@ public interface ILocalRepositoryModel : IRepositoryModel, INotifyPropertyChange /// Gets the path to the repository on the filesystem. /// string LocalPath { get; } - - /// - /// Gets the current branch. - /// - IBranch CurrentBranch { get; set; } } } diff --git a/src/GitHub.Exports/Models/LocalRepositoryModel.cs b/src/GitHub.Exports/Models/LocalRepositoryModel.cs index 7feae3e151..6e9e1e521e 100644 --- a/src/GitHub.Exports/Models/LocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/LocalRepositoryModel.cs @@ -18,14 +18,6 @@ public string LocalPath get; set; } - /// - /// Gets the current branch of the repository. - /// - public IBranch CurrentBranch - { - get; set; - } - /// /// Note: We don't consider CloneUrl a part of the hash code because it can change during the lifetime /// of a repository. Equals takes care of any hash collisions because of this diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index 30b23849c6..2fee66dff7 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -49,13 +49,27 @@ public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) Icon = Octicon.repo }; - RefreshCurrentBranch(model); - return model; } + public IBranch CreateCurrentBranchModel(ILocalRepositoryModel model) + { + var localPath = model.LocalPath; + using (var repo = GetRepository(localPath)) + { + var headBranch = repo?.Head; + if (headBranch == null) + { + return null; + } + + // BranchModel doesn't keep a reference to Repository + return new BranchModel(headBranch, model, this); + } + } + /// - /// Updates the CloneUrl and CurrentBranch from the local repository. + /// Updates the CloneUrl from the "origin" remote. /// public void Refresh(ILocalRepositoryModel localRepositoryModel) { @@ -64,8 +78,6 @@ public void Refresh(ILocalRepositoryModel localRepositoryModel) return; localRepositoryModel.CloneUrl = GetUri(localPath); - - RefreshCurrentBranch(localRepositoryModel); } /// @@ -200,19 +212,5 @@ public Task GetLatestPushedSha(string path, string remote = "origin") } }); } - - void RefreshCurrentBranch(ILocalRepositoryModel model) - { - var localPath = model.LocalPath; - using (var repo = GetRepository(localPath)) - { - var headBranch = repo?.Head; - if (headBranch != null) - { - // BranchModel doesn't keep a reference to Repository - model.CurrentBranch = new BranchModel(headBranch, model, this); - } - } - } } } \ No newline at end of file diff --git a/src/GitHub.Exports/Services/IGitService.cs b/src/GitHub.Exports/Services/IGitService.cs index 1bcd0480f3..96a0f044dc 100644 --- a/src/GitHub.Exports/Services/IGitService.cs +++ b/src/GitHub.Exports/Services/IGitService.cs @@ -15,7 +15,14 @@ public interface IGitService ILocalRepositoryModel CreateLocalRepositoryModel(string localPath); /// - /// Updates the CloneUrl information based on the local path. + /// Creates a new branch model for the current branch. + /// + /// The to create a current branch model for. + /// A new branch model. + IBranch CreateCurrentBranchModel(ILocalRepositoryModel model); + + /// + /// Updates the CloneUrl information based on the "origin" remote. /// /// The repository model to refresh. void Refresh(ILocalRepositoryModel localRepositoryModel); diff --git a/src/GitHub.VisualStudio/Commands/OpenFromClipboardCommand.cs b/src/GitHub.VisualStudio/Commands/OpenFromClipboardCommand.cs index 7476a6eb58..73e77cea96 100644 --- a/src/GitHub.VisualStudio/Commands/OpenFromClipboardCommand.cs +++ b/src/GitHub.VisualStudio/Commands/OpenFromClipboardCommand.cs @@ -4,7 +4,6 @@ using GitHub.Exports; using GitHub.Services; using GitHub.Services.Vssdk.Commands; -using GitHub.VisualStudio.UI; using Microsoft.VisualStudio.Shell; using Task = System.Threading.Tasks.Task; @@ -13,11 +12,12 @@ namespace GitHub.VisualStudio.Commands [Export(typeof(IOpenFromClipboardCommand))] public class OpenFromClipboardCommand : VsCommand, IOpenFromClipboardCommand { - + readonly Lazy gitHubContextService; readonly Lazy teamExplorerContext; readonly Lazy vsServices; + readonly Lazy gitService; readonly Lazy uiContext; /// @@ -34,12 +34,14 @@ public class OpenFromClipboardCommand : VsCommand, IOpenFromClipboardCom public OpenFromClipboardCommand( Lazy gitHubContextService, Lazy teamExplorerContext, - Lazy vsServices) + Lazy vsServices, + Lazy gitService) : base(CommandSet, CommandId) { this.gitHubContextService = gitHubContextService; this.teamExplorerContext = teamExplorerContext; this.vsServices = vsServices; + this.gitService = gitService; // See https://code.msdn.microsoft.com/windowsdesktop/AllowParams-2005-9442298f ParametersDescription = "u"; // accept a single url @@ -96,9 +98,11 @@ public override async Task Execute(string url) var hasChanges = gitHubContextService.Value.HasChangesInWorkingDirectory(repositoryDir, commitish, path); if (hasChanges) { - // AnnotateFile expects a branch name so we use the current branch - var branchName = activeRepository.CurrentBranch.Name; + // TODO: What if this returns null because we're not on a branch? + var currentBranch = gitService.Value.CreateCurrentBranchModel(activeRepository); + var branchName = currentBranch.Name; + // AnnotateFile expects a branch name so we use the current branch if (await gitHubContextService.Value.TryAnnotateFile(repositoryDir, branchName, context)) { return; diff --git a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs index f539b589e0..a3fc3b5352 100644 --- a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs +++ b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs @@ -151,45 +151,22 @@ public void NoActiveRepositoryChange_SolutionChanges() public class TheStatusChangedEvent { - [TestCase(false, "name1", "sha1", "name1", "sha1", false)] - [TestCase(false, "name1", "sha1", "name2", "sha1", true)] - [TestCase(false, "name1", "sha1", "name1", "sha2", true)] - [TestCase(false, "name1", "sha1", "name2", "sha2", true)] - [TestCase(true, "name1", "sha1", "name1", "sha1", false)] - [TestCase(true, "name1", "sha1", "name2", "sha2", false)] - public void SameActiveRepository_ExpectWasRaised(bool changePath, string name1, string sha1, string name2, string sha2, bool expectWasRaised) + [TestCase(true, false)] + [TestCase(false, true)] + public void AlwaysFireWhenNoLocalPathChange(bool changePath, bool expectWasRaised) { var gitExt = CreateGitExt(); var repositoryPaths = new[] { Directory.GetCurrentDirectory(), Path.GetTempPath() }; var path1 = Directory.GetCurrentDirectory(); var path2 = changePath ? Path.GetTempPath() : path1; - var repoInfo1 = CreateRepositoryModel(path1, name1, sha1); - var repoInfo2 = CreateRepositoryModel(path2, name2, sha2); + var repoInfo1 = CreateRepositoryModel(path1); + var repoInfo2 = CreateRepositoryModel(path2); var target = CreateTeamExplorerContext(gitExt); - var eventWasRaised = false; - target.StatusChanged += (s, e) => eventWasRaised = true; - SetActiveRepository(gitExt, repoInfo1); - SetActiveRepository(gitExt, repoInfo2); - - Assert.That(eventWasRaised, Is.EqualTo(expectWasRaised)); - } - - [TestCase("trackedSha", "trackedSha", false)] - [TestCase("trackedSha1", "trackedSha2", true)] - public void TrackedShaChanges_CheckWasRaised(string trackedSha1, string trackedSha2, bool expectWasRaised) - { - var gitExt = CreateGitExt(); - var repositoryPaths = new[] { Directory.GetCurrentDirectory(), Path.GetTempPath() }; - var repoPath = Directory.GetCurrentDirectory(); - var repoInfo1 = CreateRepositoryModel(repoPath, "name", "sha", trackedSha1); - var repoInfo2 = CreateRepositoryModel(repoPath, "name", "sha", trackedSha2); - var target = CreateTeamExplorerContext(gitExt); SetActiveRepository(gitExt, repoInfo1); var eventWasRaised = false; target.StatusChanged += (s, e) => eventWasRaised = true; - SetActiveRepository(gitExt, repoInfo2); Assert.That(eventWasRaised, Is.EqualTo(expectWasRaised)); @@ -200,7 +177,7 @@ public void SolutionUnloadedAndReloaded_DontFireStatusChanged() { var gitExt = CreateGitExt(); var path = Directory.GetCurrentDirectory(); - var repoInfo1 = CreateRepositoryModel(path, "name", "sha"); + var repoInfo1 = CreateRepositoryModel(path); var repoInfo2 = CreateRepositoryModel(null); var target = CreateTeamExplorerContext(gitExt); SetActiveRepository(gitExt, repoInfo1); @@ -226,15 +203,10 @@ static TeamExplorerContext CreateTeamExplorerContext( return new TeamExplorerContext(gitExt, new AsyncLazy(() => Task.FromResult(dte)), pullRequestService, joinableTaskContext); } - static ILocalRepositoryModel CreateRepositoryModel(string path, string branchName = null, string headSha = null, string trackedSha = null) + static ILocalRepositoryModel CreateRepositoryModel(string path) { var repo = Substitute.For(); repo.LocalPath.Returns(path); - var currentBranch = Substitute.For(); - currentBranch.Name.Returns(branchName); - currentBranch.Sha.Returns(headSha); - currentBranch.TrackedSha.Returns(trackedSha); - repo.CurrentBranch.Returns(currentBranch); return repo; } diff --git a/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestCreationViewModelTests.cs b/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestCreationViewModelTests.cs index 5b21260e0a..726566dae8 100644 --- a/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestCreationViewModelTests.cs +++ b/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestCreationViewModelTests.cs @@ -6,14 +6,12 @@ using GitHub.Models; using GitHub.Primitives; using GitHub.Services; -using GitHub.ViewModels; using GitHub.ViewModels.GitHubPane; using NSubstitute; using Octokit; using UnitTests; using NUnit.Framework; using IConnection = GitHub.Models.IConnection; -using ReactiveUI.Testing; using System.Reactive.Concurrency; using GitHub.Models.Drafts; @@ -99,7 +97,7 @@ static TestData PrepareTestData( var targetRepo = targetRepoOwner == sourceRepoOwner ? sourceRepo : sourceRepo.Parent; var targetBranch = targetBranchName != targetRepo.DefaultBranch.Name ? new BranchModel(targetBranchName, targetRepo) : targetRepo.DefaultBranch; - activeRepo.CurrentBranch.Returns(sourceBranch); + gitService.CreateCurrentBranchModel(activeRepo).Returns(sourceBranch); api.GetRepository(Args.String, Args.String).Returns(Observable.Return(githubRepo)); ms.ApiClient.Returns(api); @@ -130,7 +128,8 @@ public async Task TargetBranchDisplayNameIncludesRepoOwnerWhenForkAsync() var data = PrepareTestData("octokit.net", "shana", "master", "octokit", "master", "origin", true, true); var prservice = new PullRequestService(data.GitClient, data.GitService, Substitute.For(), Substitute.For(), data.ServiceProvider.GetOperatingSystem(), Substitute.For()); prservice.GetPullRequestTemplate(data.ActiveRepo).Returns(Observable.Empty()); - var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, Substitute.For()); + var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, + Substitute.For(), data.GitService); await vm.InitializeAsync(data.ActiveRepo, data.Connection); Assert.That("octokit/master", Is.EqualTo(vm.TargetBranch.DisplayName)); } @@ -165,7 +164,8 @@ public async Task CreatingPRsAsync( var ms = data.ModelService; var prservice = new PullRequestService(data.GitClient, data.GitService, Substitute.For(), Substitute.For(), data.ServiceProvider.GetOperatingSystem(), Substitute.For()); - var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, Substitute.For()); + var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, + Substitute.For(), data.GitService); await vm.InitializeAsync(data.ActiveRepo, data.Connection); // the TargetBranch property gets set to whatever the repo default is (we assume master here), @@ -208,7 +208,7 @@ public async Task TemplateIsUsedIfPresentAsync() prservice.GetPullRequestTemplate(data.ActiveRepo).Returns(Observable.Return("Test PR template")); var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, - Substitute.For()); + Substitute.For(), data.GitService); await vm.InitializeAsync(data.ActiveRepo, data.Connection); Assert.That("Test PR template", Is.EqualTo(vm.Description)); @@ -227,7 +227,8 @@ public async Task LoadsDraft() }); var prservice = Substitute.For(); - var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, draftStore); + var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, + draftStore, data.GitService); await vm.InitializeAsync(data.ActiveRepo, data.Connection); Assert.That(vm.PRTitle, Is.EqualTo("This is a Title.")); @@ -241,7 +242,8 @@ public async Task UpdatesDraftWhenDescriptionChanges() var scheduler = new HistoricalScheduler(); var draftStore = Substitute.For(); var prservice = Substitute.For(); - var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, draftStore, scheduler); + var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, + draftStore, data.GitService, scheduler); await vm.InitializeAsync(data.ActiveRepo, data.Connection); vm.Description = "Body changed."; @@ -263,7 +265,8 @@ public async Task UpdatesDraftWhenTitleChanges() var scheduler = new HistoricalScheduler(); var draftStore = Substitute.For(); var prservice = Substitute.For(); - var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, draftStore, scheduler); + var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, + draftStore, data.GitService, scheduler); await vm.InitializeAsync(data.ActiveRepo, data.Connection); vm.PRTitle = "Title changed."; @@ -285,7 +288,8 @@ public async Task DeletesDraftWhenPullRequestSubmitted() var scheduler = new HistoricalScheduler(); var draftStore = Substitute.For(); var prservice = Substitute.For(); - var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, draftStore, scheduler); + var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, draftStore, + data.GitService, scheduler); await vm.InitializeAsync(data.ActiveRepo, data.Connection); await vm.CreatePullRequest.Execute(); @@ -300,7 +304,8 @@ public async Task DeletesDraftWhenCanceled() var scheduler = new HistoricalScheduler(); var draftStore = Substitute.For(); var prservice = Substitute.For(); - var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, draftStore, scheduler); + var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, draftStore, + data.GitService, scheduler); await vm.InitializeAsync(data.ActiveRepo, data.Connection); await vm.Cancel.Execute(); diff --git a/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs b/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs index 1a15c43943..fae1bd9fa7 100644 --- a/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs +++ b/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestDetailViewModelTests.cs @@ -552,7 +552,8 @@ static Tuple CreateTargetAndSer { var repository = Substitute.For(); var currentBranchModel = new BranchModel(currentBranch, repository); - repository.CurrentBranch.Returns(currentBranchModel); + var gitService = Substitute.For(); + gitService.CreateCurrentBranchModel(repository).Returns(currentBranchModel); repository.CloneUrl.Returns(new UriString(Uri.ToString())); repository.LocalPath.Returns(@"C:\projects\ThisRepo"); repository.Name.Returns("repo"); @@ -608,7 +609,7 @@ static Tuple CreateTargetAndSer Substitute.For(), Substitute.For(), Substitute.For(), - Substitute.For()); + gitService); vm.InitializeAsync(repository, Substitute.For(), "owner", "repo", 1).Wait(); return Tuple.Create(vm, pullRequestService); diff --git a/test/GitHub.VisualStudio.UnitTests/Commands/OpenFromClipboardCommandTests.cs b/test/GitHub.VisualStudio.UnitTests/Commands/OpenFromClipboardCommandTests.cs index 0235482263..b43a203c74 100644 --- a/test/GitHub.VisualStudio.UnitTests/Commands/OpenFromClipboardCommandTests.cs +++ b/test/GitHub.VisualStudio.UnitTests/Commands/OpenFromClipboardCommandTests.cs @@ -3,10 +3,10 @@ using System.Threading.Tasks; using GitHub; using GitHub.Exports; +using GitHub.Models; using GitHub.Primitives; using GitHub.Services; using GitHub.VisualStudio.Commands; -using GitHub.VisualStudio.UI; using Microsoft.VisualStudio; using NSubstitute; using NUnit.Framework; @@ -146,7 +146,8 @@ public async Task HasChangesInWorkingDirectory(bool annotateFileSupported, strin var resolveBlobResult = (targetBranch, "foo.cs", ""); var vsServices = Substitute.For(); var target = CreateOpenFromClipboardCommand(gitHubContextService: gitHubContextService, vsServices: vsServices, - contextFromClipboard: context, repositoryDir: repositoryDir, repositoryName: repositoryName, currentBranch: currentBranch, resolveBlobResult: resolveBlobResult, hasChanges: true); + contextFromClipboard: context, repositoryDir: repositoryDir, repositoryName: repositoryName, + currentBranchName: currentBranch, resolveBlobResult: resolveBlobResult, hasChanges: true); await target.Execute(null); @@ -179,7 +180,7 @@ static OpenFromClipboardCommand CreateOpenFromClipboardCommand( string repositoryDir = null, string repositoryName = null, string repositoryOwner = null, - string currentBranch = null, + string currentBranchName = null, (string, string, string)? resolveBlobResult = null, bool? hasChanges = null) { @@ -189,10 +190,14 @@ static OpenFromClipboardCommand CreateOpenFromClipboardCommand( vsServices = vsServices ?? Substitute.For(); gitHubContextService.FindContextFromClipboard().Returns(contextFromClipboard); - teamExplorerContext.ActiveRepository.LocalPath.Returns(repositoryDir); - teamExplorerContext.ActiveRepository.Name.Returns(repositoryName); - teamExplorerContext.ActiveRepository.Owner.Returns(repositoryOwner); - teamExplorerContext.ActiveRepository.CurrentBranch.Name.Returns(currentBranch); + var activeRepository = teamExplorerContext.ActiveRepository; + activeRepository.LocalPath.Returns(repositoryDir); + activeRepository.Name.Returns(repositoryName); + activeRepository.Owner.Returns(repositoryOwner); + var gitService = Substitute.For(); + var currentBranch = Substitute.For(); + currentBranch.Name.Returns(currentBranchName); + gitService.CreateCurrentBranchModel(activeRepository).Returns(currentBranch); if (resolveBlobResult != null) { gitHubContextService.ResolveBlob(repositoryDir, contextFromClipboard).Returns(resolveBlobResult.Value); @@ -206,7 +211,8 @@ static OpenFromClipboardCommand CreateOpenFromClipboardCommand( return new OpenFromClipboardCommand( new Lazy(() => gitHubContextService), new Lazy(() => teamExplorerContext), - new Lazy(() => vsServices)); + new Lazy(() => vsServices), + new Lazy(() => gitService)); } static string ResolveResources(string str) From 0f59f7e93b5c99b966acafde4a571c53121ebd70 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 31 Oct 2018 13:26:37 +0000 Subject: [PATCH 16/20] Make GetPullRequestForCurrentBranch return (tuple) Convert GetPullRequestForCurrentBranch to return (string owner, int number). This allows the tuple to be compared directly. --- src/GitHub.App/Services/PullRequestService.cs | 14 +++++++------- src/GitHub.App/Services/TeamExplorerContext.cs | 4 ++-- .../Services/IPullRequestService.cs | 2 +- .../Services/PullRequestSessionManager.cs | 2 +- .../Services/PullRequestSessionManagerTests.cs | 14 +++++++------- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs index 9b8015cb43..3a0f16c2bd 100644 --- a/src/GitHub.App/Services/PullRequestService.cs +++ b/src/GitHub.App/Services/PullRequestService.cs @@ -231,8 +231,8 @@ public async Task> ReadPullRequests( if (hasCheckRuns) { checksHasFailure = checkRuns - .Any(model => model.Conclusion.HasValue - && (model.Conclusion.Value == CheckConclusionState.Failure + .Any(model => model.Conclusion.HasValue + && (model.Conclusion.Value == CheckConclusionState.Failure || model.Conclusion.Value == CheckConclusionState.ActionRequired)); if (!checksHasFailure) @@ -718,7 +718,7 @@ public IObservable SwitchToBranch(ILocalRepositoryModel repository, PullRe }); } - public IObservable> GetPullRequestForCurrentBranch(ILocalRepositoryModel repository) + public IObservable<(string owner, int number)> GetPullRequestForCurrentBranch(ILocalRepositoryModel repository) { return Observable.Defer(async () => { @@ -912,7 +912,7 @@ async Task IsBranchMarkedAsPullRequest(IRepository repo, string branchName { var prConfigKey = $"branch.{branchName}.{SettingGHfVSPullRequest}"; var value = ParseGHfVSConfigKeyValue(await gitClient.GetConfig(repo, prConfigKey)); - return value != null && + return value != default && value.Item1 == pullRequest.BaseRepositoryOwner && value.Item2 == pullRequest.Number; } @@ -983,7 +983,7 @@ static string BuildGHfVSConfigKeyValue(string owner, int number) return owner + '#' + number.ToString(CultureInfo.InvariantCulture); } - static Tuple ParseGHfVSConfigKeyValue(string value) + static (string owner, int number) ParseGHfVSConfigKeyValue(string value) { if (value != null) { @@ -996,12 +996,12 @@ static Tuple ParseGHfVSConfigKeyValue(string value) if (int.TryParse(value.Substring(separator + 1), NumberStyles.None, CultureInfo.InvariantCulture, out number)) { - return Tuple.Create(owner, number); + return (owner, number); } } } - return null; + return default; } class ListItemAdapter : PullRequestListItemModel diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 555f8962f2..06353a747b 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -40,7 +40,7 @@ public class TeamExplorerContext : ITeamExplorerContext string solutionPath; string repositoryPath; UriString cloneUrl; - Tuple pullRequest; + (string owner, int number) pullRequest; ILocalRepositoryModel repositoryModel; JoinableTask refreshJoinableTask; @@ -110,7 +110,7 @@ async Task RefreshAsync() { var newRepositoryPath = repo?.LocalPath; var newCloneUrl = repo?.CloneUrl; - var newPullRequest = repo != null ? await pullRequestService.GetPullRequestForCurrentBranch(repo) : null; + var newPullRequest = repo != null ? await pullRequestService.GetPullRequestForCurrentBranch(repo) : default; if (newRepositoryPath != repositoryPath) { diff --git a/src/GitHub.Exports.Reactive/Services/IPullRequestService.cs b/src/GitHub.Exports.Reactive/Services/IPullRequestService.cs index 48837ce2d6..0d34471afb 100644 --- a/src/GitHub.Exports.Reactive/Services/IPullRequestService.cs +++ b/src/GitHub.Exports.Reactive/Services/IPullRequestService.cs @@ -173,7 +173,7 @@ IObservable CreatePullRequest(IModelService modelService, /// This method does not do an API request - it simply checks the mark left in the git /// config by . /// - IObservable> GetPullRequestForCurrentBranch(ILocalRepositoryModel repository); + IObservable<(string owner, int number)> GetPullRequestForCurrentBranch(ILocalRepositoryModel repository); /// /// Gets the encoding for the specified local file. diff --git a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs index 0c62775f9a..b8e10ccf8e 100644 --- a/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs +++ b/src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs @@ -203,7 +203,7 @@ async Task StatusChanged() var session = CurrentSession; var pr = await service.GetPullRequestForCurrentBranch(repository).FirstOrDefaultAsync(); - if (pr != null) + if (pr != default) { var changePR = pr.Item1 != (session?.PullRequest.BaseRepositoryOwner) || diff --git a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs index 5b6dc68a57..7b49b9ff2c 100644 --- a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs @@ -41,7 +41,7 @@ public void ReadsPullRequestFromCorrectFork() var sessionService = CreateSessionService(); service.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs( - Observable.Return(Tuple.Create("fork", CurrentBranchPullRequestNumber))); + Observable.Return(("fork", CurrentBranchPullRequestNumber))); var connectionManager = CreateConnectionManager(); var target = CreateTarget( @@ -80,7 +80,7 @@ public void CreatesSessionForCurrentBranch() public void CurrentSessionIsNullIfNoPullRequestForCurrentBranch() { var service = CreatePullRequestService(); - service.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs(Observable.Empty>()); + service.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs(Observable.Empty<(string, int)>()); var target = CreateTarget(service: service); @@ -98,7 +98,7 @@ public void CurrentSessionChangesWhenBranchChanges() var session = target.CurrentSession; - service.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs(Observable.Return(Tuple.Create("foo", 22))); + service.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs(Observable.Return(("foo", 22))); teamExplorerContext.StatusChanged += Raise.Event(); Assert.That(session, Is.Not.SameAs(target.CurrentSession)); @@ -124,7 +124,7 @@ public void CurrentSessionChangesToNullIfNoPullRequestForCurrentBranch() teamExplorerContext: teamExplorerContext); Assert.That(target.CurrentSession, Is.Not.Null); - Tuple newPullRequest = null; + (string owner, int number) newPullRequest = default; service.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs(Observable.Return(newPullRequest)); teamExplorerContext.StatusChanged += Raise.Event(); @@ -813,14 +813,14 @@ public async Task GetSessionUpdatesCurrentSessionIfCurrentBranchIsPullRequestBut var model = CreatePullRequestModel(); var sessionService = CreateSessionService(model); - service.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs(Observable.Empty>()); + service.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs(Observable.Empty<(string, int)>()); var target = CreateTarget(service: service, sessionService: sessionService); Assert.That(target.CurrentSession, Is.Null); service.EnsureLocalBranchesAreMarkedAsPullRequests(Arg.Any(), model).Returns(Observable.Return(true)); - service.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs(Observable.Return(Tuple.Create("owner", CurrentBranchPullRequestNumber))); + service.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs(Observable.Return(("owner", CurrentBranchPullRequestNumber))); var session = await target.GetSession("owner", "name", CurrentBranchPullRequestNumber); @@ -881,7 +881,7 @@ PullRequestDetailModel CreatePullRequestModel( IPullRequestService CreatePullRequestService(string owner = "owner") { var result = Substitute.For(); - result.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs(Observable.Return(Tuple.Create(owner, CurrentBranchPullRequestNumber))); + result.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs(Observable.Return((owner, CurrentBranchPullRequestNumber))); return result; } From 42d09faf6ebedcd82052d9e9242f9423973f515f Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 31 Oct 2018 13:30:21 +0000 Subject: [PATCH 17/20] Avoid firing StatusChanged if we're not on a repo --- src/GitHub.App/Services/TeamExplorerContext.cs | 4 ++-- .../Services/TeamExplorerContextTests.cs | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/GitHub.App/Services/TeamExplorerContext.cs b/src/GitHub.App/Services/TeamExplorerContext.cs index 06353a747b..ecbaddf742 100644 --- a/src/GitHub.App/Services/TeamExplorerContext.cs +++ b/src/GitHub.App/Services/TeamExplorerContext.cs @@ -127,9 +127,9 @@ async Task RefreshAsync() log.Debug("Fire StatusChanged event when PullRequest changes for ActiveRepository"); StatusChanged?.Invoke(this, EventArgs.Empty); } - else + else if (newRepositoryPath != null) { - log.Debug("Fire StatusChanged event when ***anything*** changes"); + log.Debug("Fire StatusChanged event when on a repository and anything changes"); StatusChanged?.Invoke(this, EventArgs.Empty); } diff --git a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs index a3fc3b5352..6e202f0aaf 100644 --- a/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs +++ b/test/GitHub.App.UnitTests/Services/TeamExplorerContextTests.cs @@ -151,14 +151,13 @@ public void NoActiveRepositoryChange_SolutionChanges() public class TheStatusChangedEvent { - [TestCase(true, false)] - [TestCase(false, true)] - public void AlwaysFireWhenNoLocalPathChange(bool changePath, bool expectWasRaised) + [TestCase("path", "path", true)] + [TestCase("path1", "path2", false)] + [TestCase(null, null, false)] + public void AlwaysFireWhenNoLocalPathChange(string path1, string path2, bool expectWasRaised) { var gitExt = CreateGitExt(); var repositoryPaths = new[] { Directory.GetCurrentDirectory(), Path.GetTempPath() }; - var path1 = Directory.GetCurrentDirectory(); - var path2 = changePath ? Path.GetTempPath() : path1; var repoInfo1 = CreateRepositoryModel(path1); var repoInfo2 = CreateRepositoryModel(path2); From a47af2b3304b1b58a698ece6a144087b14c5af80 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 31 Oct 2018 15:03:57 +0000 Subject: [PATCH 18/20] Suppress "Do not directly await a Task" warning --- src/GitHub.App/GlobalSuppressions.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/GitHub.App/GlobalSuppressions.cs b/src/GitHub.App/GlobalSuppressions.cs index f635f8ea1d..d3dcbf45d6 100644 --- a/src/GitHub.App/GlobalSuppressions.cs +++ b/src/GitHub.App/GlobalSuppressions.cs @@ -6,6 +6,8 @@ [assembly: SuppressMessage("Microsoft.Naming", "CA1703:ResourceStringsShouldBeSpelledCorrectly", MessageId = "Git", Scope = "resource", Target = "GitHub.App.Resources.resources")] [assembly: SuppressMessage("Microsoft.Globalization", "CA1305:SpecifyIFormatProvider", MessageId = "System.String.Format(System.String,System.Object,System.Object,System.Object)", Scope = "member", Target = "GitHub.Services.PullRequestService.#CreateTempFile(System.String,System.String,System.String)")] [assembly: SuppressMessage("Microsoft.Globalization", "CA1305:SpecifyIFormatProvider", MessageId = "System.String.Format(System.String,System.Object,System.Object,System.Object)", Scope = "member", Target = "GitHub.Services.PullRequestService.#CreateTempFile(System.String,System.String,System.String,System.Text.Encoding)")] +[assembly: SuppressMessage("Reliability", "CA2007:Do not directly await a Task", Justification = "We always need to think about what thread we're running on")] + // This file is used by Code Analysis to maintain SuppressMessage // attributes that are applied to this project. // Project-level suppressions either have no target or are given From db41ef99e5151b82bae548400e3e55d144d6df84 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 31 Oct 2018 16:02:37 +0000 Subject: [PATCH 19/20] Get rid of LocalRepositoryModelFactory We can now use GitService as a LocalRepositoryMode factory. --- .../Models/ILocalRepositoryModelFactory.cs | 15 ------- .../GitHub.TeamFoundation.14.csproj | 1 - .../Services/LocalRepositoryModelFactory.cs | 13 ------ .../Services/VSGitExt.cs | 24 +++++------ .../GitHub.TeamFoundation.15.csproj | 3 -- .../GitHub.TeamFoundation.16.csproj | 3 -- src/GitHub.VisualStudio/GitHubPackage.cs | 2 +- .../Services/VSGitExtFactory.cs | 10 +++-- .../VSGitExtTests.cs | 40 ++++++++++++------- 9 files changed, 44 insertions(+), 67 deletions(-) delete mode 100644 src/GitHub.Exports/Models/ILocalRepositoryModelFactory.cs delete mode 100644 src/GitHub.TeamFoundation.14/Services/LocalRepositoryModelFactory.cs diff --git a/src/GitHub.Exports/Models/ILocalRepositoryModelFactory.cs b/src/GitHub.Exports/Models/ILocalRepositoryModelFactory.cs deleted file mode 100644 index 3c8dfed0ce..0000000000 --- a/src/GitHub.Exports/Models/ILocalRepositoryModelFactory.cs +++ /dev/null @@ -1,15 +0,0 @@ -namespace GitHub.Models -{ - /// - /// A factory for objects. - /// - public interface ILocalRepositoryModelFactory - { - /// - /// Construct a new . - /// - /// The local path for the repository. - /// A new repository model. - ILocalRepositoryModel Create(string localPath); - } -} diff --git a/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj b/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj index a27bc63e3a..d4b56fc610 100644 --- a/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj +++ b/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj @@ -163,7 +163,6 @@ - diff --git a/src/GitHub.TeamFoundation.14/Services/LocalRepositoryModelFactory.cs b/src/GitHub.TeamFoundation.14/Services/LocalRepositoryModelFactory.cs deleted file mode 100644 index 06c4bc9572..0000000000 --- a/src/GitHub.TeamFoundation.14/Services/LocalRepositoryModelFactory.cs +++ /dev/null @@ -1,13 +0,0 @@ -using GitHub.Models; -using GitHub.Services; - -namespace GitHub.TeamFoundation.Services -{ - class LocalRepositoryModelFactory : ILocalRepositoryModelFactory - { - public ILocalRepositoryModel Create(string localPath) - { - return GitService.GitServiceHelper.CreateLocalRepositoryModel(localPath); - } - } -} diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs index d996b9e170..10167940e0 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitExt.cs @@ -27,18 +27,18 @@ public class VSGitExt : IVSGitExt static readonly ILogger log = LogManager.ForContext(); readonly IServiceProvider serviceProvider; - readonly ILocalRepositoryModelFactory repositoryFactory; + readonly IGitService gitService; readonly object refreshLock = new object(); - IGitExt gitService; + IGitExt gitExt; IReadOnlyList activeRepositories; - public VSGitExt(IServiceProvider serviceProvider) - : this(serviceProvider, new VSUIContextFactory(), new LocalRepositoryModelFactory(), ThreadHelper.JoinableTaskContext) + public VSGitExt(IServiceProvider serviceProvider, IGitService gitService) + : this(serviceProvider, new VSUIContextFactory(), gitService, ThreadHelper.JoinableTaskContext) { } - public VSGitExt(IServiceProvider serviceProvider, IVSUIContextFactory factory, ILocalRepositoryModelFactory repositoryFactory, + public VSGitExt(IServiceProvider serviceProvider, IVSUIContextFactory factory, IGitService gitService, JoinableTaskContext joinableTaskContext) { JoinableTaskCollection = joinableTaskContext.CreateCollection(); @@ -46,7 +46,7 @@ public VSGitExt(IServiceProvider serviceProvider, IVSUIContextFactory factory, I JoinableTaskFactory = joinableTaskContext.CreateFactory(JoinableTaskCollection); this.serviceProvider = serviceProvider; - this.repositoryFactory = repositoryFactory; + this.gitService = gitService; // Start with empty array until we have a chance to initialize. ActiveRepositories = Array.Empty(); @@ -64,7 +64,7 @@ public VSGitExt(IServiceProvider serviceProvider, IVSUIContextFactory factory, I async Task InitializeAsync() { - gitService = await GetServiceAsync(); + gitExt = await GetServiceAsync(); if (gitService == null) { log.Error("Couldn't find IGitExt service"); @@ -74,9 +74,9 @@ async Task InitializeAsync() // Refresh on background thread await Task.Run(() => RefreshActiveRepositories()); - gitService.PropertyChanged += (s, e) => + gitExt.PropertyChanged += (s, e) => { - if (e.PropertyName == nameof(gitService.ActiveRepositories)) + if (e.PropertyName == nameof(gitExt.ActiveRepositories)) { RefreshActiveRepositories(); } @@ -91,10 +91,10 @@ public void RefreshActiveRepositories() { log.Debug( "IGitExt.ActiveRepositories (#{Id}) returned {Repositories}", - gitService.GetHashCode(), - gitService.ActiveRepositories.Select(x => x.RepositoryPath)); + gitExt.GetHashCode(), + gitExt.ActiveRepositories.Select(x => x.RepositoryPath)); - ActiveRepositories = gitService?.ActiveRepositories.Select(x => repositoryFactory.Create(x.RepositoryPath)).ToList(); + ActiveRepositories = gitExt?.ActiveRepositories.Select(x => gitService.CreateLocalRepositoryModel(x.RepositoryPath)).ToList(); } } catch (Exception e) diff --git a/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj b/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj index 0f872555d1..b8a8b29374 100644 --- a/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj +++ b/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj @@ -180,9 +180,6 @@ RegistryHelper.cs - - Services\LocalRepositoryModelFactory.cs - Services\VSGitExt.cs diff --git a/src/GitHub.TeamFoundation.16/GitHub.TeamFoundation.16.csproj b/src/GitHub.TeamFoundation.16/GitHub.TeamFoundation.16.csproj index 5e727e3fed..5f7b3d4c9a 100644 --- a/src/GitHub.TeamFoundation.16/GitHub.TeamFoundation.16.csproj +++ b/src/GitHub.TeamFoundation.16/GitHub.TeamFoundation.16.csproj @@ -180,9 +180,6 @@ RegistryHelper.cs - - Services\LocalRepositoryModelFactory.cs - Services\VSGitExt.cs diff --git a/src/GitHub.VisualStudio/GitHubPackage.cs b/src/GitHub.VisualStudio/GitHubPackage.cs index 741b9ccfe8..f8fff137dd 100644 --- a/src/GitHub.VisualStudio/GitHubPackage.cs +++ b/src/GitHub.VisualStudio/GitHubPackage.cs @@ -299,7 +299,7 @@ async Task CreateService(IAsyncServiceContainer container, CancellationT else if (serviceType == typeof(IVSGitExt)) { var vsVersion = ApplicationInfo.GetHostVersionInfo().FileMajorPart; - return new VSGitExtFactory(vsVersion, this).Create(); + return new VSGitExtFactory(vsVersion, this, GitService.GitServiceHelper).Create(); } else if (serviceType == typeof(IGitHubToolWindowManager)) { diff --git a/src/GitHub.VisualStudio/Services/VSGitExtFactory.cs b/src/GitHub.VisualStudio/Services/VSGitExtFactory.cs index 928eb96d1d..1464d32c02 100644 --- a/src/GitHub.VisualStudio/Services/VSGitExtFactory.cs +++ b/src/GitHub.VisualStudio/Services/VSGitExtFactory.cs @@ -17,11 +17,13 @@ public class VSGitExtFactory readonly int vsVersion; readonly IServiceProvider serviceProvider; + readonly IGitService gitService; - public VSGitExtFactory(int vsVersion, IServiceProvider serviceProvider) + public VSGitExtFactory(int vsVersion, IServiceProvider serviceProvider, IGitService gitService) { this.vsVersion = vsVersion; this.serviceProvider = serviceProvider; + this.gitService = gitService; } public IVSGitExt Create() @@ -29,11 +31,11 @@ public IVSGitExt Create() switch (vsVersion) { case 14: - return Create(() => new VSGitExt14(serviceProvider)); + return Create(() => new VSGitExt14(serviceProvider, gitService)); case 15: - return Create(() => new VSGitExt15(serviceProvider)); + return Create(() => new VSGitExt15(serviceProvider, gitService)); case 16: - return Create(() => new VSGitExt16(serviceProvider)); + return Create(() => new VSGitExt16(serviceProvider, gitService)); default: log.Error("There is no IVSGitExt implementation for DTE version {Version}", vsVersion); return null; diff --git a/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs b/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs index 7b9ee71c58..283bc2f76d 100644 --- a/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs +++ b/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs @@ -10,10 +10,12 @@ using NUnit.Framework; using NSubstitute; using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; -using Microsoft.VisualStudio.Shell; using Microsoft.VisualStudio.Threading; using Task = System.Threading.Tasks.Task; using static Microsoft.VisualStudio.VSConstants; +using GitHub.Primitives; +using LibGit2Sharp; +using System.Threading.Tasks; public class VSGitExtTests { @@ -149,32 +151,32 @@ public void RepositoryOpenContextNotActive_IsEmpty() public void RepositoryOpenIsActive_InitializeWithActiveRepositories() { var repoPath = "repoPath"; - var repoFactory = Substitute.For(); + var gitService = Substitute.For(); var context = CreateVSUIContext(true); var gitExt = CreateGitExt(new[] { repoPath }); - var target = CreateVSGitExt(context, gitExt, repoFactory: repoFactory); + var target = CreateVSGitExt(context, gitExt, gitService: gitService); target.JoinTillEmpty(); var activeRepositories = target.ActiveRepositories; Assert.That(activeRepositories.Count, Is.EqualTo(1)); - repoFactory.Received(1).Create(repoPath); + gitService.Received(1).CreateLocalRepositoryModel(repoPath); } [Test] public void ExceptionRefreshingRepositories_ReturnsEmptyList() { var repoPath = "repoPath"; - var repoFactory = Substitute.For(); - repoFactory.Create(repoPath).ReturnsForAnyArgs(x => { throw new Exception("Boom!"); }); + var gitService = Substitute.For(); + gitService.CreateLocalRepositoryModel(repoPath).ReturnsForAnyArgs(x => { throw new Exception("Boom!"); }); var context = CreateVSUIContext(true); var gitExt = CreateGitExt(new[] { repoPath }); - var target = CreateVSGitExt(context, gitExt, repoFactory: repoFactory); + var target = CreateVSGitExt(context, gitExt, gitService: gitService); target.JoinTillEmpty(); var activeRepositories = target.ActiveRepositories; - repoFactory.Received(1).Create(repoPath); + gitService.Received(1).CreateLocalRepositoryModel(repoPath); Assert.That(activeRepositories.Count, Is.EqualTo(0)); } @@ -182,8 +184,8 @@ public void ExceptionRefreshingRepositories_ReturnsEmptyList() public async Task ActiveRepositoriesChangedOrderingShouldBeCorrectAcrossThreads() { var gitExt = new MockGitExt(); - var repoFactory = new MockRepositoryFactory(); - var target = CreateVSGitExt(gitExt: gitExt, repoFactory: repoFactory); + var gitService = new MockGitService(); + var target = CreateVSGitExt(gitExt: gitExt, gitService: gitService); var activeRepositories1 = CreateActiveRepositories("repo1"); var activeRepositories2 = CreateActiveRepositories("repo2"); var task1 = Task.Run(() => gitExt.ActiveRepositories = activeRepositories1); @@ -211,18 +213,18 @@ static IReadOnlyList CreateActiveRepositories(params string[ } static VSGitExt CreateVSGitExt(IVSUIContext context = null, IGitExt gitExt = null, IServiceProvider sp = null, - ILocalRepositoryModelFactory repoFactory = null, JoinableTaskContext joinableTaskContext = null, string contextGuidString = null) + IGitService gitService = null, JoinableTaskContext joinableTaskContext = null, string contextGuidString = null) { context = context ?? CreateVSUIContext(true); gitExt = gitExt ?? CreateGitExt(); var contextGuid = new Guid(contextGuidString ?? Guids.GitSccProviderId); sp = sp ?? Substitute.For(); - repoFactory = repoFactory ?? Substitute.For(); + gitService = gitService ?? Substitute.For(); joinableTaskContext = joinableTaskContext ?? new JoinableTaskContext(); var factory = Substitute.For(); factory.GetUIContext(contextGuid).Returns(context); sp.GetService(typeof(IGitExt)).Returns(gitExt); - var vsGitExt = new VSGitExt(sp, factory, repoFactory, joinableTaskContext); + var vsGitExt = new VSGitExt(sp, factory, gitService, joinableTaskContext); vsGitExt.JoinTillEmpty(); return vsGitExt; } @@ -291,9 +293,9 @@ public IReadOnlyList ActiveRepositories public event PropertyChangedEventHandler PropertyChanged; } - class MockRepositoryFactory : ILocalRepositoryModelFactory + class MockGitService : IGitService { - public ILocalRepositoryModel Create(string localPath) + public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) { var result = Substitute.For(); result.LocalPath.Returns(localPath); @@ -308,5 +310,13 @@ public ILocalRepositoryModel Create(string localPath) return result; } + + public IBranch CreateCurrentBranchModel(ILocalRepositoryModel model) => throw new NotImplementedException(); + public Task GetLatestPushedSha(string path, string remote = "origin") => throw new NotImplementedException(); + public UriString GetRemoteUri(IRepository repo, string remote = "origin") => throw new NotImplementedException(); + public IRepository GetRepository(string path) => throw new NotImplementedException(); + public UriString GetUri(IRepository repository, string remote = "origin") => throw new NotImplementedException(); + public UriString GetUri(string path, string remote = "origin") => throw new NotImplementedException(); + public void Refresh(ILocalRepositoryModel localRepositoryModel) => throw new NotImplementedException(); } } From a4c83863b94da433229118c29ba7f49a80455e06 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 31 Oct 2018 17:36:04 +0000 Subject: [PATCH 20/20] Rename IGitService.Refresh to RefreshCloneUrl Be explicit about what it does. --- src/GitHub.App/SampleData/GitServiceDesigner.cs | 2 +- src/GitHub.Exports/Services/GitService.cs | 2 +- src/GitHub.Exports/Services/IGitService.cs | 2 +- src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs | 2 +- test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/GitHub.App/SampleData/GitServiceDesigner.cs b/src/GitHub.App/SampleData/GitServiceDesigner.cs index 6d90eaacbb..014b79e3fa 100644 --- a/src/GitHub.App/SampleData/GitServiceDesigner.cs +++ b/src/GitHub.App/SampleData/GitServiceDesigner.cs @@ -10,7 +10,7 @@ class GitServiceDesigner : IGitService { public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) => null; public IBranch CreateCurrentBranchModel(ILocalRepositoryModel model) => null; - public void Refresh(ILocalRepositoryModel localRepositoryModel) { } + public void RefreshCloneUrl(ILocalRepositoryModel localRepositoryModel) { } public Task GetLatestPushedSha(string path, string remote = "origin") => Task.FromResult(null); public UriString GetRemoteUri(IRepository repo, string remote = "origin") => null; public IRepository GetRepository(string path) => null; diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index 2fee66dff7..053821dfce 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -71,7 +71,7 @@ public IBranch CreateCurrentBranchModel(ILocalRepositoryModel model) /// /// Updates the CloneUrl from the "origin" remote. /// - public void Refresh(ILocalRepositoryModel localRepositoryModel) + public void RefreshCloneUrl(ILocalRepositoryModel localRepositoryModel) { var localPath = localRepositoryModel.LocalPath; if (localPath == null) diff --git a/src/GitHub.Exports/Services/IGitService.cs b/src/GitHub.Exports/Services/IGitService.cs index 96a0f044dc..85209e34dc 100644 --- a/src/GitHub.Exports/Services/IGitService.cs +++ b/src/GitHub.Exports/Services/IGitService.cs @@ -25,7 +25,7 @@ public interface IGitService /// Updates the CloneUrl information based on the "origin" remote. /// /// The repository model to refresh. - void Refresh(ILocalRepositoryModel localRepositoryModel); + void RefreshCloneUrl(ILocalRepositoryModel localRepositoryModel); /// /// Returns the URL of the remote for the specified . If the repository diff --git a/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs b/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs index db91cfbe9b..d4f7b0ba6d 100644 --- a/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs +++ b/src/GitHub.TeamFoundation.14/Base/TeamExplorerServiceHolder.cs @@ -113,7 +113,7 @@ public void Subscribe(object who, Action handler) // for that, so this is a good place to refresh it in case that happened if (repo != null) { - gitService.Refresh(repo); + gitService.RefreshCloneUrl(repo); } // if the active repo hasn't changed and there's notifications queued up, diff --git a/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs b/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs index 283bc2f76d..afe09c6cd8 100644 --- a/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs +++ b/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs @@ -317,6 +317,6 @@ public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) public IRepository GetRepository(string path) => throw new NotImplementedException(); public UriString GetUri(IRepository repository, string remote = "origin") => throw new NotImplementedException(); public UriString GetUri(string path, string remote = "origin") => throw new NotImplementedException(); - public void Refresh(ILocalRepositoryModel localRepositoryModel) => throw new NotImplementedException(); + public void RefreshCloneUrl(ILocalRepositoryModel localRepositoryModel) => throw new NotImplementedException(); } }