diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index 03cc27b417..6308f0be43 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -568,17 +568,24 @@ public Result Status(bool allowObjectDownloads, bool useStatusCache, bool showUn return this.InvokeGitInWorkingDirectoryRoot(command, useReadObjectHook: allowObjectDownloads); } - public Result StatusPorcelain() + /// + /// Streams "git status" output in porcelain -z form, delivering each NUL-terminated record to + /// as it is read. This avoids buffering the entire status + /// output, which can be large in a big working tree. + /// + public Result StatusPorcelain(Action parseStdOutToken) { string command = "status -uall --porcelain -z"; - return this.InvokeGitInWorkingDirectoryRoot(command, useReadObjectHook: false); + return this.InvokeGitInWorkingDirectoryRoot(command, useReadObjectHook: false, parseStdOutToken: parseStdOutToken); } /// - /// 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 as they are read, so an arbitrarily large + /// staged set is processed without buffering the whole list. /// + /// Receives each NUL-terminated record (status, path, status, path, ...). /// Inline pathspecs to scope the diff, or null for all. /// /// Path to a file containing additional pathspecs (one per line), forwarded @@ -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). /// - public Result DiffCachedNameStatus(string[] pathspecs = null, string pathspecFromFile = null, bool pathspecFileNul = false) + public Result DiffCachedNameStatus(Action parseStdOutToken, string[] pathspecs = null, string pathspecFromFile = null, bool pathspecFileNul = false) { string command = "diff --cached --name-status -z --no-renames"; @@ -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); } /// @@ -975,13 +982,30 @@ protected virtual Result InvokeGitImpl( Action parseStdOutLine, int timeoutMs, string gitObjectsDirectory = null, - bool usePreCommandHook = true) + bool usePreCommandHook = true, + Action 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 @@ -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) { @@ -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); + } } } @@ -1075,6 +1123,42 @@ private static string GenerateCredentialVerbCommand(string verb) return $"-c {GitConfigSetting.CredentialUseHttpPath}=true credential {verb}"; } + /// + /// Reads a redirected stdout stream that is NUL-delimited (git's "-z" machine-readable format), + /// invoking 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. + /// + internal static void ReadStdOutTokens(StreamReader reader, Action 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; @@ -1128,7 +1212,8 @@ private Result InvokeGitInWorkingDirectoryRoot( string command, bool useReadObjectHook, Action writeStdIn = null, - Action parseStdOutLine = null) + Action parseStdOutLine = null, + Action parseStdOutToken = null) { return this.InvokeGitImpl( command, @@ -1137,7 +1222,8 @@ private Result InvokeGitInWorkingDirectoryRoot( useReadObjectHook: useReadObjectHook, writeStdIn: writeStdIn, parseStdOutLine: parseStdOutLine, - timeoutMs: -1); + timeoutMs: -1, + parseStdOutToken: parseStdOutToken); } /// diff --git a/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs b/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs index 8182f8dfb5..41032c6480 100644 --- a/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs @@ -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 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 tokens = ReadTokens("A\0path\0\0after-empty\0"); + tokens.ShouldMatchInOrder("A", "path", string.Empty, "after-empty"); + } + + [TestCase] + public void ReadStdOutTokens_FlushesTrailingRecordWithoutNul() + { + List 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 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 tokens = new List(); + 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 tokens = new List(); + GitProcess.Result result = git.StatusPorcelain(t => tokens.Add(t)); + + result.ExitCodeIsSuccess.ShouldBeTrue(); + tokens.ShouldMatchInOrder("A added.txt", " M modified.txt"); + } + + private static List ReadTokens(string content) + { + List tokens = new List(); + 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() { diff --git a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs index c9095cc2c8..91623acd1c 100644 --- a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs +++ b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs @@ -84,7 +84,8 @@ protected override Result InvokeGitImpl( Action parseStdOutLine, int timeoutMs, string gitObjectsDirectory = null, - bool usePrecommandHook = true) + bool usePrecommandHook = true, + Action parseStdOutToken = null) { this.CommandsRun.Add(command); @@ -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; } diff --git a/GVFS/GVFS.UnitTests/Windows/CommandLine/SparseVerbTests.cs b/GVFS/GVFS.UnitTests/Windows/CommandLine/SparseVerbTests.cs index e4ef7b2db7..e1d6b10fc1 100644 --- a/GVFS/GVFS.UnitTests/Windows/CommandLine/SparseVerbTests.cs +++ b/GVFS/GVFS.UnitTests/Windows/CommandLine/SparseVerbTests.cs @@ -12,23 +12,8 @@ namespace GVFS.UnitTests.Windows.Windows.CommandLine [TestFixture] public class SparseVerbTests { - private const char StatusPathSeparatorToken = '\0'; - private static readonly HashSet EmptySparseSet = new HashSet(); - [TestCase] - public void GetNextGitPathGetsPaths() - { - string testStatusOutput = $"a.txt{StatusPathSeparatorToken}"; - ConfirmGitPathsParsed(testStatusOutput, new List() { "a.txt" }); - - testStatusOutput = $"a.txt{StatusPathSeparatorToken}b.txt{StatusPathSeparatorToken}c.txt{StatusPathSeparatorToken}"; - ConfirmGitPathsParsed(testStatusOutput, new List() { "a.txt", "b.txt", "c.txt" }); - - testStatusOutput = $"a.txt{StatusPathSeparatorToken}d/b.txt{StatusPathSeparatorToken}d/c.txt{StatusPathSeparatorToken}"; - ConfirmGitPathsParsed(testStatusOutput, new List() { "a.txt", "d/b.txt", "d/c.txt" }); - } - [TestCase] public void PathCoveredBySparseFolders_RootPaths() { @@ -126,20 +111,6 @@ private static void ConfirmAllPathsNotCovered(List paths, HashSet 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 paths, HashSet sparseSet, bool shouldBeCovered) { foreach (string path in paths) diff --git a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs index f2a45623cb..689a55d7c8 100644 --- a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs +++ b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs @@ -433,43 +433,35 @@ public bool AddStagedFilesToModifiedPaths(string messageBody, out int addedCount } } - // Query all staged files in one call using --name-status -z. - // Output format: "A\0path1\0M\0path2\0D\0path3\0" - GitProcess.Result result = gitProcess.DiffCachedNameStatus(pathspecs, pathspecFromFile, pathspecFileNul); - - if (result.OutputTruncated) - { - // The staged-file list exceeded the capture buffer. Acting on a partial list would leave - // some staged files out of ModifiedPaths (skip-worktree not cleared, stale placeholders), - // which is worse than failing. Fail safe and let the caller retry. - EventMetadata metadata = new EventMetadata(); - metadata.Add("ExitCode", result.ExitCode); - this.context.Tracer.RelatedError( - metadata, - nameof(this.AddStagedFilesToModifiedPaths) + ": git diff --cached output was truncated; refusing to update ModifiedPaths from a partial staged-file list"); - return false; - } - - if (result.ExitCodeIsSuccess && !string.IsNullOrEmpty(result.Output)) - { - string[] parts = result.Output.Split(new[] { '\0' }, StringSplitOptions.RemoveEmptyEntries); - List addedFilePaths = new List(); - - // Parts alternate: status, path, status, path, ... - for (int i = 0; i + 1 < parts.Length; i += 2) + // Query all staged files in one call using --name-status -z, streaming the records so we + // never buffer the entire (potentially huge) staged file list in memory. Records arrive in + // pairs: a status token ("A", "M", "D", ...) followed by a path token. + List addedFilePaths = new List(); + int added = 0; + string pendingStatus = null; + + GitProcess.Result result = gitProcess.DiffCachedNameStatus( + token => { - string status = parts[i]; - string gitPath = parts[i + 1]; + if (pendingStatus == null) + { + pendingStatus = token; + return; + } + + string status = pendingStatus; + pendingStatus = null; + string gitPath = token; if (string.IsNullOrEmpty(gitPath)) { - continue; + return; } string platformPath = gitPath.Replace(GVFSConstants.GitPathSeparator, Path.DirectorySeparatorChar); if (this.modifiedPaths.TryAdd(platformPath, isFolder: false, isRetryable: out _)) { - addedCount++; + added++; } // Added files (in index but not in HEAD) are ProjFS placeholders that @@ -479,8 +471,15 @@ public bool AddStagedFilesToModifiedPaths(string messageBody, out int addedCount { addedFilePaths.Add(gitPath); } - } + }, + pathspecs, + pathspecFromFile, + pathspecFileNul); + + addedCount = added; + if (result.ExitCodeIsSuccess) + { // Write added files from the git object store to disk as full files // so they persist across projection changes. Batched into as few git // process invocations as possible. @@ -492,7 +491,7 @@ public bool AddStagedFilesToModifiedPaths(string messageBody, out int addedCount } } } - else if (!result.ExitCodeIsSuccess) + else { EventMetadata metadata = new EventMetadata(); metadata.Add("ExitCode", result.ExitCode); diff --git a/GVFS/GVFS/CommandLine/SparseVerb.cs b/GVFS/GVFS/CommandLine/SparseVerb.cs index 334e4d1ca4..cec3783b08 100644 --- a/GVFS/GVFS/CommandLine/SparseVerb.cs +++ b/GVFS/GVFS/CommandLine/SparseVerb.cs @@ -18,7 +18,6 @@ public class SparseVerb : GVFSVerb.ForExistingEnlistment { private const string SparseVerbName = "sparse"; private const string FolderListSeparator = ";"; - private const char StatusPathSeparatorToken = '\0'; private const char StatusRenameToken = 'R'; private const string PruneOptionName = "prune"; @@ -107,14 +106,6 @@ public static System.CommandLine.Command CreateCommand() protected override string VerbName => SparseVerbName; - internal static string GetNextGitPath(ref int index, string statusOutput) - { - int endOfPathIndex = statusOutput.IndexOf(StatusPathSeparatorToken, index); - string gitPath = statusOutput.Substring(index, endOfPathIndex - index); - index = endOfPathIndex + 1; - return gitPath; - } - internal static bool PathCoveredBySparseFolders(string gitPath, HashSet sparseFolders) { string filePath = gitPath.Replace(GVFSConstants.GitPathSeparator, Path.DirectorySeparatorChar); @@ -628,29 +619,48 @@ private void ForceProjectionChange(ITracer tracer, GVFSEnlistment enlistment) private void CheckGitStatus(ITracer tracer, GVFSEnlistment enlistment, HashSet sparseFolders) { GitProcess.Result statusResult = null; - HashSet dirtyPathsNotInSparseSet = null; + HashSet dirtyPathsNotInSparseSet = new HashSet(); if (!this.ShowStatusWhileRunning( () => { + dirtyPathsNotInSparseSet.Clear(); GitProcess git = new GitProcess(enlistment); - statusResult = git.StatusPorcelain(); - if (statusResult.ExitCodeIsFailure) - { - return false; - } - if (statusResult.OutputTruncated) + // Stream porcelain -z records so we never buffer the whole status output. Each entry + // is a primary "XY " token; a rename adds a second token for the original path. + bool expectingRenameOrigin = false; + statusResult = git.StatusPorcelain( + token => + { + string gitPath; + if (expectingRenameOrigin) + { + expectingRenameOrigin = false; + gitPath = token; + } + else + { + if (token.Length < 3) + { + return; + } + + // Two status chars (XY) then a space, then the path. + expectingRenameOrigin = token[0] == StatusRenameToken || token[1] == StatusRenameToken; + gitPath = token.Substring(3); + } + + if (!PathCoveredBySparseFolders(gitPath, sparseFolders)) + { + dirtyPathsNotInSparseSet.Add(gitPath); + } + }); + + if (statusResult.ExitCodeIsFailure) { - // git status output exceeded the capture buffer. A partial status could omit - // dirty paths and let sparse proceed over uncommitted changes (data loss), so - // treat truncation as "cannot verify clean" and abort. - tracer.RelatedError( - new EventMetadata(), - "git status output was truncated; aborting sparse to avoid acting on an incomplete status"); return false; } - dirtyPathsNotInSparseSet = this.GetPathsNotCoveredBySparseFolders(statusResult.Output, sparseFolders); return dirtyPathsNotInSparseSet.Count == 0; }, "Running git status", @@ -662,10 +672,6 @@ private void CheckGitStatus(ITracer tracer, GVFSEnlistment enlistment, HashSet GetPathsNotCoveredBySparseFolders(string statusOutput, HashSet sparseFolders) - { - HashSet uncoveredPaths = new HashSet(); - int index = 0; - while (index < statusOutput.Length - 1) - { - bool isRename = statusOutput[index] == StatusRenameToken || statusOutput[index + 1] == StatusRenameToken; - index = index + 3; - - string gitPath = GetNextGitPath(ref index, statusOutput); - if (!PathCoveredBySparseFolders(gitPath, sparseFolders)) - { - uncoveredPaths.Add(gitPath); - } - - if (isRename) - { - gitPath = GetNextGitPath(ref index, statusOutput); - if (!PathCoveredBySparseFolders(gitPath, sparseFolders)) - { - uncoveredPaths.Add(gitPath); - } - } - } - - return uncoveredPaths; - } - private void WriteMessage(ITracer tracer, string message) { this.Output.WriteLine(message);