From 413c643f679d31155445ca795d0490cf4b7f90eb Mon Sep 17 00:00:00 2001 From: Andreia Gaita Date: Mon, 16 Oct 2017 14:49:56 +0200 Subject: [PATCH] Only scan the diff once when processing carriage returns --- src/GitHub.Exports/Models/DiffUtilities.cs | 73 +++++++++++-------- .../Models/DiffUtilitiesTests.cs | 40 +++++----- 2 files changed, 66 insertions(+), 47 deletions(-) diff --git a/src/GitHub.Exports/Models/DiffUtilities.cs b/src/GitHub.Exports/Models/DiffUtilities.cs index 43af7793a0..dcc7ecefb3 100644 --- a/src/GitHub.Exports/Models/DiffUtilities.cs +++ b/src/GitHub.Exports/Models/DiffUtilities.cs @@ -3,6 +3,8 @@ using System.Collections.Generic; using System.Text.RegularExpressions; using System.Diagnostics.CodeAnalysis; +using System.Text; +using GitHub.Extensions; namespace GitHub.Models { @@ -13,14 +15,15 @@ public static class DiffUtilities public static IEnumerable ParseFragment(string diff) { var reader = new LineReader(diff); - string line; + LineReader.LineInformation lineInfo; DiffChunk chunk = null; int diffLine = -1; int oldLine = -1; int newLine = -1; - while ((line = reader.ReadLine()) != null) + while ((lineInfo = reader.ReadLine()).Line != null) { + var line = lineInfo.Line; var headerMatch = ChunkHeaderRegex.Match(line); if (headerMatch.Success) @@ -55,8 +58,7 @@ public static IEnumerable ParseFragment(string diff) Content = line, }); - var lineCount = 1; - lineCount += CountCarriageReturns(line); + var lineCount = lineInfo.CarriageReturns + 1; switch (type) { @@ -116,61 +118,72 @@ public static DiffLine Match(IEnumerable diff, IList target public class LineReader { + static readonly LineInformation Default = new LineInformation(null, 0); + static readonly LineInformation Empty = new LineInformation(String.Empty, 0); readonly string text; + readonly int length = 0; int index = 0; public LineReader(string text) { + Guard.ArgumentNotNull(text, nameof(text)); this.text = text; + this.length = text.Length; } - public string ReadLine() + public LineInformation ReadLine() { if (EndOfText) { if (StartOfText) { index = -1; - return string.Empty; + return Empty; } - - return null; + return Default; } - var startIndex = index; - index = text.IndexOf('\n', index); - var endIndex = index != -1 ? index : text.Length; - var length = endIndex - startIndex; - - if (index != -1) + var carriageReturns = 0; + StringBuilder sb = new StringBuilder(); + for (; index < length; index++) { - if (index > 0 && text[index - 1] == '\r') + if (text[index] == '\n') { - length--; + index++; + break; + } + else if (text[index] == '\r') + { + // if we're at the end or the next character isn't a new line, count this carriage return + if (index == length - 1 || index < length - 1 && text[index + 1] != '\n') + { + carriageReturns++; + sb.Append(text[index]); + } + } + else + { + sb.Append(text[index]); } - - index++; } - return text.Substring(startIndex, length); + return new LineInformation(sb.ToString(), carriageReturns); } bool StartOfText => index == 0; - bool EndOfText => index == -1 || index == text.Length; - } + bool EndOfText => index == -1 || index == length; - static int CountCarriageReturns(string text) - { - int count = 0; - int index = 0; - while ((index = text.IndexOf('\r', index)) != -1) + public class LineInformation { - index++; - count++; + public string Line { get; private set; } + public int CarriageReturns { get; private set; } + public LineInformation(string line, int carriageReturns) + { + this.Line = line; + this.CarriageReturns = carriageReturns; + } } - - return count; } [SuppressMessage("Microsoft.Globalization", "CA1305:SpecifyIFormatProvider", MessageId = "System.String.Format(System.String,System.Object)")] diff --git a/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs b/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs index 138ec5fc10..a6d7a12805 100644 --- a/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs +++ b/test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs @@ -319,27 +319,33 @@ public void NoLineMatchesFromNoLines() public class TheLineReaderClass { [Theory] - [InlineData("", "", null, null)] - [InlineData("\n", "", null, null)] - [InlineData("\r\n", "", null, null)] - [InlineData("1", "1", null, null)] - [InlineData("1\n2\n", "1", "2", null)] - [InlineData("1\n2", "1", "2", null)] - [InlineData("1\r\n2\n", "1", "2", null)] - [InlineData("1\r\n2", "1", "2", null)] - [InlineData("\r", "\r", null, null)] - [InlineData("\r\r", "\r\r", null, null)] - [InlineData("\r\r\n", "\r", null, null)] - [InlineData("\r_\n", "\r_", null, null)] - public void ReadLines(string text, string line1, string line2, string line3) + [InlineData(null, new string[] { "", null })] + [InlineData("", new string[] { "", null })] + [InlineData("\n", new string[] { "", null })] + [InlineData("\r\n", new string[] { "", null })] + [InlineData("1", new string[] { "1", null })] + [InlineData("1\n2\n", new string[] { "1", "2", null })] + [InlineData("1\n2", new string[] { "1", "2", null })] + [InlineData("1\r\n2\n", new string[] { "1", "2", null })] + [InlineData("1\r\n2", new string[] { "1", "2", null })] + [InlineData("\r", new string[] { "\r", null, null })] + [InlineData("\r\r", new string[] { "\r\r", null })] + [InlineData("\r\r\n", new string[] { "\r", null })] + [InlineData("\r_\n", new string[] { "\r_", null })] + public void ReadLines(string text, string[] expectedLines) { - var lines = new[] { line1, line2, line3 }; + if (text == null) + { + Assert.Throws(() => new DiffUtilities.LineReader(text)); + return; + } var lineReader = new DiffUtilities.LineReader(text); - foreach (var expectLine in lines) + foreach (var expectedLine in expectedLines) { - var line = lineReader.ReadLine(); - Assert.Equal(expectLine, line); + var lineAndCarriageReturns = lineReader.ReadLine(); + Assert.Equal(expectedLine, lineAndCarriageReturns.Line); + Assert.Equal(expectedLine?.Count(c => c == '\r') ?? 0, lineAndCarriageReturns.CarriageReturns); } } }