diff --git a/src/GitHub.Exports/Models/DiffUtilities.cs b/src/GitHub.Exports/Models/DiffUtilities.cs index 6d781075a1..3edf903b0d 100644 --- a/src/GitHub.Exports/Models/DiffUtilities.cs +++ b/src/GitHub.Exports/Models/DiffUtilities.cs @@ -5,6 +5,7 @@ using System.Text.RegularExpressions; using System.Diagnostics.CodeAnalysis; using GitHub.Extensions; +using System.Linq; #pragma warning disable CA1034 // Nested types should not be visible @@ -57,8 +58,10 @@ public static IEnumerable ParseFragment(string diff) chunk.Lines.Add(new DiffLine { Type = type, - OldLineNumber = type != DiffChangeType.Add ? oldLine : -1, - NewLineNumber = type != DiffChangeType.Delete ? newLine : -1, + //OldLineNumber = type != DiffChangeType.Add ? oldLine : -1, + //NewLineNumber = type != DiffChangeType.Delete ? newLine : -1, + OldLineNumber = oldLine, + NewLineNumber = newLine, DiffLineNumber = diffLine, Content = line, }); @@ -92,12 +95,36 @@ public static IEnumerable ParseFragment(string diff) } public static DiffLine Match(IEnumerable diff, IList target) + { + var diffLine = MatchLine(diff, target); + if (diffLine != null) + { + return diffLine; + } + + diffLine = MatchLineIgnoreComments(diff, target); + if (diffLine != null) + { + return diffLine; + } + + diffLine = MatchChunk(diff, target); + if (diffLine != null) + { + return diffLine; + } + + return null; + } + + public static DiffLine MatchChunk(IEnumerable diff, IList target) { if (target.Count == 0) { return null; // no lines to match } + int j = 0; foreach (var source in diff) { var matches = 0; @@ -122,6 +149,80 @@ public static DiffLine Match(IEnumerable diff, IList target return null; } + public static DiffLine MatchLine(IEnumerable chunks1, IList targetLines) + { + var targetLine = targetLines.FirstOrDefault(); + if (targetLine == null) + { + return null; + } + + foreach (var chunk in chunks1) + { + foreach (var line in chunk.Lines) + { + if (targetLine.OldLineNumber == line.OldLineNumber) + { + if(targetLine.Content == line.Content) + { + return line; + } + } + } + } + + return null; + } + + public static DiffLine MatchLineIgnoreComments(IEnumerable chunks1, IList targetLines) + { + var ignoreChars = new[] { ' ', '\t', '/' }; + + var targetLine = targetLines.FirstOrDefault(); + if (targetLine == null) + { + return null; + } + + foreach (var chunk in chunks1) + { + bool loose = false; + foreach (var line in chunk.Lines) + { + if (targetLine.OldLineNumber == line.OldLineNumber || loose) + { + var targetContent = targetLine.Content; + var lineContent = line.Content; + if (targetContent == lineContent) + { + return line; + } + + targetContent = GetSignificantContent(targetContent, ignoreChars); + lineContent = GetSignificantContent(lineContent, ignoreChars); + if (targetContent == lineContent) + { + if (line.Type == DiffChangeType.Delete) + { + loose = true; + } + else + { + return line; + } + } + } + } + } + + return null; + } + + static string GetSignificantContent(string content, char[] ignoreChars) + { + return content.Substring(1).TrimStart(ignoreChars); + } + /// Here are some alternative implementations we tried: /// https://gist.github.com/shana/200e4719d4f571caab9dbf5921fa5276 /// Scanning with `text.IndexOf('\n', index)` appears to the the best compromise for average .diff files. diff --git a/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs b/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs index cee0012780..84eec12491 100644 --- a/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs @@ -15,28 +15,28 @@ public void EmptyDiff_NoDiffChunks() { var chunks = DiffUtilities.ParseFragment(""); - Assert.That(chunks, Is.Empty); + Assert.That(chunks, Is.Empty); } [TestCase("@@ -1 +1 @@")] - [TestCase("@@ -1 +1,0 @@")] - [TestCase("@@ -1,0 +1 @@")] - [TestCase("@@ -1,0 +1,0 @@")] - [TestCase("@@ -1,0 +1,0 @@ THIS IS A COMMENT THAT WILL BE IGNORED")] - public void HeaderOnly_OneChunkNoLines(string header) + [TestCase("@@ -1 +1,0 @@")] + [TestCase("@@ -1,0 +1 @@")] + [TestCase("@@ -1,0 +1,0 @@")] + [TestCase("@@ -1,0 +1,0 @@ THIS IS A COMMENT THAT WILL BE IGNORED")] + public void HeaderOnly_OneChunkNoLines(string header) { var chunks = DiffUtilities.ParseFragment(header); Assert.That(chunks, Has.One.Items); - var chunk = chunks.First(); - Assert.That(chunk.Lines, Is.Empty); + var chunk = chunks.First(); + Assert.That(chunk.Lines, Is.Empty); } - [TestCase("@@ -1 +2 @@", 1, 2)] - [TestCase("@@ -1 +2,0 @@", 1, 2)] - [TestCase("@@ -1,0 +2 @@", 1, 2)] - [TestCase("@@ -1,0 +2,0 @@", 1, 2)] - [TestCase("@@ -1,0 +2,0 @@ THIS IS A COMMENT THAT WILL BE IGNORED", 1, 2)] + [TestCase("@@ -1 +2 @@", 1, 2)] + [TestCase("@@ -1 +2,0 @@", 1, 2)] + [TestCase("@@ -1,0 +2 @@", 1, 2)] + [TestCase("@@ -1,0 +2,0 @@", 1, 2)] + [TestCase("@@ -1,0 +2,0 @@ THIS IS A COMMENT THAT WILL BE IGNORED", 1, 2)] [TestCase( @"diff --git a/src/Foo.cs b/src/Foo.cs index b02decb..f7dadae 100644 @@ -44,7 +44,7 @@ index b02decb..f7dadae 100644 +++ b/src/Foo.cs @@ -1 +2 @@", 1, 2)] - public void HeaderOnly_OldAndNewLineNumbers(string header, int expectOldLineNumber, int expectNewLineNumber) + public void HeaderOnly_OldAndNewLineNumbers(string header, int expectOldLineNumber, int expectNewLineNumber) { var chunks = DiffUtilities.ParseFragment(header); var chunk = chunks.First(); @@ -63,7 +63,7 @@ public void HeaderOnlyNoNewLineAtEnd_NoLines() var chunks = DiffUtilities.ParseFragment(header); var chunk = chunks.First(); - Assert.That(chunk.Lines, Is.Empty); + Assert.That(chunk.Lines, Is.Empty); } [Test] @@ -95,11 +95,11 @@ public void NoNewLineNotAtEndOfChunk_CheckDiffLineNumber() Assert.That(3, Is.EqualTo(line.DiffLineNumber)); } - [TestCase("+foo\n+bar\n", "+foo", "+bar")] - [TestCase("+fo\ro\n+bar\n", "+fo\ro", "+bar")] - [TestCase("+foo\r\r\n+bar\n", "+foo\r", "+bar")] - [TestCase("+\\r\n+\r\n", "+\\r", "+")] - public void FirstChunk_CheckLineContent(string diffLines, string contentLine0, string contentLine1) + [TestCase("+foo\n+bar\n", "+foo", "+bar")] + [TestCase("+fo\ro\n+bar\n", "+fo\ro", "+bar")] + [TestCase("+foo\r\r\n+bar\n", "+foo\r", "+bar")] + [TestCase("+\\r\n+\r\n", "+\\r", "+")] + public void FirstChunk_CheckLineContent(string diffLines, string contentLine0, string contentLine1) { var header = "@@ -1 +1 @@"; var diff = header + "\n" + diffLines; @@ -110,10 +110,10 @@ public void FirstChunk_CheckLineContent(string diffLines, string contentLine0, s Assert.That(contentLine1, Is.EqualTo(chunk.Lines[1].Content)); } - [TestCase("+foo\n+bar\n", 1, 2)] - [TestCase("+fo\ro\n+bar\n", 1, 3)] - [TestCase("+foo\r\r\n+bar\n", 1, 3)] - public void FirstChunk_CheckNewLineNumber(string diffLines, int lineNumber0, int lineNumber1) + [TestCase("+foo\n+bar\n", 1, 2)] + [TestCase("+fo\ro\n+bar\n", 1, 3)] + [TestCase("+foo\r\r\n+bar\n", 1, 3)] + public void FirstChunk_CheckNewLineNumber(string diffLines, int lineNumber0, int lineNumber1) { var header = "@@ -1 +1 @@"; var diff = header + "\n" + diffLines; @@ -124,10 +124,10 @@ public void FirstChunk_CheckNewLineNumber(string diffLines, int lineNumber0, int Assert.That(lineNumber1, Is.EqualTo(chunk.Lines[1].NewLineNumber)); } - [TestCase("-foo\n-bar\n", 1, 2)] - [TestCase("-fo\ro\n-bar\n", 1, 3)] - [TestCase("-foo\r\r\n-bar\n", 1, 3)] - public void FirstChunk_CheckOldLineNumber(string diffLines, int lineNumber0, int lineNumber1) + [TestCase("-foo\n-bar\n", 1, 2)] + [TestCase("-fo\ro\n-bar\n", 1, 3)] + [TestCase("-foo\r\r\n-bar\n", 1, 3)] + public void FirstChunk_CheckOldLineNumber(string diffLines, int lineNumber0, int lineNumber1) { var header = "@@ -1 +1 @@"; var diff = header + "\n" + diffLines; @@ -149,8 +149,8 @@ public void FirstChunk_CheckDiffLineZeroBased() Assert.That(expectDiffLine, Is.EqualTo(chunk.DiffLine)); } - [TestCase(1, 2)] - public void FirstChunk_CheckLineNumbers(int oldLineNumber, int newLineNumber) + [TestCase(1, 2)] + public void FirstChunk_CheckLineNumbers(int oldLineNumber, int newLineNumber) { var header = $"@@ -{oldLineNumber} +{newLineNumber} @@"; @@ -160,10 +160,10 @@ public void FirstChunk_CheckLineNumbers(int oldLineNumber, int newLineNumber) Assert.That(newLineNumber, Is.EqualTo(chunk.NewLineNumber)); } - [TestCase(1, 2, " 1", 1, 2)] - [TestCase(1, 2, "+1", -1, 2)] - [TestCase(1, 2, "-1", 1, -1)] - public void FirstLine_CheckLineNumbers(int oldLineNumber, int newLineNumber, string line, int expectOldLineNumber, int expectNewLineNumber) + [TestCase(1, 2, " 1", 1, 2)] + [TestCase(1, 2, "+1", 1, 2)] + [TestCase(1, 2, "-1", 1, 2)] + public void FirstLine_CheckLineNumbers(int oldLineNumber, int newLineNumber, string line, int expectOldLineNumber, int expectNewLineNumber) { var header = $"@@ -{oldLineNumber} +{newLineNumber} @@\n{line}"; @@ -174,10 +174,10 @@ public void FirstLine_CheckLineNumbers(int oldLineNumber, int newLineNumber, str Assert.That(expectNewLineNumber, Is.EqualTo(diffLine.NewLineNumber)); } - [TestCase(" 1", 0, 1)] - [TestCase(" 1\n 2", 1, 2)] - [TestCase(" 1\n 2\n 3", 2, 3)] - public void SkipNLines_CheckDiffLineNumber(string lines, int skip, int expectDiffLineNumber) + [TestCase(" 1", 0, 1)] + [TestCase(" 1\n 2", 1, 2)] + [TestCase(" 1\n 2\n 3", 2, 3)] + public void SkipNLines_CheckDiffLineNumber(string lines, int skip, int expectDiffLineNumber) { var fragment = $"@@ -1 +1 @@\n{lines}"; @@ -187,10 +187,10 @@ public void SkipNLines_CheckDiffLineNumber(string lines, int skip, int expectDif Assert.That(expectDiffLineNumber, Is.EqualTo(firstLine.DiffLineNumber)); } - [TestCase(" FIRST")] - [TestCase("+FIRST")] - [TestCase("-FIRST")] - public void FirstLine_CheckToString(string line) + [TestCase(" FIRST")] + [TestCase("+FIRST")] + [TestCase("-FIRST")] + public void FirstLine_CheckToString(string line) { var fragment = $"@@ -1 +1 @@\n{line}"; var result = DiffUtilities.ParseFragment(fragment); @@ -201,10 +201,10 @@ public void FirstLine_CheckToString(string line) Assert.That(line, Is.EqualTo(str)); } - [TestCase(" FIRST")] - [TestCase("+FIRST")] - [TestCase("-FIRST")] - public void FirstLine_CheckContent(string line) + [TestCase(" FIRST")] + [TestCase("+FIRST")] + [TestCase("-FIRST")] + public void FirstLine_CheckContent(string line) { var fragment = $"@@ -1,4 +1,4 @@\n{line}"; @@ -214,10 +214,10 @@ public void FirstLine_CheckContent(string line) Assert.That(line, Is.EqualTo(firstLine.Content)); } - [TestCase(" FIRST", DiffChangeType.None)] - [TestCase("+FIRST", DiffChangeType.Add)] - [TestCase("-FIRST", DiffChangeType.Delete)] - public void FirstLine_CheckDiffChangeTypes(string line, DiffChangeType expectType) + [TestCase(" FIRST", DiffChangeType.None)] + [TestCase("+FIRST", DiffChangeType.Add)] + [TestCase("-FIRST", DiffChangeType.Delete)] + public void FirstLine_CheckDiffChangeTypes(string line, DiffChangeType expectType) { var fragment = $"@@ -1 +1 @@\n{line}"; @@ -227,8 +227,8 @@ public void FirstLine_CheckDiffChangeTypes(string line, DiffChangeType expectTyp Assert.That(expectType, Is.EqualTo(firstLine.Type)); } - [TestCase("?FIRST", "Invalid diff line change char: '?'.")] - public void InvalidDiffLineChangeChar(string line, string expectMessage) + [TestCase("?FIRST", "Invalid diff line change char: '?'.")] + public void InvalidDiffLineChangeChar(string line, string expectMessage) { var fragment = $"@@ -1,4 +1,4 @@\n{line}"; @@ -239,7 +239,58 @@ public void InvalidDiffLineChangeChar(string line, string expectMessage) } } - public class TheMatchMethod + public class TheMatchLineMethod + { + // context line moves + [TestCase("@@ -1 +1 @@. 1", "@@ -2 +1 @@. 1", -1)] + [TestCase("@@ -1 +1 @@. 1", "@@ -1 +1 @@. 1", 0)] + [TestCase("@@ -1 +1 @@. 1.+2", "@@ -1 +1 @@. 1", 0)] + [TestCase("@@ -1 +1 @@.+2. 1", "@@ -1 +1 @@. 1", 1)] + + // lines added or deleted + [TestCase("@@ -1 +1 @@.+1", "@@ -1 +1 @@.+1", 0)] + [TestCase("@@ -1 +1 @@.-1", "@@ -1 +1 @@.-1", 0)] + + // context line changed + [TestCase("@@ -1 +1 @@.-x.+y", "@@ -1 +1 @@. x", -1)] + + // no target lines + [TestCase("@@ -1 +1 @@.+1", "@@ -1 +1 @@", -1)] + public void MatchLine(string lines1, string lines2, int skip /* -1 for no match */) + { + lines1 = lines1.Replace(".", "\r\n"); + lines2 = lines2.Replace(".", "\r\n"); + var chunks1 = DiffUtilities.ParseFragment(lines1).ToList(); + var chunks2 = DiffUtilities.ParseFragment(lines2).ToList(); + var expectLine = (skip != -1) ? chunks1.First().Lines.Skip(skip).First() : null; + var targetLines = chunks2.First().Lines.Reverse().ToList(); + + var matchLine = DiffUtilities.MatchLine(chunks1, targetLines); + Assert.That(matchLine, Is.EqualTo(expectLine)); + } + } + + public class TheMatchLineIgnoreCommentsMethod + { + [TestCase("@@ -1 +1 @@. x", "@@ -1 +1 @@-x.+//x", 0)] // Ignore extra '/' char. + [TestCase("@@ -1 +1 @@. x", "@@ -1 +1 @@-x.+ x", 0)] // Ignore extra ' ' char. + [TestCase("@@ -1 +1 @@. x", "@@ -1 +1 @@-x.+\tx", 0)] // Ignore extra '\t' char. + [TestCase("@@ -1 +1 @@. x", "@@ -1 +1 @@-x.+!x", -1)] // Don't ignore extra '!' char. + public void MatchLineIgnoreComments(string lines1, string lines2, int skip /* -1 for no match */) + { + lines1 = lines1.Replace(".", Environment.NewLine); + lines2 = lines2.Replace(".", Environment.NewLine); + var chunks1 = DiffUtilities.ParseFragment(lines1).ToList(); + var chunks2 = DiffUtilities.ParseFragment(lines2).ToList(); + var expectLine = (skip != -1) ? chunks1.First().Lines.Skip(skip).First() : null; + var targetLines = chunks2.First().Lines.Reverse().ToList(); + + var matchLine = DiffUtilities.MatchLineIgnoreComments(chunks1, targetLines); + Assert.That(matchLine, Is.EqualTo(expectLine)); + } + } + + public class TheMatchChunkMethod { /// Target diff chunk with header (with '.' as line separator) /// Diff lines to match (with '.' as line separator) @@ -271,7 +322,7 @@ public class TheMatchMethod var chunks2 = DiffUtilities.ParseFragment(header + "\n" + matchLines).ToList(); var targetLines = chunks2.First().Lines.Reverse().ToList(); - var line = DiffUtilities.Match(chunks1, targetLines); + var line = DiffUtilities.MatchChunk(chunks1, targetLines); var diffLineNumber = (line != null) ? line.DiffLineNumber : -1; Assert.That(expectedDiffLineNumber, Is.EqualTo(diffLineNumber)); @@ -287,7 +338,7 @@ public void MatchSameLine() var targetLine = chunks2.First().Lines.First(); var targetLines = new[] { targetLine }; - var line = DiffUtilities.Match(chunks1, targetLines); + var line = DiffUtilities.MatchChunk(chunks1, targetLines); Assert.That(expectLine, Is.EqualTo(line)); } @@ -298,7 +349,7 @@ public void NoLineMatchesFromNoLines() var chunks = Array.Empty(); var lines = Array.Empty(); - var line = DiffUtilities.Match(chunks, lines); + var line = DiffUtilities.MatchChunk(chunks, lines); Assert.That(line, Is.Null); } @@ -321,7 +372,7 @@ public class TheLineReaderClass public void ReadLines(string text, string[] expectLines) { var lineReader = new DiffUtilities.LineReader(text); - + foreach (var expectLine in expectLines) { var line = lineReader.ReadLine(); diff --git a/test/GitHub.InlineReviews.UnitTests/Services/DiffServiceTests.cs b/test/GitHub.InlineReviews.UnitTests/Services/DiffServiceTests.cs index 16586848d2..d20ea6e7da 100644 --- a/test/GitHub.InlineReviews.UnitTests/Services/DiffServiceTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Services/DiffServiceTests.cs @@ -40,11 +40,11 @@ public void ShouldParsePr960() // - public class UsageTracker : IUsageTracker Assert.That(17, Is.EqualTo(result[0].Lines[7].OldLineNumber)); - Assert.That(-1, Is.EqualTo(result[0].Lines[7].NewLineNumber)); + Assert.That(18, Is.EqualTo(result[0].Lines[7].NewLineNumber)); Assert.That(8, Is.EqualTo(result[0].Lines[7].DiffLineNumber)); // + public sealed class UsageTracker : IUsageTracker, IDisposable - Assert.That(-1, Is.EqualTo(result[0].Lines[8].OldLineNumber)); + Assert.That(18, Is.EqualTo(result[0].Lines[8].OldLineNumber)); Assert.That(18, Is.EqualTo(result[0].Lines[8].NewLineNumber)); Assert.That(9, Is.EqualTo(result[0].Lines[8].DiffLineNumber)); diff --git a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionServiceTests.cs b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionServiceTests.cs index 514b19927e..1bca336756 100644 --- a/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionServiceTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionServiceTests.cs @@ -152,7 +152,7 @@ Line 2 "HEAD_SHA"); Assert.That(result.Count, Is.EqualTo(2)); - Assert.That(result[1].LineNumber, Is.EqualTo(-1)); + Assert.That(result[1].LineNumber, Is.EqualTo(2)); } } @@ -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" },