diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 062edc339..0ac9967b2 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2728,19 +2728,17 @@ static void test_autosuggestion_combining() { do_test(combine_command_and_autosuggestion(L"alpha", L"ALPHA") == L"alpha"); } -static void test_history_matches(history_search_t &search, size_t matches, unsigned from_line) { - size_t i; - for (i = 0; i < matches; i++) { - do_test(search.go_backwards()); +static void test_history_matches(history_search_t &search, const wcstring_list_t &expected, + unsigned from_line) { + wcstring_list_t found; + while (search.go_backwards()) { + found.push_back(search.current_string()); } - // do_test_from(!search.go_backwards(), from_line); - bool result = search.go_backwards(); - do_test_from(!result, from_line); - - for (i = 1; i < matches; i++) { - do_test_from(search.go_forwards(), from_line); + do_test_from(expected == found, from_line); + if (expected != found) { + fprintf(stderr, "Expected %ls, found %ls\n", comma_join(expected).c_str(), + comma_join(found).c_str()); } - do_test_from(!search.go_forwards(), from_line); } static bool history_contains(history_t *history, const wcstring &txt) { @@ -3028,64 +3026,82 @@ static wcstring random_string() { return result; } +// Helper to lowercase a string. +static wcstring lower(const wcstring &s) { + wcstring result; + for (wchar_t c : s) { + result.push_back(towlower(c)); + } + return result; +} + void history_tests_t::test_history() { history_search_t searcher; say(L"Testing history"); + const wcstring_list_t items = {L"Gamma", L"beta", L"BetA", L"Beta", L"alpha", + L"AlphA", L"Alpha", L"alph", L"ALPH", L"ZZZ"}; + const history_search_flags_t nocase = history_search_ignore_case; + + // Populate a history. history_t &history = history_t::history_with_name(L"test_history"); history.clear(); - history.add(L"Gamma"); - history.add(L"beta"); - history.add(L"BetA"); - history.add(L"Beta"); - history.add(L"alpha"); - history.add(L"AlphA"); - history.add(L"Alpha"); - history.add(L"alph"); - history.add(L"ALPH"); - history.add(L"ZZZ"); + for (const wcstring &s : items) { + history.add(s); + } + + // Helper to set expected items to those matching a predicate, in reverse order. + wcstring_list_t expected; + auto set_expected = [&](const std::function &filt) { + expected.clear(); + for (const auto &s : items) { + if (filt(s)) expected.push_back(s); + } + std::reverse(expected.begin(), expected.end()); + }; // Items matching "a", case-sensitive. searcher = history_search_t(history, L"a"); - test_history_matches(searcher, 6, __LINE__); - do_test(searcher.current_string() == L"alph"); + set_expected([](const wcstring &s) { return s.find(L'a') != wcstring::npos; }); + test_history_matches(searcher, expected, __LINE__); // Items matching "alpha", case-insensitive. - searcher = history_search_t(history, L"AlPhA", HISTORY_SEARCH_TYPE_CONTAINS, false); - test_history_matches(searcher, 3, __LINE__); - do_test(searcher.current_string() == L"Alpha"); + searcher = history_search_t(history, L"AlPhA", HISTORY_SEARCH_TYPE_CONTAINS, nocase); + set_expected([](const wcstring &s) { return lower(s).find(L"alpha") != wcstring::npos; }); + test_history_matches(searcher, expected, __LINE__); // Items matching "et", case-sensitive. searcher = history_search_t(history, L"et"); - test_history_matches(searcher, 3, __LINE__); - do_test(searcher.current_string() == L"Beta"); + set_expected([](const wcstring &s) { return s.find(L"et") != wcstring::npos; }); + test_history_matches(searcher, expected, __LINE__); // Items starting with "be", case-sensitive. - searcher = history_search_t(history, L"be", HISTORY_SEARCH_TYPE_PREFIX, true); - test_history_matches(searcher, 1, __LINE__); - do_test(searcher.current_string() == L"beta"); + searcher = history_search_t(history, L"be", HISTORY_SEARCH_TYPE_PREFIX, 0); + set_expected([](const wcstring &s) { return string_prefixes_string(L"be", s); }); + test_history_matches(searcher, expected, __LINE__); // Items starting with "be", case-insensitive. - searcher = history_search_t(history, L"be", HISTORY_SEARCH_TYPE_PREFIX, false); - test_history_matches(searcher, 3, __LINE__); - do_test(searcher.current_string() == L"Beta"); + searcher = history_search_t(history, L"be", HISTORY_SEARCH_TYPE_PREFIX, nocase); + set_expected( + [](const wcstring &s) { return string_prefixes_string_case_insensitive(L"be", s); }); + test_history_matches(searcher, expected, __LINE__); // Items exactly matching "alph", case-sensitive. - searcher = history_search_t(history, L"alph", HISTORY_SEARCH_TYPE_EXACT, true); - test_history_matches(searcher, 1, __LINE__); - do_test(searcher.current_string() == L"alph"); + searcher = history_search_t(history, L"alph", HISTORY_SEARCH_TYPE_EXACT, 0); + set_expected([](const wcstring &s) { return s == L"alph"; }); + test_history_matches(searcher, expected, __LINE__); // Items exactly matching "alph", case-insensitive. - searcher = history_search_t(history, L"alph", HISTORY_SEARCH_TYPE_EXACT, false); - test_history_matches(searcher, 2, __LINE__); - do_test(searcher.current_string() == L"ALPH"); + searcher = history_search_t(history, L"alph", HISTORY_SEARCH_TYPE_EXACT, nocase); + set_expected([](const wcstring &s) { return lower(s) == L"alph"; }); + test_history_matches(searcher, expected, __LINE__); // Test item removal case-sensitive. searcher = history_search_t(history, L"Alpha"); - test_history_matches(searcher, 1, __LINE__); + test_history_matches(searcher, {L"Alpha"}, __LINE__); history.remove(L"Alpha"); searcher = history_search_t(history, L"Alpha"); - test_history_matches(searcher, 0, __LINE__); + test_history_matches(searcher, {}, __LINE__); // Test history escaping and unescaping, yaml, etc. history_item_list_t before, after; diff --git a/src/history.cpp b/src/history.cpp index d578b0a81..48fa97c4f 100644 --- a/src/history.cpp +++ b/src/history.cpp @@ -2,7 +2,6 @@ #include "config.h" // IWYU pragma: keep #include -#include #include #include #include @@ -10,6 +9,7 @@ #include #include #include +#include // We need the sys/file.h for the flock() declaration on Linux but not OS X. #include // IWYU pragma: keep #include @@ -564,7 +564,7 @@ history_item_t::history_item_t(const wcstring &str, time_t when, history_identif bool history_item_t::matches_search(const wcstring &term, enum history_search_type_t type, bool case_sensitive) const { - // Note that this->term has already been lowercased when constructing the + // Note that 'term' has already been lowercased when constructing the // search object if we're doing a case insensitive search. const wcstring &content_to_match = case_sensitive ? contents : contents_lower; @@ -1083,74 +1083,47 @@ void history_t::load_old_if_needed() { } } -void history_search_t::skip_matches(const wcstring_list_t &skips) { - external_skips = skips; - std::sort(external_skips.begin(), external_skips.end()); -} - -bool history_search_t::should_skip_match(const wcstring &str) const { - return std::binary_search(external_skips.begin(), external_skips.end(), str); -} - -bool history_search_t::go_forwards() { - // Pop the top index (if more than one) and return if we have any left. - if (prev_matches.size() > 1) { - prev_matches.pop_back(); - return true; - } - return false; -} - bool history_search_t::go_backwards() { // Backwards means increasing our index. - const size_t max_idx = (size_t)-1; - - size_t idx = 0; - if (!prev_matches.empty()) idx = prev_matches.back().first; - - if (idx == max_idx) return false; + const size_t max_index = (size_t)-1; + if (current_index_ == max_index) return false; const bool main_thread = is_main_thread(); - while (++idx < max_idx) { + size_t index = current_index_; + while (++index < max_index) { if (main_thread ? reader_interrupted() : reader_thread_job_is_stale()) { return false; } - const history_item_t item = history->item_at_index(idx); + history_item_t item = history_->item_at_index(index); + // We're done if it's empty or we cancelled. if (item.empty()) { return false; } - // Look for a term that matches and that we haven't seen before. - const wcstring &str = item.str(); - if (item.matches_search(term, search_type, case_sensitive) && !match_already_made(str) && - !should_skip_match(str)) { - prev_matches.push_back(prev_match_t(idx, item)); - return true; + // Look for an item that matches and (if deduping) that we haven't seen before. + if (!item.matches_search(canon_term_, search_type_, !ignores_case())) { + continue; } + + // Skip if deduplicating. + if (dedup() && !deduper_.insert(item.str()).second) { + continue; + } + + // This is our new item. + current_item_ = std::move(item); + current_index_ = index; + return true; } return false; } -/// Goes to the end (forwards). -void history_search_t::go_to_end() { prev_matches.clear(); } - -/// Returns if we are at the end, which is where we start. -bool history_search_t::is_at_end() const { return prev_matches.empty(); } - -/// Goes to the beginning (backwards). -void history_search_t::go_to_beginning() { - // Go backwards as far as we can. - while (go_backwards()) { //!OCLINT(empty while statement) - // Do nothing. - } -} - history_item_t history_search_t::current_item() const { - assert(!prev_matches.empty()); //!OCLINT(double negative) - return prev_matches.back().second; + assert(current_item_ && "No current item"); + return *current_item_; } wcstring history_search_t::current_string() const { @@ -1158,14 +1131,6 @@ wcstring history_search_t::current_string() const { return item.str(); } -bool history_search_t::match_already_made(const wcstring &match) const { - for (std::vector::const_iterator iter = prev_matches.begin(); - iter != prev_matches.end(); ++iter) { - if (iter->second.str() == match) return true; - } - return false; -} - static void replace_all(std::string *str, const char *needle, const char *replacement) { size_t needle_len = strlen(needle), replacement_len = strlen(replacement); size_t offset = 0; @@ -1633,15 +1598,13 @@ bool history_t::search_with_args(history_search_type_t search_type, wcstring_lis size_t hist_size = this->size(); if (max_items > hist_size) max_items = hist_size; - for (wcstring_list_t::const_iterator iter = search_args.begin(); iter != search_args.end(); - ++iter) { - const wcstring &search_string = *iter; + for (const wcstring &search_string : search_args) { if (search_string.empty()) { streams.err.append_format(L"Searching for the empty string isn't allowed"); return false; } - history_search_t searcher = - history_search_t(*this, search_string, search_type, case_sensitive); + history_search_t searcher = history_search_t( + *this, search_string, search_type, case_sensitive ? 0 : history_search_ignore_case); while (searcher.go_backwards()) { wcstring result; auto cur_item = searcher.current_item(); @@ -1908,8 +1871,9 @@ wcstring history_session_id() { } else if (valid_var_name(session_id)) { result = session_id; } else { - debug(0, _(L"History session ID '%ls' is not a valid variable name. " - L"Falling back to `%ls`."), + debug(0, + _(L"History session ID '%ls' is not a valid variable name. " + L"Falling back to `%ls`."), session_id.c_str(), result.c_str()); } } diff --git a/src/history.h b/src/history.h index 178dc8cc4..b221f99ec 100644 --- a/src/history.h +++ b/src/history.h @@ -241,6 +241,7 @@ class history_t { bool search(history_search_type_t search_type, wcstring_list_t search_args, const wchar_t *show_time_format, size_t max_items, bool case_sensitive, bool null_terminate, bool reverse, io_streams_t &streams); + bool search_with_args(history_search_type_t search_type, wcstring_list_t search_args, const wchar_t *show_time_format, size_t max_items, bool case_sensitive, bool null_terminate, bool reverse, io_streams_t &streams); @@ -281,52 +282,57 @@ class history_t { size_t size(); }; +/// Flags for history searching. +enum { + // If set, ignore case. + history_search_ignore_case = 1 << 0, + + // If set, do not deduplicate, which can help performance. + history_search_no_dedup = 1 << 1 +}; +using history_search_flags_t = uint32_t; + +/// Support for searching a history backwards. +/// Note this does NOT de-duplicate; it is the caller's responsibility to do so. class history_search_t { private: // The history in which we are searching. - history_t *history; + history_t *history_; - // The search term. - wcstring term; + // The original search term. + wcstring orig_term_; + + // The (possibly lowercased) search term. + wcstring canon_term_; // Our search type. - enum history_search_type_t search_type; - bool case_sensitive; + enum history_search_type_t search_type_ { HISTORY_SEARCH_TYPE_CONTAINS }; - // Our list of previous matches as index, value. The end is the current match. - typedef std::pair prev_match_t; - std::vector prev_matches; + // Our flags. + history_search_flags_t flags_{0}; - // Returns yes if a given term is in prev_matches. - bool match_already_made(const wcstring &match) const; + // The current history item. + maybe_t current_item_; - // Additional strings to skip (sorted). - wcstring_list_t external_skips; + // Index of the current history item. + size_t current_index_{0}; - bool should_skip_match(const wcstring &str) const; + // If deduping, the items we've seen. + std::unordered_set deduper_; + + // return whether we are case insensitive. + bool ignores_case() const { return flags_ & history_search_ignore_case; } + + // return whether we deduplicate items. + bool dedup() const { return !(flags_ & history_search_no_dedup); } public: - // Gets the search term. - const wcstring &get_term() const { return term; } - - // Sets additional string matches to skip. - void skip_matches(const wcstring_list_t &skips); - - // Finds the next search term (forwards in time). Returns true if one was found. - bool go_forwards(); + // Gets the original search term. + const wcstring &original_term() const { return orig_term_; } // Finds the previous search result (backwards in time). Returns true if one was found. bool go_backwards(); - // Goes to the end (forwards). - void go_to_end(); - - // Returns if we are at the end. We start out at the end. - bool is_at_end() const; - - // Goes to the beginning (backwards). - void go_to_beginning(); - // Returns the current search result item. asserts if there is no current item. history_item_t current_item() const; @@ -336,19 +342,15 @@ class history_search_t { // Constructor. history_search_t(history_t &hist, const wcstring &str, enum history_search_type_t type = HISTORY_SEARCH_TYPE_CONTAINS, - bool case_sensitive = true) - : history(&hist), term(str), search_type(type), case_sensitive(case_sensitive) { - if (!case_sensitive) { - term = wcstring(); - for (wcstring::const_iterator it = str.begin(); it != str.end(); ++it) { - term.push_back(towlower(*it)); - } + history_search_flags_t flags = 0) + : history_(&hist), orig_term_(str), canon_term_(str), search_type_(type), flags_(flags) { + if (ignores_case()) { + std::transform(canon_term_.begin(), canon_term_.end(), canon_term_.begin(), towlower); } } // Default constructor. - history_search_t() - : history(), term(), search_type(HISTORY_SEARCH_TYPE_CONTAINS), case_sensitive(true) {} + history_search_t() = default; }; /// Saves the new history to disk. diff --git a/src/reader.cpp b/src/reader.cpp index 526eb1419..b449174f6 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -179,8 +179,6 @@ class reader_history_search_t { /// Attempt to append matches from the current history item. /// \return true if something was appended. bool append_matches_from_search() { - if (search_.is_at_end()) return false; - const size_t before = matches_.size(); wcstring text = search_.current_string(); if (mode_ == line) { @@ -260,7 +258,7 @@ class reader_history_search_t { } /// \return the string we are searching for. - const wcstring &search_string() const { return search_.get_term(); } + const wcstring &search_string() const { return search_.original_term(); } /// \return whether we are at the end (most recent) of our search. bool is_at_end() const { return match_index_ == 0; } @@ -277,7 +275,9 @@ class reader_history_search_t { matches_ = {text}; match_index_ = 0; mode_ = mode; - search_ = history_search_t(*hist, text); + // We can skip dedup in history_search_t because we do it ourselves in skips_. + search_ = + history_search_t(*hist, text, HISTORY_SEARCH_TYPE_CONTAINS, history_search_no_dedup); } /// Reset to inactive search.