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

@@ -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 */
         }
 
     }

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; i<lst.size(); i++)
         {

builtin_set.cpp

@@ -379,58 +379,21 @@ static void print_variables(int include_values, int esc, bool shorten_ok, int sc
 */
 static int builtin_set(parser_t &parser, wchar_t **argv)
 {
-
-    /**
-       Variables used for parsing the argument list
-    */
-    static const struct woption
-            long_options[] =
+    /** Variables used for parsing the argument list */
+    const struct woption long_options[] =
     {
-        {
-            L"export", no_argument, 0, 'x'
-        }
-        ,
-        {
-            L"global", no_argument, 0, 'g'
-        }
-        ,
-        {
-            L"local", no_argument, 0, 'l'
-        }
-        ,
-        {
-            L"erase", no_argument, 0, 'e'
-        }
-        ,
-        {
-            L"names", no_argument, 0, 'n'
-        }
-        ,
-        {
-            L"unexport", no_argument, 0, 'u'
-        }
-        ,
-        {
-            L"universal", no_argument, 0, 'U'
-        }
-        ,
-        {
-            L"long", no_argument, 0, 'L'
-        }
-        ,
-        {
-            L"query", no_argument, 0, 'q'
-        }
-        ,
-        {
-            L"help", no_argument, 0, 'h'
-        }
-        ,
-        {
-            0, 0, 0, 0
-        }
-    }
-    ;
+        { L"export", no_argument, 0, 'x' },
+        { L"global", no_argument, 0, 'g' },
+        { L"local", no_argument, 0, 'l' },
+        { L"erase", no_argument, 0, 'e' },
+        { L"names", no_argument, 0, 'n' },
+        { L"unexport", no_argument, 0, 'u' },
+        { L"universal", no_argument, 0, 'U' },
+        { L"long", no_argument, 0, 'L' },
+        { L"query", no_argument, 0, 'q' },
+        { L"help", no_argument, 0, 'h' },
+        { 0, 0, 0, 0 }
+    } ;
 
     const wchar_t *short_options = L"+xglenuULqh";
 
@@ -443,6 +406,8 @@ static int builtin_set(parser_t &parser, wchar_t **argv)
     int erase = 0, list = 0, unexport=0;
     int universal = 0, query=0;
     bool shorten_ok = true;
+    bool preserve_incoming_failure_exit_status = true;
+    const int incoming_exit_status = proc_get_last_status();
 
     /*
       Variables used for performing the actual work
@@ -474,10 +439,12 @@ static int builtin_set(parser_t &parser, wchar_t **argv)
 
             case 'e':
                 erase = 1;
+                preserve_incoming_failure_exit_status = false;
                 break;
 
             case 'n':
                 list = 1;
+                preserve_incoming_failure_exit_status = false;
                 break;
 
             case 'x':
@@ -506,6 +473,7 @@ static int builtin_set(parser_t &parser, wchar_t **argv)
 
             case 'q':
                 query = 1;
+                preserve_incoming_failure_exit_status = false;
                 break;
 
             case 'h':
@@ -825,6 +793,8 @@ static int builtin_set(parser_t &parser, wchar_t **argv)
 
     free(dest);
 
+    if (retcode == STATUS_BUILTIN_OK && preserve_incoming_failure_exit_status)
+        retcode = incoming_exit_status;
     return retcode;
 
 }

complete.cpp

@@ -465,7 +465,7 @@ bool completer_t::condition_test(const wcstring &condition)
     if (cached_entry == condition_cache.end())
     {
         /* Compute new value and reinsert it */
-        test_res = (0 == exec_subshell(condition));
+        test_res = (0 == exec_subshell(condition, false /* don't apply exit status */));
         condition_cache[condition] = test_res;
     }
     else
@@ -1007,7 +1007,7 @@ void completer_t::complete_cmd_desc(const wcstring &str)
       since apropos is only called once.
     */
     wcstring_list_t list;
-    if (exec_subshell(lookup_cmd, list) != -1)
+    if (exec_subshell(lookup_cmd, list, false /* don't apply exit status */) != -1)
     {
 
         /*

doc_src/fish_prompt.txt

@@ -11,8 +11,7 @@ By defining the \c fish_prompt function, the user can choose a custom
 prompt. The \c fish_prompt function is executed when the prompt is to
 be shown, and the output is used as a prompt.
 
-Please keep in mind that the function is executed by <a
-href='index.html#expand-command-substitution'>command substitution</a>, and so the exit status of commands within fish_prompt will not modify the <a href="index.html#variables-status">$status</a> seen outside of fish_prompt.
+The exit status of commands within \c fish_prompt will not modify the <a href="index.html#variables-status">$status</a> seen outside of fish_prompt.
 
 \subsection fish_prompt-example Example
 

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 <a
-href='#variables-status'>status</a> variable outside of the command
-substitution.
+The exit status of the last run command substitution is available in the <a
+href='#variables-status'>status</a> variable.
 
 Only part of the output can be used, see <a href='#expand-index-range'>index
 range expansion</a> for details.

doc_src/set.txt

@@ -68,13 +68,14 @@ non-switch arguments. For example, <code>set flags -l</code> will have
 the effect of setting the value of the variable <code>flags</code> 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. <code>if set output
+(command)</code>. 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.
 <code>set -e smurf</code> removes the variable \c smurf.
 
 <code>set PATH[4] ~/bin</code> changes the fourth element of the \c PATH array to \c ~/bin
+
+<pre>if set python_path (which python)
+    echo "Python is at $python_path"
+end</pre>
+
+The above outputs the path to Python if \c which returns true.

exec.cpp

@@ -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_t> 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_t> 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;
-        }
-        else
-        {
-            status = proc_get_last_status();
+            subcommand_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<wcstring> &outputs)
+int exec_subshell(const wcstring &cmd, std::vector<wcstring> &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);
 }

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<wcstring> &outputs);
-__warn_unused int exec_subshell(const wcstring &cmd);
+__warn_unused int exec_subshell(const wcstring &cmd, std::vector<wcstring> &outputs, bool preserve_exit_status);
+__warn_unused int exec_subshell(const wcstring &cmd, bool preserve_exit_status);
 
 
 /**

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;

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<list.size(); i++)

parser.cpp

@@ -2900,7 +2900,8 @@ int parser_t::test_args(const  wchar_t * buff, wcstring *out, const wchar_t *pre
 }
 
 // helper type used in parser::test below
-struct block_info_t {
+struct block_info_t
+{
     int position; //tokenizer position
     block_type_t type; //type of the block
     int indentation; //indentation associated with the block

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++)
             {

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

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

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)
         {