Skip to content

[BUGFIX] Consider output of stderr in successful cases - #206

Open
andreaskienast wants to merge 1 commit into
gitonomy:mainfrom
andreaskienast:bugfix/205
Open

[BUGFIX] Consider output of stderr in successful cases#206
andreaskienast wants to merge 1 commit into
gitonomy:mainfrom
andreaskienast:bugfix/205

Conversation

@andreaskienast

Copy link
Copy Markdown

Git writes the output of some commands into stderr instead of stdout (e.g. push). Using the option --porcelain doesn't always help in such cases as other information may be missing. To get the full output, stderr is now fetched in case stdout is empty.

Fixes: #205

Git writes the output of some commands into `stderr` instead of
`stdout` (e.g. `push`). Using the option `--porcelain` doesn't always
help in such cases as other information may be missing. To get the full
output, `stderr` is now fetched in case `stdout` is empty.

Fixes: gitonomy#205
andreaskienast added a commit to TYPO3GmbH/site-intercept that referenced this pull request Jul 11, 2023
The library `gitonomy/gitlib` has an issue with fetching the output of
Git commands, as some output is written to `stderr`. There's a pending
pull request addressing this issue [1], which is now installed instead.

[1] gitonomy/gitlib#206
@andreaskienast

Copy link
Copy Markdown
Author

It's been three years, any update on this?

@lyrixx

lyrixx commented Aug 28, 2026

Copy link
Copy Markdown
Member

Sorry, I missed that. Could you write a test?

@lyrixx

lyrixx commented Sep 7, 2026

Copy link
Copy Markdown
Member

Thanks for the fix! I looked into this closer and I think the current approach has a correctness problem that's worth addressing before merging.

The core issue

$output = $process->getOutput() ?: $process->getErrorOutput();

This conflates two different situations:

  • "the command succeeded and stdout is legitimately empty" (very common: a clean diff, an empty blob/tree, a log query with zero matches, a commit with no branches, etc.)
  • "the command succeeded but wrote its real output to stderr" (the push-specific case this PR is fixing)

Since the fallback is applied unconditionally inside the single shared run() method (used by ~25 internal call sites, plus external consumers since it's public API), any command that legitimately produces empty stdout on success will now return stderr content instead — even if that stderr content is just an unrelated warning.

Concrete regressions this introduces

  • Blob::getContent() — for an empty blob (e.g. a tracked .gitkeep), cat-file -p succeeds with empty stdout. If anything lands on stderr (locale warning, filter/clean driver notice), the blob's content is silently replaced by that stderr text. No exception, just wrong data.
  • WorkingCopy::getDiffPending()/getDiffStaged(), Repository::getDiff(), Commit::getDiff() — a clean/no-op diff succeeds with empty stdout. If stderr has any content (e.g. core.safecrlf=warn, textconv warnings), DiffParser::doParse()'s consumeRegexp("/diff --git .../") fails to match and throws a RuntimeException instead of returning an empty Diff.
  • Tree::initialize()cat-file -p on an empty tree (e.g. the well-known empty-tree hash) succeeds with empty stdout; stderr noise makes TreeParser throw instead of yielding zero entries.
  • Log::getCommits() — a query matching zero commits succeeds with empty stdout; stderr noise makes LogParser::doParse()'s consume('commit ') throw instead of returning [].
  • Blame::getLines() — blaming a genuinely empty file succeeds with empty stdout; stderr noise makes BlameParser throw instead of returning zero lines.
  • Commit::getIncludingBranches() / Branch::isMergedTo() — both guard with if (!$result) { ... }. git branch --contains <rev> -a commonly emits warning: ignoring dangling symref refs/remotes/origin/HEAD on stderr (e.g. after a remote's default branch was renamed) even when the real answer is "zero branches". That warning bypasses the falsy check and gets parsed as a branch name, which then throws ReferenceNotFoundException in getIncludingBranches().

Smaller points

  • ?: is a loose/falsy check, so a legitimate successful stdout of exactly "0" (no trailing newline) would also be discarded in favor of stderr. Not triggered by current internal call sites (git always appends \n), but run() is documented as a general-purpose public method, so this is a latent trap for external callers.
  • The fallback is computed unconditionally before the success/failure branch, which also affects the debug log line's contents.
  • No test was added, even though it was already requested in this thread — given how many existing call sites depend on "empty stdout = valid empty result", a regression test seems important here.

Suggested direction

Rather than changing the contract of the shared run() for every caller, it'd be safer to scope the stderr fallback to the commands that actually need it — e.g. an opt-in parameter (run($command, $args, $mergeStderrOnEmptyOutput = false)), a dedicated wrapper for push/fetch-like commands, or command-aware branching in getProcess(). That would fix the push output issue without risking the regressions above for diff, log, cat-file, blame, and branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Library takes stdout into consideration only

2 participants