Explicitly track persistence mode in history_item_t

Commands that start with a space should not be written to the history
file. Prior to this change, that was implemented by simply not adding them
to history. Items with leading spaces were simply dropped.

With this change, we add a 'history_persistence_mode_t' to
history_item_t, which tracks how the item persists. Items with leading
spaces are now marked as "ephemeral": they can be recovered via up arrow,
until the user runs another command, or types a space and hits return.
This matches zsh's HIST_IGNORE_SPACE feature.

Fixes #1383

src/fish_tests.cpp

@@ -4406,7 +4406,7 @@ static bool history_equals(history_t &hist, const wchar_t *const *strings) {
         history_item_t item = hist.item_at_index(history_idx);
         if (expected == NULL) {
             if (!item.empty()) {
-                err(L"Expected empty item at history index %lu", history_idx);
+                err(L"Expected empty item at history index %lu, instead found: %ls", history_idx, item.str().c_str());
             }
             break;
         } else {

src/history.cpp

@@ -172,22 +172,28 @@ class history_lru_cache_t : public lru_cache_t<history_lru_cache_t, history_item
 /// We can merge two items if they are the same command. We use the more recent timestamp, more
 /// recent identifier, and the longer list of required paths.
 bool history_item_t::merge(const history_item_t &item) {
-    bool result = false;
-    if (this->contents == item.contents) {
-        this->creation_timestamp = std::max(this->creation_timestamp, item.creation_timestamp);
-        if (this->required_paths.size() < item.required_paths.size()) {
-            this->required_paths = item.required_paths;
-        }
-        if (this->identifier < item.identifier) {
-            this->identifier = item.identifier;
-        }
-        result = true;
+    // We can only merge items if they agree on their text and persistence mode.
+    if (this->contents != item.contents || this->persist_mode != item.persist_mode) {
+        return false;
     }
-    return result;
+
+    // Ok, merge this item.
+    this->creation_timestamp = std::max(this->creation_timestamp, item.creation_timestamp);
+    if (this->required_paths.size() < item.required_paths.size()) {
+        this->required_paths = item.required_paths;
+    }
+    if (this->identifier < item.identifier) {
+        this->identifier = item.identifier;
+    }
+    return true;
 }
 
-history_item_t::history_item_t(wcstring str, time_t when, history_identifier_t ident)
-    : contents(trim(std::move(str))), creation_timestamp(when), identifier(ident) {}
+history_item_t::history_item_t(wcstring str, time_t when, history_identifier_t ident,
+                               history_persistence_mode_t persist_mode)
+    : contents(std::move(str)),
+      creation_timestamp(when),
+      identifier(ident),
+      persist_mode(persist_mode) {}
 
 bool history_item_t::matches_search(const wcstring &term, enum history_search_type_t type,
                                     bool case_sensitive) const {
@@ -228,9 +234,11 @@ bool history_item_t::matches_search(const wcstring &term, enum history_search_ty
 }
 
 struct history_impl_t {
-    // Privately add an item. If pending, the item will not be returned by history searches until a
-    // call to resolve_pending.
-    void add(const history_item_t &item, bool pending = false, bool do_save = true);
+    // Add a new history item to the end. If pending is set, the item will not be returned by
+    // item_at_index until a call to resolve_pending(). Pending items are tracked with an offset
+    // into the array of new items, so adding a non-pending item has the effect of resolving all
+    // pending items.
+    void add(history_item_t item, bool pending = false, bool do_save = true);
 
     // Internal function.
     void clear_file_state();
@@ -266,7 +274,10 @@ struct history_impl_t {
     // the boundary are considered "old". Items whose timestemps are > the boundary are new, and are
     // ignored by this instance (unless they came from this instance). The timestamp may be adjusted
     // by incorporate_external_changes().
-    time_t boundary_timestamp{time(nullptr)};
+    time_t boundary_timestamp{};
+
+    /// The most recent "unique" identifier for a history item.
+    history_identifier_t last_identifier{0};
 
     // How many items we add until the next vacuum. Initially a random value.
     int countdown_to_vacuum{-1};
@@ -277,6 +288,12 @@ struct history_impl_t {
     // List of old items, as offsets into out mmap data.
     std::deque<size_t> old_item_offsets{};
 
+    /// \return a timestamp for new items - see the implementation for a subtlety.
+    time_t timestamp_now() const;
+
+    /// \return a new item identifier, incrementing our counter.
+    history_identifier_t next_identifier() { return ++last_identifier; }
+
     // Figure out the offsets of our file contents.
     void populate_from_file_contents();
 
@@ -286,6 +303,11 @@ struct history_impl_t {
     // Deletes duplicates in new_items.
     void compact_new_items();
 
+    // Removes trailing ephemeral items.
+    // Ephemeral items have leading spaces, and can only be retrieved immediately; adding any item
+    // removes them.
+    void remove_ephemeral_items();
+
     // Attempts to rewrite the existing file to a target temporary file
     // Returns false on error, true on success
     bool rewrite_to_temporary_file(int existing_fd, int dst_fd) const;
@@ -302,7 +324,9 @@ struct history_impl_t {
     // Saves history unless doing so is disabled.
     void save_unless_disabled();
 
-    explicit history_impl_t(wcstring name) : name(std::move(name)) {}
+    explicit history_impl_t(wcstring name)
+        : name(std::move(name)), boundary_timestamp(time(nullptr)) {}
+
     history_impl_t(history_impl_t &&) = default;
     ~history_impl_t() = default;
 
@@ -313,20 +337,9 @@ struct history_impl_t {
     // require populating the history.
     bool is_empty();
 
-    // Add a new history item to the end. If pending is set, the item will not be returned by
-    // item_at_index until a call to resolve_pending(). Pending items are tracked with an offset
-    // into the array of new items, so adding a non-pending item has the effect of resolving all
-    // pending items.
-    void add(const wcstring &str, history_identifier_t ident = 0, bool pending = false,
-             bool save = true);
-
     // Remove a history item.
     void remove(const wcstring &str);
 
-    // Add a new pending history item to the end, and then begin file detection on the items to
-    // determine which arguments are paths
-    void add_pending_with_file_detection(const wcstring &str, const wcstring &working_dir_slash);
-
     // Resolves any pending history items, so that they may be returned in history searches.
     void resolve_pending();
 
@@ -366,12 +379,14 @@ struct history_impl_t {
     size_t size();
 };
 
-void history_impl_t::add(const history_item_t &item, bool pending, bool do_save) {
+void history_impl_t::add(history_item_t item, bool pending, bool do_save) {
+    assert(item.timestamp() != 0 && "Should not add an item with a 0 timestamp");
     // We use empty items as sentinels to indicate the end of history.
     // Do not allow them to be added (#6032).
     if (item.contents.empty()) {
         return;
     }
+
     // Try merging with the last item.
     if (!new_items.empty() && new_items.back().merge(item)) {
         // We merged, so we don't have to add anything. Maybe this item was pending, but it just got
@@ -422,20 +437,6 @@ void history_impl_t::save_unless_disabled() {
     countdown_to_vacuum--;
 }
 
-void history_impl_t::add(const wcstring &str, history_identifier_t ident, bool pending,
-                         bool do_save) {
-    time_t when = time(nullptr);
-    // Big hack: do not allow timestamps equal to our boundary date. This is because we include
-    // items whose timestamps are equal to our boundary when reading old history, so we can catch
-    // "just closed" items. But this means that we may interpret our own items, that we just wrote,
-    // as old items, if we wrote them in the same second as our birthdate.
-    if (when == this->boundary_timestamp) {
-        when++;
-    }
-
-    this->add(history_item_t(str, when, ident), pending, do_save);
-}
-
 // Remove matching history entries from our list of new items. This only supports literal,
 // case-sensitive, matches.
 void history_impl_t::remove(const wcstring &str_to_remove) {
@@ -557,6 +558,18 @@ std::unordered_map<long, wcstring> history_impl_t::items_at_indexes(const std::v
     return result;
 }
 
+time_t history_impl_t::timestamp_now() const {
+    time_t when = time(nullptr);
+    // Big hack: do not allow timestamps equal to our boundary date. This is because we include
+    // items whose timestamps are equal to our boundary when reading old history, so we can catch
+    // "just closed" items. But this means that we may interpret our own items, that we just wrote,
+    // as old items, if we wrote them in the same second as our birthdate.
+    if (when == this->boundary_timestamp) {
+        when++;
+    }
+    return when;
+}
+
 void history_impl_t::populate_from_file_contents() {
     old_item_offsets.clear();
     if (file_contents) {
@@ -648,12 +661,15 @@ void history_impl_t::clear_file_state() {
 }
 
 void history_impl_t::compact_new_items() {
-    // Keep only the most recent items with the given contents. This algorithm could be made more
-    // efficient, but likely would consume more memory too.
+    // Keep only the most recent items with the given contents.
     std::unordered_set<wcstring> seen;
     size_t idx = new_items.size();
     while (idx--) {
         const history_item_t &item = new_items[idx];
+
+        // Only compact persisted items.
+        if (!item.should_write_to_disk()) continue;
+
         if (!seen.insert(item.contents).second) {
             // This item was not inserted because it was already in the set, so delete the item at
             // this index.
@@ -668,6 +684,14 @@ void history_impl_t::compact_new_items() {
     }
 }
 
+void history_impl_t::remove_ephemeral_items() {
+    while (!new_items.empty() &&
+           new_items.back().persist_mode == history_persistence_mode_t::ephemeral) {
+        new_items.pop_back();
+    }
+    first_unwritten_new_item_index = std::min(first_unwritten_new_item_index, new_items.size());
+}
+
 // Given the fd of an existing history file, or -1 if none, write
 // a new history file to temp_fd. Returns true on success, false
 // on error
@@ -698,7 +722,9 @@ bool history_impl_t::rewrite_to_temporary_file(int existing_fd, int dst_fd) cons
     // Insert any unwritten new items
     for (auto iter = new_items.cbegin() + this->first_unwritten_new_item_index;
          iter != new_items.cend(); ++iter) {
-        lru.add_item(*iter);
+        if (iter->should_write_to_disk()) {
+            lru.add_item(*iter);
+        }
     }
 
     // Stable-sort our items by timestamp
@@ -920,10 +946,12 @@ bool history_impl_t::save_internal_via_appending() {
         std::string buffer;
         while (first_unwritten_new_item_index < new_items.size()) {
             const history_item_t &item = new_items.at(first_unwritten_new_item_index);
-            append_history_item_to_buffer(item, &buffer);
-            err = flush_to_fd(&buffer, history_fd.fd(), HISTORY_OUTPUT_BUFFER_SIZE);
-            if (err) break;
-            // We wrote this item, hooray.
+            if (item.should_write_to_disk()) {
+                append_history_item_to_buffer(item, &buffer);
+                err = flush_to_fd(&buffer, history_fd.fd(), HISTORY_OUTPUT_BUFFER_SIZE);
+                if (err) break;
+            }
+            // We wrote or skipped this item, hooray.
             first_unwritten_new_item_index++;
         }
 
@@ -1057,6 +1085,12 @@ bool history_impl_t::is_empty() {
     return empty;
 }
 
+void history_t::add(wcstring str) {
+    auto imp = this->impl();
+    time_t when =  imp->timestamp_now();
+    imp->add(history_item_t(std::move(str), when));
+}
+
 /// Populates from older location (in config path, rather than data path) This is accomplished by
 /// clearing ourselves, and copying the contents of the old history file to the new history file.
 /// The new contents will automatically be re-mapped later.
@@ -1132,6 +1166,8 @@ static bool should_import_bash_history_line(const wcstring &line) {
 /// encode multiline commands.
 void history_impl_t::populate_from_bash(FILE *stream) {
     // Process the entire history file until EOF is observed.
+    // Pretend all items were created at this time.
+    const auto when = this->timestamp_now();
     bool eof = false;
     while (!eof) {
         auto line = std::string();
@@ -1151,10 +1187,11 @@ void history_impl_t::populate_from_bash(FILE *stream) {
             if (a_newline) break;
         }
 
-        wcstring wide_line = str2wcstring(line);
+        wcstring wide_line = trim(str2wcstring(line));
         // Add this line if it doesn't contain anything we know we can't handle.
         if (should_import_bash_history_line(wide_line)) {
-            this->add(wide_line, 0, false /* pending */, false /* do_save */);
+            this->add(history_item_t(std::move(wide_line), when), false /* pending */,
+                      false /* do_save */);
         }
     }
     this->save_unless_disabled();
@@ -1163,7 +1200,7 @@ void history_impl_t::populate_from_bash(FILE *stream) {
 void history_impl_t::incorporate_external_changes() {
     // To incorporate new items, we simply update our timestamp to now, so that items from previous
     // instances get added. We then clear the file state so that we remap the file. Note that this
-    // is somehwhat expensive because we will be going back over old items. An optimization would be
+    // is somewhat expensive because we will be going back over old items. An optimization would be
     // to preserve old_item_offsets so that they don't have to be recomputed. (However, then items
     // *deleted* in other instances would not show up here).
     time_t new_timestamp = time(nullptr);
@@ -1174,9 +1211,11 @@ void history_impl_t::incorporate_external_changes() {
         this->boundary_timestamp = new_timestamp;
         this->clear_file_state();
 
-        // We also need to erase new_items, since we go through those first, and that means we
+        // We also need to erase new items, since we go through those first, and that means we
         // will not properly interleave them with items from other instances.
         // We'll pick them up from the file (#2312)
+        // TODO: this will drop items that had no_persist set, how can we avoid that while still
+        // properly interleaving?
         this->save(false);
         this->new_items.clear();
         this->first_unwritten_new_item_index = 0;
@@ -1258,16 +1297,15 @@ bool history_t::is_default() const { return impl()->is_default(); }
 
 bool history_t::is_empty() { return impl()->is_empty(); }
 
-void history_t::add(const history_item_t &item, bool pending) { impl()->add(item, pending); }
-
-void history_t::add(const wcstring &str, history_identifier_t ident, bool pending) {
-    impl()->add(str, ident, pending);
-}
+void history_t::add(history_item_t item, bool pending) { impl()->add(std::move(item), pending); }
 
 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 wcstring &working_dir_slash,
+                                                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).
     if (str.empty()) {
@@ -1311,17 +1349,18 @@ void history_t::add_pending_with_file_detection(const wcstring &str,
     bool wants_file_detection = !potential_paths.empty() && !needs_sync_write;
     auto imp = this->impl();
 
-    history_identifier_t identifier = 0;
+    // Make our history item.
+    time_t when = imp->timestamp_now();
+    history_identifier_t identifier = imp->next_identifier();
+    history_item_t item{str, when, identifier, persist_mode};
+
     if (wants_file_detection) {
-        // Grab the next identifier.
-        static relaxed_atomic_t<history_identifier_t> s_last_identifier{0};
-        identifier = ++s_last_identifier;
         imp->disable_automatic_saving();
 
         // Add the item. Then check for which paths are valid on a background thread,
         // and unblock the item.
         // Don't hold the lock while we perform this file detection.
-        imp->add(str, identifier, true /* pending */);
+        imp->add(std::move(item), true /* pending */);
         iothread_perform([=]() {
             auto validated_paths = valid_paths(potential_paths, working_dir_slash);
             auto imp = this->impl();
@@ -1332,7 +1371,7 @@ void history_t::add_pending_with_file_detection(const wcstring &str,
         // Add the item.
         // If we think we're about to exit, save immediately, regardless of any disabling. This may
         // cause us to lose file hinting for some commands, but it beats losing history items.
-        imp->add(str, identifier, true /* pending */);
+        imp->add(std::move(item), true /* pending */);
         if (needs_sync_write) {
             imp->save();
         }

src/history.h

@@ -62,11 +62,20 @@ enum class history_search_type_t {
 
 typedef uint64_t history_identifier_t;
 
+/// Ways that a history item may be written to disk (or omitted).
+enum class history_persistence_mode_t : uint8_t {
+    disk,       // the history item is written to disk normally
+    memory,     // the history item is stored in-memory only, not written to disk
+    ephemeral,  // the history item is stored in-memory and deleted when a new item is added
+};
+
 class history_item_t {
    public:
-    /// Construct from a text, optional timestamp, and optional identifier.
-    explicit history_item_t(wcstring str = wcstring(), time_t when = 0,
-                            history_identifier_t ident = 0);
+    /// Construct from a text, timestamp, and optional identifier.
+    /// If \p no_persist is set, then do not write this item to disk.
+    explicit history_item_t(
+        wcstring str = {}, time_t when = 0, history_identifier_t ident = 0,
+        history_persistence_mode_t persist_mode = history_persistence_mode_t::disk);
 
     /// \return the text as a string.
     const wcstring &str() const { return contents; }
@@ -81,6 +90,9 @@ class history_item_t {
     /// \return the timestamp for creating this history item.
     time_t timestamp() const { return creation_timestamp; }
 
+    /// \return whether this item should be persisted (written to disk).
+    bool should_write_to_disk() const { return persist_mode == history_persistence_mode_t::disk; }
+
     /// Get and set the list of arguments which referred to files.
     /// This is used for autosuggestion hinting.
     const path_list_t &get_required_paths() const { return required_paths; }
@@ -96,11 +108,14 @@ class history_item_t {
     // Original creation time for the entry.
     time_t creation_timestamp;
 
+    // Paths that we require to be valid for this item to be autosuggested.
+    path_list_t required_paths;
+
     // Sometimes unique identifier used for hinting.
     history_identifier_t identifier;
 
-    // Paths that we require to be valid for this item to be autosuggested.
-    path_list_t required_paths;
+    // If set, do not write this item to disk.
+    history_persistence_mode_t persist_mode;
 
     friend class history_t;
     friend struct history_impl_t;
@@ -129,8 +144,11 @@ class history_t {
     acquired_lock<const history_impl_t> impl() const;
 
     // Privately add an item. If pending, the item will not be returned by history searches until a
-    // call to resolve_pending.
-    void add(const history_item_t &item, bool pending = false);
+    // call to resolve_pending. Any trailing ephemeral items are dropped.
+    void add(history_item_t item, bool pending = false);
+
+    // Add a new history item with text \p str to the end of history.
+    void add(wcstring str);
 
    public:
     explicit history_t(wcstring name);
@@ -153,18 +171,17 @@ class history_t {
     // require populating the history.
     bool is_empty();
 
-    // Add a new history item to the end. If pending is set, the item will not be returned by
-    // item_at_index until a call to resolve_pending(). Pending items are tracked with an offset
-    // into the array of new items, so adding a non-pending item has the effect of resolving all
-    // pending items.
-    void add(const wcstring &str, history_identifier_t ident = 0, bool pending = false);
-
     // Remove a history item.
     void remove(const wcstring &str);
 
+    /// Remove any trailing ephemeral items.
+    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
-    void add_pending_with_file_detection(const wcstring &str, const wcstring &working_dir_slash);
+    // determine which arguments are paths. The item has the given \p persist_mode.
+    void add_pending_with_file_detection(
+        const wcstring &str, const wcstring &working_dir_slash,
+        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.
     void resolve_pending();

src/history_file.cpp

@@ -431,6 +431,7 @@ static size_t offset_of_next_item_fish_2_0(const history_file_contents_t &conten
 }
 
 void append_history_item_to_buffer(const history_item_t &item, std::string *buffer) {
+    assert(item.should_write_to_disk() && "Item should not be persisted");
     auto append = [=](const char *a, const char *b = nullptr, const char *c = nullptr) {
         if (a) buffer->append(a);
         if (b) buffer->append(b);

src/reader.cpp

@@ -1576,10 +1576,6 @@ void reader_data_t::completion_insert(const wcstring &val, size_t token_end,
     set_buffer_maintaining_pager(new_command_line, cursor);
 }
 
-static bool may_add_to_history(const wcstring &commandline_prefix) {
-    return !commandline_prefix.empty() && commandline_prefix.at(0) != L' ';
-}
-
 // Returns a function that can be invoked (potentially
 // on a background thread) to determine the autosuggestion
 static std::function<autosuggestion_t(void)> get_autosuggestion_performer(
@@ -1603,21 +1599,20 @@ static std::function<autosuggestion_t(void)> get_autosuggestion_performer(
             return nothing;
         }
 
-        if (may_add_to_history(search_string)) {
-            history_search_t searcher(*history, search_string, history_search_type_t::prefix,
-                                      history_search_flags_t{});
-            while (!ctx.check_cancel() && searcher.go_backwards()) {
-                const history_item_t &item = searcher.current_item();
+        // Search history for a matching item.
+        history_search_t searcher(*history, search_string, history_search_type_t::prefix,
+                                  history_search_flags_t{});
+        while (!ctx.check_cancel() && searcher.go_backwards()) {
+            const history_item_t &item = searcher.current_item();
 
-                // Skip items with newlines because they make terrible autosuggestions.
-                if (item.str().find(L'\n') != wcstring::npos) continue;
+            // Skip items with newlines because they make terrible autosuggestions.
+            if (item.str().find(L'\n') != wcstring::npos) continue;
 
-                if (autosuggest_validate_from_history(item, working_directory, ctx)) {
-                    // The command autosuggestion was handled specially, so we're done.
-                    // History items are case-sensitive, see #3978.
-                    return autosuggestion_t{searcher.current_string(), search_string,
-                                            false /* icase */};
-                }
+            if (autosuggest_validate_from_history(item, working_directory, ctx)) {
+                // The command autosuggestion was handled specially, so we're done.
+                // History items are case-sensitive, see #3978.
+                return autosuggestion_t{searcher.current_string(), search_string,
+                                        false /* icase */};
             }
         }
 
@@ -3158,11 +3153,28 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
             }
 
             if (command_test_result == 0) {
-                // Finished command, execute it. Don't add items that start with a leading
-                // space, or if in silent mode (#7230).
-                const editable_line_t *el = &command_line;
-                if (history != nullptr && !conf.in_silent_mode && may_add_to_history(el->text())) {
-                    history->add_pending_with_file_detection(el->text(), vars.get_pwd_slash());
+                // Finished command, execute it. Don't add items in silent mode (#7230).
+                wcstring text = command_line.text();
+                if (text.empty()) {
+                    // Here the user just hit return. Make a new prompt, don't remove ephemeral
+                    // items.
+                    rls.finished = true;
+                    break;
+                }
+
+                // Historical behavior is to trim trailing spaces.
+                while (!text.empty() && text.back() == L' ') {
+                    text.pop_back();
+                }
+                if (history && !conf.in_silent_mode) {
+                    // Remove ephemeral items.
+                    // Note we fall into this case if the user just types a space and hits return.
+                    history->remove_ephemeral_items();
+
+                    // Mark this item as ephemeral if there is a leading space (#615).
+                    auto mode = text.front() == L' ' ? history_persistence_mode_t::ephemeral
+                                                     : history_persistence_mode_t::disk;
+                    history->add_pending_with_file_detection(text, vars.get_pwd_slash(), mode);
                 }
                 rls.finished = true;
                 update_buff_pos(&command_line, command_line.size());