From 2ca66cff53c1c97a52b38fcd2f727d573f82447f Mon Sep 17 00:00:00 2001
From: ridiculousfish <rf@fishshell.com>
Date: Mon, 26 Jul 2021 13:12:30 -0700
Subject: [PATCH] Disable job control inside command substitutions

This disables job control inside command substitutions. Prior to this
change, a cmdsub might get its own process group. This caused it to fail
to cancel loops properly. For example:

    while true ; echo (sleep 5) ; end

could not be control-C cancelled, because the signal would go to sleep,
and so the loop would continue on. The simplest way to fix this is to
match other shells and not use job control in cmdsubs.

Related is #1362
---
 CHANGELOG.rst             |  1 +
 src/parse_execution.cpp   |  8 +++++---
 src/parser.cpp            | 12 ++++++++++++
 src/parser.h              |  3 +++
 tests/checks/sigint2.fish | 15 +++++++++++++++
 tests/pexpects/signals.py |  7 +++++++
 6 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index a4ad6635b..46bbaf7fd 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -29,6 +29,7 @@ Deprecations and removed features
 
 - ``$status`` is now forbidden as a command, to prevent a surprisingly common error among new users: Running ``if $status`` (:issue:`8171`). This applies *only* to ``$status``, other variables are still allowed.
 - ``set --query`` now returns a falsy status of 255 if given no variable names. This means ``if set -q $foo`` will not enter the if-block if ``$foo`` is empty or unset. To restore the previous behavior you would use something like ``if not set -q foo; or set -q $foo``. We do not expect anyone to have used this on purpose, any places this happens are almost certainly buggy (:issue:`8214`).
+- Command substitutions no longer respect job control, instead running inside fish's own process group. This more closely matches other shells, and improves :kbd:`Control-C` reliability inside a command substitution.
 
 Scripting improvements
 ----------------------
diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp
index 5987e42b1..de10e9e38 100644
--- a/src/parse_execution.cpp
+++ b/src/parse_execution.cpp
@@ -1342,10 +1342,12 @@ end_execution_reason_t parse_execution_context_t::run_1_job(const ast::job_t &jo
     const auto &ld = parser->libdata();
 
     auto job_control_mode = get_job_control_mode();
+    // Run all command substitutions in our pgroup.
     bool wants_job_control =
-        (job_control_mode == job_control_t::all) ||
-        ((job_control_mode == job_control_t::interactive) && parser->is_interactive()) ||
-        (ctx.job_group && ctx.job_group->wants_job_control());
+        !parser->is_command_substitution() &&
+        ((job_control_mode == job_control_t::all) ||
+         ((job_control_mode == job_control_t::interactive) && parser->is_interactive()) ||
+         (ctx.job_group && ctx.job_group->wants_job_control()));
 
     job_t::properties_t props{};
     props.initial_background = job_node.bg.has_value();
diff --git a/src/parser.cpp b/src/parser.cpp
index 777b2d493..ff94fb581 100644
--- a/src/parser.cpp
+++ b/src/parser.cpp
@@ -398,6 +398,18 @@ bool parser_t::is_breakpoint() const {
     return false;
 }
 
+bool parser_t::is_command_substitution() const {
+    for (const auto &b : block_list) {
+        if (b.type() == block_type_t::subst) {
+            return true;
+        } else if (b.type() == block_type_t::source) {
+            // If a function sources a file, don't descend further.
+            break;
+        }
+    }
+    return false;
+}
+
 maybe_t<wcstring> parser_t::get_function_name(int level) {
     if (level == 0) {
         // Return the function name for the level preceding the most recent breakpoint. If there
diff --git a/src/parser.h b/src/parser.h
index f92259465..4b73884b6 100644
--- a/src/parser.h
+++ b/src/parser.h
@@ -279,6 +279,9 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
     /// \return whether we are currently evaluating a function.
     bool is_function() const;
 
+    /// \return whether we are currently evaluating a command substitution.
+    bool is_command_substitution() const;
+
     /// Create a parser.
     parser_t();
     parser_t(std::shared_ptr<env_stack_t> vars);
diff --git a/tests/checks/sigint2.fish b/tests/checks/sigint2.fish
index e925f2d7b..0160cf3b5 100644
--- a/tests/checks/sigint2.fish
+++ b/tests/checks/sigint2.fish
@@ -2,6 +2,21 @@
 # This hangs on OpenBSD
 #REQUIRES: test "$(uname)" != OpenBSD
 
+# Command subs run in same pgroup as fish, even if job control is 'all'.
+# Verify that they get the same pgroup across runs (presumably fish's).
+status job-control full
+set g1 ($helper print_pgrp)
+for i in (seq 10)
+    if test $g1 -ne ($helper print_pgrp)
+        echo "Unexpected pgroup"
+    end
+end
+status job-control interactive
+
+echo "Finished testing pgroups"
+#CHECK: Finished testing pgroups
+
+
 # Ensure that if child processes SIGINT, we exit our loops
 # Test for #3780
 
diff --git a/tests/pexpects/signals.py b/tests/pexpects/signals.py
index a76a95e25..35bc9a64c 100644
--- a/tests/pexpects/signals.py
+++ b/tests/pexpects/signals.py
@@ -19,6 +19,13 @@ import sys
 
 expect_prompt()
 
+# Verify that SIGINT inside a command sub cancels it.
+# Negate the pid to send to the pgroup (which should include sleep).
+sendline("while true; echo (sleep 1000); end")
+sleep(0.5)
+os.kill(-sp.spawn.pid, signal.SIGINT)
+expect_prompt()
+
 sendline("sleep 10 &")
 expect_prompt()