gh-123856: Fix PyREPL failure when a keyboard interrupt is triggered after using a history search - #124396
Conversation
vstinner
left a comment
There was a problem hiding this comment.
LGTM.
I confirm that this change fix #123856. The fix works with forward and backward isearch.
Moreover, CTRL+C in an interactive search (CTRL+S or CTRL+R) still resets the prompt to >>> as expected.
Using r.isearch_trans looks safe to me since r is created by _ReadlineWrapper.get_reader() which returns a ReadlineAlikeReader and ReadlineAlikeReader inherits from HistoricalReader.
|
@vstinner thanks for reviewing the draft version of this - I refactored the clean-up process to better mimic what happens when the search is allowed to clean up after itself (see the PR description for a little more info) |
…gered after using a history search
…-up command; add tests
5bf83d6 to
7389752
Compare
vstinner
left a comment
There was a problem hiding this comment.
LGTM. The fix still works as expected. I tried different scenario and it just works.
I just have minor comments on the test, you can ignore them.
| self.assertNotIn("\\040", pathlib.Path(hfile.name).read_text()) | ||
|
|
||
| def test_keyboard_interrupt_after_isearch(self): | ||
| output, exit_code = self.run_repl(["\x12", "\x03", "exit"]) |
There was a problem hiding this comment.
Would it be possible to add (module-level) constants for "\x12" and "\x03"?
There was a problem hiding this comment.
We'll do that later as part of more tests for control characters.
| cmd.extend(cmdline_args) | ||
|
|
||
| term_attr = termios.tcgetattr(slave_fd) | ||
| term_attr[6][termios.VREPRINT] = 0 # pass through CTRL-R |
There was a problem hiding this comment.
What is the hardcoded constant 6?
|
Address sanitizer is failing |
…its own class so it still runs
|
After trying to find which TERM's on address sanitizer, we're still seeing which suggests TERM is unset or empty. Let's just add that case to our TestMain ignore. |
| self.assertNotIn("\\040", pathlib.Path(hfile.name).read_text()) | ||
|
|
||
| def test_keyboard_interrupt_after_isearch(self): | ||
| output, exit_code = self.run_repl(["\x12", "\x03", "exit"]) |
There was a problem hiding this comment.
We'll do that later as part of more tests for control characters.
| if r.input_trans is r.isearch_trans: | ||
| r.do_cmd(("isearch-end", [""])) |
|
Thanks @emilyemorehouse for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
…gered after using a history search (pythonGH-124396) (cherry picked from commit c1600c7) Co-authored-by: Emily Morehouse <emily@cuttlesoft.com> Co-authored-by: Łukasz Langa <lukasz@langa.pl>
|
GH-124530 is a backport of this pull request to the 3.13 branch. |
…ggered after using a history search (GH-124396) (#124530) gh-123856: Fix PyREPL failure when a keyboard interrupt is triggered after using a history search (GH-124396) (cherry picked from commit c1600c7) Co-authored-by: Emily Morehouse <emily@cuttlesoft.com> Co-authored-by: Łukasz Langa <lukasz@langa.pl>
The original check in
Lib/_pyrepl/simple_interact.pyfor handling the manual clean up when aKeyboardInterruptis received would result in duplicate calls to clean up the search translator in some scenarios. When the search translator is allowed to exit cleanly without aKeyboardInterrupt, it calls its clean-up function (isearch_end) on its own. If aKeyboardInterruptwas triggered immediately after, the most recent command was stillisearch, which would trigger a similar clean-up routine incorrectly. This now ensures that the clean-up is only executed once.I've also added tests – we had to make modifications to the way the full version of the REPL is run – this now allows the necessary control characters to pass through. Additional overrides for other control characters may be required here as the test suite is bolstered.