Properly swallow errors if deleting a remote fails #505
Conversation
This file contains 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
`deleteRemote()` intends to return successfully if the to-be-deleted remote does not exist. In this case, git returns an exit code of 2, and outputs a message to stderr. Depending on the locale of the user's system, this error message might be localized. `spawnPromise()` and `spawnStream()` try to set the locale to `en_US`, but there are no guarantees that this locale is actually available on the user's system. Instead of doing fragile locale-dependent string parsing, simply use the exit code we get from git in this case and act on that. Fix the mocks in the tests to behave like the real-world git, and add a test for the non-English case as well.
imphil
added a commit
to imphil/backport
that referenced
this pull request
Aug 19, 2024
`LANG=en_US` is not guaranteed to exist on any system (it does not, for strange reasons, exist on mine). Choose `LANG=C`, which is guaranteed to exist. It has the side-effect of typically using an encoding other than UTF-8 for the tool output (e.g., ISO 8859-1). Apart from messages printed into the log in this encoding that should hopefully have no negative side-effects. See sorenlouv#505 for a case where code was assuming that output would always be English, and failed if it wasn't.
This was referenced Aug 19, 2024
sorenlouv
added a commit
that referenced
this pull request
Sep 6, 2024
`LANG=en_US` is not guaranteed to exist on any system (it does not, for strange reasons, exist on mine). Choose `LANG=C`, which is guaranteed to exist. It has the side-effect of typically using an encoding other than UTF-8 for the tool output (e.g., ISO 8859-1). Apart from messages printed into the log in this encoding that should hopefully have no negative side-effects. See #505 for a case where code was assuming that output would always be English, and failed if it wasn't. Co-authored-by: Søren Louv-Jansen <[email protected]>
|
Released in 9.6.0 |
imphil
added a commit
to imphil/backport
that referenced
this pull request
Sep 23, 2024
Git 2.30.0 introduced a dedicated exit code (2) for when `git remote rm` failed because the remote does not exist. Earlier versions of git always returned code 128, and only parsing stderr could tell the error conditions apart. Support all versions of git by checking for both exit code 2, and exit code 128 with the specific error message. The exit code 2 check remains more robust against localized output, hence it's kept in addition to the output parsing path that was removed in sorenlouv#505. Fixes sorenlouv#509
sorenlouv
pushed a commit
to imphil/backport
that referenced
this pull request
Sep 26, 2024
Git 2.30.0 introduced a dedicated exit code (2) for when `git remote rm` failed because the remote does not exist. Earlier versions of git always returned code 128, and only parsing stderr could tell the error conditions apart. Support all versions of git by checking for both exit code 2, and exit code 128 with the specific error message. The exit code 2 check remains more robust against localized output, hence it's kept in addition to the output parsing path that was removed in sorenlouv#505. Fixes sorenlouv#509
sorenlouv
pushed a commit
that referenced
this pull request
Sep 26, 2024
Git 2.30.0 introduced a dedicated exit code (2) for when `git remote rm` failed because the remote does not exist. Earlier versions of git always returned code 128, and only parsing stderr could tell the error conditions apart. Support all versions of git by checking for both exit code 2, and exit code 128 with the specific error message. The exit code 2 check remains more robust against localized output, hence it's kept in addition to the output parsing path that was removed in #505. Fixes #509
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
deleteRemote()intends to return successfully ifthe to-be-deleted remote does not exist. In this case, git returns an
exit code of 2, and outputs a message to stderr. Depending on the locale
of the user's system, this error message might be localized.
spawnPromise()andspawnStream()try to set the locale toen_US,but there are no guarantees that this locale is actually available on
the user's system.
Instead of doing fragile locale-dependent string parsing, simply use the
exit code we get from git in this case and act on that.
Fix the mocks in the tests to behave like the real-world git, and add a
test for the non-English case as well.
Fixes #507