Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Only scan the diff once when processing carriage returns #1270

Closed
wants to merge 1 commit into from
Closed

Only scan the diff once when processing carriage returns #1270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 43 additions & 30 deletions src/GitHub.Exports/Models/DiffUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -13,14 +15,15 @@ public static class DiffUtilities
public static IEnumerable<DiffChunk> 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)
Expand Down Expand Up @@ -55,8 +58,7 @@ public static IEnumerable<DiffChunk> ParseFragment(string diff)
Content = line,
});

var lineCount = 1;
lineCount += CountCarriageReturns(line);
var lineCount = lineInfo.CarriageReturns + 1;

switch (type)
{
Expand Down Expand Up @@ -116,61 +118,72 @@ public static DiffLine Match(IEnumerable<DiffChunk> diff, IList<DiffLine> 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++)
Copy link
Collaborator

@jcansdale jcansdale Oct 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could maybe adapt this implementation to use, text.IndexOfAny(new[] {'\r', '\n'}, index)? With new[] {'\r', '\n'}, stored in a static. 😉

{
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)")]
Expand Down
40 changes: 23 additions & 17 deletions test/GitHub.InlineReviews.UnitTests/Models/DiffUtilitiesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ArgumentNullException>(() => 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);
}
}
}
Expand Down