Rationalize path-getting

This cleans up the path_get_path function which is used to resolve a
command name against $PATH, by removing the dependence on errno and
being explicit about which error is returned.

Should be no user-visible change here.

src/builtins/command.cpp

@@ -102,9 +102,8 @@ maybe_t<int> builtin_command(parser_t &parser, io_streams_t &streams, const wcha
                 ++found;
             }
         } else {  // Either find_path explicitly or just quiet.
-            wcstring path;
+            if (auto path = path_get_path(command_name, parser.vars())) {
-            if (path_get_path(command_name, &path, parser.vars())) {
+                if (!opts.quiet) streams.out.append_format(L"%ls\n", path->c_str());
-                if (!opts.quiet) streams.out.append_format(L"%ls\n", path.c_str());
                 ++found;
             }
         }

src/complete.cpp

@@ -513,8 +513,9 @@ static completion_entry_t &complete_get_exact_entry(completion_entry_set_t &comp
 /// Find the full path and commandname from a command string 'str'.
 static void parse_cmd_string(const wcstring &str, wcstring *path, wcstring *cmd,
                              const environment_t &vars) {
-    bool found = path_get_path(str, path, vars);
+    auto path_result = path_try_get_path(str, vars);
-    // If the command was not found, 'path' is the empty string.
+    bool found = (path_result.err == 0);
+    *path = std::move(path_result.path);
     // Resolve commands that use relative paths because we compare full paths with "complete -p".
     if (found && !str.empty() && str.at(0) != L'/') {
         if (auto full_path = wrealpath(*path)) {
@@ -888,11 +889,8 @@ bool completer_t::complete_param_for_command(const wcstring &cmd_orig, const wcs
     // Only reload environment variables if builtin_exists returned false, as an optimization
     if (!head_exists) {
         head_exists = function_exists_no_autoload(cmd);
-        // While it may seem like first testing `path_get_path` before resorting to an env lookup
-        // may be faster, path_get_path can potentially do a lot of FS/IO access, so env.get() +
-        // function_exists() should still be faster.
         // Use cmd_orig here as it is potentially pathed.
-        head_exists = head_exists || path_get_path(cmd_orig, nullptr, ctx.vars);
+        head_exists = head_exists || path_get_path(cmd_orig, ctx.vars).has_value();
     }
 
     if (!head_exists) {

src/highlight.cpp

@@ -450,7 +450,7 @@ bool autosuggest_validate_from_history(const history_item_t &item,
 
     // Not handled specially. Is the command valid?
     bool cmd_ok = builtin_exists(parsed_command) || function_exists_no_autoload(parsed_command) ||
-                  path_get_path(parsed_command, nullptr, ctx.vars);
+                  path_get_path(parsed_command, ctx.vars).has_value();
     if (!cmd_ok) {
         return false;
     }
@@ -1318,7 +1318,7 @@ static bool command_is_valid(const wcstring &cmd, enum statement_decoration_t de
     if (!is_valid && abbreviation_ok) is_valid = expand_abbreviation(cmd, vars).has_value();
 
     // Regular commands
-    if (!is_valid && command_ok) is_valid = path_get_path(cmd, nullptr, vars);
+    if (!is_valid && command_ok) is_valid = path_get_path(cmd, vars).has_value();
 
     // Implicit cd
     if (!is_valid && implicit_cd_ok) {

src/parse_execution.cpp

@@ -735,8 +735,9 @@ end_execution_reason_t parse_execution_context_t::handle_command_not_found(
         // but this mainly applies to EACCES. We could also feasibly get:
         // ELOOP
         // ENAMETOOLONG
-        return this->report_error(STATUS_NOT_EXECUTABLE, statement,
+        return this->report_error(
-                                  _(L"Unknown command. '%ls' exists but is not an executable file."), cmd);
+            STATUS_NOT_EXECUTABLE, statement,
+            _(L"Unknown command. '%ls' exists but is not an executable file."), cmd);
     }
 
     // Handle unrecognized commands with standard command not found handler that can make better
@@ -852,14 +853,11 @@ end_execution_reason_t parse_execution_context_t::populate_plain_process(
     // Determine the process type.
     enum process_type_t process_type = process_type_for_command(statement, cmd);
 
-    wcstring path_to_external_command;
+    get_path_result_t external_cmd{};
     if (process_type == process_type_t::external || process_type == process_type_t::exec) {
         // Determine the actual command. This may be an implicit cd.
-        bool has_command = path_get_path(cmd, &path_to_external_command, parser->vars());
+        external_cmd = path_try_get_path(cmd, parser->vars());
-
+        bool has_command = external_cmd.err == 0;
-        // If there was no command, then we care about the value of errno after checking for it, to
-        // distinguish between e.g. no file vs permissions problem.
-        const int no_cmd_err_code = errno;
 
         // If the specified command does not exist, and is undecorated, try using an implicit cd.
         if (!has_command && statement.decoration() == statement_decoration_t::none) {
@@ -875,7 +873,8 @@ end_execution_reason_t parse_execution_context_t::populate_plain_process(
         if (!has_command && !use_implicit_cd) {
             // No command. If we're --no-execute return okay - it might be a function.
             if (no_exec()) return end_execution_reason_t::ok;
-            return this->handle_command_not_found(path_to_external_command.empty() ? cmd : path_to_external_command, statement, no_cmd_err_code);
+            return this->handle_command_not_found(
+                external_cmd.path.empty() ? cmd : external_cmd.path, statement, external_cmd.err);
         }
     }
 
@@ -885,7 +884,7 @@ end_execution_reason_t parse_execution_context_t::populate_plain_process(
     if (use_implicit_cd) {
         // Implicit cd is simple.
         cmd_args = {L"cd", cmd};
-        path_to_external_command.clear();
+        external_cmd = get_path_result_t{};
 
         // If we have defined a wrapper around cd, use it, otherwise use the cd builtin.
         process_type =
@@ -917,7 +916,7 @@ end_execution_reason_t parse_execution_context_t::populate_plain_process(
     proc->type = process_type;
     proc->set_argv(std::move(cmd_args));
     proc->set_redirection_specs(std::move(redirections));
-    proc->actual_cmd = std::move(path_to_external_command);
+    proc->actual_cmd = std::move(external_cmd.path);
     return end_execution_reason_t::ok;
 }
 
@@ -1036,8 +1035,7 @@ end_execution_reason_t parse_execution_context_t::populate_not_process(
 
 template <typename Type>
 end_execution_reason_t parse_execution_context_t::populate_block_process(
-    process_t *proc, const ast::statement_t &statement,
+    process_t *proc, const ast::statement_t &statement, const Type &specific_statement) {
-    const Type &specific_statement) {
     using namespace ast;
     // We handle block statements by creating process_type_t::block_node, that will bounce back to
     // us when it's time to execute them.
@@ -1154,8 +1152,8 @@ end_execution_reason_t parse_execution_context_t::populate_job_process(
                                                   *specific_statement.as<switch_statement_t>());
             break;
         case type_t::decorated_statement: {
-            result = this->populate_plain_process(proc,
+            result =
-                                                  *specific_statement.as<decorated_statement_t>());
+                this->populate_plain_process(proc, *specific_statement.as<decorated_statement_t>());
             break;
         }
         default: {

src/path.cpp

@@ -29,82 +29,73 @@
 #include "wcstringutil.h"
 #include "wutil.h"  // IWYU pragma: keep
 
-// Note that PREFIX is defined in the `Makefile` and is thus defined when this module is compiled.
+// PREFIX is defined at build time.
-// This ensures we always default to "/bin", "/usr/bin" and the bin dir defined for the fish
+const wcstring_list_t kDefaultPath({L"/bin", L"/usr/bin", PREFIX L"/bin"});
-// programs. Possibly with a duplicate dir if PREFIX is empty, "/", "/usr" or "/usr/". If the PREFIX
+
-// duplicates /bin or /usr/bin that is harmless other than a trivial amount of time testing a path
+static get_path_result_t path_get_path_core(const wcstring &cmd, const wcstring_list_t &pathsv,
-// we've already tested.
+                                            const environment_t &vars) {
-const wcstring_list_t dflt_pathsv({L"/bin", L"/usr/bin", PREFIX L"/bin"});
+    const get_path_result_t noent_res{ENOENT, wcstring{}};
+    get_path_result_t result{};
+
+    /// Test if the given path can be executed.
+    /// \return 0 on success, an errno value on failure.
+    auto test_path = [](const wcstring &path) -> int {
+        std::string narrow = wcs2string(path);
+        struct stat buff;
+        if (access(narrow.c_str(), X_OK) != 0 || stat(narrow.c_str(), &buff) != 0) {
+            return errno;
+        }
+        return S_ISREG(buff.st_mode) ? 0 : EACCES;
+    };
+
+    // Commands cannot contain NUL byte.
+    if (cmd.find(L'\0') != wcstring::npos) {
+        return noent_res;
+    }
 
-static bool path_get_path_core(const wcstring &cmd, wcstring *out_path,
-                               const maybe_t<env_var_t> &bin_path_var) {
-    // Unix paths can't include a NULL-byte, that's the separator.
-    // If we let this through, we'd end up checking up to the NULL,
-    // so we'd get the wrong path.
-    if (cmd.find(L'\0') != wcstring::npos) return false;
     // If the command has a slash, it must be an absolute or relative path and thus we don't bother
     // looking for a matching command.
     if (cmd.find(L'/') != wcstring::npos) {
-        std::string narrow = wcs2string(cmd);
+        wcstring abs_cmd = path_apply_working_directory(cmd, vars.get_pwd_slash());
-        if (access(narrow.c_str(), X_OK) != 0) {
+        int merr = test_path(abs_cmd);
-            return false;
+        return get_path_result_t{merr, std::move(abs_cmd)};
-        }
-
-        struct stat buff;
-        if (stat(narrow.c_str(), &buff)) {
-            return false;
-        }
-        if (S_ISREG(buff.st_mode)) {
-            if (out_path) out_path->assign(cmd);
-            return true;
-        }
-        errno = EACCES;
-        return false;
     }
 
-    const wcstring_list_t *pathsv;
+    get_path_result_t best = noent_res;
-    if (bin_path_var) {
+    wcstring proposed_path;
-        pathsv = &bin_path_var->as_list();
+    for (const auto &next_path : pathsv) {
-    } else {
-        pathsv = &dflt_pathsv;
-    }
-
-    int err = ENOENT;
-    for (auto next_path : *pathsv) {
         if (next_path.empty()) continue;
-        append_path_component(next_path, cmd);
+        proposed_path = next_path;
-        std::string narrow = wcs2string(next_path);
+        append_path_component(proposed_path, cmd);
-        if (access(narrow.c_str(), X_OK) == 0) {
+        int merr = test_path(proposed_path);
-            struct stat buff;
+        if (merr == 0) {
-            if (stat(narrow.c_str(), &buff) == -1) {
+            // We found one.
-                if (errno != EACCES) {
+            best = get_path_result_t{merr, std::move(proposed_path)};
-                    wperror(L"stat");
+            break;
-                }
+        } else if (merr != ENOENT && best.err == ENOENT) {
-                continue;
-            }
-            if (S_ISREG(buff.st_mode)) {
-                if (out_path) *out_path = std::move(next_path);
-                return true;
-            }
-            err = EACCES;
-            if (out_path) *out_path = std::move(next_path);
-        } else if (errno != ENOENT && err == ENOENT) {
             // Keep the first *interesting* error and path around.
             // ENOENT isn't interesting because not having a file is the normal case.
-            auto tmperr = errno;
             // Ignore if the parent directory is already inaccessible.
-            if (access(wcs2string(wdirname(next_path)).c_str(), X_OK) != 0) continue;
+            if (waccess(wdirname(proposed_path), X_OK) == 0) {
-            err = tmperr;
+                best = get_path_result_t{merr, std::move(proposed_path)};
-            if (out_path) *out_path = std::move(next_path);
+            }
         }
     }
-
+    return best;
-    errno = err;
-    return false;
 }
 
-bool path_get_path(const wcstring &cmd, wcstring *out_path, const environment_t &vars) {
+maybe_t<wcstring> path_get_path(const wcstring &cmd, const environment_t &vars) {
-    return path_get_path_core(cmd, out_path, vars.get(L"PATH"));
+    auto result = path_try_get_path(cmd, vars);
+    if (result.err != 0) {
+        return none();
+    }
+    wcstring path = std::move(result.path);
+    return path;
+}
+
+get_path_result_t path_try_get_path(const wcstring &cmd, const environment_t &vars) {
+    auto pathvar = vars.get(L"PATH");
+    return path_get_path_core(cmd, pathvar ? pathvar->as_list() : kDefaultPath, vars);
 }
 
 static bool path_is_executable(const std::string &path) {

src/path.h

@@ -44,17 +44,21 @@ dir_remoteness_t path_get_config_remoteness();
 class env_stack_t;
 void path_emit_config_directory_messages(env_stack_t &vars);
 
-/// Finds the path of an executable.
+/// Finds the path of an executable named \p cmd, by looking in $PATH taken from \p vars.
-///
+/// \returns the path if found, none if not.
-/// Args:
+maybe_t<wcstring> path_get_path(const wcstring &cmd, const environment_t &vars);
-/// cmd - The name of the executable.
+
-/// output_or_NULL - If non-NULL, store the path.
+/// Finds the path of an executable named \p cmd, by looking in $PATH taken from \p vars.
-/// vars - The environment variables to use
+/// On success, err will be 0 and the path is returned.
-///
+/// On failure, we return the "best path" with err set appropriately.
-/// Returns:
+/// For example, if we find a non-executable file, we will return its path and EACCESS.
-/// false if the command can not be found else true. The result
+/// If no candidate path is found, path will be empty and err will be set to ENOENT.
-/// should be freed with free().
+/// Possible err values are taken from access().
-bool path_get_path(const wcstring &cmd, wcstring *out_path, const environment_t &vars);
+struct get_path_result_t {
+    int err;
+    wcstring path;
+};
+get_path_result_t path_try_get_path(const wcstring &cmd, const environment_t &vars);
 
 /// Return all the paths that match the given command.
 wcstring_list_t path_get_paths(const wcstring &cmd, const environment_t &vars);