Merge pull request #1178 from github/fixes/1149-comments-not-displaying

Fix diff line matching when there's a partial match

Author: grokys

src/GitHub.Exports/Models/DiffUtilities.cs

@@ -82,27 +82,28 @@ public static IEnumerable<DiffChunk> ParseFragment(string diff)
 
         public static DiffLine Match(IEnumerable<DiffChunk> diff, IList<DiffLine> target)
         {
-            int j = 0;
-
             if (target.Count == 0)
             {
                 return null; // no lines to match
             }
 
             foreach (var source in diff)
             {
+                var matches = 0;
                 for (var i = source.Lines.Count - 1; i >= 0; --i)
                 {
-                    if (source.Lines[i].Content == target[j].Content)
+                    if (source.Lines[i].Content == target[matches].Content)
                     {
-                        if (++j == target.Count || i == 0)
+                        matches++;
+                        if (matches == target.Count || i == 0)
                         {
-                            return source.Lines[i + j - 1];
+                            return source.Lines[i + matches - 1];
                         }
                     }
                     else
                     {
-                        j = 0;
+                        i += matches;
+                        matches = 0;
                     }
                 }
             }

test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs

@@ -185,30 +185,41 @@ public void InvalidDiffLineChangeChar(string line, string expectMessage)
 
         public class TheMatchMethod
         {
+            /// <param name="diffLines">Target diff chunk with header (with '.' as line separator)</param>
+            /// <param name="matchLines">Diff lines to match (with '.' as line separator)</param>
+            /// <param name="expectedDiffLineNumber">The DiffLineNumber that the last line of matchLines falls on</param>
             [Theory]
-            [InlineData(" 1", " 1", 0)]
-            [InlineData(" 1. 2", " 2", 1)]
-            [InlineData(" 1. 1", " 1", 1)] // match the later line
+            [InlineData(" 1", " 1", 1)]
+            [InlineData(" 1. 2", " 2", 2)]
+            [InlineData(" 1. 1", " 1", 2)] // match the later line
             [InlineData("+x", "-x", -1)]
             [InlineData("", " x", -1)]
             [InlineData(" x", "", -1)]
 
-            [InlineData(" 1. 2.", " 2. 1.", 1)] // matched full context
-            [InlineData(" 1. 2.", " 2. 3.", -1)] // didn't match full context
-            [InlineData(" 2.", " 2. 1.", 0)] // match if we run out of context lines
-            public void MatchLine(string lines1, string lines2, int skip /* -1 for no match */)
+            [InlineData(" 1. 2.", " 1. 2.", 2)] // matched full context
+            [InlineData(" 1. 2.", " 3. 2.", -1)] // didn't match full context
+            [InlineData(" 2.", " 1. 2.", 1)] // match if we run out of context lines
+
+            // Tests for https://github.com/github/VisualStudio/issues/1149
+            // Matching algorithm got confused when there was a partial match.
+            [InlineData("+a.+x.+x.", "+a.+x.", 2)]
+            [InlineData("+a.+x.+x.", "+a.+x.+x.", 3)]
+            [InlineData("+a.+x.+x.+b.+x.+x.", "+a.+x.", 2)]
+            [InlineData("+a.+x.+x.+b.+x.+x.", "+b.+x.", 5)]
+            [InlineData("+a.+b.+x", "+a.+x.", -1)] // backtrack when there is a failed match
+            public void MatchLine(string diffLines, string matchLines, int expectedDiffLineNumber /* -1 for no match */)
             {
                 var header = "@@ -1 +1 @@";
-                lines1 = lines1.Replace(".", "\r\n");
-                lines2 = lines2.Replace(".", "\r\n");
-                var chunks1 = DiffUtilities.ParseFragment(header + "\n" + lines1).ToList();
-                var chunks2 = DiffUtilities.ParseFragment(header + "\n" + lines2).ToList();
-                var expectLine = (skip != -1) ? chunks1.First().Lines.Skip(skip).First() : null;
-                var targetLines = chunks2.First().Lines;
+                diffLines = diffLines.Replace(".", "\r\n");
+                matchLines = matchLines.Replace(".", "\r\n");
+                var chunks1 = DiffUtilities.ParseFragment(header + "\n" + diffLines).ToList();
+                var chunks2 = DiffUtilities.ParseFragment(header + "\n" + matchLines).ToList();
+                var targetLines = chunks2.First().Lines.Reverse().ToList();
 
                 var line = DiffUtilities.Match(chunks1, targetLines);
 
-                Assert.Equal(expectLine, line);
+                var diffLineNumber = (line != null) ? line.DiffLineNumber : -1;
+                Assert.Equal(expectedDiffLineNumber, diffLineNumber);
             }
 
             [Fact]