From 051a8d8e4413f155f619f4dcea467d7a79954e26 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 5 Jul 2017 11:19:22 +0100 Subject: [PATCH 1/2] Match diff lines using their old line number Added alternative diff line matching strategy based on the original line number and content. --- src/GitHub.Exports/Models/DiffUtilities.cs | 50 +++++++++++++++++-- .../Models/DiffUtilitiesTests.cs | 38 +++++++++++--- .../Services/DiffServiceTests.cs | 4 +- 3 files changed, 81 insertions(+), 11 deletions(-) diff --git a/src/GitHub.Exports/Models/DiffUtilities.cs b/src/GitHub.Exports/Models/DiffUtilities.cs index d1e123bcff..4b3cbd8a6b 100644 --- a/src/GitHub.Exports/Models/DiffUtilities.cs +++ b/src/GitHub.Exports/Models/DiffUtilities.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Text.RegularExpressions; using System.Diagnostics.CodeAnalysis; +using System.Linq; namespace GitHub.Models { @@ -50,8 +51,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, }); @@ -83,13 +86,29 @@ public static IEnumerable ParseFragment(string diff) public static DiffLine Match(IEnumerable diff, IList target) { - int j = 0; + var diffLine = MatchLine(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) { for (var i = source.Lines.Count - 1; i >= 0; --i) @@ -108,6 +127,31 @@ 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; + } + [SuppressMessage("Microsoft.Globalization", "CA1305:SpecifyIFormatProvider", MessageId = "System.String.Format(System.String,System.Object)")] static DiffChangeType GetLineChange(char c) { diff --git a/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs b/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs index 78cd796831..deeb92c456 100644 --- a/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs @@ -86,8 +86,8 @@ public void FirstChunk_CheckLineNumbers(int oldLineNumber, int newLineNumber) [Theory] [InlineData(1, 2, " 1", 1, 2)] - [InlineData(1, 2, "+1", -1, 2)] - [InlineData(1, 2, "-1", 1, -1)] + [InlineData(1, 2, "+1", 1, 2)] + [InlineData(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}"; @@ -169,7 +169,33 @@ public void InvalidDiffLineChangeChar(string line, string expectMessage) } } - public class TheMatchMethod + public class TheMatchLineMethod + { + [Theory] + [InlineData("@@ -1 +1 @@. 1", "@@ -2 +1 @@. 1", -1)] + [InlineData("@@ -1 +1 @@. 1", "@@ -1 +1 @@. 1", 0)] + [InlineData("@@ -1 +1 @@. 1.+2", "@@ -1 +1 @@. 1", 0)] + [InlineData("@@ -1 +1 @@.+2. 1", "@@ -1 +1 @@. 1", 1)] + + [InlineData("@@ -1 +1 @@.+1", "@@ -1 +1 @@.+1", 0)] + [InlineData("@@ -1 +1 @@.-1", "@@ -1 +1 @@.-1", 0)] + + [InlineData("@@ -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; + + var matchLine = DiffUtilities.MatchLine(chunks1, targetLines); + Assert.Equal(expectLine, matchLine); + } + } + + public class TheMatchChunkMethod { [Theory] [InlineData(" 1", " 1", 0)] @@ -186,7 +212,7 @@ public class TheMatchMethod var expectLine = (skip != -1) ? chunks1.First().Lines.Skip(skip).First() : null; var targetLines = chunks2.First().Lines; - var line = DiffUtilities.Match(chunks1, targetLines); + var line = DiffUtilities.MatchChunk(chunks1, targetLines); Assert.Equal(expectLine, line); } @@ -201,7 +227,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.Equal(expectLine, line); } @@ -212,7 +238,7 @@ public void NoLineMatchesFromNoLines() var chunks = new DiffChunk[0]; var lines = new DiffLine[0]; - var line = DiffUtilities.Match(chunks, lines); + var line = DiffUtilities.MatchChunk(chunks, lines); Assert.Equal(null, line); } diff --git a/test/GitHub.InlineReviews.UnitTests/Services/DiffServiceTests.cs b/test/GitHub.InlineReviews.UnitTests/Services/DiffServiceTests.cs index 09c25f11c0..5266c10a6e 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.Equal(17, result[0].Lines[7].OldLineNumber); - Assert.Equal(-1, result[0].Lines[7].NewLineNumber); + Assert.Equal(18, result[0].Lines[7].NewLineNumber); Assert.Equal(12, result[0].Lines[7].DiffLineNumber); // + public sealed class UsageTracker : IUsageTracker, IDisposable - Assert.Equal(-1, result[0].Lines[8].OldLineNumber); + Assert.Equal(18, result[0].Lines[8].OldLineNumber); Assert.Equal(18, result[0].Lines[8].NewLineNumber); Assert.Equal(13, result[0].Lines[8].DiffLineNumber); From 12ab471e746b8877fb333169cb68e4fdf1c28637 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Wed, 5 Jul 2017 16:16:35 +0100 Subject: [PATCH 2/2] Add diff line matching that ignores "//" comments This lets the user add whitespace/comments to line without losing inline-comments. --- src/GitHub.Exports/Models/DiffUtilities.cs | 55 +++++++++++++++++++ .../Models/DiffUtilitiesTests.cs | 29 +++++++++- 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/src/GitHub.Exports/Models/DiffUtilities.cs b/src/GitHub.Exports/Models/DiffUtilities.cs index 4b3cbd8a6b..3972810a0b 100644 --- a/src/GitHub.Exports/Models/DiffUtilities.cs +++ b/src/GitHub.Exports/Models/DiffUtilities.cs @@ -92,6 +92,12 @@ public static DiffLine Match(IEnumerable diff, IList target return diffLine; } + diffLine = MatchLineIgnoreComments(diff, target); + if (diffLine != null) + { + return diffLine; + } + diffLine = MatchChunk(diff, target); if (diffLine != null) { @@ -152,6 +158,55 @@ public static DiffLine MatchLine(IEnumerable chunks1, IList 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); + } + [SuppressMessage("Microsoft.Globalization", "CA1305:SpecifyIFormatProvider", MessageId = "System.String.Format(System.String,System.Object)")] static DiffChangeType GetLineChange(char c) { diff --git a/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs b/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs index deeb92c456..6566f458e5 100644 --- a/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs @@ -172,14 +172,20 @@ public void InvalidDiffLineChangeChar(string line, string expectMessage) public class TheMatchLineMethod { [Theory] + // context line moves [InlineData("@@ -1 +1 @@. 1", "@@ -2 +1 @@. 1", -1)] [InlineData("@@ -1 +1 @@. 1", "@@ -1 +1 @@. 1", 0)] [InlineData("@@ -1 +1 @@. 1.+2", "@@ -1 +1 @@. 1", 0)] [InlineData("@@ -1 +1 @@.+2. 1", "@@ -1 +1 @@. 1", 1)] + // lines added or deleted [InlineData("@@ -1 +1 @@.+1", "@@ -1 +1 @@.+1", 0)] [InlineData("@@ -1 +1 @@.-1", "@@ -1 +1 @@.-1", 0)] + // context line changed + [InlineData("@@ -1 +1 @@.-x.+y", "@@ -1 +1 @@. x", -1)] + + // no target lines [InlineData("@@ -1 +1 @@.+1", "@@ -1 +1 @@", -1)] public void MatchLine(string lines1, string lines2, int skip /* -1 for no match */) { @@ -188,13 +194,34 @@ public class TheMatchLineMethod 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; + var targetLines = chunks2.First().Lines.Reverse().ToList(); var matchLine = DiffUtilities.MatchLine(chunks1, targetLines); Assert.Equal(expectLine, matchLine); } } + public class TheMatchLineIgnoreCommentsMethod + { + [Theory] + [InlineData("@@ -1 +1 @@. x", "@@ -1 +1 @@-x.+//x", 0)] // Ignore extra '/' char. + [InlineData("@@ -1 +1 @@. x", "@@ -1 +1 @@-x.+ x", 0)] // Ignore extra ' ' char. + [InlineData("@@ -1 +1 @@. x", "@@ -1 +1 @@-x.+\tx", 0)] // Ignore extra '\t' char. + [InlineData("@@ -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.Equal(expectLine, matchLine); + } + } + public class TheMatchChunkMethod { [Theory]