Support for a "pending item" in history.

Before running a command, we add the command to history, so
that if the command causes us to exit it's still captured in
history. But that command should not be considered part of
history when expanding the history within the command itself.
For example, `echo $history[1]` should be the previously
run command, not `echo $history[1]` itself.

Fixes #2028

history.cpp

@@ -538,6 +538,7 @@ history_t & history_t::history_with_name(const wcstring &name)
 history_t::history_t(const wcstring &pname) :
     name(pname),
     first_unwritten_new_item_index(0),
+    has_pending_item(false),
     disable_automatic_save_counter(0),
     mmap_start(NULL),
     mmap_length(0),
@@ -555,19 +556,21 @@ history_t::~history_t()
     pthread_mutex_destroy(&lock);
 }
 
-void history_t::add(const history_item_t &item)
+void history_t::add(const history_item_t &item, bool pending)
 {
     scoped_lock locker(lock);
 
     /* 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 */
+        /* We merged, so we don't have to add anything. Maybe this item was pending, but it just got merged with an item that is not pending, so pending just becomes false. */
+        this->has_pending_item = false;
     }
     else
     {
         /* We have to add a new item */
         new_items.push_back(item);
+        this->has_pending_item = pending;
         save_internal_unless_disabled();
     }
 }
@@ -609,7 +612,7 @@ void history_t::save_internal_unless_disabled()
     countdown_to_vacuum--;
 }
 
-void history_t::add(const wcstring &str, history_identifier_t ident)
+void history_t::add(const wcstring &str, history_identifier_t ident, bool pending)
 {
     time_t when = time(NULL);
     /* 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.
@@ -619,7 +622,7 @@ void history_t::add(const wcstring &str, history_identifier_t ident)
         when++;
     }
 
-    this->add(history_item_t(str, when, ident));
+    this->add(history_item_t(str, when, ident), pending);
 }
 
 void history_t::remove(const wcstring &str)
@@ -675,9 +678,19 @@ void history_t::get_string_representation(wcstring *result, const wcstring &sepa
 
     std::set<wcstring> seen;
 
+    /* If we have a pending item, we skip the first encountered (i.e. last) new item */
+    bool next_is_pending = this->has_pending_item;
+
     /* Append new items. Note that in principle we could use const_reverse_iterator, but we do not because reverse_iterator is not convertible to const_reverse_iterator ( http://github.com/fish-shell/fish-shell/issues/431 ) */
     for (history_item_list_t::reverse_iterator iter=new_items.rbegin(); iter < new_items.rend(); ++iter)
     {
+        /* Skip a pending item if we have one */
+        if (next_is_pending)
+        {
+            next_is_pending = false;
+            continue;
+        }
+
         /* Skip duplicates */
         if (! seen.insert(iter->str()).second)
             continue;
@@ -714,15 +727,21 @@ history_item_t history_t::item_at_index(size_t idx)
     assert(idx > 0);
     idx--;
 
-    /* idx=0 corresponds to last item in new_items */
-    size_t new_item_count = new_items.size();
-    if (idx < new_item_count)
+    /* Determine how many "resolved" (non-pending) items we have. We can have at most one pending item, and it's always the last one. */
+    size_t resolved_new_item_count = new_items.size();
+    if (this->has_pending_item && resolved_new_item_count > 0)
     {
-        return new_items.at(new_item_count - idx - 1);
+        resolved_new_item_count -= 1;
+    }
+    
+    /* idx=0 corresponds to the last resolved item */
+    if (idx < resolved_new_item_count)
+    {
+        return new_items.at(resolved_new_item_count - idx - 1);
     }
 
     /* Now look in our old items */
-    idx -= new_item_count;
+    idx -= resolved_new_item_count;
     load_old_if_needed();
     size_t old_item_count = old_item_offsets.size();
     if (idx < old_item_count)
@@ -1495,7 +1514,7 @@ bool history_t::save_internal_via_appending()
            Periodically we "clean up" the file by rewriting it, so that most of the time it doesn't have duplicates, although we don't yet sort by timestamp (the timestamp isn't really used for much anyways).
         */
 
-        /* So far so good. Write all items at or after first_unwritten_new_item_index */
+        /* So far so good. Write all items at or after first_unwritten_new_item_index. Note that we write even a pending item - pending items are ignored by history within the command itself, but should still be written to the file. */
 
         bool errored = false;
         history_output_buffer_t buffer;
@@ -1800,7 +1819,7 @@ static bool string_could_be_path(const wcstring &potential_path)
     return true;
 }
 
-void history_t::add_with_file_detection(const wcstring &str)
+void history_t::add_pending_with_file_detection(const wcstring &str)
 {
     ASSERT_IS_MAIN_THREAD();
     path_list_t potential_paths;
@@ -1865,8 +1884,8 @@ void history_t::add_with_file_detection(const wcstring &str)
         iothread_perform(threaded_perform_file_detection, perform_file_detection_done, context);
     }
 
-    /* Actually add the item to the history */
-    this->add(str, identifier);
+    /* Actually add the item to the history. */
+    this->add(str, identifier, true /* pending */);
 
     /* 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 */
     if (impending_exit)
@@ -1874,3 +1893,10 @@ void history_t::add_with_file_detection(const wcstring &str)
         this->save();
     }
 }
+
+/* Very simple, just mark that we have no more pending items */
+void history_t::resolve_pending()
+{
+    scoped_lock locker(lock);
+    this->has_pending_item = false;
+}

history.h

@@ -115,8 +115,8 @@ private:
     /** Private creator */
     history_t(const wcstring &pname);
 
-    /** Privately add an item */
-    void add(const history_item_t &item);
+    /** 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);
 
     /** Destructor */
     ~history_t();
@@ -136,6 +136,9 @@ private:
     /** The index of the first new item that we have not yet written. */
     size_t first_unwritten_new_item_index;
 
+    /** Whether we have a pending item. If so, the most recently added item is ignored by item_at_index. */
+    bool has_pending_item;
+    
     /** Whether we should disable saving to the file for a time */
     uint32_t disable_automatic_save_counter;
 
@@ -208,14 +211,17 @@ public:
     /** Determines whether the history is empty. Unfortunately this cannot be const, since it may require populating the history. */
     bool is_empty(void);
 
-    /** Add a new history item to the end */
-    void add(const wcstring &str, history_identifier_t ident = 0);
+    /** 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);
 
-    /** Add a new history item to the end */
-    void add_with_file_detection(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);
+    
+    /** Resolves any pending history items, so that they may be returned in history searches. */
+    void resolve_pending();
 
     /** Saves history */
     void save();

reader.cpp

@@ -2982,6 +2982,11 @@ static int read_i(void)
             event_fire_generic(L"fish_preexec", &argv);
             reader_run_command(parser, command);
             event_fire_generic(L"fish_postexec", &argv);
+            /* Allow any pending history items to be returned in the history array. */
+            if (data->history)
+            {
+                data->history->resolve_pending();
+            }
             if (data->end_loop)
             {
                 handle_end_loop();
@@ -3592,7 +3597,7 @@ const wchar_t *reader_readline(int nchars)
                         const editable_line_t *el = &data->command_line;
                         if (data->history != NULL && ! el->empty() && el->text.at(0) != L' ')
                         {
-                            data->history->add_with_file_detection(el->text);
+                            data->history->add_pending_with_file_detection(el->text);
                         }
                         finished=1;
                         update_buff_pos(&data->command_line, data->command_line.size());

tests/generic.expect

@@ -11,3 +11,25 @@ send_line "echo "
 expect_prompt "" {} unmatched {
     puts stderr "Couldn't type apple key ()"
 }
+
+# check that history is returned in the right order (#2028)
+# this hist_command nonsense is the cleanest way to send the $ char
+set hist_command "echo \$history\[1\]"
+
+# first send 'echo stuff'
+send_line "echo stuff"
+expect_prompt "stuff" {} unmatched {
+    puts stderr "Couldn't find expected output 'stuff'"
+}
+
+# last history item should be 'echo stuff'
+send_line $hist_command
+expect_prompt "echo stuff" {} unmatched {
+    puts stderr "Couldn't find expected output 'echo stuff'"
+}
+
+# last history command should be the one that printed the history
+send_line $hist_command
+expect_prompt -re {echo .history.*} {} unmatched {
+    puts stderr "Couldn't find expected output $hist_command"
+}