Skip to content

Stream large -z git output instead of buffering it against the cap #2045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
tyrielv wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Stream large -z git output instead of buffering it against the cap #2045

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
134 changes: 110 additions & 24 deletions GVFS/GVFS.Common/Git/GitProcess.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,17 +568,24 @@ public Result Status(bool allowObjectDownloads, bool useStatusCache, bool showUn
return this.InvokeGitInWorkingDirectoryRoot(command, useReadObjectHook: allowObjectDownloads);
}

public Result StatusPorcelain()
/// <summary>
/// Streams "git status" output in porcelain -z form, delivering each NUL-terminated record to
/// <paramref name="parseStdOutToken"/> as it is read. This avoids buffering the entire status
/// output, which can be large in a big working tree.
/// </summary>
public Result StatusPorcelain(Action<string> parseStdOutToken)
{
string command = "status -uall --porcelain -z";
return this.InvokeGitInWorkingDirectoryRoot(command, useReadObjectHook: false);
return this.InvokeGitInWorkingDirectoryRoot(command, useReadObjectHook: false, parseStdOutToken: parseStdOutToken);
}

/// <summary>
/// Returns staged file changes (index vs HEAD) as null-separated pairs of
/// status and path: "A\0path1\0M\0path2\0D\0path3\0".
/// Status codes: A=added, M=modified, D=deleted, R=renamed, C=copied.
/// Streams staged file changes (index vs HEAD) as NUL-separated records: each change is emitted
/// as two records, a status token ("A", "M", "D", ...) followed by a path token. The records are
/// delivered to <paramref name="parseStdOutToken"/> as they are read, so an arbitrarily large
/// staged set is processed without buffering the whole list.
/// </summary>
/// <param name="parseStdOutToken">Receives each NUL-terminated record (status, path, status, path, ...).</param>
/// <param name="pathspecs">Inline pathspecs to scope the diff, or null for all.</param>
/// <param name="pathspecFromFile">
/// Path to a file containing additional pathspecs (one per line), forwarded
Expand All @@ -588,7 +595,7 @@ public Result StatusPorcelain()
/// When true and pathspecFromFile is set, pathspec entries in the file are
/// separated by NUL instead of newline (--pathspec-file-nul).
/// </param>
public Result DiffCachedNameStatus(string[] pathspecs = null, string pathspecFromFile = null, bool pathspecFileNul = false)
public Result DiffCachedNameStatus(Action<string> parseStdOutToken, string[] pathspecs = null, string pathspecFromFile = null, bool pathspecFileNul = false)
{
string command = "diff --cached --name-status -z --no-renames";

Expand All @@ -606,7 +613,7 @@ public Result DiffCachedNameStatus(string[] pathspecs = null, string pathspecFro
command += " -- " + string.Join(" ", pathspecs.Select(p => QuoteGitPath(p)));
}

return this.InvokeGitInWorkingDirectoryRoot(command, useReadObjectHook: false);
return this.InvokeGitInWorkingDirectoryRoot(command, useReadObjectHook: false, parseStdOutToken: parseStdOutToken);
}

/// <summary>
Expand Down Expand Up @@ -975,13 +982,30 @@ protected virtual Result InvokeGitImpl(
Action<string> parseStdOutLine,
int timeoutMs,
string gitObjectsDirectory = null,
bool usePreCommandHook = true)
bool usePreCommandHook = true,
Action<string> parseStdOutToken = null)
{
if (failedToSetEncoding && writeStdIn != null)
{
return new Result(string.Empty, "Attempting to use to stdin, but the process does not have the right input encodings set.", Result.GenericFailureCode);
}

// NUL-delimited streaming reads stdout synchronously on this thread, so it cannot honor a
// finite timeout (the timeout is only enforced once stdout reaches EOF). It also cannot be
// combined with line streaming. Callers that use it always pass an infinite timeout.
if (parseStdOutToken != null)
{
if (parseStdOutLine != null)
{
throw new InvalidOperationException($"{nameof(parseStdOutToken)} cannot be combined with {nameof(parseStdOutLine)}.");
}

if (timeoutMs != -1)
{
throw new InvalidOperationException($"{nameof(parseStdOutToken)} requires an infinite timeout.");
}
}

try
{
// From https://msdn.microsoft.com/en-us/library/system.diagnostics.process.standardoutput.aspx
Expand All @@ -1004,20 +1028,26 @@ protected virtual Result InvokeGitImpl(
errors.AppendLine(args.Data);
}
};
this.executingProcess.OutputDataReceived += (sender, args) =>

// In NUL-delimited streaming mode we read stdout ourselves (below) rather than using
// the line-based async reader, so we do not subscribe OutputDataReceived.
if (parseStdOutToken == null)
{
if (args.Data != null)
this.executingProcess.OutputDataReceived += (sender, args) =>
{
if (parseStdOutLine != null)
{
parseStdOutLine(args.Data);
}
else
if (args.Data != null)
{
output.AppendLine(args.Data);
if (parseStdOutLine != null)
{
parseStdOutLine(args.Data);
}
else
{
output.AppendLine(args.Data);
}
}
}
};
};
}

lock (this.executionLock)
{
Expand Down Expand Up @@ -1046,14 +1076,32 @@ protected virtual Result InvokeGitImpl(
writeStdIn?.Invoke(this.executingProcess.StandardInput);
this.executingProcess.StandardInput.Close();

this.executingProcess.BeginOutputReadLine();
// Always drain stderr asynchronously so the child can never block writing to it.
this.executingProcess.BeginErrorReadLine();

if (!this.executingProcess.WaitForExit(timeoutMs))
if (parseStdOutToken != null)
{
this.executingProcess.Kill();
// Read stdout synchronously, splitting on NUL and handing each record to the
// callback as it arrives. Because stderr is drained asynchronously above, a
// synchronous stdout read cannot deadlock. Only a single record is held in
// memory at a time, so an arbitrarily large result (e.g. every staged file in
// a monorepo) is processed without buffering the whole thing.
ReadStdOutTokens(this.executingProcess.StandardOutput, parseStdOutToken);

// stdout is at EOF; block until the process fully exits so the async stderr
// reads complete before we read ExitCode/Errors.
this.executingProcess.WaitForExit();
}
else
{
this.executingProcess.BeginOutputReadLine();

if (!this.executingProcess.WaitForExit(timeoutMs))
{
this.executingProcess.Kill();

return new Result(output.ToString(), "Operation timed out: " + errors.ToString(), Result.GenericFailureCode, output.Truncated, errors.Truncated);
return new Result(output.ToString(), "Operation timed out: " + errors.ToString(), Result.GenericFailureCode, output.Truncated, errors.Truncated);
}
}
}

Expand All @@ -1075,6 +1123,42 @@ private static string GenerateCredentialVerbCommand(string verb)
return $"-c {GitConfigSetting.CredentialUseHttpPath}=true credential {verb}";
}

/// <summary>
/// Reads a redirected stdout stream that is NUL-delimited (git's "-z" machine-readable format),
/// invoking <paramref name="parseStdOutToken"/> once per NUL-terminated record as it is read.
/// Only a single record is accumulated at a time, so an arbitrarily large result is processed
/// without buffering the entire stream.
/// </summary>
internal static void ReadStdOutTokens(StreamReader reader, Action<string> parseStdOutToken)
{
StringBuilder token = new StringBuilder();
char[] buffer = new char[8192];
int read;

while ((read = reader.Read(buffer, 0, buffer.Length)) > 0)
{
for (int i = 0; i < read; i++)
{
if (buffer[i] == '\0')
{
parseStdOutToken(token.ToString());
token.Clear();
}
else
{
token.Append(buffer[i]);
}
}
}

// git's -z output always terminates the final record with a NUL, so there should be nothing
// left here. Flush any trailing partial record defensively rather than dropping it.
if (token.Length > 0)
{
parseStdOutToken(token.ToString());
}
}

private static string ParseValue(string contents, string prefix)
{
int startIndex = contents.IndexOf(prefix) + prefix.Length;
Expand Down Expand Up @@ -1128,7 +1212,8 @@ private Result InvokeGitInWorkingDirectoryRoot(
string command,
bool useReadObjectHook,
Action<StreamWriter> writeStdIn = null,
Action<string> parseStdOutLine = null)
Action<string> parseStdOutLine = null,
Action<string> parseStdOutToken = null)
{
return this.InvokeGitImpl(
command,
Expand All @@ -1137,7 +1222,8 @@ private Result InvokeGitInWorkingDirectoryRoot(
useReadObjectHook: useReadObjectHook,
writeStdIn: writeStdIn,
parseStdOutLine: parseStdOutLine,
timeoutMs: -1);
timeoutMs: -1,
parseStdOutToken: parseStdOutToken);
}

/// <summary>
Expand Down
88 changes: 88 additions & 0 deletions GVFS/GVFS.UnitTests/Git/GitProcessTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,102 @@
using GVFS.Common.Git;
using GVFS.Tests.Should;
using GVFS.UnitTests.Mock.Common;
using GVFS.UnitTests.Mock.Git;
using NUnit.Framework;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Text;

namespace GVFS.UnitTests.Git
{
[TestFixture]
public class GitProcessTests
{
[TestCase]
public void ReadStdOutTokens_SplitsOnNul()
{
List<string> tokens = ReadTokens("a.txt\0d/b.txt\0d/c.txt\0");
tokens.ShouldMatchInOrder("a.txt", "d/b.txt", "d/c.txt");
}

[TestCase]
public void ReadStdOutTokens_EmptyInputYieldsNoTokens()
{
ReadTokens(string.Empty).Count.ShouldEqual(0);
}

[TestCase]
public void ReadStdOutTokens_PreservesEmptyRecords()
{
// diff --name-status -z emits status and path as separate records; an empty record must
// still be delivered so a caller's status/path state machine stays aligned.
List<string> tokens = ReadTokens("A\0path\0\0after-empty\0");
tokens.ShouldMatchInOrder("A", "path", string.Empty, "after-empty");
}

[TestCase]
public void ReadStdOutTokens_FlushesTrailingRecordWithoutNul()
{
List<string> tokens = ReadTokens("a.txt\0trailing");
tokens.ShouldMatchInOrder("a.txt", "trailing");
}

[TestCase]
public void ReadStdOutTokens_ReassemblesRecordSpanningReadBoundary()
{
// A single record longer than the internal 8192-char read buffer must be reassembled across
// multiple reads rather than split.
string longPath = new string('x', 20000);
List<string> tokens = ReadTokens("short\0" + longPath + "\0");

tokens.Count.ShouldEqual(2);
tokens[0].ShouldEqual("short");
tokens[1].ShouldEqual(longPath);
}

[TestCase]
public void DiffCachedNameStatus_StreamsRecordsAsTokens()
{
MockGitProcess git = new MockGitProcess();
git.SetExpectedCommandResult(
"diff --cached --name-status -z --no-renames",
() => new GitProcess.Result("A\0added.txt\0M\0modified.txt\0", string.Empty, GitProcess.Result.SuccessCode));

List<string> tokens = new List<string>();
GitProcess.Result result = git.DiffCachedNameStatus(t => tokens.Add(t));

result.ExitCodeIsSuccess.ShouldBeTrue();
tokens.ShouldMatchInOrder("A", "added.txt", "M", "modified.txt");
}

[TestCase]
public void StatusPorcelain_StreamsRecordsAsTokens()
{
MockGitProcess git = new MockGitProcess();
git.SetExpectedCommandResult(
"status -uall --porcelain -z",
() => new GitProcess.Result("A added.txt\0 M modified.txt\0", string.Empty, GitProcess.Result.SuccessCode));

List<string> tokens = new List<string>();
GitProcess.Result result = git.StatusPorcelain(t => tokens.Add(t));

result.ExitCodeIsSuccess.ShouldBeTrue();
tokens.ShouldMatchInOrder("A added.txt", " M modified.txt");
}

private static List<string> ReadTokens(string content)
{
List<string> tokens = new List<string>();
using (MemoryStream stream = new MemoryStream(Encoding.UTF8.GetBytes(content)))
using (StreamReader reader = new StreamReader(stream, Encoding.UTF8))
{
GitProcess.ReadStdOutTokens(reader, token => tokens.Add(token));
}

return tokens;
}

[TestCase]
public void BoundedGitOutputBuffer_KeepsShortOutput()
{
Expand Down
16 changes: 15 additions & 1 deletion GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ protected override Result InvokeGitImpl(
Action<string> parseStdOutLine,
int timeoutMs,
string gitObjectsDirectory = null,
bool usePrecommandHook = true)
bool usePrecommandHook = true,
Action<string> parseStdOutToken = null)
{
this.CommandsRun.Add(command);

Expand Down Expand Up @@ -122,6 +123,19 @@ protected override Result InvokeGitImpl(
}
/* Future: result.Output should be set to null in this case */
}

if (parseStdOutToken != null && !string.IsNullOrEmpty(result.Output))
{
// Mirror production ReadStdOutTokens: deliver every NUL-terminated record, including
// empty ones, and drop only the trailing empty element produced by the final NUL.
string[] tokens = result.Output.Split('\0');
int count = (tokens.Length > 0 && tokens[tokens.Length - 1].Length == 0) ? tokens.Length - 1 : tokens.Length;
for (int i = 0; i < count; i++)
{
parseStdOutToken(tokens[i]);
}
}

return result;
}

Expand Down
29 changes: 0 additions & 29 deletions GVFS/GVFS.UnitTests/Windows/CommandLine/SparseVerbTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,8 @@ namespace GVFS.UnitTests.Windows.Windows.CommandLine
[TestFixture]
public class SparseVerbTests
{
private const char StatusPathSeparatorToken = '\0';

private static readonly HashSet<string> EmptySparseSet = new HashSet<string>();

[TestCase]
public void GetNextGitPathGetsPaths()
{
string testStatusOutput = $"a.txt{StatusPathSeparatorToken}";
ConfirmGitPathsParsed(testStatusOutput, new List<string>() { "a.txt" });

testStatusOutput = $"a.txt{StatusPathSeparatorToken}b.txt{StatusPathSeparatorToken}c.txt{StatusPathSeparatorToken}";
ConfirmGitPathsParsed(testStatusOutput, new List<string>() { "a.txt", "b.txt", "c.txt" });

testStatusOutput = $"a.txt{StatusPathSeparatorToken}d/b.txt{StatusPathSeparatorToken}d/c.txt{StatusPathSeparatorToken}";
ConfirmGitPathsParsed(testStatusOutput, new List<string>() { "a.txt", "d/b.txt", "d/c.txt" });
}

[TestCase]
public void PathCoveredBySparseFolders_RootPaths()
{
Expand Down Expand Up @@ -126,20 +111,6 @@ private static void ConfirmAllPathsNotCovered(List<string> paths, HashSet<string
CheckIfPathsCovered(paths, sparseSet, shouldBeCovered: false);
}

private static void ConfirmGitPathsParsed(string paths, List<string> expectedPaths)
{
int index = 0;
int listIndex = 0;
while (index < paths.Length - 1)
{
int nextSeparatorIndex = paths.IndexOf(StatusPathSeparatorToken, index);
string expectedGitPath = expectedPaths[listIndex];
SparseVerb.GetNextGitPath(ref index, paths).ShouldEqual(expectedGitPath);
index.ShouldEqual(nextSeparatorIndex + 1);
++listIndex;
}
}

private static void CheckIfPathsCovered(List<string> paths, HashSet<string> sparseSet, bool shouldBeCovered)
{
foreach (string path in paths)
Expand Down
Loading
Loading