gh-151485: Fix command quoting in subprocess.CalledProcessError.__str__#151486
Open
BenjyWiener wants to merge 3 commits into
Open
gh-151485: Fix command quoting in subprocess.CalledProcessError.__str__#151486BenjyWiener wants to merge 3 commits into
BenjyWiener wants to merge 3 commits into
Conversation
CalledProcessError previously formatted cmd as `"... '%s' ..."`. This lead to unbalanced quoting when cmd contains single-quotes or, more commonly, when cmd is a list. This change updates the relevant format strings to use %r instead.
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
vstinner
reviewed
Jun 15, 2026
vstinner
left a comment
vstinner
left a comment
Member
There was a problem hiding this comment.
Can you please add a test to test_subprocess?
vstinner
reviewed
Jun 16, 2026
Comment on lines
2431
to
+2461
| def test_CalledProcessError_str_signal(self): | ||
| err = subprocess.CalledProcessError(-int(signal.SIGABRT), "fake cmd") | ||
| error_string = str(err) | ||
| # We're relying on the repr() of the signal.Signals intenum to provide | ||
| # the word signal, the signal name and the numeric value. | ||
| self.assertIn("signal", error_string.lower()) | ||
| # We're not being specific about the signal name as some signals have | ||
| # multiple names and which name is revealed can vary. | ||
| self.assertIn("SIG", error_string) | ||
| self.assertIn(str(signal.SIGABRT), error_string) | ||
|
|
||
| def test_CalledProcessError_str_unknown_signal(self): | ||
| err = subprocess.CalledProcessError(-9876543, "fake cmd") | ||
| error_string = str(err) | ||
| self.assertIn("unknown signal 9876543.", error_string) | ||
|
|
||
| def test_CalledProcessError_str_non_zero(self): | ||
| err = subprocess.CalledProcessError(2, "fake cmd") | ||
| error_string = str(err) | ||
| self.assertIn("non-zero exit status 2.", error_string) | ||
|
|
||
| def test_CalledProcessError_str_quote_in_cmd(self): | ||
| err = subprocess.CalledProcessError(2, "fake ' cmd") | ||
| error_string = str(err) | ||
| self.assertStartsWith(error_string, 'Command "fake \' cmd" ') | ||
|
|
||
| def test_CalledProcessError_str_list_cmd(self): | ||
| err = subprocess.CalledProcessError(2, ["fake", "cmd"]) | ||
| error_string = str(err) | ||
| self.assertStartsWith(error_string, "Command ['fake', 'cmd'] ") | ||
|
|
Member
There was a problem hiding this comment.
Hum, IMO it's now time to check the whole string using assertEqual(). I suggest rewriting existing tests in a single method, and add your 2 new tests there:
Suggested change
| def test_CalledProcessError_str(self): | |
| # command string | |
| err = subprocess.CalledProcessError(2, "fake cmd") | |
| self.assertEqual(str(err), "Command 'fake cmd' returned non-zero exit status 2.") | |
| # command string with a quote | |
| err = subprocess.CalledProcessError(2, "fake ' cmd") | |
| self.assertEqual(str(err), 'Command "fake \' cmd" returned non-zero exit status 2.') | |
| # command list | |
| err = subprocess.CalledProcessError(2, ["fake", "cmd"]) | |
| self.assertEqual(str(err), "Command ['fake', 'cmd'] returned non-zero exit status 2.") | |
| # signal | |
| err = subprocess.CalledProcessError(-int(signal.SIGABRT), "fake cmd") | |
| self.assertEqual(str(err), f"Command 'fake cmd' died with {repr(signal.SIGABRT)}.") | |
| # unknown signal | |
| err = subprocess.CalledProcessError(-9876543, "fake cmd") | |
| self.assertEqual(str(err), "Command 'fake cmd' died with unknown signal 9876543.") | |
Author
There was a problem hiding this comment.
Sounds good. Is there a preference for f'{repr(...)}' over {...!r} (for the signal case)?
Member
There was a problem hiding this comment.
It's the same, pick your favorite syntax.
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
2 participants
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.
subprocess.CalledProcessError.__str__previously formattedcmdas"... '%s' ...". This lead to unbalanced quoting when cmd contains single-quotes or, more commonly, whencmdis a list. This change updates the relevant format strings to use%rinstead.In terms of backwards compatibility, the only impact I anticipate is on doctests, and I suspect doctests for
subprocess.CalledProcessErrorto be quite rare.Before this PR:
After: