Skip to content
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.

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

child_process: remove extra newline in errors #9343

Merged
merged 1 commit into from
Nov 2, 2016

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Oct 28, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

child_process

Description of change

checkExecSyncError() creates error objects for execSync() and execFileSync(). If the child process created stderr output, then it is attached to the end of the error message. However, stderr can be an empty Buffer object, which always passes the truthy check, leading to an extra newline in the error message. This commit adds a length check, which will work with both strings and Buffers.

@cjihrig cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 28, 2016
@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Oct 28, 2016
@geek
Copy link
Member

geek commented Nov 1, 2016

LGTM

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 2, 2016

checkExecSyncError() creates error objects for execSync() and
execFileSync(). If the child process created stderr output, then
it is attached to the end of the error message. However, stderr
can be an empty Buffer object, which always passes the truthy
check, leading to an extra newline in the error message. This
commit adds a length check, which will work with both strings and
Buffers.

PR-URL: nodejs#9343
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig cjihrig merged commit 49d1c36 into nodejs:master Nov 2, 2016
@cjihrig cjihrig deleted the sync-err branch November 2, 2016 23:31
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
checkExecSyncError() creates error objects for execSync() and
execFileSync(). If the child process created stderr output, then
it is attached to the end of the error message. However, stderr
can be an empty Buffer object, which always passes the truthy
check, leading to an extra newline in the error message. This
commit adds a length check, which will work with both strings and
Buffers.

PR-URL: nodejs#9343
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell mentioned this pull request Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants