From bd299e96b25c41eee3d54dad8727bf807bbf57aa Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Mon, 19 Jun 2017 21:05:34 -0700 Subject: [PATCH] implement `status is-breakpoint` This implements `status is-breakpoint` that returns true if the current shell prompt is displayed in the context of a `breakpoint` command. This also fixes several bugs. Most notably making `breakpoint` a no-op if the shell isn't interactive. Also, typing `breakpoint` at an interactive prompt should be an error rather than creating a new nested debugging context. Partial fix for #1310 --- CHANGELOG.md | 1 + doc_src/status.txt | 3 +++ share/completions/status.fish | 3 ++- src/builtin.cpp | 19 +++++++++++++++---- src/builtin.h | 4 ++++ src/builtin_status.cpp | 10 ++++++++++ src/builtin_string.cpp | 7 ++++--- src/event.cpp | 1 + src/exec.cpp | 4 ++-- src/fish.cpp | 4 ++-- src/fish_tests.cpp | 2 +- src/parser.cpp | 25 ++++++++++++++++++++----- src/proc.cpp | 11 ++++++----- src/proc.h | 22 +++++++++++++--------- tests/string.err | 2 +- 15 files changed, 85 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dcbba1381..6a21b1910 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Notable fixes and improvements - The `COLUMNS` and `LINES` env vars are now correctly set the first time `fish_prompt` is run (#4141). +- New `status is-breakpoint` command that is true when a prompt is displayed in response to a `breakpoint` command (#1310). ## Other significant changes diff --git a/doc_src/status.txt b/doc_src/status.txt index e9ab98b96..72054e9b3 100644 --- a/doc_src/status.txt +++ b/doc_src/status.txt @@ -6,6 +6,7 @@ status status is-login status is-interactive status is-block +status is-breakpoint status is-command-substitution status is-no-job-control status is-full-job-control @@ -27,6 +28,8 @@ The following operations (sub-commands) are available: - `is-block` returns 0 if fish is currently executing a block of code. Also `-b` or `--is-block`. +- `is-breakpoint` returns 0 if fish is currently showing a prompt in the context of a `breakpoint` command. See also the `fish_breakpoint_prompt` function. + - `is-interactive` returns 0 if fish is interactive - that is, connected to a keyboard. Also `-i` or `--is-interactive`. - `is-login` returns 0 if fish is a login shell - that is, if fish should perform login tasks such as setting up the PATH. Also `-l` or `--is-login`. diff --git a/share/completions/status.fish b/share/completions/status.fish index 0b85406b9..fbefb894c 100644 --- a/share/completions/status.fish +++ b/share/completions/status.fish @@ -1,5 +1,5 @@ # Note that when a completion file is sourced a new block scope is created so `set -l` works. -set -l __fish_status_all_commands is-login is-interactive is-block is-command-substitution is-no-job-control is-interactive-job-control is-full-job-control current-filename current-line-number print-stack-trace job-control +set -l __fish_status_all_commands is-login is-interactive is-block is-breakpoint is-command-substitution is-no-job-control is-interactive-job-control is-full-job-control current-filename current-line-number print-stack-trace job-control # These are the recognized flags. complete -c status -s h -l help --description "Display help and exit" @@ -9,6 +9,7 @@ complete -f -c status -n "not __fish_seen_subcommand_from $__fish_status_all_com complete -f -c status -n "not __fish_seen_subcommand_from $__fish_status_all_commands" -a is-interactive -d "Test if this is an interactive shell" complete -f -c status -n "not __fish_seen_subcommand_from $__fish_status_all_commands" -a is-command-substitution -d "Test if a command substitution is currently evaluated" complete -f -c status -n "not __fish_seen_subcommand_from $__fish_status_all_commands" -a is-block -d "Test if a code block is currently evaluated" +complete -f -c status -n "not __fish_seen_subcommand_from $__fish_status_all_commands" -a is-breakpoing -d "Test if a breakpoint is currently in effect" complete -f -c status -n "not __fish_seen_subcommand_from $__fish_status_all_commands" -a is-no-job-control -d "Test if new jobs are never put under job control" complete -f -c status -n "not __fish_seen_subcommand_from $__fish_status_all_commands" -a is-interactive-job-control -d "Test if only interactive new jobs are put under job control" complete -f -c status -n "not __fish_seen_subcommand_from $__fish_status_all_commands" -a is-full-job-control -d "Test if all new jobs are put under job control" diff --git a/src/builtin.cpp b/src/builtin.cpp index 90e9f887f..2a0bf67d5 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -347,17 +347,28 @@ static int builtin_break_continue(parser_t &parser, io_streams_t &streams, wchar /// Implementation of the builtin breakpoint command, used to launch the interactive debugger. static int builtin_breakpoint(parser_t &parser, io_streams_t &streams, wchar_t **argv) { + wchar_t *cmd = argv[0]; if (argv[1] != NULL) { - streams.err.append_format(BUILTIN_ERR_ARG_COUNT1, argv[0], 0, builtin_count_args(argv)); + streams.err.append_format(BUILTIN_ERR_ARG_COUNT1, cmd, 0, builtin_count_args(argv) - 1); return STATUS_INVALID_ARGS; } + // If we're not interactive then we can't enter the debugger. So treat this command as a no-op. + if (!shell_is_interactive()) { + return STATUS_CMD_ERROR; + } + + // Ensure we don't allow creating a breakpoint at an interactive prompt. There may be a simpler + // or clearer way to do this but this works. + const block_t *block1 = parser.block_at_index(1); + if (!block1 || block1->type() == BREAKPOINT) { + streams.err.append_format(_(L"%ls: Command not valid at an interactive prompt\n"), cmd); + return STATUS_ILLEGAL_CMD; + } + const breakpoint_block_t *bpb = parser.push_block(); - reader_read(STDIN_FILENO, streams.io_chain ? *streams.io_chain : io_chain_t()); - parser.pop_block(bpb); - return proc_get_last_status(); } diff --git a/src/builtin.h b/src/builtin.h index cf7c29ffa..34d904018 100644 --- a/src/builtin.h +++ b/src/builtin.h @@ -69,6 +69,10 @@ enum { COMMAND_NOT_BUILTIN, BUILTIN_REGULAR, BUILTIN_FUNCTION }; /// Error message when number expected #define BUILTIN_ERR_NOT_NUMBER _(L"%ls: Argument '%ls' is not a number\n") +/// Command that requires a subcommand was invoked without a recognized subcommand. +#define BUILTIN_ERR_MISSING_SUBCMD _(L"%ls: Expected a subcommand to follow the command\n") +#define BUILTIN_ERR_INVALID_SUBCMD _(L"%ls: Subcommand '%ls' is not valid\n") + /// The send stuff to foreground message. #define FG_MSG _(L"Send job %d, '%ls' to foreground\n") diff --git a/src/builtin_status.cpp b/src/builtin_status.cpp index ece5bbb3d..60993ad18 100644 --- a/src/builtin_status.cpp +++ b/src/builtin_status.cpp @@ -20,6 +20,7 @@ enum status_cmd_t { STATUS_IS_LOGIN = 1, STATUS_IS_INTERACTIVE, STATUS_IS_BLOCK, + STATUS_IS_BREAKPOINT, STATUS_IS_COMMAND_SUB, STATUS_IS_FULL_JOB_CTRL, STATUS_IS_INTERACTIVE_JOB_CTRL, @@ -38,6 +39,7 @@ const enum_map status_enum_map[] = { {STATUS_CURRENT_FUNCTION, L"current-function"}, {STATUS_CURRENT_LINE_NUMBER, L"current-line-number"}, {STATUS_IS_BLOCK, L"is-block"}, + {STATUS_IS_BREAKPOINT, L"is-breakpoint"}, {STATUS_IS_COMMAND_SUB, L"is-command-substitution"}, {STATUS_IS_FULL_JOB_CTRL, L"is-full-job-control"}, {STATUS_IS_INTERACTIVE, L"is-interactive"}, @@ -237,6 +239,9 @@ int builtin_status(parser_t &parser, io_streams_t &streams, wchar_t **argv) { return STATUS_CMD_ERROR; } optind++; + } else { + streams.err.append_format(BUILTIN_ERR_INVALID_SUBCMD, cmd, argv[1]); + return STATUS_INVALID_ARGS; } } @@ -315,6 +320,11 @@ int builtin_status(parser_t &parser, io_streams_t &streams, wchar_t **argv) { retval = !is_block; break; } + case STATUS_IS_BREAKPOINT: { + CHECK_FOR_UNEXPECTED_STATUS_ARGS(opts.status_cmd) + retval = !is_breakpoint; + break; + } case STATUS_IS_LOGIN: { CHECK_FOR_UNEXPECTED_STATUS_ARGS(opts.status_cmd) retval = !is_login; diff --git a/src/builtin_string.cpp b/src/builtin_string.cpp index 9ba7c75ad..d2f8a6c26 100644 --- a/src/builtin_string.cpp +++ b/src/builtin_string.cpp @@ -1208,9 +1208,10 @@ string_subcommands[] = { /// The string builtin, for manipulating strings. int builtin_string(parser_t &parser, io_streams_t &streams, wchar_t **argv) { + wchar_t *cmd = argv[0]; int argc = builtin_count_args(argv); if (argc <= 1) { - streams.err.append_format(_(L"string: Expected subcommand\n")); + streams.err.append_format(BUILTIN_ERR_MISSING_SUBCMD, cmd); builtin_print_help(parser, streams, L"string", streams.err); return STATUS_INVALID_ARGS; } @@ -1224,8 +1225,8 @@ int builtin_string(parser_t &parser, io_streams_t &streams, wchar_t **argv) { while (subcmd->name != 0 && wcscmp(subcmd->name, argv[1]) != 0) { subcmd++; } - if (subcmd->handler == 0) { - streams.err.append_format(_(L"string: Unknown subcommand '%ls'\n"), argv[1]); + if (!subcmd->handler) { + streams.err.append_format(BUILTIN_ERR_INVALID_SUBCMD, cmd, argv[1]); builtin_print_help(parser, streams, L"string", streams.err); return STATUS_INVALID_ARGS; } diff --git a/src/event.cpp b/src/event.cpp index eea1e2d4b..5b8ea051d 100644 --- a/src/event.cpp +++ b/src/event.cpp @@ -444,6 +444,7 @@ void event_fire(const event_t *event) { } } is_event--; + assert(is_event >= 0); } } diff --git a/src/exec.cpp b/src/exec.cpp index 35bfb91a6..4d1095813 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -1130,7 +1130,7 @@ void exec_job(parser_t &parser, job_t *j) { static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst, bool apply_exit_status) { ASSERT_IS_MAIN_THREAD(); - int prev_subshell = is_subshell; + bool prev_subshell = is_subshell; const int prev_status = proc_get_last_status(); bool split_output = false; @@ -1139,7 +1139,7 @@ static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst, split_output = true; } - is_subshell = 1; + is_subshell = true; int subcommand_status = -1; // assume the worst // IO buffer creation may fail (e.g. if we have too many open files to make a pipe), so this may diff --git a/src/fish.cpp b/src/fish.cpp index 230b0efd4..b5b06b657 100644 --- a/src/fish.cpp +++ b/src/fish.cpp @@ -255,11 +255,11 @@ static int fish_parse_opt(int argc, char **argv, std::vector *cmds) break; } case 'i': { - is_interactive_session = 1; + is_interactive_session = true; break; } case 'l': { - is_login = 1; + is_login = true; break; } case 'n': { diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 91e1f8a6d..b3e83c95c 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -805,7 +805,7 @@ static void test_cancellation() { // Test for #3780 // Ugly hack - temporarily set is_interactive_session // else we will SIGINT ourselves in response to our child death - scoped_push iis(&is_interactive_session, 1); + scoped_push iis(&is_interactive_session, true); const wchar_t *child_self_destructor = L"while true ; sh -c 'sleep .25; kill -s INT $$' ; end"; parser_t::principal_parser().eval(child_self_destructor, io_chain_t(), TOP); iis.restore(); diff --git a/src/parser.cpp b/src/parser.cpp index 61ef22676..9ce07ec93 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -148,9 +148,13 @@ void parser_t::push_block_int(block_t *new_current) { // Push it onto our stack. This acquires ownership because of unique_ptr. this->block_stack.emplace_back(new_current); - // Types TOP and SUBST are not considered blocks for the purposes of `status -b`. + // Types TOP and SUBST are not considered blocks for the purposes of `status is-block`. if (type != TOP && type != SUBST) { - is_block = 1; + is_block = true; + } + + if (type == BREAKPOINT) { + is_breakpoint = true; } if (new_current->type() != TOP) { @@ -174,16 +178,27 @@ void parser_t::pop_block(const block_t *expected) { if (old->wants_pop_env) env_pop(); - // Figure out if `status -b` should consider us to be in a block now. - int new_is_block = 0; + // Figure out if `status is-block` should consider us to be in a block now. + bool new_is_block = false; for (const auto &b : block_stack) { const enum block_type_t type = b->type(); if (type != TOP && type != SUBST) { - new_is_block = 1; + new_is_block = true; break; } } is_block = new_is_block; + + // Are we still in a breakpoint? + bool new_is_breakpoint = false; + for (const auto &b : block_stack) { + const enum block_type_t type = b->type(); + if (type == BREAKPOINT) { + new_is_breakpoint = true; + break; + } + } + is_breakpoint = new_is_breakpoint; } const wchar_t *parser_t::get_block_desc(int block) const { diff --git a/src/proc.cpp b/src/proc.cpp index 0864f1383..063f01feb 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -88,11 +88,12 @@ void print_jobs(void) } #endif -int is_interactive_session = 0; -int is_subshell = 0; -int is_block = 0; -int is_login = 0; -int is_event = 0; +bool is_interactive_session = false; +bool is_subshell = false; +bool is_block = false; +bool is_breakpoint = false; +bool is_login = false; +int is_event = false; pid_t proc_last_bg_pid = 0; int job_control_mode = JOB_CONTROL_INTERACTIVE; int no_exec = 0; diff --git a/src/proc.h b/src/proc.h index 0277fe548..874fb8230 100644 --- a/src/proc.h +++ b/src/proc.h @@ -217,22 +217,26 @@ class job_t { io_chain_t all_io_redirections() const; }; -/// Whether we are running a subshell command. -extern int is_subshell; - -/// Whether we are running a block of commands. -extern int is_block; - /// Whether we are reading from the keyboard right now. bool shell_is_interactive(void); +/// Whether we are running a subshell command. +extern bool is_subshell; + +/// Whether we are running a block of commands. +extern bool is_block; + +/// Whether we are running due to a `breakpoint` command. +extern bool is_breakpoint; + /// Whether this shell is attached to the keyboard at all. -extern int is_interactive_session; +extern bool is_interactive_session; /// Whether we are a login shell. -extern int is_login; +extern bool is_login; -/// Whether we are running an event handler. +/// Whether we are running an event handler. This is not a bool because we keep count of the event +/// nesting level. extern int is_event; // List of jobs. We sometimes mutate this while iterating - hence it must be a list, not a vector diff --git a/tests/string.err b/tests/string.err index a27fc87f4..33e9e847f 100644 --- a/tests/string.err +++ b/tests/string.err @@ -4,7 +4,7 @@ string match: [ string match: ^ # string invalidarg -string: Unknown subcommand 'invalidarg' +string: Subcommand 'invalidarg' is not valid Standard input (line 183): string invalidarg; and echo "unexpected exit 0" >&2 ^