From 3cb105adbd842b21a14076bb0478cb8e3310e658 Mon Sep 17 00:00:00 2001
From: ridiculousfish <rf@fishshell.com>
Date: Wed, 10 Mar 2021 21:02:32 -0800
Subject: [PATCH] Restore terminal modes after running key bindings with
 external commands

This concerns the behavior when running an external command from a key
binding. The history is:

Prior to 5f16a299a728, fish would run these external commands in shell
modes. This meant that fish would pick up any tty changes from external
commands (see #2114).

After 5f16a299a728, fish would save and restore its shell modes around
these external commands. This introduced a regression where anything the
user typed while a bound external command was executing would be echoed,
because external command mode has ECHO set in c_lflag. (This can be
reproed easily with `bind -q 'sleep 1'` and then pressing q and typing).
So 5f16a299a728 was reverted in fd9355966.

This commit partially reverts fd9355966. It has it both ways: external
commands are launched with shell modes, but/and shell modes are restored
after the external command completes. This allows commands to muck with
the tty, as long as they can handle getting shell modes; but it does not
enable ECHO mode so it fixes the regression found in #7770.

Fixes #7770. Fixes #2114 (for the third time!)

This partially reverts commit fd9355966ed4392b77137e522e7fb4b28503d674.
---
 src/reader.cpp       | 23 ++++++++++++++---------
 tests/pexpects/fg.py | 12 +++++-------
 2 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/src/reader.cpp b/src/reader.cpp
index af08ea2b2..b1167c2bc 100644
--- a/src/reader.cpp
+++ b/src/reader.cpp
@@ -2827,20 +2827,25 @@ struct readline_loop_state_t {
 
 /// Run a sequence of commands from an input binding.
 void reader_data_t::run_input_command_scripts(const wcstring_list_t &cmds) {
-    // Need to donate/steal the tty - see #2114.
-    // Unfortunately this causes us to enable ECHO,
-    // which means if input arrives while we're running a bind function
-    // it will turn up on screen, see #7770.
-    //
-    // What needs to happen is to tell the parser to acquire the terminal
-    // when it's running an external command, but that's a lot more involved.
-    // term_donate();
     auto last_statuses = parser().get_last_statuses();
     for (const wcstring &cmd : cmds) {
         parser().eval(cmd, io_chain_t{});
     }
     parser().set_last_statuses(std::move(last_statuses));
-    // term_steal();
+
+    // Restore tty to shell modes.
+    // Some input commands will take over the tty - see #2114 for an example where vim is invoked
+    // from a key binding. However we do NOT want to invoke term_donate(), because that will enable
+    // ECHO mode, causing a race between new input and restoring the mode (#7770). So we leave the
+    // tty alone, run the commands in shell mode, and then restore shell modes.
+    int res;
+    do {
+        res = tcsetattr(STDIN_FILENO, TCSANOW, &shell_modes);
+    } while (res < 0 && errno == EINTR);
+    if (res < 0) {
+        wperror(L"tcsetattr");
+    }
+    termsize_container_t::shared().invalidate_tty();
 }
 
 /// Read normal characters, inserting them into the command line.
diff --git a/tests/pexpects/fg.py b/tests/pexpects/fg.py
index 1089a811b..7ad86ab77 100644
--- a/tests/pexpects/fg.py
+++ b/tests/pexpects/fg.py
@@ -92,13 +92,11 @@ send("\x12")  # ctrl-r, placing fth in foreground
 expect_str("SIGCONT")
 
 # Do it again.
-# FIXME: Unfortunately the fix for #2114 had to be reverted because of #7770,
-# so this is broken.
-# send("\x1A")
-# expect_str("SIGTSTP")
-# sleep(0.1)
-# send("\x12")
-# expect_str("SIGCONT")
+send("\x1A")
+expect_str("SIGTSTP")
+sleep(0.1)
+send("\x12")
+expect_str("SIGCONT")
 
 # End fth by sending it anything.
 send("\x12")