Treat non-zero exit code as error by default #597
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.
Note: this would be a breaking change
The default behavior of Node's execFile is to treat any exitCode other than 0 as an error. Since dugite's inception we've chosen to ignore these errors and pretend everything was fine, leaving it up to the caller to inspect the
exitCode
property to detect if the command succeeded or not.GitHub Desktop worked around this by throwing a new error of its own if the exit code was not part of the 'successExitCodes` set.
This change would copy that approach into Dugite and I'm exploring what that would look like in this PR. I'm not necessarily committed to the change.
Why the change?
Partially to align closer with
execFile
and thereby making it easier for callers to opt out of using ourexec
function and rolling their own instead. Partially because it feels weird to put it on the caller to always inspectexitCode
instead of notifying them when something unexpected occurred.Why include
ignoreExitCodes
in dugite?So I had wanted to avoid this but it turns out the change I made in #582 made it awkward for callers to catch this specific exception and then convert it to an
IGitResult
on their own as it'd break the nice type matching onIGitStringResult
vsIGitBufferResult
.@sergiou87 @tidy-dev We can start with the question of whether y'all think this change makes sense before we dive too deep into code review.