From e1b5f6a57cb4de33d0df71b4dccfdf1c0791b198 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 23 Nov 2017 14:58:01 +0000 Subject: [PATCH 01/10] =?UTF-8?q?=C2=ADKeep=C2=AD=20alive=20reference=20to?= =?UTF-8?q?=20Repository=20and=20Dispose?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By wrapping the Repository object in a using statement we prevent it from being being GCed when there are only native pointers to it. --- src/GitHub.App/Services/PullRequestService.cs | 312 ++++++++++-------- .../LocalRepositoryModelExtensions.cs | 6 +- .../Models/LocalRepositoryModel.cs | 13 +- src/GitHub.Exports/Services/GitService.cs | 46 +-- src/GitHub.VisualStudio/Menus/MenuBase.cs | 8 +- 5 files changed, 216 insertions(+), 169 deletions(-) diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs index d2e38c64ae..96fefc65cd 100644 --- a/src/GitHub.App/Services/PullRequestService.cs +++ b/src/GitHub.App/Services/PullRequestService.cs @@ -95,8 +95,11 @@ public IObservable> GetMessagesForUniqueCommits( { return Observable.Defer(() => { - var repo = gitService.GetRepository(repository.LocalPath); - return gitClient.GetMessagesForUniqueCommits(repo, baseBranch, compareBranch, maxCommits).ToObservable(); + // CommitMessage doesn't keep a reference to Repository + using (var repo = gitService.GetRepository(repository.LocalPath)) + { + return gitClient.GetMessagesForUniqueCommits(repo, baseBranch, compareBranch, maxCommits).ToObservable(); + } }); } @@ -129,8 +132,10 @@ public IObservable Pull(ILocalRepositoryModel repository) { return Observable.Defer(() => { - var repo = gitService.GetRepository(repository.LocalPath); - return gitClient.Pull(repo).ToObservable(); + using (var repo = gitService.GetRepository(repository.LocalPath)) + { + return gitClient.Pull(repo).ToObservable(); + } }); } @@ -138,10 +143,12 @@ public IObservable Push(ILocalRepositoryModel repository) { return Observable.Defer(async () => { - var repo = gitService.GetRepository(repository.LocalPath); - var remoteName = repo.Head.RemoteName; - var remote = await gitClient.GetHttpRemote(repo, remoteName); - return gitClient.Push(repo, repo.Head.TrackedBranch.UpstreamBranchCanonicalName, remote.Name).ToObservable(); + using (var repo = gitService.GetRepository(repository.LocalPath)) + { + var remoteName = repo.Head.RemoteName; + var remote = await gitClient.GetHttpRemote(repo, remoteName); + return gitClient.Push(repo, repo.Head.TrackedBranch.UpstreamBranchCanonicalName, remote.Name).ToObservable(); + } }); } @@ -149,35 +156,37 @@ public IObservable Checkout(ILocalRepositoryModel repository, IPullRequest { return Observable.Defer(async () => { - var repo = gitService.GetRepository(repository.LocalPath); - var existing = repo.Branches[localBranchName]; - - if (existing != null) + using (var repo = gitService.GetRepository(repository.LocalPath)) { - await gitClient.Checkout(repo, localBranchName); - } - else if (repository.CloneUrl.ToRepositoryUrl() == pullRequest.Head.RepositoryCloneUrl.ToRepositoryUrl()) - { - var remote = await gitClient.GetHttpRemote(repo, "origin"); - await gitClient.Fetch(repo, remote.Name); - await gitClient.Checkout(repo, localBranchName); - } - else - { - var refSpec = $"{pullRequest.Head.Ref}:{localBranchName}"; - var remoteName = await CreateRemote(repo, pullRequest.Head.RepositoryCloneUrl); + var existing = repo.Branches[localBranchName]; - await gitClient.Fetch(repo, remoteName); - await gitClient.Fetch(repo, remoteName, new[] { refSpec }); - await gitClient.Checkout(repo, localBranchName); - await gitClient.SetTrackingBranch(repo, localBranchName, $"refs/remotes/{remoteName}/{pullRequest.Head.Ref}"); - } + if (existing != null) + { + await gitClient.Checkout(repo, localBranchName); + } + else if (repository.CloneUrl.ToRepositoryUrl() == pullRequest.Head.RepositoryCloneUrl.ToRepositoryUrl()) + { + var remote = await gitClient.GetHttpRemote(repo, "origin"); + await gitClient.Fetch(repo, remote.Name); + await gitClient.Checkout(repo, localBranchName); + } + else + { + var refSpec = $"{pullRequest.Head.Ref}:{localBranchName}"; + var remoteName = await CreateRemote(repo, pullRequest.Head.RepositoryCloneUrl); - // Store the PR number in the branch config with the key "ghfvs-pr". - var prConfigKey = $"branch.{localBranchName}.{SettingGHfVSPullRequest}"; - await gitClient.SetConfig(repo, prConfigKey, BuildGHfVSConfigKeyValue(pullRequest)); + await gitClient.Fetch(repo, remoteName); + await gitClient.Fetch(repo, remoteName, new[] { refSpec }); + await gitClient.Checkout(repo, localBranchName); + await gitClient.SetTrackingBranch(repo, localBranchName, $"refs/remotes/{remoteName}/{pullRequest.Head.Ref}"); + } - return Observable.Return(Unit.Default); + // Store the PR number in the branch config with the key "ghfvs-pr". + var prConfigKey = $"branch.{localBranchName}.{SettingGHfVSPullRequest}"; + await gitClient.SetConfig(repo, prConfigKey, BuildGHfVSConfigKeyValue(pullRequest)); + + return Observable.Return(Unit.Default); + } }); } @@ -187,12 +196,14 @@ public IObservable GetDefaultLocalBranchName(ILocalRepositoryModel repos { var initial = "pr/" + pullRequestNumber + "-" + GetSafeBranchName(pullRequestTitle); var current = initial; - var repo = gitService.GetRepository(repository.LocalPath); - var index = 2; - - while (repo.Branches[current] != null) + using (var repo = gitService.GetRepository(repository.LocalPath)) { - current = initial + '-' + index++; + var index = 2; + + while (repo.Branches[current] != null) + { + current = initial + '-' + index++; + } } return Observable.Return(current.TrimEnd('-')); @@ -203,15 +214,18 @@ public IObservable CalculateHistoryDivergence(ILocalRepos { return Observable.Defer(async () => { - var repo = gitService.GetRepository(repository.LocalPath); - var remoteName = repo.Head.RemoteName; - if (remoteName != null) + // TODO: Find out if BranchTrackingDetails depends on Repository + using (var repo = gitService.GetRepository(repository.LocalPath)) { - var remote = await gitClient.GetHttpRemote(repo, remoteName); - await gitClient.Fetch(repo, remote.Name); - } + var remoteName = repo.Head.RemoteName; + if (remoteName != null) + { + var remote = await gitClient.GetHttpRemote(repo, remoteName); + await gitClient.Fetch(repo, remote.Name); + } - return Observable.Return(repo.Head.TrackingDetails); + return Observable.Return(repo.Head.TrackingDetails); + } }); } @@ -219,11 +233,14 @@ public IObservable GetTreeChanges(ILocalRepositoryModel repository, { return Observable.Defer(async () => { - var repo = gitService.GetRepository(repository.LocalPath); - var remote = await gitClient.GetHttpRemote(repo, "origin"); - await gitClient.Fetch(repo, remote.Name); - var changes = await gitClient.Compare(repo, pullRequest.Base.Sha, pullRequest.Head.Sha, detectRenames: true); - return Observable.Return(changes); + // TODO: Find out if TreeChanges depends on Repository + using (var repo = gitService.GetRepository(repository.LocalPath)) + { + var remote = await gitClient.GetHttpRemote(repo, "origin"); + await gitClient.Fetch(repo, remote.Name); + var changes = await gitClient.Compare(repo, pullRequest.Base.Sha, pullRequest.Head.Sha, detectRenames: true); + return Observable.Return(changes); + } }); } @@ -231,9 +248,12 @@ public IObservable GetLocalBranches(ILocalRepositoryModel repository, I { return Observable.Defer(() => { - var repo = gitService.GetRepository(repository.LocalPath); - var result = GetLocalBranchesInternal(repository, repo, pullRequest).Select(x => new BranchModel(x, repository)); - return result.ToObservable(); + // TODO: Find out if IBranch depends on Repository + using (var repo = gitService.GetRepository(repository.LocalPath)) + { + var result = GetLocalBranchesInternal(repository, repo, pullRequest).Select(x => new BranchModel(x, repository)); + return result.ToObservable(); + } }); } @@ -241,20 +261,22 @@ public IObservable EnsureLocalBranchesAreMarkedAsPullRequests(ILocalReposi { return Observable.Defer(async () => { - var repo = gitService.GetRepository(repository.LocalPath); - var branches = GetLocalBranchesInternal(repository, repo, pullRequest).Select(x => new BranchModel(x, repository)); - var result = false; - - foreach (var branch in branches) + using (var repo = gitService.GetRepository(repository.LocalPath)) { - if (!await IsBranchMarkedAsPullRequest(repo, branch.Name, pullRequest)) + var branches = GetLocalBranchesInternal(repository, repo, pullRequest).Select(x => new BranchModel(x, repository)); + var result = false; + + foreach (var branch in branches) { - await MarkBranchAsPullRequest(repo, branch.Name, pullRequest); - result = true; + if (!await IsBranchMarkedAsPullRequest(repo, branch.Name, pullRequest)) + { + await MarkBranchAsPullRequest(repo, branch.Name, pullRequest); + result = true; + } } - } - return Observable.Return(result); + return Observable.Return(result); + } }); } @@ -272,36 +294,39 @@ public IObservable SwitchToBranch(ILocalRepositoryModel repository, IPullR { return Observable.Defer(async () => { - var repo = gitService.GetRepository(repository.LocalPath); - var branchName = GetLocalBranchesInternal(repository, repo, pullRequest).FirstOrDefault(); - - Log.Assert(branchName != null, "PullRequestService.SwitchToBranch called but no local branch found"); - - if (branchName != null) + using (var repo = gitService.GetRepository(repository.LocalPath)) { - var remote = await gitClient.GetHttpRemote(repo, "origin"); - await gitClient.Fetch(repo, remote.Name); + var branchName = GetLocalBranchesInternal(repository, repo, pullRequest).FirstOrDefault(); - var branch = repo.Branches[branchName]; + Log.Assert(branchName != null, "PullRequestService.SwitchToBranch called but no local branch found"); - if (branch == null) + if (branchName != null) { - var trackedBranchName = $"refs/remotes/{remote.Name}/" + branchName; - var trackedBranch = repo.Branches[trackedBranchName]; + var remote = await gitClient.GetHttpRemote(repo, "origin"); + await gitClient.Fetch(repo, remote.Name); - if (trackedBranch != null) - { - branch = repo.CreateBranch(branchName, trackedBranch.Tip); - await gitClient.SetTrackingBranch(repo, branchName, trackedBranchName); - } - else + var branch = repo.Branches[branchName]; + + if (branch == null) { - throw new InvalidOperationException($"Could not find branch '{trackedBranchName}'."); + var trackedBranchName = $"refs/remotes/{remote.Name}/" + branchName; + var trackedBranch = repo.Branches[trackedBranchName]; + + if (trackedBranch != null) + { + branch = repo.CreateBranch(branchName, trackedBranch.Tip); + await gitClient.SetTrackingBranch(repo, branchName, trackedBranchName); + } + else + { + throw new InvalidOperationException($"Could not find branch '{trackedBranchName}'."); + } } - } - await gitClient.Checkout(repo, branchName); - await MarkBranchAsPullRequest(repo, branchName, pullRequest); + await gitClient.Checkout(repo, branchName); + await MarkBranchAsPullRequest(repo, branchName, pullRequest); + + } } return Observable.Return(Unit.Default); @@ -312,14 +337,16 @@ public IObservable> GetPullRequestForCurrentBranch(ILocalRepo { return Observable.Defer(async () => { - var repo = gitService.GetRepository(repository.LocalPath); - var configKey = string.Format( - CultureInfo.InvariantCulture, - "branch.{0}.{1}", - repo.Head.FriendlyName, - SettingGHfVSPullRequest); - var value = await gitClient.GetConfig(repo, configKey); - return Observable.Return(ParseGHfVSConfigKeyValue(value)); + using (var repo = gitService.GetRepository(repository.LocalPath)) + { + var configKey = string.Format( + CultureInfo.InvariantCulture, + "branch.{0}.{1}", + repo.Head.FriendlyName, + SettingGHfVSPullRequest); + var value = await gitClient.GetConfig(repo, configKey); + return Observable.Return(ParseGHfVSConfigKeyValue(value)); + } }); } @@ -332,34 +359,36 @@ public IObservable ExtractFile( { return Observable.Defer(async () => { - var repo = gitService.GetRepository(repository.LocalPath); - var remote = await gitClient.GetHttpRemote(repo, "origin"); - string sha; - - if (head) + using (var repo = gitService.GetRepository(repository.LocalPath)) { - sha = pullRequest.Head.Sha; - } - else - { - try + var remote = await gitClient.GetHttpRemote(repo, "origin"); + string sha; + + if (head) { - sha = await gitClient.GetPullRequestMergeBase( - repo, - pullRequest.Base.RepositoryCloneUrl, - pullRequest.Base.Sha, - pullRequest.Head.Sha, - pullRequest.Base.Ref, - pullRequest.Number); + sha = pullRequest.Head.Sha; } - catch (NotFoundException ex) + else { - throw new NotFoundException($"The Pull Request file failed to load. Please check your network connection and click refresh to try again. If this issue persists, please let us know at support@github.com", ex); + try + { + sha = await gitClient.GetPullRequestMergeBase( + repo, + pullRequest.Base.RepositoryCloneUrl, + pullRequest.Base.Sha, + pullRequest.Head.Sha, + pullRequest.Base.Ref, + pullRequest.Number); + } + catch (NotFoundException ex) + { + throw new NotFoundException($"The Pull Request file failed to load. Please check your network connection and click refresh to try again. If this issue persists, please let us know at support@github.com", ex); + } } - } - var file = await ExtractToTempFile(repo, pullRequest.Number, sha, fileName, encoding); - return Observable.Return(file); + var file = await ExtractToTempFile(repo, pullRequest.Number, sha, fileName, encoding); + return Observable.Return(file); + } }); } @@ -399,24 +428,26 @@ public IObservable RemoveUnusedRemotes(ILocalRepositoryModel repository) { return Observable.Defer(async () => { - var repo = gitService.GetRepository(repository.LocalPath); - var usedRemotes = new HashSet( - repo.Branches - .Where(x => !x.IsRemote && x.RemoteName != null) - .Select(x => x.RemoteName)); - - foreach (var remote in repo.Network.Remotes) + using (var repo = gitService.GetRepository(repository.LocalPath)) { - var key = $"remote.{remote.Name}.{SettingCreatedByGHfVS}"; - var createdByUs = await gitClient.GetConfig(repo, key); + var usedRemotes = new HashSet( + repo.Branches + .Where(x => !x.IsRemote && x.RemoteName != null) + .Select(x => x.RemoteName)); - if (createdByUs && !usedRemotes.Contains(remote.Name)) + foreach (var remote in repo.Network.Remotes) { - repo.Network.Remotes.Remove(remote.Name); + var key = $"remote.{remote.Name}.{SettingCreatedByGHfVS}"; + var createdByUs = await gitClient.GetConfig(repo, key); + + if (createdByUs && !usedRemotes.Contains(remote.Name)) + { + repo.Network.Remotes.Remove(remote.Name); + } } - } - return Observable.Return(Unit.Default); + return Observable.Return(Unit.Default); + } }); } @@ -525,20 +556,23 @@ async Task PushAndCreatePR(IModelService modelService, IBranch sourceBranch, IBranch targetBranch, string title, string body) { - var repo = await Task.Run(() => gitService.GetRepository(sourceRepository.LocalPath)); - var remote = await gitClient.GetHttpRemote(repo, "origin"); - await gitClient.Push(repo, sourceBranch.Name, remote.Name); + // TODO: Find out if IPullRequestModel depends on Repository + using (var repo = await Task.Run(() => gitService.GetRepository(sourceRepository.LocalPath))) + { + var remote = await gitClient.GetHttpRemote(repo, "origin"); + await gitClient.Push(repo, sourceBranch.Name, remote.Name); - if (!repo.Branches[sourceBranch.Name].IsTracking) - await gitClient.SetTrackingBranch(repo, sourceBranch.Name, remote.Name); + if (!repo.Branches[sourceBranch.Name].IsTracking) + await gitClient.SetTrackingBranch(repo, sourceBranch.Name, remote.Name); - // delay things a bit to avoid a race between pushing a new branch and creating a PR on it - if (!Splat.ModeDetector.Current.InUnitTestRunner().GetValueOrDefault()) - await Task.Delay(TimeSpan.FromSeconds(5)); + // delay things a bit to avoid a race between pushing a new branch and creating a PR on it + if (!Splat.ModeDetector.Current.InUnitTestRunner().GetValueOrDefault()) + await Task.Delay(TimeSpan.FromSeconds(5)); - var ret = await modelService.CreatePullRequest(sourceRepository, targetRepository, sourceBranch, targetBranch, title, body); - await usageTracker.IncrementCounter(x => x.NumberOfUpstreamPullRequests); - return ret; + var ret = await modelService.CreatePullRequest(sourceRepository, targetRepository, sourceBranch, targetBranch, title, body); + await usageTracker.IncrementCounter(x => x.NumberOfUpstreamPullRequests); + return ret; + } } static string GetSafeBranchName(string name) diff --git a/src/GitHub.Exports/Extensions/LocalRepositoryModelExtensions.cs b/src/GitHub.Exports/Extensions/LocalRepositoryModelExtensions.cs index f065f5f33b..4cb75caa17 100644 --- a/src/GitHub.Exports/Extensions/LocalRepositoryModelExtensions.cs +++ b/src/GitHub.Exports/Extensions/LocalRepositoryModelExtensions.cs @@ -10,8 +10,10 @@ public static class LocalRepositoryModelExtensions { public static bool HasCommits(this ILocalRepositoryModel repository) { - var repo = GitService.GitServiceHelper.GetRepository(repository.LocalPath); - return repo?.Commits.Any() ?? false; + using (var repo = GitService.GitServiceHelper.GetRepository(repository.LocalPath)) + { + return repo?.Commits.Any() ?? false; + } } public static bool MightContainSolution(this ILocalRepositoryModel repository) diff --git a/src/GitHub.Exports/Models/LocalRepositoryModel.cs b/src/GitHub.Exports/Models/LocalRepositoryModel.cs index 5d7a83e315..010274202d 100644 --- a/src/GitHub.Exports/Models/LocalRepositoryModel.cs +++ b/src/GitHub.Exports/Models/LocalRepositoryModel.cs @@ -157,8 +157,10 @@ public string HeadSha { get { - var repo = GitService.GitServiceHelper.GetRepository(LocalPath); - return repo?.Commits.FirstOrDefault()?.Sha ?? String.Empty; + using (var repo = GitService.GitServiceHelper.GetRepository(LocalPath)) + { + return repo?.Commits.FirstOrDefault()?.Sha ?? string.Empty; + } } } @@ -169,8 +171,11 @@ public IBranch CurrentBranch { get { - var repo = GitService.GitServiceHelper.GetRepository(LocalPath); - return new BranchModel(repo?.Head, this); + // BranchModel doesn't keep a reference to Repository + using (var repo = GitService.GitServiceHelper.GetRepository(LocalPath)) + { + return new BranchModel(repo?.Head, this); + } } } diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index 9b1cc4bc0d..a7e31eb852 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -38,7 +38,10 @@ public UriString GetUri(IRepository repository, string remote = "origin") /// Returns a representing the uri of the remote normalized to a GitHub repository url or null if none found. public UriString GetUri(string path, string remote = "origin") { - return GetUri(GetRepository(path), remote); + using (var repo = GetRepository(path)) + { + return GetUri(repo, remote); + } } /// @@ -82,29 +85,30 @@ public UriString GetRemoteUri(IRepository repo, string remote = "origin") public Task GetLatestPushedSha(string path) { Guard.ArgumentNotNull(path, nameof(path)); - var repo = GetRepository(path); - - if (repo == null) - return null; - - if (repo.Head.IsTracking && repo.Head.Tip.Sha == repo.Head.TrackedBranch.Tip.Sha) + using (var repo = GetRepository(path)) { - return Task.FromResult(repo.Head.Tip.Sha); - } + if (repo == null) + return null; + + if (repo.Head.IsTracking && repo.Head.Tip.Sha == repo.Head.TrackedBranch.Tip.Sha) + { + return Task.FromResult(repo.Head.Tip.Sha); + } - return Task.Factory.StartNew(() => - { - var remoteHeads = repo.Refs.Where(r => r.IsRemoteTrackingBranch).ToList(); + return Task.Factory.StartNew(() => + { + var remoteHeads = repo.Refs.Where(r => r.IsRemoteTrackingBranch).ToList(); - foreach (var c in repo.Commits) - { - if (repo.Refs.ReachableFrom(remoteHeads, new[] { c }).Any()) - { - return c.Sha; - } - } - return null; - }); + foreach (var c in repo.Commits) + { + if (repo.Refs.ReachableFrom(remoteHeads, new[] { c }).Any()) + { + return c.Sha; + } + } + return null; + }); + } } } } diff --git a/src/GitHub.VisualStudio/Menus/MenuBase.cs b/src/GitHub.VisualStudio/Menus/MenuBase.cs index 3f05733ab5..68292f8bd6 100644 --- a/src/GitHub.VisualStudio/Menus/MenuBase.cs +++ b/src/GitHub.VisualStudio/Menus/MenuBase.cs @@ -39,7 +39,7 @@ protected ISimpleApiClient SimpleApiClient protected ISimpleApiClientFactory ApiFactory => apiFactory.Value; protected MenuBase() - {} + { } protected MenuBase(IGitHubServiceProvider serviceProvider) { @@ -55,8 +55,10 @@ protected ILocalRepositoryModel GetRepositoryByPath(string path) { if (!string.IsNullOrEmpty(path)) { - var repo = ServiceProvider.TryGetService().GetRepository(path); - return new LocalRepositoryModel(repo.Info.WorkingDirectory.TrimEnd('\\')); + using (var repo = ServiceProvider.TryGetService().GetRepository(path)) + { + return new LocalRepositoryModel(repo.Info.WorkingDirectory.TrimEnd('\\')); + } } } catch (Exception ex) From 7cfbda3b931337cea42b6d0c2056493e8eec7d8c Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 23 Nov 2017 15:09:30 +0000 Subject: [PATCH 02/10] Test GitService.GetRepository in a consistent way Use with a using statement. --- .../Services/PullRequestServiceTests.cs | 86 ++++++++++--------- 1 file changed, 45 insertions(+), 41 deletions(-) diff --git a/test/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs b/test/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs index 4152c634a3..6111ef4b9a 100644 --- a/test/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs +++ b/test/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs @@ -591,21 +591,23 @@ public async void ShouldUseUniquelyNamedRemoteForFork() var localRepo = Substitute.For(); localRepo.CloneUrl.Returns(new UriString("https://foo.bar/owner/repo")); - var repo = gitService.GetRepository(localRepo.CloneUrl); - var remote = Substitute.For(); - var remoteCollection = Substitute.For(); - remoteCollection["fork"].Returns(remote); - repo.Network.Remotes.Returns(remoteCollection); + using (var repo = gitService.GetRepository(localRepo.CloneUrl)) + { + var remote = Substitute.For(); + var remoteCollection = Substitute.For(); + remoteCollection["fork"].Returns(remote); + repo.Network.Remotes.Returns(remoteCollection); - var pr = Substitute.For(); - pr.Number.Returns(5); - pr.Base.Returns(new GitReferenceModel("master", "owner:master", "123", "https://foo.bar/owner/repo.git")); - pr.Head.Returns(new GitReferenceModel("prbranch", "fork:prbranch", "123", "https://foo.bar/fork/repo.git")); + var pr = Substitute.For(); + pr.Number.Returns(5); + pr.Base.Returns(new GitReferenceModel("master", "owner:master", "123", "https://foo.bar/owner/repo.git")); + pr.Head.Returns(new GitReferenceModel("prbranch", "fork:prbranch", "123", "https://foo.bar/fork/repo.git")); - await service.Checkout(localRepo, pr, "pr/5-fork-branch"); + await service.Checkout(localRepo, pr, "pr/5-fork-branch"); - gitClient.Received().SetRemote(Arg.Any(), "fork1", new Uri("https://foo.bar/fork/repo.git")).Forget(); - gitClient.Received().SetConfig(Arg.Any(), "remote.fork1.created-by-ghfvs", "true").Forget(); + gitClient.Received().SetRemote(Arg.Any(), "fork1", new Uri("https://foo.bar/fork/repo.git")).Forget(); + gitClient.Received().SetConfig(Arg.Any(), "remote.fork1.created-by-ghfvs", "true").Forget(); + } } } @@ -745,35 +747,37 @@ public async Task ShouldRemoveUnusedRemote() var localRepo = Substitute.For(); localRepo.CloneUrl.Returns(new UriString("https://github.com/foo/bar")); - var repo = gitService.GetRepository(localRepo.CloneUrl); - var remote1 = Substitute.For(); - var remote2 = Substitute.For(); - var remote3 = Substitute.For(); - var remotes = new List { remote1, remote2, remote3 }; - var remoteCollection = Substitute.For(); - remote1.Name.Returns("remote1"); - remote2.Name.Returns("remote2"); - remote3.Name.Returns("remote3"); - remoteCollection.GetEnumerator().Returns(_ => remotes.GetEnumerator()); - repo.Network.Remotes.Returns(remoteCollection); - - var branch1 = Substitute.For(); - var branch2 = Substitute.For(); - var branches = new List { branch1, branch2 }; - var branchCollection = Substitute.For(); - branch1.RemoteName.Returns("remote1"); - branch2.RemoteName.Returns("remote1"); - branchCollection.GetEnumerator().Returns(_ => branches.GetEnumerator()); - repo.Branches.Returns(branchCollection); - - gitClient.GetConfig(Arg.Any(), "remote.remote1.created-by-ghfvs").Returns(Task.FromResult(true)); - gitClient.GetConfig(Arg.Any(), "remote.remote2.created-by-ghfvs").Returns(Task.FromResult(true)); - - await service.RemoveUnusedRemotes(localRepo); - - remoteCollection.DidNotReceive().Remove("remote1"); - remoteCollection.Received().Remove("remote2"); - remoteCollection.DidNotReceive().Remove("remote3"); + using (var repo = gitService.GetRepository(localRepo.CloneUrl)) + { + var remote1 = Substitute.For(); + var remote2 = Substitute.For(); + var remote3 = Substitute.For(); + var remotes = new List { remote1, remote2, remote3 }; + var remoteCollection = Substitute.For(); + remote1.Name.Returns("remote1"); + remote2.Name.Returns("remote2"); + remote3.Name.Returns("remote3"); + remoteCollection.GetEnumerator().Returns(_ => remotes.GetEnumerator()); + repo.Network.Remotes.Returns(remoteCollection); + + var branch1 = Substitute.For(); + var branch2 = Substitute.For(); + var branches = new List { branch1, branch2 }; + var branchCollection = Substitute.For(); + branch1.RemoteName.Returns("remote1"); + branch2.RemoteName.Returns("remote1"); + branchCollection.GetEnumerator().Returns(_ => branches.GetEnumerator()); + repo.Branches.Returns(branchCollection); + + gitClient.GetConfig(Arg.Any(), "remote.remote1.created-by-ghfvs").Returns(Task.FromResult(true)); + gitClient.GetConfig(Arg.Any(), "remote.remote2.created-by-ghfvs").Returns(Task.FromResult(true)); + + await service.RemoveUnusedRemotes(localRepo); + + remoteCollection.DidNotReceive().Remove("remote1"); + remoteCollection.Received().Remove("remote2"); + remoteCollection.DidNotReceive().Remove("remote3"); + } } } From 6f1a89099c208d8f39f1259ff27871d8fcd2cf43 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 23 Nov 2017 15:57:19 +0000 Subject: [PATCH 03/10] Dispose after using PullRequestSessionService.GetRepository --- .../Services/PullRequestSessionService.cs | 77 +++++++++++-------- .../TestDoubles/FakeDiffService.cs | 4 +- 2 files changed, 47 insertions(+), 34 deletions(-) diff --git a/src/GitHub.InlineReviews/Services/PullRequestSessionService.cs b/src/GitHub.InlineReviews/Services/PullRequestSessionService.cs index 6895ad7cc1..83a456f2f2 100644 --- a/src/GitHub.InlineReviews/Services/PullRequestSessionService.cs +++ b/src/GitHub.InlineReviews/Services/PullRequestSessionService.cs @@ -53,15 +53,19 @@ public PullRequestSessionService( /// public virtual async Task> Diff(ILocalRepositoryModel repository, string baseSha, string headSha, string relativePath) { - var repo = await GetRepository(repository); - return await diffService.Diff(repo, baseSha, headSha, relativePath); + using (var repo = await GetRepository(repository)) + { + return await diffService.Diff(repo, baseSha, headSha, relativePath); + } } /// public virtual async Task> Diff(ILocalRepositoryModel repository, string baseSha, string headSha, string relativePath, byte[] contents) { - var repo = await GetRepository(repository); - return await diffService.Diff(repo, baseSha, headSha, relativePath, contents); + using (var repo = await GetRepository(repository)) + { + return await diffService.Diff(repo, baseSha, headSha, relativePath, contents); + } } /// @@ -175,18 +179,22 @@ public ITextDocument GetDocument(ITextBuffer buffer) /// public virtual async Task GetTipSha(ILocalRepositoryModel repository) { - var repo = await GetRepository(repository); - return repo.Head.Tip.Sha; + using (var repo = await GetRepository(repository)) + { + return repo.Head.Tip.Sha; + } } /// public async Task IsUnmodifiedAndPushed(ILocalRepositoryModel repository, string relativePath, byte[] contents) { - var repo = await GetRepository(repository); - var modified = await gitClient.IsModified(repo, relativePath, contents); - var pushed = await gitClient.IsHeadPushed(repo); + using (var repo = await GetRepository(repository)) + { + var modified = await gitClient.IsModified(repo, relativePath, contents); + var pushed = await gitClient.IsHeadPushed(repo); - return !modified && pushed; + return !modified && pushed; + } } public async Task ExtractFileFromGit( @@ -195,17 +203,18 @@ public async Task ExtractFileFromGit( string sha, string relativePath) { - var repo = await GetRepository(repository); - - try - { - return await gitClient.ExtractFileBinary(repo, sha, relativePath); - } - catch (FileNotFoundException) + using (var repo = await GetRepository(repository)) { - var pullHeadRef = $"refs/pull/{pullRequestNumber}/head"; - await gitClient.Fetch(repo, "origin", sha, pullHeadRef); - return await gitClient.ExtractFileBinary(repo, sha, relativePath); + try + { + return await gitClient.ExtractFileBinary(repo, sha, relativePath); + } + catch (FileNotFoundException) + { + var pullHeadRef = $"refs/pull/{pullRequestNumber}/head"; + await gitClient.Fetch(repo, "origin", sha, pullHeadRef); + return await gitClient.ExtractFileBinary(repo, sha, relativePath); + } } } @@ -242,21 +251,23 @@ public virtual async Task GetPullRequestMergeBase(ILocalRepositoryModel return mergeBase; } - var repo = await GetRepository(repository); - var targetUrl = pullRequest.Base.RepositoryCloneUrl; - var headUrl = pullRequest.Head.RepositoryCloneUrl; - var baseRef = pullRequest.Base.Ref; - var pullNumber = pullRequest.Number; - try + using (var repo = await GetRepository(repository)) { - mergeBase = await gitClient.GetPullRequestMergeBase(repo, targetUrl, baseSha, headSha, baseRef, pullNumber); - } - catch (NotFoundException ex) - { - throw new NotFoundException("The Pull Request failed to load. Please check your network connection and click refresh to try again. If this issue persists, please let us know at support@github.com", ex); - } + var targetUrl = pullRequest.Base.RepositoryCloneUrl; + var headUrl = pullRequest.Head.RepositoryCloneUrl; + var baseRef = pullRequest.Base.Ref; + var pullNumber = pullRequest.Number; + try + { + mergeBase = await gitClient.GetPullRequestMergeBase(repo, targetUrl, baseSha, headSha, baseRef, pullNumber); + } + catch (NotFoundException ex) + { + throw new NotFoundException("The Pull Request failed to load. Please check your network connection and click refresh to try again. If this issue persists, please let us know at support@github.com", ex); + } - return mergeBaseCache[key] = mergeBase; + return mergeBaseCache[key] = mergeBase; + } } /// diff --git a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs b/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs index f6d02228f1..6afbe800f0 100644 --- a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs +++ b/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs @@ -54,7 +54,9 @@ public string AddFile(string path, string contents, string commitAlias) public void Dispose() { var path = repository.Info.WorkingDirectory; - repository.Dispose(); + + // NOTE: IDiffService doesn't own the Repository object + //repository.Dispose(); // The .git folder has some files marked as readonly, meaning that a simple // Directory.Delete doesn't work here. From 8b17782ed235510dd765fc7b24dc8fb9d8f30e78 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 23 Nov 2017 16:09:58 +0000 Subject: [PATCH 04/10] Use IRepository returned by GetActiveRepo inside using --- .../Services/RepositoryPublishService.cs | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/GitHub.App/Services/RepositoryPublishService.cs b/src/GitHub.App/Services/RepositoryPublishService.cs index 2bddb565e7..48d4a21fc4 100644 --- a/src/GitHub.App/Services/RepositoryPublishService.cs +++ b/src/GitHub.App/Services/RepositoryPublishService.cs @@ -26,10 +26,12 @@ public string LocalRepositoryName { get { - var activeRepo = vsGitServices.GetActiveRepo(); - if (!string.IsNullOrEmpty(activeRepo?.Info?.WorkingDirectory)) - return new DirectoryInfo(activeRepo.Info.WorkingDirectory).Name ?? ""; - return string.Empty; + using (var activeRepo = vsGitServices.GetActiveRepo()) + { + if (!string.IsNullOrEmpty(activeRepo?.Info?.WorkingDirectory)) + return new DirectoryInfo(activeRepo.Info.WorkingDirectory).Name ?? ""; + return string.Empty; + } } } @@ -43,11 +45,14 @@ public string LocalRepositoryName .Select(remoteRepo => new { RemoteRepo = remoteRepo, LocalRepo = vsGitServices.GetActiveRepo() })) .SelectMany(async repo => { - await gitClient.SetRemote(repo.LocalRepo, "origin", new Uri(repo.RemoteRepo.CloneUrl)); - await gitClient.Push(repo.LocalRepo, "master", "origin"); - await gitClient.Fetch(repo.LocalRepo, "origin"); - await gitClient.SetTrackingBranch(repo.LocalRepo, "master", "origin"); - return repo.RemoteRepo; + using (repo.LocalRepo) + { + await gitClient.SetRemote(repo.LocalRepo, "origin", new Uri(repo.RemoteRepo.CloneUrl)); + await gitClient.Push(repo.LocalRepo, "master", "origin"); + await gitClient.Fetch(repo.LocalRepo, "origin"); + await gitClient.SetTrackingBranch(repo.LocalRepo, "master", "origin"); + return repo.RemoteRepo; + } }); } } From 604a82c7d25c7fc34496467e29406fe1636604e7 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 23 Nov 2017 16:16:30 +0000 Subject: [PATCH 05/10] Access GetRepositoryFromSolution from inside using Dispose of Repository object. --- src/GitHub.TeamFoundation.14/Services/VSGitServices.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/GitHub.TeamFoundation.14/Services/VSGitServices.cs b/src/GitHub.TeamFoundation.14/Services/VSGitServices.cs index 00814568d8..59b341f440 100644 --- a/src/GitHub.TeamFoundation.14/Services/VSGitServices.cs +++ b/src/GitHub.TeamFoundation.14/Services/VSGitServices.cs @@ -116,7 +116,12 @@ public string GetActiveRepoPath() if (repo != null) ret = repo.RepositoryPath; if (ret == null) - ret = serviceProvider.GetSolution().GetRepositoryFromSolution()?.Info?.Path; + { + using (var repository = serviceProvider.GetSolution().GetRepositoryFromSolution()) + { + ret = repository?.Info?.Path; + } + } return ret ?? String.Empty; } From c1746aefc672003c5f92c99d0700d64b5d5d2814 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 23 Nov 2017 17:33:05 +0000 Subject: [PATCH 06/10] Check we don't return reference to disposed Repository --- src/GitHub.App/Services/PullRequestService.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs index 96fefc65cd..65aadf6444 100644 --- a/src/GitHub.App/Services/PullRequestService.cs +++ b/src/GitHub.App/Services/PullRequestService.cs @@ -214,7 +214,7 @@ public IObservable CalculateHistoryDivergence(ILocalRepos { return Observable.Defer(async () => { - // TODO: Find out if BranchTrackingDetails depends on Repository + // BranchTrackingDetails doesn't keep a reference to Repository using (var repo = gitService.GetRepository(repository.LocalPath)) { var remoteName = repo.Head.RemoteName; @@ -233,7 +233,7 @@ public IObservable GetTreeChanges(ILocalRepositoryModel repository, { return Observable.Defer(async () => { - // TODO: Find out if TreeChanges depends on Repository + // TreeChanges doesn't keep a reference to Repository using (var repo = gitService.GetRepository(repository.LocalPath)) { var remote = await gitClient.GetHttpRemote(repo, "origin"); @@ -248,7 +248,7 @@ public IObservable GetLocalBranches(ILocalRepositoryModel repository, I { return Observable.Defer(() => { - // TODO: Find out if IBranch depends on Repository + // BranchModel doesn't keep a reference to repo using (var repo = gitService.GetRepository(repository.LocalPath)) { var result = GetLocalBranchesInternal(repository, repo, pullRequest).Select(x => new BranchModel(x, repository)); @@ -325,7 +325,6 @@ public IObservable SwitchToBranch(ILocalRepositoryModel repository, IPullR await gitClient.Checkout(repo, branchName); await MarkBranchAsPullRequest(repo, branchName, pullRequest); - } } @@ -556,7 +555,7 @@ async Task PushAndCreatePR(IModelService modelService, IBranch sourceBranch, IBranch targetBranch, string title, string body) { - // TODO: Find out if IPullRequestModel depends on Repository + // PullRequestModel doesn't keep a reference to repo using (var repo = await Task.Run(() => gitService.GetRepository(sourceRepository.LocalPath))) { var remote = await gitClient.GetHttpRemote(repo, "origin"); From 7fb09a3282369eeb944b3b3b5d1b1c45c5d26c3f Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 23 Nov 2017 17:40:26 +0000 Subject: [PATCH 07/10] Keep reference to TreeChanges and Dispose --- src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs b/src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs index 1791da8921..bd5195d51e 100644 --- a/src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs +++ b/src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs @@ -387,8 +387,10 @@ public async Task Load(string remoteRepositoryOwner, IPullRequestModel pullReque CommentCount = pullRequest.Comments.Count + pullRequest.ReviewComments.Count; Body = !string.IsNullOrWhiteSpace(pullRequest.Body) ? pullRequest.Body : Resources.NoDescriptionProvidedMarkdown; - var changes = await pullRequestsService.GetTreeChanges(LocalRepository, pullRequest); - ChangedFilesTree = (await CreateChangedFilesTree(pullRequest, changes)).Children.ToList(); + using (var changes = await pullRequestsService.GetTreeChanges(LocalRepository, pullRequest)) + { + ChangedFilesTree = (await CreateChangedFilesTree(pullRequest, changes)).Children.ToList(); + } var localBranches = await pullRequestsService.GetLocalBranches(LocalRepository, pullRequest).ToList(); From 4db51b21487bc478039b2466675a0c72097fb8e7 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 24 Nov 2017 12:36:16 +0000 Subject: [PATCH 08/10] Don't dispose of Repository before it's used --- src/GitHub.App/Services/PullRequestService.cs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs index 65aadf6444..462973da89 100644 --- a/src/GitHub.App/Services/PullRequestService.cs +++ b/src/GitHub.App/Services/PullRequestService.cs @@ -93,12 +93,13 @@ public IObservable> GetMessagesForUniqueCommits( string compareBranch, int maxCommits) { - return Observable.Defer(() => + return Observable.Defer(async () => { // CommitMessage doesn't keep a reference to Repository using (var repo = gitService.GetRepository(repository.LocalPath)) { - return gitClient.GetMessagesForUniqueCommits(repo, baseBranch, compareBranch, maxCommits).ToObservable(); + var messages = await gitClient.GetMessagesForUniqueCommits(repo, baseBranch, compareBranch, maxCommits); + return Observable.Return(messages); } }); } @@ -130,11 +131,12 @@ static bool IsFilthy(RepositoryStatus status) public IObservable Pull(ILocalRepositoryModel repository) { - return Observable.Defer(() => + return Observable.Defer(async () => { using (var repo = gitService.GetRepository(repository.LocalPath)) { - return gitClient.Pull(repo).ToObservable(); + await gitClient.Pull(repo); + return Observable.Return(Unit.Default); } }); } @@ -147,7 +149,8 @@ public IObservable Push(ILocalRepositoryModel repository) { var remoteName = repo.Head.RemoteName; var remote = await gitClient.GetHttpRemote(repo, remoteName); - return gitClient.Push(repo, repo.Head.TrackedBranch.UpstreamBranchCanonicalName, remote.Name).ToObservable(); + await gitClient.Push(repo, repo.Head.TrackedBranch.UpstreamBranchCanonicalName, remote.Name); + return Observable.Return(Unit.Default); } }); } From 88a753aa271a90cbcdee270675a51dce01e8e980 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Fri, 24 Nov 2017 14:57:01 +0000 Subject: [PATCH 09/10] Force the enumeration to complete before disposing We don't want to dispose of Repository while still enumerating its branches. --- src/GitHub.App/Services/PullRequestService.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs index 462973da89..b20f65b759 100644 --- a/src/GitHub.App/Services/PullRequestService.cs +++ b/src/GitHub.App/Services/PullRequestService.cs @@ -251,11 +251,11 @@ public IObservable GetLocalBranches(ILocalRepositoryModel repository, I { return Observable.Defer(() => { - // BranchModel doesn't keep a reference to repo + // BranchModel doesn't keep a reference to rep using (var repo = gitService.GetRepository(repository.LocalPath)) { var result = GetLocalBranchesInternal(repository, repo, pullRequest).Select(x => new BranchModel(x, repository)); - return result.ToObservable(); + return result.ToList().ToObservable(); } }); } From b92389940ccc1770e3f201001542bf9b31be82fb Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 27 Nov 2017 17:21:26 +0000 Subject: [PATCH 10/10] Clean up dead code --- .../TestDoubles/FakeDiffService.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs b/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs index 6afbe800f0..bf3b0d25d5 100644 --- a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs +++ b/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs @@ -55,9 +55,6 @@ public void Dispose() { var path = repository.Info.WorkingDirectory; - // NOTE: IDiffService doesn't own the Repository object - //repository.Dispose(); - // The .git folder has some files marked as readonly, meaning that a simple // Directory.Delete doesn't work here. DeleteDirectory(path);