From a2b2bcef6e8212ee1cd3ecb99a74e19f93c6feb8 Mon Sep 17 00:00:00 2001 From: Soumya Date: Sun, 2 Aug 2020 13:37:19 -0700 Subject: [PATCH] Add a `$status_generation` variable that's incremented for each interactive command that produces a status. This can be used to determine whether the previous command produced a real status, or just carried over the status from the command before it. Backgrounded commands and variable assignments will not increment status_generation, all other commands will. --- CHANGELOG.rst | 1 + doc_src/index.rst | 2 + src/env.cpp | 4 ++ src/parse_execution.cpp | 1 + src/parser.cpp | 8 +++- src/parser.h | 10 +++- src/proc.cpp | 1 + src/reader.cpp | 18 +++++-- src/reader.h | 4 ++ tests/pexpects/postexec.py | 98 ++++++++++++++++++++++++++++++++++++++ 10 files changed, 140 insertions(+), 7 deletions(-) create mode 100644 tests/pexpects/postexec.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a6ac134d4..8a7f81a28 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -70,6 +70,7 @@ Interactive improvements - Control-Z is now available for binding (#7152). - ``fish_key_reader`` sets the exit status to 0 when used with ``--help`` or ``--version`` (#6964). - ``fish_key_reader`` and ``fish_indent`` send output from ``--version`` to standard output, matching other fish binaries (#6964). +- A new variable ``$status_generation`` is incremented only when the previous command produces a status. This can be used, for example, to check whether a failure status is a holdover due to a background job, or actually produced by the last run command. New or improved bindings diff --git a/doc_src/index.rst b/doc_src/index.rst index e08ff042d..769995769 100644 --- a/doc_src/index.rst +++ b/doc_src/index.rst @@ -1225,6 +1225,8 @@ You can change the settings of ``fish`` by changing the values of certain variab - ``status``, the `exit status <#variables-status>`_ of the last foreground job to exit. If the job was terminated through a signal, the exit status will be 128 plus the signal number. +- ``status_generation``, the "generation" count of ``$status``. This will be incremented only when the previous command produced an explicit status. (For example, background jobs will not increment this). + - ``USER``, the current username. This variable can be changed by the user. - ``version``, the version of the currently running fish (also available as ``FISH_VERSION`` for backward compatibility). diff --git a/src/env.cpp b/src/env.cpp index ba85aaada..15bc174b1 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -95,6 +95,7 @@ static const std::vector electric_variables{ {L"hostname", electric_var_t::freadonly}, {L"pipestatus", electric_var_t::freadonly | electric_var_t::fcomputed}, {L"status", electric_var_t::freadonly | electric_var_t::fcomputed}, + {L"status_generation", electric_var_t::freadonly | electric_var_t::fcomputed}, {L"umask", electric_var_t::fcomputed}, {L"version", electric_var_t::freadonly}, }; @@ -701,6 +702,9 @@ maybe_t env_scoped_impl_t::try_get_computed(const wcstring &key) cons } else if (key == L"status") { const auto &js = perproc_data().statuses; return env_var_t(L"status", to_string(js.status)); + } else if (key == L"status_generation") { + auto status_generation = reader_status_count(); + return env_var_t(L"status_generation", to_string(status_generation)); } else if (key == L"fish_kill_signal") { const auto &js = perproc_data().statuses; return env_var_t(L"fish_kill_signal", to_string(js.kill_signal)); diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 5e383eb8f..eee1fc14e 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -393,6 +393,7 @@ end_execution_reason_t parse_execution_context_t::run_function_statement( buffered_output_stream_t errs(0); io_streams_t streams(outs, errs); int err = builtin_function(*parser, streams, arguments, pstree, statement); + parser->libdata().status_count++; parser->set_last_statuses(statuses_t::just(err)); wcstring errtext = errs.contents(); diff --git a/src/parser.cpp b/src/parser.cpp index 506565623..183f0e353 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -649,7 +649,8 @@ eval_res_t parser_t::eval(const parsed_source_ref_t &ps, const io_chain_t &io, auto status = proc_status_t::from_exit_code(get_last_status()); bool break_expand = false; bool was_empty = true; - return eval_res_t{status, break_expand, was_empty}; + bool no_status = true; + return eval_res_t{status, break_expand, was_empty, no_status}; } } @@ -713,8 +714,10 @@ eval_res_t parser_t::eval_node(const parsed_source_ref_t &ps, const T &node, // Check the exec count so we know if anything got executed. const size_t prev_exec_count = libdata().exec_count; + const size_t prev_status_count = libdata().status_count; end_execution_reason_t reason = execution_context->eval_node(node, scope_block); const size_t new_exec_count = libdata().exec_count; + const size_t new_status_count = libdata().status_count; exc.restore(); this->pop_block(scope_block); @@ -728,7 +731,8 @@ eval_res_t parser_t::eval_node(const parsed_source_ref_t &ps, const T &node, auto status = proc_status_t::from_exit_code(this->get_last_status()); bool break_expand = (reason == end_execution_reason_t::error); bool was_empty = !break_expand && prev_exec_count == new_exec_count; - return eval_res_t{status, break_expand, was_empty}; + bool no_status = prev_status_count == new_status_count; + return eval_res_t{status, break_expand, was_empty, no_status}; } } diff --git a/src/parser.h b/src/parser.h index 356f2d177..28c4a96c5 100644 --- a/src/parser.h +++ b/src/parser.h @@ -142,6 +142,9 @@ struct library_data_t { /// A counter incremented every time a command executes. uint64_t exec_count{0}; + /// A counter incremented every time a command produces a $status. + uint64_t status_count{0}; + /// Last reader run count. uint64_t last_exec_run_counter{UINT64_MAX}; @@ -219,9 +222,12 @@ struct eval_res_t { /// If set, no commands were executed and there we no errors. bool was_empty{false}; + /// If set, no commands produced a $status value. + bool no_status{false}; + /* implicit */ eval_res_t(proc_status_t status, bool break_expand = false, - bool was_empty = false) - : status(status), break_expand(break_expand), was_empty(was_empty) {} + bool was_empty = false, bool no_status = false) + : status(status), break_expand(break_expand), was_empty(was_empty), no_status(no_status) {} }; class parser_t : public std::enable_shared_from_this { diff --git a/src/proc.cpp b/src/proc.cpp index b530bb57d..150f0dddd 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -998,6 +998,7 @@ void job_t::continue_job(parser_t &parser, bool in_foreground) { const auto &p = processes.back(); if (p->status.normal_exited() || p->status.signal_exited()) { parser.set_last_statuses(get_statuses()); + parser.libdata().status_count++; } } } diff --git a/src/reader.cpp b/src/reader.cpp index cbc92c538..50c2f3af3 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -2171,7 +2171,7 @@ void set_env_cmd_duration(struct timeval *after, struct timeval *before, env_sta vars.set_one(ENV_CMD_DURATION, ENV_UNEXPORT, std::to_wstring((secs * 1000) + (usecs / 1000))); } -void reader_run_command(parser_t &parser, const wcstring &cmd) { +eval_res_t reader_run_command(parser_t &parser, const wcstring &cmd) { struct timeval time_before, time_after; wcstring ft = tok_command(cmd); @@ -2185,7 +2185,7 @@ void reader_run_command(parser_t &parser, const wcstring &cmd) { gettimeofday(&time_before, nullptr); - parser.eval(cmd, io_chain_t{}); + auto eval_res = parser.eval(cmd, io_chain_t{}); job_reap(parser, true); gettimeofday(&time_after, nullptr); @@ -2202,6 +2202,8 @@ void reader_run_command(parser_t &parser, const wcstring &cmd) { if (have_proc_stat()) { proc_update_jiffies(parser); } + + return eval_res; } static parser_test_error_bits_t reader_shell_test(parser_t &parser, const wcstring &b) { @@ -2457,6 +2459,13 @@ static relaxed_atomic_t run_count{0}; /// Returns the current interactive loop count uint64_t reader_run_count() { return run_count; } +static relaxed_atomic_t status_count{0}; + +/// Returns the current "generation" of interactive status. +/// This is not incremented if the command being run produces no status, +/// (e.g. background job, or variable assignment). +uint64_t reader_status_count() { return status_count; } + /// Read interactively. Read input from stdin while providing editing facilities. static int read_i(parser_t &parser) { reader_config_t conf; @@ -2499,8 +2508,11 @@ static int read_i(parser_t &parser) { data->command_line_changed(&data->command_line); wcstring_list_t argv(1, command); event_fire_generic(parser, L"fish_preexec", &argv); - reader_run_command(parser, command); + auto eval_res = reader_run_command(parser, command); signal_clear_cancel(); + if (!eval_res.no_status) { + status_count++; + } event_fire_generic(parser, L"fish_postexec", &argv); // Allow any pending history items to be returned in the history array. if (data->history) { diff --git a/src/reader.h b/src/reader.h index be468084a..1307e1a16 100644 --- a/src/reader.h +++ b/src/reader.h @@ -278,4 +278,8 @@ wcstring completion_apply_to_command_line(const wcstring &val_str, complete_flag /// been executed between invocations of code. uint64_t reader_run_count(); +/// Returns the current "generation" of interactive status. Useful for determining whether the +/// previous command produced a status. +uint64_t reader_status_count(); + #endif diff --git a/tests/pexpects/postexec.py b/tests/pexpects/postexec.py new file mode 100644 index 000000000..7be94535b --- /dev/null +++ b/tests/pexpects/postexec.py @@ -0,0 +1,98 @@ +#!/usr/bin/env python3 +from pexpect_helper import SpawnedProc + +sp = SpawnedProc() +sendline, expect_prompt, expect_str = sp.sendline, sp.expect_prompt, sp.expect_str + +# Test fish_postexec and $status_generation for interactive shells. +expect_prompt() + +sendline("function test_fish_postexec --on-event fish_postexec; printf 'pipestatus:%s, generation:%d, command:%s\\n' (string join '|' $pipestatus) $status_generation $argv; end") +expect_prompt() + +generation = 1 + +# fish_postexec is called when foreground job ends. +sendline("true") +generation += 1 +expect_str("pipestatus:0, generation:%d, command:true" % generation) +expect_prompt() + +# Command has multiple jobs. +sendline("true;false") +generation += 1 +expect_str("pipestatus:1, generation:%d, command:true;false" % generation) +expect_prompt() + +# Command is a pipeline. +sendline("true|false") +generation += 1 +expect_str("pipestatus:0|1, generation:%d, command:true|false" % generation) +expect_prompt() + +# Does not increment $status_generation for background jobs. +# status, pipestatus remain unchanged +sendline("sleep 1000 &") +expect_str("pipestatus:0|1, generation:%d, command:sleep 1000 &" % generation) +expect_prompt() + +# multiple backgrounded jobs +sendline("sleep 1000 &; sleep 2000 &") +expect_str("pipestatus:0|1, generation:%d, command:sleep 1000 &; sleep 2000 &" % generation) +expect_prompt() + +# Increments $status_generation if any job was foreground. +sendline("false|true; sleep 1000 &") +generation += 1 +expect_str("pipestatus:1|0, generation:%d, command:false|true; sleep 1000 &" % generation) +expect_prompt() + +sendline("sleep 1000 &; true|false|true") +generation += 1 +expect_str("pipestatus:0|1|0, generation:%d, command:sleep 1000 &; true|false|true" % generation) +expect_prompt() + +# Increments $status_generation for empty if/while blocks. +sendline("if true;end") +generation += 1 +expect_str("pipestatus:0, generation:%d, command:if true;end" % generation) +expect_prompt() + +sendline("while false;end") +generation += 1 +expect_str("pipestatus:0, generation:%d, command:while false;end" % generation) +expect_prompt() + +# or non-matching if. +sendline("if false;false;end") +generation += 1 +expect_str("pipestatus:0, generation:%d, command:if false;false;end" % generation) +expect_prompt() + +# or a function definition. +# This is an implementation detail, but the test case should prevent regressions. +sendline("function fail; false; end") +generation += 1 +expect_str("pipestatus:0, generation:%d, command:function fail; false; end" % generation) +expect_prompt() + +# This is just to set a memorable pipestatus. +sendline("true|false|true") +generation += 1 +expect_str("pipestatus:0|1|0") +expect_prompt() + +# Does not increment $status_generation for empty begin/end block. +sendline("begin;end") +expect_str("pipestatus:0|1|0, generation:%d, command:begin;end" % generation) +expect_prompt() + +# Or begin/end block with only backgrounded jobs. +sendline("begin; sleep 200 &; sleep 400 &; end") +expect_str("pipestatus:0|1|0, generation:%d, command:begin; sleep 200 &; sleep 400 &; end" % generation) +expect_prompt() + +# Or a combination of begin/end block and backgrounded job. +sendline("begin; sleep 200 &; end; sleep 400 &") +expect_str("pipestatus:0|1|0, generation:%d, command:begin; sleep 200 &; end; sleep 400 &" % generation) +expect_prompt()