Stream large -z git output instead of buffering it against the cap - #2045
Stream large -z git output instead of buffering it against the cap#2045tyrielv wants to merge 1 commit into
Conversation
c0cca88 to
e4198f0
Compare
4b8cc4a to
1a2dc13
Compare
1a2dc13 to
9328962
Compare
703e798 to
5cfb8df
Compare
| // Parts alternate: status, path, status, path, ... | ||
| for (int i = 0; i + 1 < parts.Length; i += 2) | ||
| // Added files (in index but not in HEAD) are ProjFS placeholders that | ||
| // would vanish when the projection reverts to HEAD. Collect them for |
There was a problem hiding this comment.
Before this PR, the method parsed and applied Result.Output only after result.ExitCodeIsSuccess. With streaming, the callback runs while git is still executing and before the final exit code is known.
We should be careful mutating ModifiedPaths before git diff succeeds
There was a problem hiding this comment.
Good catch — fixed. The streaming path now buffers the parsed (status, path) records and only applies them to ModifiedPaths after git diff --cached exits successfully, so a mid-stream failure can't leave a partial mutation behind (matching the pre-PR "apply only on success" behavior). Holding the parsed records as small strings still avoids the single large-array allocation that caused the original OOM. Also note streaming is now off by default (gvfs.stream-git-status-output), so this path only runs once enabled.
Building on the git-output bounding change, this adds an opt-in path that removes
the truncation exposure for the two commands whose full result is
correctness-critical by streaming their output instead of buffering it.
DiffCachedNameStatus (diff --cached --name-status -z) feeds every staged path
into ModifiedPaths; StatusPorcelain (status --porcelain -z) drives the sparse
dirty check. Both use -z (NUL-delimited), so line-based streaming cannot chunk
them - git delivers the whole blob as a single line.
Add a NUL-delimited streaming mode to InvokeGitImpl: a new parseStdOutToken
callback reads stdout synchronously and splits on NUL, invoking the callback
once per record as it arrives. stderr stays async (BeginErrorReadLine), so the
synchronous stdout read cannot deadlock. Only one record is held in memory, so
an arbitrarily large result streams without buffering, truncation, or OOM.
The staged-file callback collects the parsed (status, path) records and applies
them to ModifiedPaths only after git exits successfully, so a mid-stream failure
never leaves a partial ModifiedPaths mutation behind. Holding the parsed records
as many small strings still avoids the single large-array allocation that caused
the OOM.
Both commands expose a streaming overload and a buffered overload. The callers
choose at runtime from the gvfs.stream-git-status-output config key, which
defaults to false (off) per the feature-flag convention: by default they use the
bounded-buffer path and its OutputTruncated fail-safes (the proven behavior), and
streaming is enabled only when the rollout infrastructure turns the flag on.
Add an optional streaming watchdog gated by gvfs.git-status-stream-timeout-seconds
(default -1 = infinite/disabled): when set, a timer kills the git process tree if
the synchronous read does not finish in time and the result reports a timeout.
The default is infinite so a legitimately long status on a very large working
tree is never killed. The watchdog disarms under processLock once the read
completes, so a late callback cannot report a false timeout or kill a reused
process.
Hardening from self-review: the streaming read kills the git child if a callback
throws (no orphaned process); AddStagedFilesToModifiedPaths fails on an unpaired
trailing status token rather than acting on an incomplete list; MockGitProcess
feeds output through the production tokenizer so the test double cannot drift.
Tests:
- ReadStdOutTokens: NUL splitting, empty input, embedded empty records, a lone
NUL, a trailing record without a NUL, and a record spanning the 8KB buffer.
- Streaming and buffered overloads of DiffCachedNameStatus/StatusPorcelain.
- GetNextGitPath (buffered fallback) parsing; PathCoveredBySparseFolders
unchanged.
Assisted-by: Claude Opus 4.8
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5cfb8df to
7428a5a
Compare
Summary
Follow-up to #2048. That PR bounded the previously-unbounded git stdout/stderr capture and, for the two commands whose full result is correctness-critical, added a large stdout cap plus fail-safe valves if the cap was ever exceeded. This PR removes that truncation exposure entirely by streaming those commands' output instead of buffering it.
DiffCachedNameStatus(diff --cached --name-status -z) feeds every staged path intoModifiedPaths.StatusPorcelain(status --porcelain -z) drives the sparse dirty check.Both use
-z(NUL-delimited), so line-based streaming can't chunk them — git delivers the whole blob as a single "line."Change
Add a NUL-delimited streaming mode to
InvokeGitImpl: a newparseStdOutTokencallback reads stdout synchronously and splits on\0, invoking the callback once per record as it arrives. stderr stays async (BeginErrorReadLine), so the synchronous stdout read is deadlock-safe ("one sync, one async"). Only one record is held in memory, so an arbitrarily large result streams with no buffering, truncation, or OOM. It keeps git's robust-zformat (no path-unquoting) and is guarded totimeoutMs == -1and mutual exclusion withparseStdOutLine.DiffCachedNameStatusandStatusPorcelainnow stream tokens; their callers drive small status/path state machines. This removes the interim fail-safe valves from #2048 (there's no longer a partial result to guard against) and the dead-zstring parsers (GetPathsNotCoveredBySparseFolders,GetNextGitPath).Result.OutputTruncated/ErrorsTruncatedremain for the still-buffered commands (stderr is still bounded), but the two converted commands can no longer tripOutputTruncated.Tests
ReadStdOutTokens: NUL splitting, empty input, embedded empty records, a trailing record without a NUL, and a record spanning the 8KB read buffer.DiffCachedNameStatus/StatusPorcelainstream records through the mock.GetNextGitPathtest;PathCoveredBySparseFolderstests unchanged.Full unit suite: 889 passed, 0 failed (11 pre-existing native-hook skips).