From 2d3c914a9db01a2646abf2fbd8a42d5025fde198 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 26 Apr 2020 12:02:50 -0700 Subject: [PATCH] Thread pgroups into command substitutions Give string expansion an (optional) parent pgroup. This is threaded all the way into eval(). This ensures that in a mixed pipeline like: cmd | begin ; something (cmd2) ; end that cmd2 and cmd have the same pgroup. Add a test to ensure that command substitutions inherit pgroups properly. cherry-pick of 938b6838951e24dff631a9dce8c5131ef5940f4b Fixes #6624 --- src/exec.cpp | 13 +++++++------ src/exec.h | 4 +++- src/expand.cpp | 17 +++++++++-------- src/operation_context.h | 5 +++++ src/parser.cpp | 10 +++++++--- src/parser.h | 7 +++++-- tests/checks/pipeline-pgroup.fish | 6 ++++-- 7 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 304b5a287..ee7568543 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -1121,8 +1121,8 @@ bool exec_job(parser_t &parser, const shared_ptr &j, const job_lineage_t return true; } -static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstring_list_t *lst, - bool apply_exit_status, bool is_subcmd) { +static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, maybe_t parent_pgid, + wcstring_list_t *lst, bool apply_exit_status, bool is_subcmd) { ASSERT_IS_MAIN_THREAD(); auto &ld = parser.libdata(); bool prev_subshell = ld.is_subshell; @@ -1144,7 +1144,8 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstrin // be null. std::shared_ptr buffer; if (auto bufferfill = io_bufferfill_t::create(fd_set_t{}, ld.read_limit)) { - if (parser.eval(cmd, io_chain_t{bufferfill}, block_type_t::subst) == eval_result_t::ok) { + if (parser.eval(cmd, io_chain_t{bufferfill}, block_type_t::subst, parent_pgid) == + eval_result_t::ok) { subcommand_statuses = parser.get_last_statuses(); } buffer = io_bufferfill_t::finish(std::move(bufferfill)); @@ -1215,12 +1216,12 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstrin } int exec_subshell(const wcstring &cmd, parser_t &parser, wcstring_list_t &outputs, - bool apply_exit_status, bool is_subcmd) { + bool apply_exit_status, bool is_subcmd, maybe_t parent_pgid) { ASSERT_IS_MAIN_THREAD(); - return exec_subshell_internal(cmd, parser, &outputs, apply_exit_status, is_subcmd); + return exec_subshell_internal(cmd, parser, parent_pgid, &outputs, apply_exit_status, is_subcmd); } int exec_subshell(const wcstring &cmd, parser_t &parser, bool apply_exit_status, bool is_subcmd) { ASSERT_IS_MAIN_THREAD(); - return exec_subshell_internal(cmd, parser, nullptr, apply_exit_status, is_subcmd); + return exec_subshell_internal(cmd, parser, none(), nullptr, apply_exit_status, is_subcmd); } diff --git a/src/exec.h b/src/exec.h index af3f1481b..6d2e92a8b 100644 --- a/src/exec.h +++ b/src/exec.h @@ -22,10 +22,12 @@ bool exec_job(parser_t &parser, const std::shared_ptr &j, const job_linea /// /// \param cmd the command to execute /// \param outputs The list to insert output into. +/// \param parent_pgid if set, the pgid for any spawned jobs /// /// \return the status of the last job to exit, or -1 if en error was encountered. int exec_subshell(const wcstring &cmd, parser_t &parser, wcstring_list_t &outputs, - bool apply_exit_status, bool is_subcmd = false); + bool apply_exit_status, bool is_subcmd = false, + maybe_t parent_pgid = none()); int exec_subshell(const wcstring &cmd, parser_t &parser, bool apply_exit_status, bool is_subcmd = false); diff --git a/src/expand.cpp b/src/expand.cpp index 48bd4f72b..43b71caf5 100644 --- a/src/expand.cpp +++ b/src/expand.cpp @@ -577,8 +577,9 @@ static expand_result_t expand_braces(const wcstring &instr, expand_flags_t flags } /// Perform cmdsubst expansion. -static bool expand_cmdsubst(wcstring input, parser_t &parser, completion_list_t *out_list, - parse_error_list_t *errors) { +static bool expand_cmdsubst(wcstring input, const operation_context_t &ctx, + completion_list_t *out_list, parse_error_list_t *errors) { + assert(ctx.parser && "Cannot expand without a parser"); wchar_t *paren_begin = nullptr, *paren_end = nullptr; wchar_t *tail_begin = nullptr; size_t i, j; @@ -605,14 +606,14 @@ static bool expand_cmdsubst(wcstring input, parser_t &parser, completion_list_t wcstring_list_t sub_res; const wcstring subcmd(paren_begin + 1, paren_end - paren_begin - 1); - if (exec_subshell(subcmd, parser, sub_res, true /* apply_exit_status */, - true /* is_subcmd */) == -1) { + if (exec_subshell(subcmd, *ctx.parser, sub_res, true /* apply_exit_status */, + true /* is_subcmd */, ctx.parent_pgid) == -1) { append_cmdsub_error(errors, SOURCE_LOCATION_UNKNOWN, L"Unknown error while evaluating command substitution"); return false; } - if (parser.get_last_status() == STATUS_READ_TOO_MUCH) { + if (ctx.parser->get_last_status() == STATUS_READ_TOO_MUCH) { append_cmdsub_error( errors, in - paren_begin, _(L"Too much data emitted by command substitution so it was discarded\n")); @@ -652,7 +653,7 @@ static bool expand_cmdsubst(wcstring input, parser_t &parser, completion_list_t // Recursively call ourselves to expand any remaining command substitutions. The result of this // recursive call using the tail of the string is inserted into the tail_expand array list completion_list_t tail_expand; - expand_cmdsubst(tail_begin, parser, &tail_expand, errors); // TODO: offset error locations + expand_cmdsubst(tail_begin, ctx, &tail_expand, errors); // TODO: offset error locations // Combine the result of the current command substitution with the result of the recursive tail // expansion. @@ -686,7 +687,7 @@ static bool expand_cmdsubst(wcstring input, parser_t &parser, completion_list_t } } - return parser.get_last_status() != STATUS_READ_TOO_MUCH; + return ctx.parser->get_last_status() != STATUS_READ_TOO_MUCH; } // Given that input[0] is HOME_DIRECTORY or tilde (ugh), return the user's name. Return the empty @@ -897,7 +898,7 @@ expand_result_t expander_t::stage_cmdsubst(wcstring input, completion_list_t *ou } } else { assert(ctx.parser && "Must have a parser to expand command substitutions"); - bool cmdsubst_ok = expand_cmdsubst(std::move(input), *ctx.parser, out, errors); + bool cmdsubst_ok = expand_cmdsubst(std::move(input), ctx, out, errors); if (!cmdsubst_ok) return expand_result_t::error; } diff --git a/src/operation_context.h b/src/operation_context.h index a230596a8..6eb79831e 100644 --- a/src/operation_context.h +++ b/src/operation_context.h @@ -29,6 +29,11 @@ class operation_context_t { // context itself. const environment_t &vars; + /// The pgid of the parental job. + /// This is used only when expanding command substitutions. If this is set, any jobs created by + /// the command substitions should use this pgid. + maybe_t parent_pgid{}; + // A function which may be used to poll for cancellation. cancel_checker_t cancel_checker; diff --git a/src/parser.cpp b/src/parser.cpp index 9e7cfa6de..041f3e84d 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -634,11 +634,11 @@ profile_item_t *parser_t::create_profile_item() { } eval_result_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, - enum block_type_t block_type) { + enum block_type_t block_type, maybe_t parent_pgid) { // Parse the source into a tree, if we can. parse_error_list_t error_list; if (parsed_source_ref_t ps = parse_source(cmd, parse_flag_none, &error_list)) { - return this->eval(ps, io, block_type); + return this->eval(ps, io, block_type, parent_pgid); } else { // Get a backtrace. This includes the message. wcstring backtrace_and_desc; @@ -651,11 +651,12 @@ eval_result_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, } eval_result_t parser_t::eval(const parsed_source_ref_t &ps, const io_chain_t &io, - enum block_type_t block_type) { + enum block_type_t block_type, maybe_t parent_pgid) { assert(block_type == block_type_t::top || block_type == block_type_t::subst); if (!ps->tree.empty()) { job_lineage_t lineage; lineage.block_io = io; + lineage.parent_pgid = parent_pgid; // Execute the first node. tnode_t start{&ps->tree, &ps->tree.front()}; return this->eval_node(ps, start, std::move(lineage), block_type); @@ -689,6 +690,9 @@ eval_result_t parser_t::eval_node(const parsed_source_ref_t &ps, tnode_t node operation_context_t op_ctx = this->context(); block_t *scope_block = this->push_block(block_t::scope_block(block_type)); + // Propogate any parent pgid. + op_ctx.parent_pgid = lineage.parent_pgid; + // Create and set a new execution context. using exc_ctx_ref_t = std::unique_ptr; scoped_push exc(&execution_context, make_unique( diff --git a/src/parser.h b/src/parser.h index dd10c9a01..464503e1a 100644 --- a/src/parser.h +++ b/src/parser.h @@ -266,15 +266,18 @@ class parser_t : public std::enable_shared_from_this { /// \param io io redirections to perform on all started jobs /// \param block_type The type of block to push on the block stack, which must be either 'top' /// or 'subst'. + /// \param parent_pgid if set, the pgid to give to spawned jobs /// /// \return the eval result, eval_result_t eval(const wcstring &cmd, const io_chain_t &io, - block_type_t block_type = block_type_t::top); + block_type_t block_type = block_type_t::top, + maybe_t parent_pgid = {}); /// Evaluate the parsed source ps. /// Because the source has been parsed, a syntax error is impossible. eval_result_t eval(const parsed_source_ref_t &ps, const io_chain_t &io, - block_type_t block_type = block_type_t::top); + block_type_t block_type = block_type_t::top, + maybe_t parent_pgid = {}); /// Evaluates a node. /// The node type must be grammar::statement or grammar::job_list. diff --git a/tests/checks/pipeline-pgroup.fish b/tests/checks/pipeline-pgroup.fish index a604b3bd8..3b59ce702 100644 --- a/tests/checks/pipeline-pgroup.fish +++ b/tests/checks/pipeline-pgroup.fish @@ -9,11 +9,13 @@ end # Here everything should live in the pgroup of the first fish_test_helper. $fth print_pgrp | read -g global_group | save_pgroup g1 | begin save_pgroup g2 +end | begin + echo (save_pgroup g3) >/dev/null end -[ "$global_group" -eq "$g1" ] && [ "$g1" -eq "$g2" ] +[ "$global_group" -eq "$g1" ] && [ "$g1" -eq "$g2" ] && [ "$g2" -eq "$g3" ] and echo "All pgroups agreed" -or echo "Pgroups disagreed. Should be in $global_group but found $g1 and $g2" +or echo "Pgroups disagreed. Should be in $global_group but found $g1, $g2, $g3" # CHECK: All pgroups agreed # Here everything should live in fish's pgroup.