diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 2784e2b72..05c4017df 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -4353,17 +4353,23 @@ void history_tests_t::test_history_path_detection() { } fclose(f); - test_environment_t vars; - vars.vars[L"PWD"] = tmpdir; - vars.vars[L"HOME"] = tmpdir; + std::shared_ptr vars = std::make_shared(); + vars->vars[L"PWD"] = tmpdir; + vars->vars[L"HOME"] = tmpdir; history_t &history = history_t::history_with_name(L"path_detection"); - history.add_pending_with_file_detection(L"cmd0 not/a/valid/path", tmpdir); - history.add_pending_with_file_detection(L"cmd1 " + filename, tmpdir); - history.add_pending_with_file_detection(L"cmd2 " + tmpdir + L"/" + filename, tmpdir); + history.add_pending_with_file_detection(L"cmd0 not/a/valid/path", vars); + history.add_pending_with_file_detection(L"cmd1 " + filename, vars); + history.add_pending_with_file_detection(L"cmd2 " + tmpdir + L"/" + filename, vars); + history.add_pending_with_file_detection(L"cmd3 $HOME/" + filename, vars); + history.add_pending_with_file_detection(L"cmd4 $HOME/notafile", vars); + history.add_pending_with_file_detection(L"cmd5 ~/" + filename, vars); + history.add_pending_with_file_detection(L"cmd6 ~/notafile", vars); + history.add_pending_with_file_detection(L"cmd7 ~/*f*", vars); + history.add_pending_with_file_detection(L"cmd8 ~/*zzz*", vars); history.resolve_pending(); - constexpr size_t hist_size = 3; + constexpr size_t hist_size = 9; if (history.size() != hist_size) { err(L"history has wrong size: %lu but expected %lu", (unsigned long)history.size(), (unsigned long)hist_size); history.clear(); @@ -4372,9 +4378,9 @@ void history_tests_t::test_history_path_detection() { // Expected sets of paths. wcstring_list_t expected[hist_size] = { + {}, {filename}, {tmpdir + L"/" + filename}, {L"$HOME/" + filename}, {}, {L"~/" + filename}, + {}, {}, // we do not expand globs {}, - {filename}, - {tmpdir + L"/" + filename}, }; size_t lap; diff --git a/src/highlight.cpp b/src/highlight.cpp index 444b9a4d1..b092b04aa 100644 --- a/src/highlight.cpp +++ b/src/highlight.cpp @@ -468,7 +468,7 @@ bool autosuggest_validate_from_history(const history_item_t &item, } // Did the historical command have arguments that look like paths, which aren't paths now? - if (!all_paths_are_valid(item.get_required_paths(), working_directory)) { + if (!all_paths_are_valid(item.get_required_paths(), ctx)) { return false; } diff --git a/src/history.cpp b/src/history.cpp index 1ae664cb5..647176094 100644 --- a/src/history.cpp +++ b/src/history.cpp @@ -369,7 +369,7 @@ struct history_impl_t { std::unordered_map items_at_indexes(const std::vector &idxs); // Sets the valid file paths for the history item with the given identifier. - void set_valid_file_paths(const wcstring_list_t &valid_file_paths, history_identifier_t ident); + void set_valid_file_paths(wcstring_list_t &&valid_file_paths, history_identifier_t ident); // Return the specified history at the specified index. 0 is the index of the current // commandline. (So the most recent item is at index 1.) @@ -459,7 +459,7 @@ void history_impl_t::remove(const wcstring &str_to_remove) { assert(first_unwritten_new_item_index <= new_items.size()); } -void history_impl_t::set_valid_file_paths(const wcstring_list_t &valid_file_paths, +void history_impl_t::set_valid_file_paths(wcstring_list_t &&valid_file_paths, history_identifier_t ident) { // 0 identifier is used to mean "not necessary". if (ident == 0) { @@ -469,7 +469,7 @@ void history_impl_t::set_valid_file_paths(const wcstring_list_t &valid_file_path // Look for an item with the given identifier. It is likely to be at the end of new_items. for (auto iter = new_items.rbegin(); iter != new_items.rend(); ++iter) { if (iter->identifier == ident) { // found it - iter->required_paths = valid_file_paths; + iter->required_paths = std::move(valid_file_paths); break; } } @@ -1246,21 +1246,42 @@ wcstring history_session_id(const environment_t &vars) { return result; } -path_list_t valid_paths(const path_list_t &paths, const wcstring &working_directory) { +path_list_t expand_and_detect_paths(const path_list_t &paths, const environment_t &vars) { ASSERT_IS_BACKGROUND_THREAD(); wcstring_list_t result; + wcstring working_directory = vars.get_pwd_slash(); + operation_context_t ctx(vars, kExpansionLimitBackground); for (const wcstring &path : paths) { - if (path_is_valid(path, working_directory)) { - result.push_back(path); + // Suppress cmdsubs since we are on a background thread and don't want to execute fish + // script. + // Suppress wildcards because we want to suggest e.g. `rm *` even if the directory + // is empty (and so rm will fail); this is nevertheless a useful command because it + // confirms the directory is empty. + wcstring expanded_path = path; + if (expand_one(expanded_path, {expand_flag::skip_cmdsubst, expand_flag::skip_wildcards}, + ctx)) { + if (path_is_valid(expanded_path, working_directory)) { + // Note we return the original (unexpanded) path. + result.push_back(path); + } } } return result; } -bool all_paths_are_valid(const path_list_t &paths, const wcstring &working_directory) { +bool all_paths_are_valid(const path_list_t &paths, const operation_context_t &ctx) { ASSERT_IS_BACKGROUND_THREAD(); + wcstring working_directory = ctx.vars.get_pwd_slash(); for (const wcstring &path : paths) { - if (!path_is_valid(path, working_directory)) { + if (ctx.cancel_checker()) { + return false; + } + wcstring expanded_path = path; + if (!expand_one(expanded_path, {expand_flag::skip_cmdsubst, expand_flag::skip_wildcards}, + ctx)) { + return false; + } + if (!path_is_valid(expanded_path, working_directory)) { return false; } } @@ -1304,7 +1325,7 @@ void history_t::remove(const wcstring &str) { impl()->remove(str); } void history_t::remove_ephemeral_items() { impl()->remove_ephemeral_items(); } void history_t::add_pending_with_file_detection(const wcstring &str, - const wcstring &working_dir_slash, + const std::shared_ptr &vars, history_persistence_mode_t persist_mode) { // We use empty items as sentinels to indicate the end of history. // Do not allow them to be added (#6032). @@ -1321,9 +1342,8 @@ void history_t::add_pending_with_file_detection(const wcstring &str, for (const node_t &node : ast) { if (const argument_t *arg = node.try_as()) { wcstring potential_path = arg->source(str); - bool unescaped = unescape_string_in_place(&potential_path, UNESCAPE_DEFAULT); - if (unescaped && string_could_be_path(potential_path)) { - potential_paths.push_back(potential_path); + if (string_could_be_path(potential_path)) { + potential_paths.push_back(std::move(potential_path)); } } else if (const decorated_statement_t *stmt = node.try_as()) { // Hack hack hack - if the command is likely to trigger an exit, then don't do @@ -1362,9 +1382,10 @@ void history_t::add_pending_with_file_detection(const wcstring &str, // Don't hold the lock while we perform this file detection. imp->add(std::move(item), true /* pending */); iothread_perform([=]() { - auto validated_paths = valid_paths(potential_paths, working_dir_slash); + // Don't hold the lock while we perform this file detection. + auto validated_paths = expand_and_detect_paths(potential_paths, *vars); auto imp = this->impl(); - imp->set_valid_file_paths(validated_paths, identifier); + imp->set_valid_file_paths(std::move(validated_paths), identifier); imp->enable_automatic_saving(); }); } else { diff --git a/src/history.h b/src/history.h index 7506f62ab..9f5e24c6c 100644 --- a/src/history.h +++ b/src/history.h @@ -24,6 +24,7 @@ struct io_streams_t; class env_stack_t; class environment_t; +class operation_context_t; // Fish supports multiple shells writing to history at once. Here is its strategy: // @@ -178,9 +179,10 @@ class history_t { void remove_ephemeral_items(); // Add a new pending history item to the end, and then begin file detection on the items to - // determine which arguments are paths. The item has the given \p persist_mode. + // determine which arguments are paths. Arguments may be expanded (e.g. with PWD and variables) + // using the given \p vars. The item has the given \p persist_mode void add_pending_with_file_detection( - const wcstring &str, const wcstring &working_dir_slash, + const wcstring &str, const std::shared_ptr &vars, history_persistence_mode_t persist_mode = history_persistence_mode_t::disk); // Resolves any pending history items, so that they may be returned in history searches. @@ -301,14 +303,18 @@ void history_save_all(); /// Return the prefix for the files to be used for command and read history. wcstring history_session_id(const environment_t &vars); -/// Given a list of paths and a working directory, return the paths that are valid -/// This does disk I/O and may only be called in a background thread -path_list_t valid_paths(const path_list_t &paths, const wcstring &working_directory); +/// Given a list of proposed paths and a context, perform variable and home directory expansion, +/// and detect if the result expands to a value which is also the path to a file. +/// Wildcard expansions are suppressed - see implementation comments for why. +/// This is used for autosuggestion hinting. If we add an item to history, and one of its arguments +/// refers to a file, then we only want to suggest it if there is a valid file there. +/// This does disk I/O and may only be called in a background thread. +path_list_t expand_and_detect_paths(const path_list_t &paths, const environment_t &vars); -/// Given a list of paths and a working directory, -/// return true if all paths in the list are valid -/// Returns true for if paths is empty -bool all_paths_are_valid(const path_list_t &paths, const wcstring &working_directory); +/// Given a list of proposed paths and a context, expand each one and see if it refers to a file. +/// Wildcard expansions are suppressed. +/// \return true if \p paths is empty or every path is valid. +bool all_paths_are_valid(const path_list_t &paths, const operation_context_t &ctx); /// Sets private mode on. Once in private mode, it cannot be turned off. void start_private_mode(env_stack_t &vars); diff --git a/src/operation_context.h b/src/operation_context.h index 56b731d99..6f89853e6 100644 --- a/src/operation_context.h +++ b/src/operation_context.h @@ -60,8 +60,9 @@ class operation_context_t { size_t expansion_limit = kExpansionLimitDefault); /// Construct from vars alone. - explicit operation_context_t(const environment_t &vars) - : operation_context_t(nullptr, vars, no_cancel) {} + explicit operation_context_t(const environment_t &vars, + size_t expansion_limit = kExpansionLimitDefault) + : operation_context_t(nullptr, vars, no_cancel, expansion_limit) {} ~operation_context_t(); }; diff --git a/src/reader.cpp b/src/reader.cpp index 2c4b8e5b3..3b3f1a514 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -3241,7 +3241,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat } else { mode = history_persistence_mode_t::disk; } - history->add_pending_with_file_detection(text, vars.get_pwd_slash(), mode); + history->add_pending_with_file_detection(text, vars.snapshot(), mode); } rls.finished = true;