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 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 { /// \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 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()