diff --git a/src/GitHub.App/GitHub.App.csproj b/src/GitHub.App/GitHub.App.csproj index a987b966bd..d9b0ef5354 100644 --- a/src/GitHub.App/GitHub.App.csproj +++ b/src/GitHub.App/GitHub.App.csproj @@ -42,8 +42,7 @@ - - + diff --git a/src/GitHub.App/SampleData/GitServiceDesigner.cs b/src/GitHub.App/SampleData/GitServiceDesigner.cs index cabbf30a4c..522bde697b 100644 --- a/src/GitHub.App/SampleData/GitServiceDesigner.cs +++ b/src/GitHub.App/SampleData/GitServiceDesigner.cs @@ -15,5 +15,8 @@ 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; + 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 0216d187bf..ee1bd9ea47 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; @@ -44,12 +43,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 +63,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 +79,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 +105,7 @@ public Task Fetch(IRepository repo, UriString cloneUrl, params string[] refspecs } } - return Task.Factory.StartNew(() => + return Task.Run(() => { try { @@ -114,17 +115,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 +148,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,117 +169,32 @@ public Task Checkout(IRepository repository, string branchName) Guard.ArgumentNotNull(repository, nameof(repository)); Guard.ArgumentNotEmptyString(branchName, nameof(branchName)); - return Task.Factory.StartNew(() => - { -#pragma warning disable 0618 // TODO: Replace `IRepository.Checkout` with `Commands.Checkout`. - repository.Checkout(branchName); -#pragma warning restore 0618 - }); - } - - public async Task CommitExists(IRepository repository, string sha) - { - return await Task.Run(() => repository.Lookup(sha) != null).ConfigureAwait(false); - } - - public Task CreateBranch(IRepository repository, string branchName) - { - Guard.ArgumentNotNull(repository, nameof(repository)); - Guard.ArgumentNotEmptyString(branchName, nameof(branchName)); - - return Task.Factory.StartNew(() => - { - repository.CreateBranch(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.Factory.StartNew(() => + return Task.Run(() => { - var options = new CompareOptions + if (repository is Repository repo) { - Similarity = detectRenames ? SimilarityOptions.Renames : SimilarityOptions.None - }; - - var commit1 = repository.Lookup(sha1); - var commit2 = repository.Lookup(sha2); - - if (commit1 != null && commit2 != null) - { - return repository.Diff.Compare(commit1.Tree, commit2.Tree, options); + LibGit2Sharp.Commands.Checkout(repo, branchName); } else { - return null; + log.Error("Couldn't checkout because {Variable} isn't an instance of {Type}", nameof(repository), typeof(Repository)); } }); } - public Task Compare( - IRepository repository, - string sha1, - string sha2, - string path) + public async Task CommitExists(IRepository repository, string sha) { - Guard.ArgumentNotNull(repository, nameof(repository)); - Guard.ArgumentNotEmptyString(sha1, nameof(sha1)); - Guard.ArgumentNotEmptyString(sha2, nameof(sha2)); - Guard.ArgumentNotEmptyString(path, nameof(path)); - - return Task.Factory.StartNew(() => - { - 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 }); - } - else - { - return null; - } - }); + return await Task.Run(() => repository.Lookup(sha) != null).ConfigureAwait(false); } - public Task CompareWith(IRepository repository, string sha1, string sha2, string path, byte[] contents) + public Task CreateBranch(IRepository repository, string branchName) { Guard.ArgumentNotNull(repository, nameof(repository)); - Guard.ArgumentNotEmptyString(sha1, nameof(sha1)); - Guard.ArgumentNotEmptyString(sha2, nameof(sha1)); - Guard.ArgumentNotEmptyString(path, nameof(path)); + Guard.ArgumentNotEmptyString(branchName, nameof(branchName)); - return Task.Factory.StartNew(() => + return Task.Run(() => { - var commit1 = repository.Lookup(sha1); - var commit2 = repository.Lookup(sha2); - - var treeChanges = repository.Diff.Compare(commit1.Tree, commit2.Tree); - 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); - } - - return null; + repository.CreateBranch(branchName); }); } @@ -292,7 +203,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); @@ -305,7 +216,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); }); @@ -316,7 +227,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 + "/*"); @@ -329,7 +240,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]; @@ -347,7 +258,7 @@ public Task UnsetConfig(IRepository repository, string key) { Guard.ArgumentNotEmptyString(key, nameof(key)); - return Task.Factory.StartNew(() => + return Task.Run(() => { repository.Config.Unset(key); }); @@ -358,7 +269,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"; @@ -373,9 +284,9 @@ 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.Factory.StartNew(() => + return Task.Run(() => { var commit = repository.Lookup(commitSha); if (commit == null) @@ -392,9 +303,9 @@ 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.Factory.StartNew(() => + return Task.Run(() => { var commit = repository.Lookup(commitSha); if (commit == null) @@ -421,9 +332,9 @@ 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.Factory.StartNew(() => + return Task.Run(() => { if (repository.RetrieveStatus(path) == FileStatus.Unaltered) { @@ -491,7 +402,7 @@ public Task IsHeadPushed(IRepository repo) { Guard.ArgumentNotNull(repo, nameof(repo)); - return Task.Factory.StartNew(() => + return Task.Run(() => { return repo.Head.TrackingDetails.AheadBy == 0; }); @@ -503,7 +414,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); diff --git a/src/GitHub.App/Services/GitHubContextService.cs b/src/GitHub.App/Services/GitHubContextService.cs index 4cbae0167b..df14cdbfa2 100644 --- a/src/GitHub.App/Services/GitHubContextService.cs +++ b/src/GitHub.App/Services/GitHubContextService.cs @@ -406,6 +406,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); diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs index 90ac1d1b77..2e2eaa0239 100644 --- a/src/GitHub.App/Services/PullRequestService.cs +++ b/src/GitHub.App/Services/PullRequestService.cs @@ -63,7 +63,7 @@ public class PullRequestService : IssueishService, IPullRequestService, IStaticR readonly IUsageTracker usageTracker; readonly IDictionary tempFileMappings; - + [ImportingConstructor] public PullRequestService( IGitClient gitClient, @@ -656,7 +656,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); } }); @@ -770,20 +770,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; @@ -913,22 +913,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)); diff --git a/src/GitHub.Exports.Reactive/Services/IGitClient.cs b/src/GitHub.Exports.Reactive/Services/IGitClient.cs index 8579ab683a..fccb36f260 100644 --- a/src/GitHub.Exports.Reactive/Services/IGitClient.cs +++ b/src/GitHub.Exports.Reactive/Services/IGitClient.cs @@ -80,44 +80,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); - - /// - /// 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/GitHub.Exports.csproj b/src/GitHub.Exports/GitHub.Exports.csproj index 68b625c8ca..b6fa2fc7f5 100644 --- a/src/GitHub.Exports/GitHub.Exports.csproj +++ b/src/GitHub.Exports/GitHub.Exports.csproj @@ -23,8 +23,7 @@ - - + diff --git a/src/GitHub.Exports/Services/GitService.cs b/src/GitHub.Exports/Services/GitService.cs index f0813370d2..e2e886acb7 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 { IndentHeuristic = true }; } /// @@ -228,5 +229,96 @@ 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.ArgumentIsGitPath(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.ArgumentIsGitPath(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 change = treeChanges.FirstOrDefault(x => x.Path == path); + var oldPath = change?.OldPath; + + 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()); + var blob2 = repository.ObjectDatabase.CreateBlob(contentStream, path); + return repository.Diff.Compare(blob1, blob2, defaultCompareOptions); + } + + 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 4ff850c21b..7ad16ad6de 100644 --- a/src/GitHub.Exports/Services/IGitService.cs +++ b/src/GitHub.Exports/Services/IGitService.cs @@ -71,5 +71,44 @@ 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 (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); + + /// + /// 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/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/src/GitHub.InlineReviews/GitHub.InlineReviews.csproj b/src/GitHub.InlineReviews/GitHub.InlineReviews.csproj index 7cee7dd38e..314d26ae3d 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.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/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj b/src/GitHub.TeamFoundation.14/GitHub.TeamFoundation.14.csproj index 7fd0b34a92..743854e5f3 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..5de1b808a3 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 69f409eaae..7557eddc83 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 14ec7f1acb..bea7fcdbc0 100644 --- a/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj +++ b/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj @@ -304,6 +304,7 @@ 14.1.24720 + true diff --git a/test/GitHub.App.UnitTests/Services/GitClientTests.cs b/test/GitHub.App.UnitTests/Services/GitClientTests.cs index 1efe891a1a..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, null).ReturnsForAnyArgs(changes); + repo.Diff.Compare(null as Blob, null as Blob).ReturnsForAnyArgs(changes); var gitClient = CreateGitClient(); var modified = await gitClient.IsModified(repo, path, null); @@ -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.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/GitServiceIntegrationTests.cs b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs index 6ad96ea7ec..30b6a49381 100644 --- a/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs +++ b/test/GitHub.Exports.UnitTests/GitServiceIntegrationTests.cs @@ -1,5 +1,7 @@ using System; using System.IO; +using System.Linq; +using System.Text; using System.Threading.Tasks; using GitHub.Services; using LibGit2Sharp; @@ -7,6 +9,149 @@ 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)); + } + } + + [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)); + } + } + + + [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 + { + [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)); + } + } + + [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 Can_Handle_Renames(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)); + } + } + + [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 { [Test] @@ -296,19 +441,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 +456,30 @@ 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; + DeleteFilesNotInGit(dir); + var file = Path.Combine(dir, path); + Directory.CreateDirectory(Path.GetDirectoryName(file)); + File.WriteAllText(file, content); + 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() 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(); 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/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 112c1bc83c..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); } @@ -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; } @@ -66,21 +64,21 @@ 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()); } - 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(); 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()); } - 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)); } @@ -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; diff --git a/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs b/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs index ddf11c119d..aea7f6090a 100644 --- a/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs +++ b/test/GitHub.TeamFoundation.UnitTests/VSGitExtTests.cs @@ -319,5 +319,8 @@ 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(); + public Task Compare(IRepository repository, string sha1, string sha2, bool detectRenames = false) => throw new NotImplementedException(); } }