Stream large -z git output instead of buffering it against the cap#2045
Draft
tyrielv wants to merge 1 commit into
Draft
Stream large -z git output instead of buffering it against the cap#2045tyrielv wants to merge 1 commit into
tyrielv wants to merge 1 commit into
Conversation
tyrielv
force-pushed
the
tyrielv/fix-invokegit-stderr-oom
branch
2 times, most recently
from
c0cca88 to
e4198f0
Compare
tyrielv
changed the title
tyrielv
force-pushed
the
tyrielv/fix-invokegit-stderr-oom
branch
from
e4198f0 to
4b8cc4a
Compare
Building on the git-output bounding change, this 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. The prior change bounded
these at a large stdout cap and made the callers fail safe if the cap was ever
exceeded; this change makes truncation impossible instead.
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
mode keeps git's robust -z format (no path-unquoting logic) and is guarded to an
infinite timeout (a synchronous read cannot honor a finite one) and is mutually
exclusive with line streaming.
DiffCachedNameStatus and StatusPorcelain now stream tokens; their callers drive
small status/path state machines. This removes the interim fail-safe valves the
callers had (there is no longer a partial result to guard against) and the dead
-z string parsers (GetPathsNotCoveredBySparseFolders, GetNextGitPath).
Result.OutputTruncated/ErrorsTruncated remain for the still-buffered commands
(stderr is still bounded), but the two converted commands can no longer trip
OutputTruncated.
Tests:
- ReadStdOutTokens: NUL splitting, empty input, embedded empty records, a
trailing record without a NUL, and a record spanning the 8KB read buffer.
- DiffCachedNameStatus/StatusPorcelain stream records through the mock.
- Replaces the removed GetNextGitPath test; PathCoveredBySparseFolders tests
are unchanged.
Assisted-by: Claude Opus 4.8
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
tyrielv
force-pushed
the
tyrielv/fix-invokegit-stderr-oom
branch
from
4b8cc4a to
1a2dc13
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).