From fd61bad946d1ba9f35be90d488d2f7c8dc168af3 Mon Sep 17 00:00:00 2001
From: Johannes Altmanninger <aclopte@gmail.com>
Date: Sun, 21 Apr 2024 16:19:39 +0200
Subject: [PATCH] Further simplify terminal_protocols scoping

Remove the last non scoped place where we disable protocols (just before
exec(1)); it's not necessary with the current approach because we always
disable inside eval.
There is an edge case where we don't:

    fish -ic "exec bash"

leaving bash with CSI u enabled.  Disable that also in -ic mode where we
don't have a reader.

In future we should use the same approach for restore_term_mode() but I'm
not sure which one is better.
---
 src/exec.rs         |  6 +-----
 src/input_common.rs | 10 +++++++---
 src/parser.rs       | 10 ++++------
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/src/exec.rs b/src/exec.rs
index 2b6096944..a7a45f3aa 100644
--- a/src/exec.rs
+++ b/src/exec.rs
@@ -24,7 +24,6 @@ use crate::fork_exec::postfork::{
 #[cfg(FISH_USE_POSIX_SPAWN)]
 use crate::fork_exec::spawn::PosixSpawner;
 use crate::function::{self, FunctionProperties};
-use crate::input_common::{terminal_protocols_disable, TERMINAL_PROTOCOLS};
 use crate::io::{
     BufferedOutputStream, FdOutputStream, IoBufferfill, IoChain, IoClose, IoMode, IoPipe,
     IoStreams, OutputStream, SeparatedBuffer, StringOutputStream,
@@ -40,7 +39,7 @@ use crate::proc::{
     print_exit_warning_for_jobs, InternalProc, Job, JobGroupRef, ProcStatus, Process, ProcessType,
     TtyTransfer, INVALID_PID,
 };
-use crate::reader::{reader_current_data, reader_run_count, restore_term_mode};
+use crate::reader::{reader_run_count, restore_term_mode};
 use crate::redirection::{dup2_list_resolve_chain, Dup2List};
 use crate::threads::{iothread_perform_cant_wait, is_forked_child};
 use crate::timer::push_timer;
@@ -440,9 +439,6 @@ fn launch_process_nofork(vars: &EnvStack, p: &Process) -> ! {
 
     // Ensure the terminal modes are what they were before we changed them.
     restore_term_mode();
-    if reader_current_data().is_some() && TERMINAL_PROTOCOLS.get().borrow().is_some() {
-        terminal_protocols_disable();
-    }
     // Bounce to launch_process. This never returns.
     safe_launch_process(p, &actual_cmd, &argv, &*envp);
 }
diff --git a/src/input_common.rs b/src/input_common.rs
index 206e30354..6f5044ed6 100644
--- a/src/input_common.rs
+++ b/src/input_common.rs
@@ -438,7 +438,7 @@ fn terminal_protocols_enable() {
         .replace(Some(TerminalProtocols::new()));
 }
 
-pub fn terminal_protocols_disable() {
+fn terminal_protocols_disable() {
     assert!(TERMINAL_PROTOCOLS.get().borrow().is_some());
     TERMINAL_PROTOCOLS.get().replace(None);
 }
@@ -454,7 +454,9 @@ pub fn terminal_protocols_disable_scoped() -> impl ScopeGuarding<Target = ()> {
         // If a child is stopped, this will already be enabled.
         if TERMINAL_PROTOCOLS.get().borrow().is_none() {
             terminal_protocols_enable();
-            reader_current_data().unwrap().save_screen_state();
+            if let Some(data) = reader_current_data() {
+                data.save_screen_state();
+            }
         }
     })
 }
@@ -529,7 +531,9 @@ pub(crate) fn focus_events_enable_ifn() {
     if !term_protocols.focus_events {
         term_protocols.focus_events = true;
         let _ = write_to_fd("\x1b[?1004h".as_bytes(), STDOUT_FILENO);
-        reader_current_data().unwrap().save_screen_state();
+        if let Some(data) = reader_current_data() {
+            data.save_screen_state();
+        }
     }
 }
 
diff --git a/src/parser.rs b/src/parser.rs
index 9e1aefd96..6ec0cf2b1 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -25,7 +25,6 @@ use crate::parse_constants::{
 use crate::parse_execution::{EndExecutionReason, ParseExecutionContext};
 use crate::parse_tree::{parse_source, ParsedSourceRef};
 use crate::proc::{job_reap, JobGroupRef, JobList, JobRef, ProcStatus};
-use crate::reader::reader_current_data;
 use crate::signal::{signal_check_cancel, signal_clear_cancel, Signal};
 use crate::threads::{assert_is_main_thread, MainThread};
 use crate::util::get_time;
@@ -564,11 +563,10 @@ impl Parser {
             Some(ParseExecutionContext::new(ps.clone(), block_io.clone())),
         );
 
-        let terminal_protocols_disabled = (
-            // If interactive or inside noninteractive builtin read.
-            reader_current_data().is_some() && TERMINAL_PROTOCOLS.get().borrow().is_some()
-        )
-        .then(terminal_protocols_disable_scoped);
+        // If interactive or inside noninteractive builtin read.
+        let terminal_protocols_enabled = TERMINAL_PROTOCOLS.get().borrow().is_some();
+        let terminal_protocols_disabled =
+            terminal_protocols_enabled.then(terminal_protocols_disable_scoped);
 
         // Check the exec count so we know if anything got executed.
         let prev_exec_count = self.libdata().pods.exec_count;