From e8d9527cdd2a0019e0889783a893a3482f3c45ad Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 18 Dec 2018 10:54:15 +0000 Subject: [PATCH 01/21] Update to be compatible with LigGit2Sharp 0.26 Stop using obsolete LigGit2Sharp methods. Also changed from using Task.Factory.StartNew to Task.Run. --- src/GitHub.App/Services/GitClient.cs | 82 ++++++++++++++-------------- 1 file changed, 42 insertions(+), 40 deletions(-) diff --git a/src/GitHub.App/Services/GitClient.cs b/src/GitHub.App/Services/GitClient.cs index 6114d82169..e933ab3fbc 100644 --- a/src/GitHub.App/Services/GitClient.cs +++ b/src/GitHub.App/Services/GitClient.cs @@ -44,12 +44,17 @@ public GitClient(IGitHubCredentialProvider credentialProvider, IGitService gitSe public Task Pull(IRepository repository) { Guard.ArgumentNotNull(repository, nameof(repository)); - return Task.Factory.StartNew(() => + return Task.Run(() => { var signature = repository.Config.BuildSignature(DateTimeOffset.UtcNow); -#pragma warning disable 0618 // TODO: Replace `Network.Pull` with `Commands.Pull`. - repository.Network.Pull(signature, pullOptions); -#pragma warning restore 0618 + if (repository is Repository repo) + { + LibGit2Sharp.Commands.Pull(repo, signature, pullOptions); + } + else + { + log.Error("Couldn't pull because {Variable} isn't an instance of {Type}", nameof(repository), typeof(Repository)); + } }); } @@ -59,7 +64,7 @@ public Task Push(IRepository repository, string branchName, string remoteName) Guard.ArgumentNotEmptyString(branchName, nameof(branchName)); Guard.ArgumentNotEmptyString(remoteName, nameof(remoteName)); - return Task.Factory.StartNew(() => + return Task.Run(() => { if (repository.Head?.Commits != null && repository.Head.Commits.Any()) { @@ -75,14 +80,11 @@ public Task Fetch(IRepository repository, string remoteName) Guard.ArgumentNotNull(repository, nameof(repository)); Guard.ArgumentNotEmptyString(remoteName, nameof(remoteName)); - return Task.Factory.StartNew(() => + return Task.Run(() => { try { - var remote = repository.Network.Remotes[remoteName]; -#pragma warning disable 0618 // TODO: Replace `Network.Fetch` with `Commands.Fetch`. - repository.Network.Fetch(remote, fetchOptions); -#pragma warning restore 0618 + repository.Network.Fetch(remoteName, new[] { "+refs/heads/*:refs/remotes/origin/*" }, fetchOptions); } catch (Exception ex) { @@ -104,7 +106,7 @@ public Task Fetch(IRepository repo, UriString cloneUrl, params string[] refspecs } } - return Task.Factory.StartNew(() => + return Task.Run(() => { try { @@ -114,17 +116,15 @@ public Task Fetch(IRepository repo, UriString cloneUrl, params string[] refspecs var removeRemote = false; if (repo.Network.Remotes[remoteName] != null) { - // If a remote with this neme already exists, use a unique name and remove remote afterwards + // If a remote with this name already exists, use a unique name and remove remote afterwards remoteName = cloneUrl.Owner + "-" + Guid.NewGuid(); removeRemote = true; } - var remote = repo.Network.Remotes.Add(remoteName, remoteUri.ToString()); + repo.Network.Remotes.Add(remoteName, remoteUri.ToString()); try { -#pragma warning disable 0618 // TODO: Replace `Network.Fetch` with `Commands.Fetch`. - repo.Network.Fetch(remote, refspecs, fetchOptions); -#pragma warning restore 0618 + repo.Network.Fetch(remoteName, refspecs, fetchOptions); } finally { @@ -149,14 +149,11 @@ public Task Fetch(IRepository repository, string remoteName, params string[] ref Guard.ArgumentNotNull(repository, nameof(repository)); Guard.ArgumentNotEmptyString(remoteName, nameof(remoteName)); - return Task.Factory.StartNew(() => + return Task.Run(() => { try { - var remote = repository.Network.Remotes[remoteName]; -#pragma warning disable 0618 // TODO: Replace `Network.Fetch` with `Commands.Fetch`. - repository.Network.Fetch(remote, refspecs, fetchOptions); -#pragma warning restore 0618 + repository.Network.Fetch(remoteName, refspecs, fetchOptions); } catch (Exception ex) { @@ -173,11 +170,16 @@ public Task Checkout(IRepository repository, string branchName) Guard.ArgumentNotNull(repository, nameof(repository)); Guard.ArgumentNotEmptyString(branchName, nameof(branchName)); - return Task.Factory.StartNew(() => + return Task.Run(() => { -#pragma warning disable 0618 // TODO: Replace `IRepository.Checkout` with `Commands.Checkout`. - repository.Checkout(branchName); -#pragma warning restore 0618 + if (repository is Repository repo) + { + LibGit2Sharp.Commands.Checkout(repo, branchName); + } + else + { + log.Error("Couldn't checkout because {Variable} isn't an instance of {Type}", nameof(repository), typeof(Repository)); + } }); } @@ -186,7 +188,7 @@ public Task CreateBranch(IRepository repository, string branchName) Guard.ArgumentNotNull(repository, nameof(repository)); Guard.ArgumentNotEmptyString(branchName, nameof(branchName)); - return Task.Factory.StartNew(() => + return Task.Run(() => { repository.CreateBranch(branchName); }); @@ -202,7 +204,7 @@ public Task Compare( Guard.ArgumentNotEmptyString(sha1, nameof(sha1)); Guard.ArgumentNotEmptyString(sha2, nameof(sha2)); - return Task.Factory.StartNew(() => + return Task.Run(() => { var options = new CompareOptions { @@ -234,7 +236,7 @@ public Task Compare( Guard.ArgumentNotEmptyString(sha2, nameof(sha2)); Guard.ArgumentNotEmptyString(path, nameof(path)); - return Task.Factory.StartNew(() => + return Task.Run(() => { var commit1 = repository.Lookup(sha1); var commit2 = repository.Lookup(sha2); @@ -260,7 +262,7 @@ public Task CompareWith(IRepository repository, string sha1, str Guard.ArgumentNotEmptyString(sha2, nameof(sha1)); Guard.ArgumentNotEmptyString(path, nameof(path)); - return Task.Factory.StartNew(() => + return Task.Run(() => { var commit1 = repository.Lookup(sha1); var commit2 = repository.Lookup(sha2); @@ -287,7 +289,7 @@ public Task GetConfig(IRepository repository, string key) Guard.ArgumentNotNull(repository, nameof(repository)); Guard.ArgumentNotEmptyString(key, nameof(key)); - return Task.Factory.StartNew(() => + return Task.Run(() => { var result = repository.Config.Get(key); return result != null ? result.Value : default(T); @@ -300,7 +302,7 @@ public Task SetConfig(IRepository repository, string key, string value) Guard.ArgumentNotEmptyString(key, nameof(key)); Guard.ArgumentNotEmptyString(value, nameof(value)); - return Task.Factory.StartNew(() => + return Task.Run(() => { repository.Config.Set(key, value); }); @@ -311,7 +313,7 @@ public Task SetRemote(IRepository repository, string remoteName, Uri url) Guard.ArgumentNotNull(repository, nameof(repository)); Guard.ArgumentNotEmptyString(remoteName, nameof(remoteName)); - return Task.Factory.StartNew(() => + return Task.Run(() => { repository.Config.Set("remote." + remoteName + ".url", url.ToString()); repository.Config.Set("remote." + remoteName + ".fetch", "+refs/heads/*:refs/remotes/" + remoteName + "/*"); @@ -324,7 +326,7 @@ public Task SetTrackingBranch(IRepository repository, string branchName, string Guard.ArgumentNotEmptyString(branchName, nameof(branchName)); Guard.ArgumentNotEmptyString(remoteName, nameof(remoteName)); - return Task.Factory.StartNew(() => + return Task.Run(() => { var remoteBranchName = IsCanonical(remoteName) ? remoteName : "refs/remotes/" + remoteName + "/" + branchName; var remoteBranch = repository.Branches[remoteBranchName]; @@ -342,7 +344,7 @@ public Task UnsetConfig(IRepository repository, string key) { Guard.ArgumentNotEmptyString(key, nameof(key)); - return Task.Factory.StartNew(() => + return Task.Run(() => { repository.Config.Unset(key); }); @@ -353,7 +355,7 @@ public Task GetHttpRemote(IRepository repo, string remote) Guard.ArgumentNotNull(repo, nameof(repo)); Guard.ArgumentNotEmptyString(remote, nameof(remote)); - return Task.Factory.StartNew(() => + return Task.Run(() => { var uri = gitService.GetRemoteUri(repo, remote); var remoteName = uri.IsHypertextTransferProtocol ? remote : remote + "-http"; @@ -370,7 +372,7 @@ public Task ExtractFile(IRepository repository, string commitSha, string Guard.ArgumentNotEmptyString(commitSha, nameof(commitSha)); Guard.ArgumentNotEmptyString(fileName, nameof(fileName)); - return Task.Factory.StartNew(() => + return Task.Run(() => { var commit = repository.Lookup(commitSha); if (commit == null) @@ -389,7 +391,7 @@ public Task ExtractFileBinary(IRepository repository, string commitSha, Guard.ArgumentNotEmptyString(commitSha, nameof(commitSha)); Guard.ArgumentNotEmptyString(fileName, nameof(fileName)); - return Task.Factory.StartNew(() => + return Task.Run(() => { var commit = repository.Lookup(commitSha); if (commit == null) @@ -418,7 +420,7 @@ public Task IsModified(IRepository repository, string path, byte[] content Guard.ArgumentNotNull(repository, nameof(repository)); Guard.ArgumentNotEmptyString(path, nameof(path)); - return Task.Factory.StartNew(() => + return Task.Run(() => { if (repository.RetrieveStatus(path) == FileStatus.Unaltered) { @@ -486,7 +488,7 @@ public Task IsHeadPushed(IRepository repo) { Guard.ArgumentNotNull(repo, nameof(repo)); - return Task.Factory.StartNew(() => + return Task.Run(() => { return repo.Head.TrackingDetails.AheadBy == 0; }); @@ -498,7 +500,7 @@ public Task> GetMessagesForUniqueCommits( string compareBranch, int maxCommits) { - return Task.Factory.StartNew(() => + return Task.Run(() => { var baseCommit = repo.Lookup(baseBranch); var compareCommit = repo.Lookup(compareBranch); From 465812281fb5d02d0c47a9c40d3aa4c7b9cefff3 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 18 Dec 2018 11:35:00 +0000 Subject: [PATCH 02/21] Fix tests that use LibGit2Sharp --- .../Services/GitClientTests.cs | 17 ++++------------- .../TestDoubles/FakeDiffService.cs | 8 ++------ 2 files changed, 6 insertions(+), 19 deletions(-) diff --git a/test/GitHub.App.UnitTests/Services/GitClientTests.cs b/test/GitHub.App.UnitTests/Services/GitClientTests.cs index 1efe891a1a..b1956cc5ff 100644 --- a/test/GitHub.App.UnitTests/Services/GitClientTests.cs +++ b/test/GitHub.App.UnitTests/Services/GitClientTests.cs @@ -277,10 +277,7 @@ public async Task FetchFromExistingRemote(string fetchUrl, string remoteName, st await gitClient.Fetch(repo, fetchUri, refSpec); - var remote = repo.Network.Remotes[expectRemoteName]; -#pragma warning disable 0618 // TODO: Replace `Network.Fetch` with `Commands.Fetch`. - repo.Network.Received(1).Fetch(remote, Arg.Any(), Arg.Any()); -#pragma warning restore 0618 + repo.Network.Received(1).Fetch(expectRemoteName, Arg.Any(), Arg.Any()); } static IRepository CreateRepository(string remoteName, string remoteUrl) @@ -312,9 +309,7 @@ public async Task LocalBaseHeadAndMergeBase_DontFetchAsync() var mergeBaseSha = await gitClient.GetPullRequestMergeBase(repo, targetCloneUrl, baseSha, headSha, baseRef, pullNumber); -#pragma warning disable 618 // Type or member is obsolete - repo.Network.DidNotReceiveWithAnyArgs().Fetch(null as Remote, null, null as FetchOptions); -#pragma warning restore 618 // Type or member is obsolete + repo.Network.DidNotReceiveWithAnyArgs().Fetch(null as string, null, null as FetchOptions); Assert.That(expectMergeBaseSha, Is.EqualTo(mergeBaseSha)); } @@ -338,9 +333,7 @@ public async Task WhenToFetchAsync(string baseSha, string headSha, string mergeB } catch (NotFoundException) { /* We're interested in calls to Fetch even if it throws */ } -#pragma warning disable 618 // Type or member is obsolete - repo.Network.Received(receivedFetch).Fetch(Arg.Any(), Arg.Any(), Arg.Any()); -#pragma warning restore 618 // Type or member is obsolete + repo.Network.Received(receivedFetch).Fetch(Arg.Any(), Arg.Any(), Arg.Any()); } [TestCase("baseSha", null, "mergeBaseSha", "baseRef", 777, "refs/pull/777/head")] @@ -361,9 +354,7 @@ public async Task WhatToFetchAsync(string baseSha, string headSha, string mergeB } catch (NotFoundException) { /* We're interested in calls to Fetch even if it throws */ } -#pragma warning disable 618 // Type or member is obsolete - repo.Network.Received(1).Fetch(Arg.Any(), Arg.Is>(x => x.Contains(expectRefSpec)), Arg.Any()); -#pragma warning restore 618 // Type or member is obsolete + repo.Network.Received(1).Fetch(Arg.Any(), Arg.Is>(x => x.Contains(expectRefSpec)), Arg.Any()); } static IRepository MockRepo(string baseSha, string headSha, string mergeBaseSha) diff --git a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs b/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs index 112c1bc83c..d9c8938309 100644 --- a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs +++ b/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs @@ -39,9 +39,7 @@ public string AddFile(string path, string contents) var directory = Path.GetDirectoryName(fullPath); Directory.CreateDirectory(directory); File.WriteAllText(fullPath, contents); -#pragma warning disable 618 // Type or member is obsolete - repository.Stage(path); -#pragma warning restore 618 // Type or member is obsolete + LibGit2Sharp.Commands.Stage(repository, path); repository.Commit("Added " + path, signature, signature); return repository.Head.Tip.Sha; } @@ -108,9 +106,7 @@ static IRepository CreateRepository() var signature = new Signature("user", "user@user", DateTimeOffset.Now); File.WriteAllText(Path.Combine(tempPath, ".gitattributes"), "* text=auto"); -#pragma warning disable 618 // Type or member is obsolete - result.Stage("*"); -#pragma warning restore 618 // Type or member is obsolete + LibGit2Sharp.Commands.Stage(result, "*"); result.Commit("Initial commit", signature, signature); return result; From 8b6a4883c77e923c06b82e0ed4ac8e352585fabf Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 18 Dec 2018 14:34:13 +0000 Subject: [PATCH 03/21] Remove obsolete IQueryableCommitLog.QueryBy imple --- test/GitHub.App.UnitTests/TestDoubles/FakeCommitLog.cs | 7 ------- test/GitHub.Exports.UnitTests/TestDoubles/FakeCommitLog.cs | 7 ------- 2 files changed, 14 deletions(-) diff --git a/test/GitHub.App.UnitTests/TestDoubles/FakeCommitLog.cs b/test/GitHub.App.UnitTests/TestDoubles/FakeCommitLog.cs index 8504b87b53..aa8e06a745 100644 --- a/test/GitHub.App.UnitTests/TestDoubles/FakeCommitLog.cs +++ b/test/GitHub.App.UnitTests/TestDoubles/FakeCommitLog.cs @@ -22,13 +22,6 @@ public ICommitLog QueryBy(CommitFilter filter) throw new NotImplementedException(); } -#pragma warning disable 618 // Type or member is obsolete - public IEnumerable QueryBy(string path, FollowFilter filter) - { - throw new NotImplementedException(); - } -#pragma warning restore 618 // Type or member is obsolete - public IEnumerable QueryBy(string path, CommitFilter filter) { throw new NotImplementedException(); diff --git a/test/GitHub.Exports.UnitTests/TestDoubles/FakeCommitLog.cs b/test/GitHub.Exports.UnitTests/TestDoubles/FakeCommitLog.cs index 8504b87b53..aa8e06a745 100644 --- a/test/GitHub.Exports.UnitTests/TestDoubles/FakeCommitLog.cs +++ b/test/GitHub.Exports.UnitTests/TestDoubles/FakeCommitLog.cs @@ -22,13 +22,6 @@ public ICommitLog QueryBy(CommitFilter filter) throw new NotImplementedException(); } -#pragma warning disable 618 // Type or member is obsolete - public IEnumerable QueryBy(string path, FollowFilter filter) - { - throw new NotImplementedException(); - } -#pragma warning restore 618 // Type or member is obsolete - public IEnumerable QueryBy(string path, CommitFilter filter) { throw new NotImplementedException(); From 32c4aac2436276151949f6c05e3e17457babab20 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 18 Dec 2018 15:27:45 +0000 Subject: [PATCH 04/21] Update to use LibGit2Sharp 0.26.0-preview-0080 Reference LibGit2Sharp that targets .NET 4.6 --- src/GitHub.App/GitHub.App.csproj | 3 +-- src/GitHub.Exports/GitHub.Exports.csproj | 3 +-- src/GitHub.InlineReviews/GitHub.InlineReviews.csproj | 7 +------ .../GitHub.TeamFoundation.14.csproj | 7 +------ .../GitHub.TeamFoundation.15.csproj | 7 +------ .../GitHub.TeamFoundation.16.csproj | 7 +------ src/GitHub.VisualStudio/GitHub.VisualStudio.csproj | 1 + 7 files changed, 7 insertions(+), 28 deletions(-) diff --git a/src/GitHub.App/GitHub.App.csproj b/src/GitHub.App/GitHub.App.csproj index 2f1c286b1f..f918d84a1a 100644 --- a/src/GitHub.App/GitHub.App.csproj +++ b/src/GitHub.App/GitHub.App.csproj @@ -42,8 +42,7 @@ - - + diff --git a/src/GitHub.Exports/GitHub.Exports.csproj b/src/GitHub.Exports/GitHub.Exports.csproj index 68b625c8ca..a97fb52f93 100644 --- a/src/GitHub.Exports/GitHub.Exports.csproj +++ b/src/GitHub.Exports/GitHub.Exports.csproj @@ -23,8 +23,7 @@ - - + diff --git a/src/GitHub.InlineReviews/GitHub.InlineReviews.csproj b/src/GitHub.InlineReviews/GitHub.InlineReviews.csproj index 7cee7dd38e..c0c35b167f 100644 --- a/src/GitHub.InlineReviews/GitHub.InlineReviews.csproj +++ b/src/GitHub.InlineReviews/GitHub.InlineReviews.csproj @@ -280,12 +280,7 @@ - - 0.23.1 - - - 1.0.164 - + 0.2.1 diff --git a/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj b/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj index 7fd0b34a92..d35fda64a8 100644 --- a/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj +++ b/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj @@ -163,12 +163,7 @@ 8.0.2 - - 0.23.1 - - - 1.0.164 - + 14.0.25424 diff --git a/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj b/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj index 08aeb35db8..327c3b101b 100644 --- a/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj +++ b/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj @@ -211,12 +211,7 @@ 8.0.2 - - 0.23.1 - - - 1.0.164 - + 14.0.25424 diff --git a/src/GitHub.TeamFoundation.16/GitHub.TeamFoundation.16.csproj b/src/GitHub.TeamFoundation.16/GitHub.TeamFoundation.16.csproj index a297bee227..66f1a599e1 100644 --- a/src/GitHub.TeamFoundation.16/GitHub.TeamFoundation.16.csproj +++ b/src/GitHub.TeamFoundation.16/GitHub.TeamFoundation.16.csproj @@ -218,12 +218,7 @@ 8.0.2 - - 0.23.1 - - - 1.0.164 - + 2.5.0 diff --git a/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj b/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj index 6f1f618e21..0acf65de93 100644 --- a/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj +++ b/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj @@ -436,6 +436,7 @@ 14.1.24720 + true From bbc0504d4a9955e1473b1fbe9e274d09b296edb8 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 19 Dec 2018 15:04:57 +0000 Subject: [PATCH 05/21] Commit[] no longer accepts \ in path Pass gitHubFilePath not winFilePath to FakeDiffService.Diff. --- .../Services/PullRequestSessionServiceTests.cs | 4 ++-- .../TestDoubles/FakeDiffService.cs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionServiceTests.cs b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionServiceTests.cs index 514b19927e..eec4ee008e 100644 --- a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionServiceTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionServiceTests.cs @@ -181,7 +181,7 @@ Line 2 using (var diffService = new FakeDiffService(winFilePath, baseContents)) { - var diff = await diffService.Diff(winFilePath, headContents); + var diff = await diffService.Diff(gitHubFilePath, headContents); var pullRequest = CreatePullRequest(gitHubFilePath, comment); var target = CreateTarget(diffService); @@ -334,7 +334,7 @@ static PullRequestDetailModel CreatePullRequest( HeadRefName = "HEAD", HeadRefSha = "HEAD_SHA", HeadRepositoryOwner = "owner", - ChangedFiles = new [] + ChangedFiles = new[] { new PullRequestFileModel { FileName = filePath }, new PullRequestFileModel { FileName = "other.cs" }, diff --git a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs b/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs index d9c8938309..febfa8af0c 100644 --- a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs +++ b/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs @@ -68,7 +68,7 @@ public Task> Diff(IRepository repo, string baseSha, str return Task.FromResult>(DiffUtilities.ParseFragment(patch).ToList()); } - public Task> Diff(string path, string baseSha, byte[] contents) + Task> Diff(string path, string baseSha, byte[] contents) { var tip = repository.Head.Tip.Sha; var stream = contents != null ? new MemoryStream(contents) : new MemoryStream(); @@ -78,7 +78,7 @@ public Task> Diff(string path, string baseSha, byte[] c return Task.FromResult>(DiffUtilities.ParseFragment(patch).ToList()); } - public Task> Diff(string path, string contents) + internal Task> Diff(string path, string contents) { return Diff(path, repository.Head.Tip.Sha, Encoding.UTF8.GetBytes(contents)); } From 128cfbe99052cbc1475f8e42f61d034192261bf0 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 7 Jan 2019 11:02:48 +0000 Subject: [PATCH 06/21] Remove unused defaultOriginName field --- src/GitHub.App/Services/GitClient.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/GitHub.App/Services/GitClient.cs b/src/GitHub.App/Services/GitClient.cs index e933ab3fbc..b45806f3fc 100644 --- a/src/GitHub.App/Services/GitClient.cs +++ b/src/GitHub.App/Services/GitClient.cs @@ -17,7 +17,6 @@ namespace GitHub.Services [PartCreationPolicy(CreationPolicy.Shared)] public class GitClient : IGitClient { - const string defaultOriginName = "origin"; static readonly ILogger log = LogManager.ForContext(); readonly IGitService gitService; readonly PullOptions pullOptions; From 057844a997a2c01e1545f1d7a5fe8a86c0e9f4fc Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 8 Jan 2019 14:44:31 +0000 Subject: [PATCH 07/21] Enable IndentHeuristic when comparing --- src/GitHub.App/Services/GitClient.cs | 18 +++++++++++++----- .../Services/GitClientTests.cs | 2 +- .../TestDoubles/FakeDiffService.cs | 4 ++-- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/GitHub.App/Services/GitClient.cs b/src/GitHub.App/Services/GitClient.cs index b45806f3fc..7e1048627a 100644 --- a/src/GitHub.App/Services/GitClient.cs +++ b/src/GitHub.App/Services/GitClient.cs @@ -22,6 +22,7 @@ public class GitClient : IGitClient readonly PullOptions pullOptions; readonly PushOptions pushOptions; readonly FetchOptions fetchOptions; + readonly CompareOptions compareOptions; [ImportingConstructor] public GitClient(IGitHubCredentialProvider credentialProvider, IGitService gitService) @@ -38,6 +39,11 @@ public GitClient(IGitHubCredentialProvider credentialProvider, IGitService gitSe FetchOptions = fetchOptions, MergeOptions = new MergeOptions(), }; + + compareOptions = new CompareOptions + { + IndentHeuristic = true + }; } public Task Pull(IRepository repository) @@ -207,7 +213,8 @@ public Task Compare( { var options = new CompareOptions { - Similarity = detectRenames ? SimilarityOptions.Renames : SimilarityOptions.None + Similarity = detectRenames ? SimilarityOptions.Renames : SimilarityOptions.None, + IndentHeuristic = compareOptions.IndentHeuristic }; var commit1 = repository.Lookup(sha1); @@ -245,7 +252,8 @@ public Task Compare( return repository.Diff.Compare( commit1.Tree, commit2.Tree, - new[] { path }); + new[] { path }, + compareOptions); } else { @@ -266,7 +274,7 @@ public Task CompareWith(IRepository repository, string sha1, str var commit1 = repository.Lookup(sha1); var commit2 = repository.Lookup(sha2); - var treeChanges = repository.Diff.Compare(commit1.Tree, commit2.Tree); + var treeChanges = repository.Diff.Compare(commit1.Tree, commit2.Tree, compareOptions); var normalizedPath = path.Replace("/", "\\"); var renamed = treeChanges.FirstOrDefault(x => x.Path == normalizedPath); var oldPath = renamed?.OldPath ?? path; @@ -276,7 +284,7 @@ public Task CompareWith(IRepository repository, string sha1, str var contentStream = contents != null ? new MemoryStream(contents) : new MemoryStream(); var blob1 = commit1[oldPath]?.Target as Blob ?? repository.ObjectDatabase.CreateBlob(new MemoryStream()); var blob2 = repository.ObjectDatabase.CreateBlob(contentStream, path); - return repository.Diff.Compare(blob1, blob2); + return repository.Diff.Compare(blob1, blob2, compareOptions); } return null; @@ -433,7 +441,7 @@ public Task IsModified(IRepository repository, string path, byte[] content using (var s = contents != null ? new MemoryStream(contents) : new MemoryStream()) { var blob2 = repository.ObjectDatabase.CreateBlob(s, path); - var diff = repository.Diff.Compare(blob1, blob2); + var diff = repository.Diff.Compare(blob1, blob2, compareOptions); return diff.LinesAdded != 0 || diff.LinesDeleted != 0; } } diff --git a/test/GitHub.App.UnitTests/Services/GitClientTests.cs b/test/GitHub.App.UnitTests/Services/GitClientTests.cs index b1956cc5ff..021eae565b 100644 --- a/test/GitHub.App.UnitTests/Services/GitClientTests.cs +++ b/test/GitHub.App.UnitTests/Services/GitClientTests.cs @@ -82,7 +82,7 @@ public async Task ContentChangesAsync(int linesAdded, int linesDeleted, bool exp var changes = Substitute.For(); changes.LinesAdded.Returns(linesAdded); changes.LinesDeleted.Returns(linesDeleted); - repo.Diff.Compare(null, null).ReturnsForAnyArgs(changes); + repo.Diff.Compare(null as Blob, null as Blob, null as CompareOptions).ReturnsForAnyArgs(changes); var gitClient = CreateGitClient(); var modified = await gitClient.IsModified(repo, path, null); diff --git a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs b/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs index febfa8af0c..0c01fba2e8 100644 --- a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs +++ b/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs @@ -64,7 +64,7 @@ public Task> Diff(IRepository repo, string baseSha, str { var blob1 = GetBlob(path, baseSha); var blob2 = GetBlob(path, headSha); - var patch = repository.Diff.Compare(blob1, blob2).Patch; + var patch = repository.Diff.Compare(blob1, blob2, new CompareOptions { IndentHeuristic = true }).Patch; return Task.FromResult>(DiffUtilities.ParseFragment(patch).ToList()); } @@ -74,7 +74,7 @@ Task> Diff(string path, string baseSha, byte[] contents var stream = contents != null ? new MemoryStream(contents) : new MemoryStream(); var blob1 = GetBlob(path, baseSha); var blob2 = repository.ObjectDatabase.CreateBlob(stream, path); - var patch = repository.Diff.Compare(blob1, blob2).Patch; + var patch = repository.Diff.Compare(blob1, blob2, new CompareOptions { IndentHeuristic = true }).Patch; return Task.FromResult>(DiffUtilities.ParseFragment(patch).ToList()); } From 62ccc9bd341f9531b2929eb9c161a6d244e7042a Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 8 Jan 2019 15:08:11 +0000 Subject: [PATCH 08/21] Move Compare and CompareWith to IGitService IGitService knows about IRepository but not remote servers. IGitClient knows about remote git repositories. IGitService is a more appropriate home for Compare and CompareWith. --- .../SampleData/GitServiceDesigner.cs | 2 + src/GitHub.App/Services/GitClient.cs | 60 ------------------ .../Services/IGitClient.cs | 25 -------- src/GitHub.Exports/Services/GitService.cs | 63 ++++++++++++++++++- src/GitHub.Exports/Services/IGitService.cs | 25 ++++++++ .../Services/DiffService.cs | 14 ++--- .../Services/DiffServiceTests.cs | 2 +- .../TestDoubles/FakeDiffService.cs | 4 +- .../VSGitExtTests.cs | 2 + 9 files changed, 101 insertions(+), 96 deletions(-) diff --git a/src/GitHub.App/SampleData/GitServiceDesigner.cs b/src/GitHub.App/SampleData/GitServiceDesigner.cs index cabbf30a4c..2a13d27e3d 100644 --- a/src/GitHub.App/SampleData/GitServiceDesigner.cs +++ b/src/GitHub.App/SampleData/GitServiceDesigner.cs @@ -15,5 +15,7 @@ class GitServiceDesigner : IGitService public IRepository GetRepository(string path) => null; public UriString GetUri(string path, string remote = "origin") => null; public UriString GetUri(IRepository repository, string remote = "origin") => null; + public Task Compare(IRepository repository, string sha1, string sha2, string path) => null; + public Task CompareWith(IRepository repository, string sha1, string sha2, string path, byte[] contents) => null; } } diff --git a/src/GitHub.App/Services/GitClient.cs b/src/GitHub.App/Services/GitClient.cs index 7e1048627a..96db071454 100644 --- a/src/GitHub.App/Services/GitClient.cs +++ b/src/GitHub.App/Services/GitClient.cs @@ -231,66 +231,6 @@ public Task Compare( }); } - public Task Compare( - IRepository repository, - string sha1, - string sha2, - string path) - { - Guard.ArgumentNotNull(repository, nameof(repository)); - Guard.ArgumentNotEmptyString(sha1, nameof(sha1)); - Guard.ArgumentNotEmptyString(sha2, nameof(sha2)); - Guard.ArgumentNotEmptyString(path, nameof(path)); - - return Task.Run(() => - { - var commit1 = repository.Lookup(sha1); - var commit2 = repository.Lookup(sha2); - - if (commit1 != null && commit2 != null) - { - return repository.Diff.Compare( - commit1.Tree, - commit2.Tree, - new[] { path }, - compareOptions); - } - else - { - return null; - } - }); - } - - public Task CompareWith(IRepository repository, string sha1, string sha2, string path, byte[] contents) - { - Guard.ArgumentNotNull(repository, nameof(repository)); - Guard.ArgumentNotEmptyString(sha1, nameof(sha1)); - Guard.ArgumentNotEmptyString(sha2, nameof(sha1)); - Guard.ArgumentNotEmptyString(path, nameof(path)); - - return Task.Run(() => - { - var commit1 = repository.Lookup(sha1); - var commit2 = repository.Lookup(sha2); - - var treeChanges = repository.Diff.Compare(commit1.Tree, commit2.Tree, compareOptions); - var normalizedPath = path.Replace("/", "\\"); - var renamed = treeChanges.FirstOrDefault(x => x.Path == normalizedPath); - var oldPath = renamed?.OldPath ?? path; - - if (commit1 != null) - { - var contentStream = contents != null ? new MemoryStream(contents) : new MemoryStream(); - var blob1 = commit1[oldPath]?.Target as Blob ?? repository.ObjectDatabase.CreateBlob(new MemoryStream()); - var blob2 = repository.ObjectDatabase.CreateBlob(contentStream, path); - return repository.Diff.Compare(blob1, blob2, compareOptions); - } - - return null; - }); - } - public Task GetConfig(IRepository repository, string key) { Guard.ArgumentNotNull(repository, nameof(repository)); diff --git a/src/GitHub.Exports.Reactive/Services/IGitClient.cs b/src/GitHub.Exports.Reactive/Services/IGitClient.cs index 81fbbafebe..97fb6db56c 100644 --- a/src/GitHub.Exports.Reactive/Services/IGitClient.cs +++ b/src/GitHub.Exports.Reactive/Services/IGitClient.cs @@ -85,31 +85,6 @@ public interface IGitClient /// Task Compare(IRepository repository, string sha1, string sha2, bool detectRenames = false); - /// - /// Compares a file in two commits. - /// - /// The repository - /// The SHA of the first commit. - /// The SHA of the second commit. - /// The relative path to the file. - /// - /// A object or null if one of the commits could not be found in the repository. - /// - Task Compare(IRepository repository, string sha1, string sha2, string path); - - /// - /// Compares a file in a commit to a string. - /// - /// The repository - /// The SHA of the first commit. - /// The SHA of the second commit. - /// The relative path to the file. - /// The contents to compare with the file. - /// - /// A object or null if the commit could not be found in the repository. - /// - Task CompareWith(IRepository repository, string sha1, string sha2, string path, byte[] contents); - /// /// Gets the value of a configuration key. /// diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index f0813370d2..439d7f3005 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -2,7 +2,6 @@ using System.IO; using System.Linq; using System.Threading.Tasks; -using System.Collections.Generic; using System.ComponentModel.Composition; using GitHub.UI; using GitHub.Models; @@ -17,11 +16,13 @@ namespace GitHub.Services public class GitService : IGitService { readonly IRepositoryFacade repositoryFacade; + readonly CompareOptions defaultCompareOptions; [ImportingConstructor] public GitService(IRepositoryFacade repositoryFacade) { this.repositoryFacade = repositoryFacade; + defaultCompareOptions = new CompareOptions(); } /// @@ -228,5 +229,65 @@ public Task GetLatestPushedSha(string path, string remote = "origin") } }); } + + public Task Compare( + IRepository repository, + string sha1, + string sha2, + string path) + { + Guard.ArgumentNotNull(repository, nameof(repository)); + Guard.ArgumentNotEmptyString(sha1, nameof(sha1)); + Guard.ArgumentNotEmptyString(sha2, nameof(sha2)); + Guard.ArgumentNotEmptyString(path, nameof(path)); + + return Task.Run(() => + { + var commit1 = repository.Lookup(sha1); + var commit2 = repository.Lookup(sha2); + + if (commit1 != null && commit2 != null) + { + return repository.Diff.Compare( + commit1.Tree, + commit2.Tree, + new[] { path }, + defaultCompareOptions); + } + else + { + return null; + } + }); + } + + public Task CompareWith(IRepository repository, string sha1, string sha2, string path, byte[] contents) + { + Guard.ArgumentNotNull(repository, nameof(repository)); + Guard.ArgumentNotEmptyString(sha1, nameof(sha1)); + Guard.ArgumentNotEmptyString(sha2, nameof(sha1)); + Guard.ArgumentNotEmptyString(path, nameof(path)); + + return Task.Run(() => + { + var commit1 = repository.Lookup(sha1); + var commit2 = repository.Lookup(sha2); + + var treeChanges = repository.Diff.Compare(commit1.Tree, commit2.Tree, defaultCompareOptions); + var normalizedPath = path.Replace("/", "\\"); + var renamed = treeChanges.FirstOrDefault(x => x.Path == normalizedPath); + var oldPath = renamed?.OldPath ?? path; + + if (commit1 != null) + { + var contentStream = contents != null ? new MemoryStream(contents) : new MemoryStream(); + var blob1 = commit1[oldPath]?.Target as Blob ?? repository.ObjectDatabase.CreateBlob(new MemoryStream()); + var blob2 = repository.ObjectDatabase.CreateBlob(contentStream, path); + return repository.Diff.Compare(blob1, blob2, defaultCompareOptions); + } + + return null; + }); + } } } \ No newline at end of file diff --git a/src/GitHub.Exports/Services/IGitService.cs b/src/GitHub.Exports/Services/IGitService.cs index 4ff850c21b..86a275ac10 100644 --- a/src/GitHub.Exports/Services/IGitService.cs +++ b/src/GitHub.Exports/Services/IGitService.cs @@ -71,5 +71,30 @@ public interface IGitService /// The remote name to look for /// Task GetLatestPushedSha(string path, string remote = "origin"); + + /// + /// Compares a file in two commits. + /// + /// The repository + /// The SHA of the first commit. + /// The SHA of the second commit. + /// The relative path to the file. + /// + /// A object or null if one of the commits could not be found in the repository. + /// + Task Compare(IRepository repository, string sha1, string sha2, string path); + + /// + /// Compares a file in a commit to a string. + /// + /// The repository + /// The SHA of the first commit. + /// The SHA of the second commit. + /// The relative path to the file. + /// The contents to compare with the file. + /// + /// A object or null if the commit could not be found in the repository. + /// + Task CompareWith(IRepository repository, string sha1, string sha2, string path, byte[] contents); } } \ No newline at end of file diff --git a/src/GitHub.InlineReviews/Services/DiffService.cs b/src/GitHub.InlineReviews/Services/DiffService.cs index 625bf7492e..8f685f93ac 100644 --- a/src/GitHub.InlineReviews/Services/DiffService.cs +++ b/src/GitHub.InlineReviews/Services/DiffService.cs @@ -16,12 +16,12 @@ namespace GitHub.InlineReviews.Services [PartCreationPolicy(CreationPolicy.NonShared)] public class DiffService : IDiffService { - readonly IGitClient gitClient; + readonly IGitService gitService; [ImportingConstructor] - public DiffService(IGitClient gitClient) + public DiffService(IGitService gitService) { - this.gitClient = gitClient; + this.gitService = gitService; } /// @@ -31,7 +31,7 @@ public async Task> Diff( string headSha, string path) { - var patch = await gitClient.Compare(repo, baseSha, headSha, path); + var patch = await gitService.Compare(repo, baseSha, headSha, path); if (patch != null) { @@ -39,7 +39,7 @@ public async Task> Diff( } else { - return new DiffChunk[0]; + return Array.Empty(); } } @@ -51,7 +51,7 @@ public async Task> Diff( string path, byte[] contents) { - var changes = await gitClient.CompareWith(repo, baseSha, headSha, path, contents); + var changes = await gitService.CompareWith(repo, baseSha, headSha, path, contents); if (changes?.Patch != null) { @@ -59,7 +59,7 @@ public async Task> Diff( } else { - return new DiffChunk[0]; + return Array.Empty(); } } } diff --git a/test/GitHub.InlineReviews.UnitTests/Services/DiffServiceTests.cs b/test/GitHub.InlineReviews.UnitTests/Services/DiffServiceTests.cs index 16586848d2..22d9409625 100644 --- a/test/GitHub.InlineReviews.UnitTests/Services/DiffServiceTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Services/DiffServiceTests.cs @@ -17,7 +17,7 @@ public class TheParseFragmentMethod [Test] public void ShouldParsePr960() { - var target = new DiffService(Substitute.For()); + var target = new DiffService(Substitute.For()); var result = DiffUtilities.ParseFragment(Properties.Resources.pr_960_diff).ToList(); Assert.That(4, Is.EqualTo(result.Count)); diff --git a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs b/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs index 0c01fba2e8..d8b27511a1 100644 --- a/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs +++ b/test/GitHub.InlineReviews.UnitTests/TestDoubles/FakeDiffService.cs @@ -22,13 +22,13 @@ sealed class FakeDiffService : IDiffService, IDisposable public FakeDiffService() { this.repository = CreateRepository(); - this.inner = new DiffService(Substitute.For()); + this.inner = new DiffService(Substitute.For()); } public FakeDiffService(string path, string contents) { this.repository = CreateRepository(); - this.inner = new DiffService(Substitute.For()); + this.inner = new DiffService(Substitute.For()); AddFile(path, contents); } diff --git a/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs b/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs index ddf11c119d..ced08560d9 100644 --- a/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs +++ b/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs @@ -319,5 +319,7 @@ public LocalRepositoryModel 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 Task Compare(IRepository repository, string sha1, string sha2, string path) => throw new NotImplementedException(); + public Task CompareWith(IRepository repository, string sha1, string sha2, string path, byte[] contents) => throw new NotImplementedException(); } } From e6ebab6068d99ef19eb2a3f9d51053864c89b52a Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 8 Jan 2019 16:05:16 +0000 Subject: [PATCH 09/21] Add failing Indent_Heuristic_Is_Enabled test --- .../GitServiceIntegrationTests.cs | 61 +++++++++++++++---- 1 file changed, 48 insertions(+), 13 deletions(-) diff --git a/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs index 6ad96ea7ec..6edc16fab0 100644 --- a/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs +++ b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs @@ -7,6 +7,41 @@ public class GitServiceIntegrationTests { + public class TheCompareMethod + { + [TestCase("1.2.", "1.2.3.4.", "+3.+4.", Description = "Two lines added")] + public async Task Simple_Diff(string content1, string content2, string expectPatch) + { + using (var temp = new TempRepository()) + { + var path = "foo.txt"; + var commit1 = AddCommit(temp.Repository, path, content1.Replace('.', '\n')); + var commit2 = AddCommit(temp.Repository, path, content2.Replace('.', '\n')); + var target = new GitService(new RepositoryFacade()); + + var patch = await target.Compare(temp.Repository, commit1.Sha, commit2.Sha, path); + + Assert.That(patch.Content.Replace('\n', '.'), Contains.Substring(expectPatch)); + } + } + + [TestCase("1.2.a..b.3.4", "1.2.a..b.a..b.3.4", "+b.+a.+.")] // This would be "+a.+.+b." without Indent-heuristic + public async Task Indent_Heuristic_Is_Enabled(string content1, string content2, string expectPatch) + { + using (var temp = new TempRepository()) + { + var path = "foo.txt"; + var commit1 = AddCommit(temp.Repository, path, content1.Replace('.', '\n')); + var commit2 = AddCommit(temp.Repository, path, content2.Replace('.', '\n')); + var target = new GitService(new RepositoryFacade()); + + var patch = await target.Compare(temp.Repository, commit1.Sha, commit2.Sha, path); + + Assert.That(patch.Content.Replace('\n', '.'), Contains.Substring(expectPatch)); + } + } + } + public class TheCreateLocalRepositoryModelMethod { [Test] @@ -296,19 +331,6 @@ public async Task BehindRemoteBranch_ReturnRemoteCommitSha(string remoteName, bo } } - static Commit AddCommit(Repository repo) - { - var dir = repo.Info.WorkingDirectory; - var path = "file.txt"; - var file = Path.Combine(dir, path); - var guidString = Guid.NewGuid().ToString(); - File.WriteAllText(file, guidString); - Commands.Stage(repo, path); - var signature = new Signature("foobar", "foobar@github.com", DateTime.Now); - var commit = repo.Commit("message", signature, signature); - return commit; - } - static void AddTrackedBranch(Repository repo, Branch branch, Commit commit, string trackedBranchName = null, string remoteName = "origin") { @@ -324,6 +346,19 @@ static void AddTrackedBranch(Repository repo, Branch branch, Commit commit, } } + static Commit AddCommit(Repository repo, string path = "file.txt", string content = null) + { + content = content ?? Guid.NewGuid().ToString(); + + var dir = repo.Info.WorkingDirectory; + var file = Path.Combine(dir, path); + File.WriteAllText(file, content); + Commands.Stage(repo, path); + var signature = new Signature("foobar", "foobar@github.com", DateTime.Now); + var commit = repo.Commit("message", signature, signature); + return commit; + } + protected class TempRepository : TempDirectory { public TempRepository() From e0eb92c6db7669417772def06210824df9db0541 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 8 Jan 2019 16:46:39 +0000 Subject: [PATCH 10/21] Add tests for GitService.CompareWith --- .../GitServiceIntegrationTests.cs | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs index 6edc16fab0..7e57ef16de 100644 --- a/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs +++ b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs @@ -1,5 +1,6 @@ using System; using System.IO; +using System.Text; using System.Threading.Tasks; using GitHub.Services; using LibGit2Sharp; @@ -42,6 +43,43 @@ public async Task Indent_Heuristic_Is_Enabled(string content1, string content2, } } + public class TheCompareWithMethod + { + [TestCase("1.2.", "1.2.3.4.", "+3.+4.", Description = "Two lines added")] + public async Task Simple_Diff(string content1, string content2, string expectPatch) + { + using (var temp = new TempRepository()) + { + var path = "foo.txt"; + var commit1 = AddCommit(temp.Repository, path, content1.Replace('.', '\n')); + var commit2 = AddCommit(temp.Repository, path, content2.Replace('.', '\n')); + var contentBytes = new UTF8Encoding(false).GetBytes(content2.Replace('.', '\n')); + var target = new GitService(new RepositoryFacade()); + + var changes = await target.CompareWith(temp.Repository, commit1.Sha, commit2.Sha, path, contentBytes); + + Assert.That(changes.Patch.Replace('\n', '.'), Contains.Substring(expectPatch)); + } + } + + [TestCase("1.2.a..b.3.4", "1.2.a..b.a..b.3.4", "+b.+a.+.")] // This would be "+a.+.+b." without Indent-heuristic + public async Task Indent_Heuristic_Is_Enabled(string content1, string content2, string expectPatch) + { + using (var temp = new TempRepository()) + { + var path = "foo.txt"; + var commit1 = AddCommit(temp.Repository, path, content1.Replace('.', '\n')); + var commit2 = AddCommit(temp.Repository, path, content2.Replace('.', '\n')); + var contentBytes = new UTF8Encoding(false).GetBytes(content2.Replace('.', '\n')); + var target = new GitService(new RepositoryFacade()); + + var changes = await target.CompareWith(temp.Repository, commit1.Sha, commit2.Sha, path, contentBytes); + + Assert.That(changes.Patch.Replace('\n', '.'), Contains.Substring(expectPatch)); + } + } + } + public class TheCreateLocalRepositoryModelMethod { [Test] From c222de20b8e2ceaf785825a16ce7afd9a0167eaf Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 8 Jan 2019 16:47:05 +0000 Subject: [PATCH 11/21] Enable IndentHeuristic and make tests pass --- src/GitHub.Exports/Services/GitService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index 439d7f3005..01463e8abd 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -22,7 +22,7 @@ public class GitService : IGitService public GitService(IRepositoryFacade repositoryFacade) { this.repositoryFacade = repositoryFacade; - defaultCompareOptions = new CompareOptions(); + defaultCompareOptions = new CompareOptions { IndentHeuristic = true }; } /// From d95149d57f40a195aaee494e3e0680cbdc9f8546 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 8 Jan 2019 17:15:36 +0000 Subject: [PATCH 12/21] Move IGitClient.Compare method to IGitService --- .../SampleData/GitServiceDesigner.cs | 1 + src/GitHub.App/Services/GitClient.cs | 40 +------------------ src/GitHub.App/Services/PullRequestService.cs | 2 +- .../Services/IGitClient.cs | 13 ------ src/GitHub.Exports/Services/GitService.cs | 32 +++++++++++++++ src/GitHub.Exports/Services/IGitService.cs | 13 ++++++ .../Services/GitClientTests.cs | 2 +- .../VSGitExtTests.cs | 1 + 8 files changed, 50 insertions(+), 54 deletions(-) diff --git a/src/GitHub.App/SampleData/GitServiceDesigner.cs b/src/GitHub.App/SampleData/GitServiceDesigner.cs index 2a13d27e3d..522bde697b 100644 --- a/src/GitHub.App/SampleData/GitServiceDesigner.cs +++ b/src/GitHub.App/SampleData/GitServiceDesigner.cs @@ -17,5 +17,6 @@ class GitServiceDesigner : IGitService public UriString GetUri(IRepository repository, string remote = "origin") => null; public Task Compare(IRepository repository, string sha1, string sha2, string path) => null; public Task CompareWith(IRepository repository, string sha1, string sha2, string path, byte[] contents) => null; + public Task Compare(IRepository repository, string sha1, string sha2, bool detectRenames = false) => null; } } diff --git a/src/GitHub.App/Services/GitClient.cs b/src/GitHub.App/Services/GitClient.cs index 96db071454..d9af790193 100644 --- a/src/GitHub.App/Services/GitClient.cs +++ b/src/GitHub.App/Services/GitClient.cs @@ -22,7 +22,6 @@ public class GitClient : IGitClient readonly PullOptions pullOptions; readonly PushOptions pushOptions; readonly FetchOptions fetchOptions; - readonly CompareOptions compareOptions; [ImportingConstructor] public GitClient(IGitHubCredentialProvider credentialProvider, IGitService gitService) @@ -39,11 +38,6 @@ public GitClient(IGitHubCredentialProvider credentialProvider, IGitService gitSe FetchOptions = fetchOptions, MergeOptions = new MergeOptions(), }; - - compareOptions = new CompareOptions - { - IndentHeuristic = true - }; } public Task Pull(IRepository repository) @@ -199,38 +193,6 @@ public Task CreateBranch(IRepository repository, string branchName) }); } - public Task Compare( - IRepository repository, - string sha1, - string sha2, - bool detectRenames) - { - Guard.ArgumentNotNull(repository, nameof(repository)); - Guard.ArgumentNotEmptyString(sha1, nameof(sha1)); - Guard.ArgumentNotEmptyString(sha2, nameof(sha2)); - - return Task.Run(() => - { - var options = new CompareOptions - { - Similarity = detectRenames ? SimilarityOptions.Renames : SimilarityOptions.None, - IndentHeuristic = compareOptions.IndentHeuristic - }; - - var commit1 = repository.Lookup(sha1); - var commit2 = repository.Lookup(sha2); - - if (commit1 != null && commit2 != null) - { - return repository.Diff.Compare(commit1.Tree, commit2.Tree, options); - } - else - { - return null; - } - }); - } - public Task GetConfig(IRepository repository, string key) { Guard.ArgumentNotNull(repository, nameof(repository)); @@ -381,7 +343,7 @@ public Task IsModified(IRepository repository, string path, byte[] content using (var s = contents != null ? new MemoryStream(contents) : new MemoryStream()) { var blob2 = repository.ObjectDatabase.CreateBlob(s, path); - var diff = repository.Diff.Compare(blob1, blob2, compareOptions); + var diff = repository.Diff.Compare(blob1, blob2); return diff.LinesAdded != 0 || diff.LinesDeleted != 0; } } diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs index abfc1feac1..c36936aea6 100644 --- a/src/GitHub.App/Services/PullRequestService.cs +++ b/src/GitHub.App/Services/PullRequestService.cs @@ -631,7 +631,7 @@ public IObservable GetTreeChanges(LocalRepositoryModel repository, { var remote = await gitClient.GetHttpRemote(repo, "origin"); await gitClient.Fetch(repo, remote.Name); - var changes = await gitClient.Compare(repo, pullRequest.BaseRefSha, pullRequest.HeadRefSha, detectRenames: true); + var changes = await gitService.Compare(repo, pullRequest.BaseRefSha, pullRequest.HeadRefSha, detectRenames: true); return Observable.Return(changes); } }); diff --git a/src/GitHub.Exports.Reactive/Services/IGitClient.cs b/src/GitHub.Exports.Reactive/Services/IGitClient.cs index 97fb6db56c..ff127be6b1 100644 --- a/src/GitHub.Exports.Reactive/Services/IGitClient.cs +++ b/src/GitHub.Exports.Reactive/Services/IGitClient.cs @@ -72,19 +72,6 @@ public interface IGitClient /// Task CreateBranch(IRepository repository, string branchName); - /// - /// Compares two commits. - /// - /// The repository - /// The SHA of the first commit. - /// The SHA of the second commit. - /// Whether to detect renames - /// - /// A object or null if one of the commits could not be found in the repository, - /// (e.g. it is from a fork). - /// - Task Compare(IRepository repository, string sha1, string sha2, bool detectRenames = false); - /// /// Gets the value of a configuration key. /// diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index 01463e8abd..575b392f50 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -289,5 +289,37 @@ public Task CompareWith(IRepository repository, string sha1, str return null; }); } + + public Task Compare( + IRepository repository, + string sha1, + string sha2, + bool detectRenames) + { + Guard.ArgumentNotNull(repository, nameof(repository)); + Guard.ArgumentNotEmptyString(sha1, nameof(sha1)); + Guard.ArgumentNotEmptyString(sha2, nameof(sha2)); + + return Task.Run(() => + { + var options = new CompareOptions + { + Similarity = detectRenames ? SimilarityOptions.Renames : SimilarityOptions.None, + IndentHeuristic = defaultCompareOptions.IndentHeuristic + }; + + var commit1 = repository.Lookup(sha1); + var commit2 = repository.Lookup(sha2); + + if (commit1 != null && commit2 != null) + { + return repository.Diff.Compare(commit1.Tree, commit2.Tree, options); + } + else + { + return null; + } + }); + } } } \ No newline at end of file diff --git a/src/GitHub.Exports/Services/IGitService.cs b/src/GitHub.Exports/Services/IGitService.cs index 86a275ac10..b107773f67 100644 --- a/src/GitHub.Exports/Services/IGitService.cs +++ b/src/GitHub.Exports/Services/IGitService.cs @@ -96,5 +96,18 @@ public interface IGitService /// A object or null if the commit could not be found in the repository. /// Task CompareWith(IRepository repository, string sha1, string sha2, string path, byte[] contents); + + /// + /// Compares two commits. + /// + /// The repository + /// The SHA of the first commit. + /// The SHA of the second commit. + /// Whether to detect renames + /// + /// A object or null if one of the commits could not be found in the repository, + /// (e.g. it is from a fork). + /// + Task Compare(IRepository repository, string sha1, string sha2, bool detectRenames = false); } } \ No newline at end of file diff --git a/test/GitHub.App.UnitTests/Services/GitClientTests.cs b/test/GitHub.App.UnitTests/Services/GitClientTests.cs index 021eae565b..c48d560ad5 100644 --- a/test/GitHub.App.UnitTests/Services/GitClientTests.cs +++ b/test/GitHub.App.UnitTests/Services/GitClientTests.cs @@ -82,7 +82,7 @@ public async Task ContentChangesAsync(int linesAdded, int linesDeleted, bool exp var changes = Substitute.For(); changes.LinesAdded.Returns(linesAdded); changes.LinesDeleted.Returns(linesDeleted); - repo.Diff.Compare(null as Blob, null as Blob, null as CompareOptions).ReturnsForAnyArgs(changes); + repo.Diff.Compare(null as Blob, null as Blob).ReturnsForAnyArgs(changes); var gitClient = CreateGitClient(); var modified = await gitClient.IsModified(repo, path, null); diff --git a/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs b/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs index ced08560d9..aea7f6090a 100644 --- a/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs +++ b/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs @@ -321,5 +321,6 @@ public LocalRepositoryModel CreateLocalRepositoryModel(string localPath) public UriString GetUri(string path, string remote = "origin") => throw new NotImplementedException(); public Task Compare(IRepository repository, string sha1, string sha2, string path) => throw new NotImplementedException(); public Task CompareWith(IRepository repository, string sha1, string sha2, string path, byte[] contents) => throw new NotImplementedException(); + public Task Compare(IRepository repository, string sha1, string sha2, bool detectRenames = false) => throw new NotImplementedException(); } } From df677afcd6298c792bba3339d43163dd4dcaa4a9 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 8 Jan 2019 17:41:27 +0000 Subject: [PATCH 13/21] Add test for Compare that returns TreeChanges --- .../GitServiceIntegrationTests.cs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs index 7e57ef16de..91a2483903 100644 --- a/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs +++ b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs @@ -1,5 +1,6 @@ using System; using System.IO; +using System.Linq; using System.Text; using System.Threading.Tasks; using GitHub.Services; @@ -41,6 +42,22 @@ public async Task Indent_Heuristic_Is_Enabled(string content1, string content2, Assert.That(patch.Content.Replace('\n', '.'), Contains.Substring(expectPatch)); } } + + [TestCase("foo", "bar")] + public async Task One_File_Is_Modified(string content1, string content2) + { + using (var temp = new TempRepository()) + { + var path = "foo.txt"; + var commit1 = AddCommit(temp.Repository, path, content1.Replace('.', '\n')); + var commit2 = AddCommit(temp.Repository, path, content2.Replace('.', '\n')); + var target = new GitService(new RepositoryFacade()); + + var treeChanges = await target.Compare(temp.Repository, commit1.Sha, commit2.Sha, false); + + Assert.That(treeChanges.Modified.FirstOrDefault()?.Path, Is.EqualTo(path)); + } + } } public class TheCompareWithMethod From b3485274a56a53731f78731b0bb39c0aff130f36 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 9 Jan 2019 12:58:09 +0000 Subject: [PATCH 14/21] Add failing test for GitService.Compare There is a bug where a renamed file is in a directory. --- .../GitServiceIntegrationTests.cs | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs index 91a2483903..4b64836337 100644 --- a/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs +++ b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs @@ -95,6 +95,25 @@ public async Task Indent_Heuristic_Is_Enabled(string content1, string content2, Assert.That(changes.Patch.Replace('\n', '.'), Contains.Substring(expectPatch)); } } + + [TestCase("foo.txt", "a.b.", "bar.txt", "a.b.c.d.", 2)] + [TestCase(@"dir\foo.txt", "a.b.", @"dir\bar.txt", "a.b.c.d.", 2)] + [TestCase(@"dir\foo.txt", "a.b.", @"dir\foo.txt", "a.b.c.d.", 2)] + [TestCase(@"dir\unrelated.txt", "x.x.x.x.", @"dir\foo.txt", "a.b.c.d.", 4)] + public async Task Rename(string oldPath, string oldContent, string newPath, string newContent, int expectLinesAdded) + { + using (var temp = new TempRepository()) + { + var commit1 = AddCommit(temp.Repository, oldPath, oldContent.Replace('.', '\n')); + var commit2 = AddCommit(temp.Repository, newPath, newContent.Replace('.', '\n')); + var contentBytes = new UTF8Encoding(false).GetBytes(newContent.Replace('.', '\n')); + var target = new GitService(new RepositoryFacade()); + + var changes = await target.CompareWith(temp.Repository, commit1.Sha, commit2.Sha, newPath, contentBytes); + + Assert.That(changes?.LinesAdded, Is.EqualTo(expectLinesAdded)); + } + } } public class TheCreateLocalRepositoryModelMethod @@ -406,14 +425,25 @@ static Commit AddCommit(Repository repo, string path = "file.txt", string conten content = content ?? Guid.NewGuid().ToString(); var dir = repo.Info.WorkingDirectory; + DeleteFilesNotInGit(dir); var file = Path.Combine(dir, path); + Directory.CreateDirectory(Path.GetDirectoryName(file)); File.WriteAllText(file, content); - Commands.Stage(repo, path); + Commands.Stage(repo, "*"); var signature = new Signature("foobar", "foobar@github.com", DateTime.Now); var commit = repo.Commit("message", signature, signature); return commit; } + static void DeleteFilesNotInGit(string dir) + { + var gitDir = Path.Combine(dir, @".git\"); + Directory.GetFiles(dir, "*", SearchOption.AllDirectories) + .Where(f => !f.StartsWith(gitDir, StringComparison.OrdinalIgnoreCase)) + .ToList() + .ForEach(File.Delete); + } + protected class TempRepository : TempDirectory { public TempRepository() From cc439656987796ad5a564681c061c7b602e68979 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 9 Jan 2019 13:24:59 +0000 Subject: [PATCH 15/21] Change to use / separator for Git path The new version of LibGit2Sharp expects / separators in Git paths. The previous version wasn't fussy. --- src/GitHub.Exports/Services/GitService.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index 575b392f50..f655c77153 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -274,11 +274,11 @@ public Task CompareWith(IRepository repository, string sha1, str var commit2 = repository.Lookup(sha2); var treeChanges = repository.Diff.Compare(commit1.Tree, commit2.Tree, defaultCompareOptions); - var normalizedPath = path.Replace("/", "\\"); - var renamed = treeChanges.FirstOrDefault(x => x.Path == normalizedPath); - var oldPath = renamed?.OldPath ?? path; + var normalizedPath = path.Replace("\\", "/"); + var change = treeChanges.FirstOrDefault(x => x.Path == normalizedPath); + var oldPath = change?.OldPath; - if (commit1 != null) + if (commit1 != null && oldPath != null) { var contentStream = contents != null ? new MemoryStream(contents) : new MemoryStream(); var blob1 = commit1[oldPath]?.Target as Blob ?? repository.ObjectDatabase.CreateBlob(new MemoryStream()); From f685a22d3831ffd6e88c5966850630d9ff9f877b Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 9 Jan 2019 15:26:49 +0000 Subject: [PATCH 16/21] Throw ArgumentException if path contains \ Check that path doesn't contain \ rather than automatically convert to use /. --- src/GitHub.Exports/Services/GitService.cs | 7 +++-- src/GitHub.Exports/Services/IGitService.cs | 3 ++- .../GitServiceIntegrationTests.cs | 26 ++++++++++++++++--- 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index f655c77153..bd0109ad9d 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -267,6 +267,10 @@ public Task CompareWith(IRepository repository, string sha1, str Guard.ArgumentNotEmptyString(sha1, nameof(sha1)); Guard.ArgumentNotEmptyString(sha2, nameof(sha1)); Guard.ArgumentNotEmptyString(path, nameof(path)); + if (path.Contains('\\')) + { + throw new ArgumentException($"The value for '{nameof(path)}' must use '/' not '\\' as directory separator", nameof(path)); + } return Task.Run(() => { @@ -274,8 +278,7 @@ public Task CompareWith(IRepository repository, string sha1, str var commit2 = repository.Lookup(sha2); var treeChanges = repository.Diff.Compare(commit1.Tree, commit2.Tree, defaultCompareOptions); - var normalizedPath = path.Replace("\\", "/"); - var change = treeChanges.FirstOrDefault(x => x.Path == normalizedPath); + var change = treeChanges.FirstOrDefault(x => x.Path == path); var oldPath = change?.OldPath; if (commit1 != null && oldPath != null) diff --git a/src/GitHub.Exports/Services/IGitService.cs b/src/GitHub.Exports/Services/IGitService.cs index b107773f67..7ad16ad6de 100644 --- a/src/GitHub.Exports/Services/IGitService.cs +++ b/src/GitHub.Exports/Services/IGitService.cs @@ -90,11 +90,12 @@ public interface IGitService /// The repository /// The SHA of the first commit. /// The SHA of the second commit. - /// The relative path to the file. + /// The relative path to the file (using '/' directory separator). /// The contents to compare with the file. /// /// A object or null if the commit could not be found in the repository. /// + /// If contains a '\'. Task CompareWith(IRepository repository, string sha1, string sha2, string path, byte[] contents); /// diff --git a/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs index 4b64836337..91b42b256a 100644 --- a/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs +++ b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs @@ -97,10 +97,10 @@ public async Task Indent_Heuristic_Is_Enabled(string content1, string content2, } [TestCase("foo.txt", "a.b.", "bar.txt", "a.b.c.d.", 2)] - [TestCase(@"dir\foo.txt", "a.b.", @"dir\bar.txt", "a.b.c.d.", 2)] - [TestCase(@"dir\foo.txt", "a.b.", @"dir\foo.txt", "a.b.c.d.", 2)] - [TestCase(@"dir\unrelated.txt", "x.x.x.x.", @"dir\foo.txt", "a.b.c.d.", 4)] - public async Task Rename(string oldPath, string oldContent, string newPath, string newContent, int expectLinesAdded) + [TestCase(@"dir/foo.txt", "a.b.", @"dir/bar.txt", "a.b.c.d.", 2)] + [TestCase(@"dir/foo.txt", "a.b.", @"dir/foo.txt", "a.b.c.d.", 2)] + [TestCase(@"dir/unrelated.txt", "x.x.x.x.", @"dir/foo.txt", "a.b.c.d.", 4)] + public async Task Can_Handle_Renames(string oldPath, string oldContent, string newPath, string newContent, int expectLinesAdded) { using (var temp = new TempRepository()) { @@ -114,6 +114,24 @@ public async Task Rename(string oldPath, string oldContent, string newPath, stri Assert.That(changes?.LinesAdded, Is.EqualTo(expectLinesAdded)); } } + + [Test] + public void Path_Must_Not_Use_Windows_Directory_Separator() + { + using (var temp = new TempRepository()) + { + var path = @"dir\foo.txt"; + var oldContent = "oldContent"; + var newContent = "newContent"; + var commit1 = AddCommit(temp.Repository, path, oldContent); + var commit2 = AddCommit(temp.Repository, path, newContent); + var contentBytes = new UTF8Encoding(false).GetBytes(newContent); + var target = new GitService(new RepositoryFacade()); + + Assert.ThrowsAsync(() => + target.CompareWith(temp.Repository, commit1.Sha, commit2.Sha, path, contentBytes)); + } + } } public class TheCreateLocalRepositoryModelMethod From 346bfe1a52ddc7b2a5a55a9346477a0eb4724cce Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 9 Jan 2019 16:17:41 +0000 Subject: [PATCH 17/21] Factor out Guard.ArgumentIsGitPath --- src/GitHub.Exports/Services/GitService.cs | 8 ++------ src/GitHub.Extensions/Guard.cs | 18 ++++++++++++++++-- .../GitServiceIntegrationTests.cs | 18 ++++++++++++++++++ 3 files changed, 36 insertions(+), 8 deletions(-) diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index bd0109ad9d..e2e886acb7 100644 --- a/src/GitHub.Exports/Services/GitService.cs +++ b/src/GitHub.Exports/Services/GitService.cs @@ -239,7 +239,7 @@ public Task Compare( Guard.ArgumentNotNull(repository, nameof(repository)); Guard.ArgumentNotEmptyString(sha1, nameof(sha1)); Guard.ArgumentNotEmptyString(sha2, nameof(sha2)); - Guard.ArgumentNotEmptyString(path, nameof(path)); + Guard.ArgumentIsGitPath(path, nameof(path)); return Task.Run(() => { @@ -266,11 +266,7 @@ public Task CompareWith(IRepository repository, string sha1, str Guard.ArgumentNotNull(repository, nameof(repository)); Guard.ArgumentNotEmptyString(sha1, nameof(sha1)); Guard.ArgumentNotEmptyString(sha2, nameof(sha1)); - Guard.ArgumentNotEmptyString(path, nameof(path)); - if (path.Contains('\\')) - { - throw new ArgumentException($"The value for '{nameof(path)}' must use '/' not '\\' as directory separator", nameof(path)); - } + Guard.ArgumentIsGitPath(path, nameof(path)); return Task.Run(() => { diff --git a/src/GitHub.Extensions/Guard.cs b/src/GitHub.Extensions/Guard.cs index 240dbd4cbd..0862f8c3f0 100644 --- a/src/GitHub.Extensions/Guard.cs +++ b/src/GitHub.Extensions/Guard.cs @@ -1,6 +1,5 @@ using System; -using System.Collections.Generic; -using System.Diagnostics; +using System.IO; using System.Globalization; using System.Linq; @@ -8,6 +7,21 @@ namespace GitHub.Extensions { public static class Guard { + public static void ArgumentIsGitPath(string value, string name) + { + ArgumentNotNull(value, name); + + if (value.Contains('\\')) + { + throw new ArgumentException($"The value '{value}' must use '/' not '\\' as directory separator", name); + } + + if (Path.IsPathRooted(value)) + { + throw new ArgumentException($"The value '{value}' must not be rooted", name); + } + } + public static void ArgumentNotNull(object value, string name) { if (value != null) return; diff --git a/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs index 91b42b256a..30b6a49381 100644 --- a/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs +++ b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs @@ -58,6 +58,24 @@ public async Task One_File_Is_Modified(string content1, string content2) Assert.That(treeChanges.Modified.FirstOrDefault()?.Path, Is.EqualTo(path)); } } + + + [Test] + public void Path_Must_Not_Use_Windows_Directory_Separator() + { + using (var temp = new TempRepository()) + { + var path = @"dir\foo.txt"; + var oldContent = "oldContent"; + var newContent = "newContent"; + var commit1 = AddCommit(temp.Repository, path, oldContent); + var commit2 = AddCommit(temp.Repository, path, newContent); + var target = new GitService(new RepositoryFacade()); + + Assert.ThrowsAsync(() => + target.Compare(temp.Repository, commit1.Sha, commit2.Sha, path)); + } + } } public class TheCompareWithMethod From 7d87a4086953cf4ba43db9ae68057fcae83ea734 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 9 Jan 2019 16:57:23 +0000 Subject: [PATCH 18/21] Add Guard.ArgumentIsGitPath to appropriate methods --- src/GitHub.App/Services/GitClient.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/GitHub.App/Services/GitClient.cs b/src/GitHub.App/Services/GitClient.cs index d9af790193..c2ff185970 100644 --- a/src/GitHub.App/Services/GitClient.cs +++ b/src/GitHub.App/Services/GitClient.cs @@ -279,7 +279,7 @@ public Task ExtractFile(IRepository repository, string commitSha, string { Guard.ArgumentNotNull(repository, nameof(repository)); Guard.ArgumentNotEmptyString(commitSha, nameof(commitSha)); - Guard.ArgumentNotEmptyString(fileName, nameof(fileName)); + Guard.ArgumentIsGitPath(fileName, nameof(fileName)); return Task.Run(() => { @@ -298,7 +298,7 @@ public Task ExtractFileBinary(IRepository repository, string commitSha, { Guard.ArgumentNotNull(repository, nameof(repository)); Guard.ArgumentNotEmptyString(commitSha, nameof(commitSha)); - Guard.ArgumentNotEmptyString(fileName, nameof(fileName)); + Guard.ArgumentIsGitPath(fileName, nameof(fileName)); return Task.Run(() => { @@ -327,7 +327,7 @@ public Task ExtractFileBinary(IRepository repository, string commitSha, public Task IsModified(IRepository repository, string path, byte[] contents) { Guard.ArgumentNotNull(repository, nameof(repository)); - Guard.ArgumentNotEmptyString(path, nameof(path)); + Guard.ArgumentIsGitPath(path, nameof(path)); return Task.Run(() => { From 700689792e4f6e0a2cc126b66684376b9ec5a8bd Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 9 Jan 2019 16:59:11 +0000 Subject: [PATCH 19/21] Fix issue where a Windows path is being used Make private ExtractToTempFile method accept Git path. --- src/GitHub.App/Services/PullRequestService.cs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs index b0850275c5..6c3e0e584a 100644 --- a/src/GitHub.App/Services/PullRequestService.cs +++ b/src/GitHub.App/Services/PullRequestService.cs @@ -62,7 +62,7 @@ public class PullRequestService : IPullRequestService, IStaticReviewFileMap readonly IUsageTracker usageTracker; readonly IDictionary tempFileMappings; - + [ImportingConstructor] public PullRequestService( IGitClient gitClient, @@ -751,20 +751,20 @@ public async Task ExtractToTempFile( Encoding encoding) { var tempFilePath = CalculateTempFileName(relativePath, commitSha, encoding); + var gitPath = relativePath.TrimStart('/').Replace('\\', '/'); if (!File.Exists(tempFilePath)) { using (var repo = gitService.GetRepository(repository.LocalPath)) { var remote = await gitClient.GetHttpRemote(repo, "origin"); - await ExtractToTempFile(repo, pullRequest.Number, commitSha, relativePath, encoding, tempFilePath); + await ExtractToTempFile(repo, pullRequest.Number, commitSha, gitPath, encoding, tempFilePath); } } - lock (this.tempFileMappings) + lock (tempFileMappings) { - string gitRelativePath = relativePath.TrimStart('/').Replace('\\', '/'); - this.tempFileMappings[CanonicalizeLocalFilePath(tempFilePath)] = (commitSha, gitRelativePath); + tempFileMappings[CanonicalizeLocalFilePath(tempFilePath)] = (commitSha, gitPath); } return tempFilePath; @@ -894,22 +894,24 @@ async Task ExtractToTempFile( IRepository repo, int pullRequestNumber, string commitSha, - string relativePath, + string path, Encoding encoding, string tempFilePath) { + Guard.ArgumentIsGitPath(path, nameof(path)); + string contents; try { - contents = await gitClient.ExtractFile(repo, commitSha, relativePath) ?? string.Empty; + contents = await gitClient.ExtractFile(repo, commitSha, path) ?? string.Empty; } catch (FileNotFoundException) { var pullHeadRef = $"refs/pull/{pullRequestNumber}/head"; var remote = await gitClient.GetHttpRemote(repo, "origin"); await gitClient.Fetch(repo, remote.Name, commitSha, pullHeadRef); - contents = await gitClient.ExtractFile(repo, commitSha, relativePath) ?? string.Empty; + contents = await gitClient.ExtractFile(repo, commitSha, path) ?? string.Empty; } Directory.CreateDirectory(Path.GetDirectoryName(tempFilePath)); From 8c5d1f7e9f8101910ab560af2d974d2cc9c0f80d Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 9 Jan 2019 19:56:27 +0000 Subject: [PATCH 20/21] Add guards to HasChangesInWorkingDirectory --- src/GitHub.App/Services/GitHubContextService.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/GitHub.App/Services/GitHubContextService.cs b/src/GitHub.App/Services/GitHubContextService.cs index 8891995f31..0875a26921 100644 --- a/src/GitHub.App/Services/GitHubContextService.cs +++ b/src/GitHub.App/Services/GitHubContextService.cs @@ -386,6 +386,10 @@ public string FindObjectishForTFSTempFile(string tempFile) /// public bool HasChangesInWorkingDirectory(string repositoryDir, string commitish, string path) { + Guard.ArgumentNotNull(path, nameof(repositoryDir)); + Guard.ArgumentNotNull(path, nameof(commitish)); + Guard.ArgumentIsGitPath(path, nameof(path)); + using (var repo = gitService.GetRepository(repositoryDir)) { var commit = repo.Lookup(commitish); From f9c9ccedcff36f92f379363d91c8068ff1a7e097 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 19 Mar 2019 11:13:10 +0000 Subject: [PATCH 21/21] Updatre to use LibGit2Sharp 0.26.0 final --- src/GitHub.App/GitHub.App.csproj | 2 +- src/GitHub.Exports/GitHub.Exports.csproj | 2 +- src/GitHub.InlineReviews/GitHub.InlineReviews.csproj | 2 +- src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj | 2 +- src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj | 2 +- src/GitHub.TeamFoundation.16/GitHub.TeamFoundation.16.csproj | 2 +- src/GitHub.VisualStudio/GitHub.VisualStudio.csproj | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/GitHub.App/GitHub.App.csproj b/src/GitHub.App/GitHub.App.csproj index a6be140d11..d9b0ef5354 100644 --- a/src/GitHub.App/GitHub.App.csproj +++ b/src/GitHub.App/GitHub.App.csproj @@ -42,7 +42,7 @@ - + diff --git a/src/GitHub.Exports/GitHub.Exports.csproj b/src/GitHub.Exports/GitHub.Exports.csproj index a97fb52f93..b6fa2fc7f5 100644 --- a/src/GitHub.Exports/GitHub.Exports.csproj +++ b/src/GitHub.Exports/GitHub.Exports.csproj @@ -23,7 +23,7 @@ - + diff --git a/src/GitHub.InlineReviews/GitHub.InlineReviews.csproj b/src/GitHub.InlineReviews/GitHub.InlineReviews.csproj index c0c35b167f..314d26ae3d 100644 --- a/src/GitHub.InlineReviews/GitHub.InlineReviews.csproj +++ b/src/GitHub.InlineReviews/GitHub.InlineReviews.csproj @@ -280,7 +280,7 @@ - + 0.2.1 diff --git a/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj b/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj index d35fda64a8..743854e5f3 100644 --- a/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj +++ b/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj @@ -163,7 +163,7 @@ 8.0.2 - + 14.0.25424 diff --git a/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj b/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj index 327c3b101b..5de1b808a3 100644 --- a/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj +++ b/src/GitHub.TeamFoundation.15/GitHub.TeamFoundation.15.csproj @@ -211,7 +211,7 @@ 8.0.2 - + 14.0.25424 diff --git a/src/GitHub.TeamFoundation.16/GitHub.TeamFoundation.16.csproj b/src/GitHub.TeamFoundation.16/GitHub.TeamFoundation.16.csproj index 02d00ec7e8..7557eddc83 100644 --- a/src/GitHub.TeamFoundation.16/GitHub.TeamFoundation.16.csproj +++ b/src/GitHub.TeamFoundation.16/GitHub.TeamFoundation.16.csproj @@ -218,7 +218,7 @@ 8.0.2 - + 2.5.0 diff --git a/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj b/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj index 518693e781..bea7fcdbc0 100644 --- a/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj +++ b/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj @@ -304,7 +304,7 @@ 14.1.24720 - + true