From ad8d68dd4390753901b5e1dae4b4c4b44be7fcea Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Thu, 31 Jan 2013 15:57:08 -0800 Subject: [PATCH] Make subcommands modify $status, and make builtin_set not modify status unless it fails https://github.com/fish-shell/fish-shell/issues/547 https://github.com/fish-shell/fish-shell/issues/214 --- autoload.cpp | 6 +-- builtin.cpp | 2 +- builtin_set.cpp | 72 +++++++++++------------------------- color.h | 2 +- common.h | 2 +- complete.cpp | 4 +- doc_src/fish_prompt.txt | 3 +- doc_src/index.hdr.in | 5 +-- doc_src/set.txt | 21 +++++++---- exec.cpp | 81 +++++++++++++++++++---------------------- exec.h | 4 +- expand.cpp | 2 +- kill.cpp | 4 +- parser.cpp | 13 ++++--- postfork.cpp | 2 +- proc.cpp | 2 +- reader.cpp | 18 +++++---- screen.cpp | 6 +-- tests/test4.in | 17 +++++++++ tests/test4.out | 14 +++++++ wildcard.cpp | 2 +- 21 files changed, 143 insertions(+), 139 deletions(-) diff --git a/autoload.cpp b/autoload.cpp index fbc917588..3470ecae6 100644 --- a/autoload.cpp +++ b/autoload.cpp @@ -358,11 +358,9 @@ bool autoload_t::locate_file_and_maybe_load_it(const wcstring &cmd, bool really_ /* If we have a script, either built-in or a file source, then run it */ if (really_load && has_script_source) { - if (exec_subshell(script_source) == -1) + if (exec_subshell(script_source, false /* do not apply exit status */) == -1) { - /* - Do nothing on failiure - */ + /* Do nothing on failure */ } } diff --git a/builtin.cpp b/builtin.cpp index d20dd27ea..646db2b04 100644 --- a/builtin.cpp +++ b/builtin.cpp @@ -215,7 +215,7 @@ wcstring builtin_help_get(parser_t &parser, const wchar_t *name) wcstring out; const wcstring name_esc = escape_string(name, 1); const wcstring cmd = format_string(L"__fish_print_help %ls", name_esc.c_str()); - if (exec_subshell(cmd, lst) >= 0) + if (exec_subshell(cmd, lst, false /* don't apply exit status */) >= 0) { for (size_t i=0; icommand substitution, and so the exit status of commands within fish_prompt will not modify the $status seen outside of fish_prompt. +The exit status of commands within \c fish_prompt will not modify the $status seen outside of fish_prompt. \subsection fish_prompt-example Example diff --git a/doc_src/index.hdr.in b/doc_src/index.hdr.in index 946bb73d1..fac56197a 100644 --- a/doc_src/index.hdr.in +++ b/doc_src/index.hdr.in @@ -578,9 +578,8 @@ this list is executed, and substituted by the output. If the output is more than one line long, each line will be expanded to a new parameter. -A command substitution will not change the value of the status variable outside of the command -substitution. +The exit status of the last run command substitution is available in the status variable. Only part of the output can be used, see index range expansion for details. diff --git a/doc_src/set.txt b/doc_src/set.txt index e2155fb7b..e46cd9c47 100644 --- a/doc_src/set.txt +++ b/doc_src/set.txt @@ -68,13 +68,14 @@ non-switch arguments. For example, set flags -l will have the effect of setting the value of the variable flags to '-l', not making the variable local. -In assignment mode, set exits with an exit status of zero it the -variable assignments where sucessfully performed, with a non-zero exit -status otherwise. In query mode, the exit status is the number of -variables that where not found. In erase mode, set exits with a zero -exit status in case of success, with a non-zero exit status if the -commandline was invalid, if the variable was write-protected or if the -variable did not exist. +In assignment mode, set exits with a non-zero exit status if variable +assignments could not be successfully performed. If the variable assignments +were performed, the exit status is unchanged. This allows simultaneous capture +of the output and exit status of a subcommand, e.g. if set output +(command). In query mode, the exit status is the number of variables that +were not found. In erase mode, set exits with a zero exit status in case of +success, with a non-zero exit status if the commandline was invalid, if the +variable was write-protected or if the variable did not exist. \subsection set-example Example @@ -85,3 +86,9 @@ variable did not exist. set -e smurf removes the variable \c smurf. set PATH[4] ~/bin changes the fourth element of the \c PATH array to \c ~/bin + +
if set python_path (which python)
+    echo "Python is at $python_path"
+end
+ +The above outputs the path to Python if \c which returns true. diff --git a/exec.cpp b/exec.cpp index abc5c0288..33e2daac8 100644 --- a/exec.cpp +++ b/exec.cpp @@ -125,7 +125,7 @@ void exec_close(int fd) int exec_pipe(int fd[2]) { ASSERT_IS_MAIN_THREAD(); - + int res; while ((res=pipe(fd))) { @@ -621,11 +621,11 @@ void exec(parser_t &parser, job_t *j) } } - + // This is a pipe that the "current" process in our loop below reads from // Only pipe_read->pipe_fd[0] is used shared_ptr pipe_read(new io_pipe_t(0, true)); - + // This is the pipe that the "current" process in our loop below writes to shared_ptr pipe_write(new io_pipe_t(1, false)); @@ -687,7 +687,7 @@ void exec(parser_t &parser, job_t *j) set_child_group(j, &keepalive, 0); } } - + /* This loop loops over every process_t in the job, starting it as appropriate. This turns out to be rather complex, since a @@ -695,17 +695,17 @@ void exec(parser_t &parser, job_t *j) The loop also has to handle pipelining between the jobs. */ - + /* We can have up to three pipes "in flight" at a time: - + 1. The pipe the current process should read from (courtesy of the previous process) 2. The pipe that the current process should write to 3. The pipe that the next process should read from (courtesy of us) - + We are careful to set these to -1 when closed, so if we exit the loop abruptly, we can still close them. - + */ - + int pipe_current_read = -1, pipe_current_write = -1, pipe_next_read = -1; for (process_t *p=j->first_process; p; p = p->next) { @@ -713,10 +713,10 @@ void exec(parser_t &parser, job_t *j) assert(pipe_current_read == -1); pipe_current_read = pipe_next_read; pipe_next_read = -1; - + /* Record the current read in pipe_read */ pipe_read->pipe_fd[0] = pipe_current_read; - + /* See if we need a pipe */ const bool pipes_to_next_command = (p->next != NULL); @@ -760,15 +760,15 @@ void exec(parser_t &parser, job_t *j) job_mark_process_as_failed(j, p); break; } - + // This tells the redirection about the fds, but the redirection does not close them memcpy(pipe_write->pipe_fd, local_pipe, sizeof(int)*2); - + // Record our pipes // The fds should be negative to indicate that we aren't overwriting an fd we failed to close assert(pipe_current_write == -1); pipe_current_write = local_pipe[1]; - + assert(pipe_next_read == -1); pipe_next_read = local_pipe[0]; } @@ -838,7 +838,7 @@ void exec(parser_t &parser, job_t *j) j->io.push_back(io_buffer); } } - + if (! exec_error) { internal_exec_helper(parser, def.c_str(), TOP, j->io); @@ -865,7 +865,7 @@ void exec(parser_t &parser, job_t *j) j->io.push_back(io_buffer); } } - + if (! exec_error) { internal_exec_helper(parser, p->argv0(), TOP, j->io); @@ -1389,7 +1389,7 @@ void exec(parser_t &parser, job_t *j) exec_close(pipe_current_read); pipe_current_read = -1; } - + /* Close the write end too, since the curent child subprocess already has a copy of it. */ if (pipe_current_write >= 0) { @@ -1397,7 +1397,7 @@ void exec(parser_t &parser, job_t *j) pipe_current_write = -1; } } - + /* Clean up any file descriptors we left open */ if (pipe_current_read >= 0) exec_close(pipe_current_read); @@ -1405,7 +1405,7 @@ void exec(parser_t &parser, job_t *j) exec_close(pipe_current_write); if (pipe_next_read >= 0) exec_close(pipe_next_read); - + /* The keepalive process is no longer needed, so we terminate it with extreme prejudice */ if (needs_keepalive) { @@ -1438,11 +1438,11 @@ void exec(parser_t &parser, job_t *j) } -static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst) +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; - int status, prev_status; + const int prev_status = proc_get_last_status(); char sep=0; const env_var_t ifs = env_get_string(L"IFS"); @@ -1456,38 +1456,33 @@ static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst) else { sep = 0; - debug(0, L"Warning - invalid command substitution separator '%lc'. Please change the firsta character of IFS", ifs[0]); + debug(0, L"Warning - invalid command substitution separator '%lc'. Please change the first character of IFS", ifs[0]); } } is_subshell=1; - prev_status = proc_get_last_status(); - const shared_ptr io_buffer(io_buffer_t::create(0)); - + 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 be null - if (io_buffer.get() == NULL) - { - status = -1; - } - else + const shared_ptr io_buffer(io_buffer_t::create(0)); + if (io_buffer.get() != NULL) { parser_t &parser = parser_t::principal_parser(); - if (parser.eval(cmd, io_chain_t(io_buffer), SUBST)) + if (parser.eval(cmd, io_chain_t(io_buffer), SUBST) == 0) { - status = -1; + subcommand_status = proc_get_last_status(); } - else - { - status = proc_get_last_status(); - } - + io_buffer->read(); } - proc_set_last_status(prev_status); + // If the caller asked us to preserve the exit status, restore the old status + // Otherwise set the status of the subcommand + proc_set_last_status(apply_exit_status ? subcommand_status : prev_status); + is_subshell = prev_subshell; @@ -1515,17 +1510,17 @@ static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst) } } - return status; + return subcommand_status; } -int exec_subshell(const wcstring &cmd, std::vector &outputs) +int exec_subshell(const wcstring &cmd, std::vector &outputs, bool apply_exit_status) { ASSERT_IS_MAIN_THREAD(); - return exec_subshell_internal(cmd, &outputs); + return exec_subshell_internal(cmd, &outputs, apply_exit_status); } -__warn_unused int exec_subshell(const wcstring &cmd) +__warn_unused int exec_subshell(const wcstring &cmd, bool apply_exit_status) { ASSERT_IS_MAIN_THREAD(); - return exec_subshell_internal(cmd, NULL); + return exec_subshell_internal(cmd, NULL, apply_exit_status); } diff --git a/exec.h b/exec.h index 5aa788c96..9a9d2dc53 100644 --- a/exec.h +++ b/exec.h @@ -54,8 +54,8 @@ void exec(parser_t &parser, job_t *j); \return the status of the last job to exit, or -1 if en error was encountered. */ -__warn_unused int exec_subshell(const wcstring &cmd, std::vector &outputs); -__warn_unused int exec_subshell(const wcstring &cmd); +__warn_unused int exec_subshell(const wcstring &cmd, std::vector &outputs, bool preserve_exit_status); +__warn_unused int exec_subshell(const wcstring &cmd, bool preserve_exit_status); /** diff --git a/expand.cpp b/expand.cpp index 4bdb563bb..a653adf4c 100644 --- a/expand.cpp +++ b/expand.cpp @@ -1352,7 +1352,7 @@ static int expand_cmdsubst(parser_t &parser, const wcstring &input, std::vector< const wcstring subcmd(paran_begin + 1, paran_end-paran_begin - 1); - if (exec_subshell(subcmd, sub_res) == -1) + if (exec_subshell(subcmd, sub_res, true /* do apply exit status */) == -1) { parser.error(CMDSUBST_ERROR, -1, L"Unknown error while evaulating command substitution"); return 0; diff --git a/kill.cpp b/kill.cpp index 846783e58..2f6790d87 100644 --- a/kill.cpp +++ b/kill.cpp @@ -112,7 +112,7 @@ void kill_add(const wcstring &str) if (! cmd.empty()) { - if (exec_subshell(cmd) == -1) + if (exec_subshell(cmd, false /* do not apply exit status */) == -1) { /* Do nothing on failiure @@ -175,7 +175,7 @@ static void kill_check_x_buffer() wcstring cmd = L"xsel -t 500 -b"; wcstring new_cut_buffer=L""; wcstring_list_t list; - if (exec_subshell(cmd, list) != -1) + if (exec_subshell(cmd, list, false /* do not apply exit status */) != -1) { for (i=0; i block_infos; int indentation_sum = 0; //sum of indentation in block_infos @@ -3040,7 +3041,7 @@ int parser_t::test(const wchar_t *buff, int *block_level, wcstring *out, const w { tok_next(&tok); tok_set_pos(&tok, mark); - + /* Test that end is not used when not inside any block */ if (block_infos.empty()) { @@ -3060,7 +3061,7 @@ int parser_t::test(const wchar_t *buff, int *block_level, wcstring *out, const w { indentation_sum -= block_infos.back().indentation; block_infos.pop_back(); - + } } diff --git a/postfork.cpp b/postfork.cpp index 6232aa219..a1d1a2527 100644 --- a/postfork.cpp +++ b/postfork.cpp @@ -10,7 +10,7 @@ #include "exec.h" #ifndef JOIN_THREADS_BEFORE_FORK - #define JOIN_THREADS_BEFORE_FORK 0 +#define JOIN_THREADS_BEFORE_FORK 0 #endif /** The number of times to try to call fork() before giving up */ diff --git a/proc.cpp b/proc.cpp index 51f34d31d..fdaaed8ca 100644 --- a/proc.cpp +++ b/proc.cpp @@ -328,7 +328,7 @@ void job_set_flag(job_t *j, unsigned int flag, int set) int job_get_flag(const job_t *j, unsigned int flag) { - return !! (j->flags & flag); + return !!(j->flags & flag); } int job_signal(job_t *j, int signal) diff --git a/reader.cpp b/reader.cpp index d0312c2d3..b405bd9b6 100644 --- a/reader.cpp +++ b/reader.cpp @@ -692,7 +692,7 @@ void reader_write_title() wcstring_list_t lst; proc_push_interactive(0); - if (exec_subshell(title, lst) != -1) + if (exec_subshell(title, lst, false /* do not apply exit status */) != -1) { size_t i; if (lst.size() > 0) @@ -718,6 +718,9 @@ static void exec_prompt() data->left_prompt_buff.clear(); data->right_prompt_buff.clear(); + /* Do not allow the exit status of the prompts to leak through */ + const bool apply_exit_status = false; + /* If we have any prompts, they must be run non-interactively */ if (data->left_prompt.size() || data->right_prompt.size()) { @@ -726,9 +729,9 @@ static void exec_prompt() if (! data->left_prompt.empty()) { wcstring_list_t prompt_list; - // status is ignored - if (exec_subshell(data->left_prompt, prompt_list)) + if (exec_subshell(data->left_prompt, prompt_list, apply_exit_status)) { + // returned status value is ignored } for (size_t i = 0; i < prompt_list.size(); i++) { @@ -741,8 +744,9 @@ static void exec_prompt() { wcstring_list_t prompt_list; // status is ignored - if (exec_subshell(data->right_prompt, prompt_list)) + if (exec_subshell(data->right_prompt, prompt_list, apply_exit_status)) { + // returned status value is ignored } for (size_t i = 0; i < prompt_list.size(); i++) { @@ -1048,10 +1052,10 @@ static void run_pager(const wcstring &prefix, int is_quoted, const std::vector in(io_buffer_t::create(true)); shared_ptr out(io_buffer_t::create(false)); - + // The above may fail e.g. if we have too many open fds if (in.get() == NULL || out.get() == NULL) return; @@ -1140,7 +1144,7 @@ static void run_pager(const wcstring &prefix, int is_quoted, const std::vectorfd = 4; - + term_donate(); parser_t &parser = parser_t::principal_parser(); io_chain_t io_chain; diff --git a/screen.cpp b/screen.cpp index fa3b3e59d..7282afbf3 100644 --- a/screen.cpp +++ b/screen.cpp @@ -1373,20 +1373,20 @@ void s_reset(screen_t *s, screen_reset_mode_t mode) } else { - // draw in "bright black" (gray) + // draw in "bright black" (gray) abandon_line_string.append(L"\x1b[0m" //bright L"\x1b[30;1m"); //black } abandon_line_string.push_back(omitted_newline_char); abandon_line_string.append(L"\x1b[0m"); //normal text ANSI escape sequence abandon_line_string.append(screen_width - non_space_width, L' '); - + } abandon_line_string.push_back(L'\r'); // now we are certainly on a new line. But we may have dropped the omitted newline char on it. So append enough spaces to overwrite the omitted newline char, and then abandon_line_string.append(non_space_width, L' '); abandon_line_string.push_back(L'\r'); - + const std::string narrow_abandon_line_string = wcs2string(abandon_line_string); write_loop(STDOUT_FILENO, narrow_abandon_line_string.c_str(), narrow_abandon_line_string.size()); s->actual.cursor.x = 0; diff --git a/tests/test4.in b/tests/test4.in index 5d94ba4a8..f207e6b79 100644 --- a/tests/test4.in +++ b/tests/test4.in @@ -160,3 +160,20 @@ else end; set -U -e baz + +echo "Verify subcommand statuses" +echo (false) $status (true) $status (false) $status + +echo "Verify that set passes through exit status, except when passed -n or -q or -e" +false ; set foo bar ; echo 1 $status # passthrough +true ; set foo bar ; echo 2 $status # passthrough +false ; set -q foo ; echo 3 $status # no passthrough +true ; set -q foo ; echo 4 $status # no passthrough +false ; set -n > /dev/null ; echo 5 $status # no passthrough +false ; set -e foo ; echo 6 $status # no passthrough +true ; set -e foo ; echo 7 $status # no passthrough +false ; set -h > /dev/null ; echo 8 $status # no passthrough +true ; set -NOT_AN_OPTION 2> /dev/null ; echo 9 $status # no passthrough +false ; set foo (echo A; true) ; echo 10 $status $foo +true ; set foo (echo B; false) ; echo 11 $status $foo +true diff --git a/tests/test4.out b/tests/test4.out index 5a14f80a2..148d870c4 100644 --- a/tests/test4.out +++ b/tests/test4.out @@ -20,3 +20,17 @@ Test 19 pass Test 20 pass Test 21 pass Test 22 pass +Verify subcommand statuses +1 0 1 +Verify that set passes through exit status, except when passed -n or -q or -e +1 1 +2 0 +3 0 +4 0 +5 0 +6 0 +7 1 +8 0 +9 1 +10 0 A +11 1 B diff --git a/wildcard.cpp b/wildcard.cpp index e21eb9548..43b377437 100644 --- a/wildcard.cpp +++ b/wildcard.cpp @@ -336,7 +336,7 @@ static wcstring complete_get_desc_suffix_internal(const wcstring &suff) wcstring_list_t lst; wcstring desc; - if (exec_subshell(cmd, lst) != -1) + if (exec_subshell(cmd, lst, false /* do not apply exit status */) != -1) { if (lst.size()>0) {