Fix indexing of $history

cherry-picked from krader1961/fish-shell commit b69df4fe72

Fixes #4353 (regression in indexing of history contents) and introduces
new unit tests to catch bad $history indexing in the future.

src/env.cpp

@@ -1364,8 +1364,8 @@ env_var_t env_get(const wcstring &key, env_mode_flags_t mode) {
             if (!history) {
                 history = &history_t::history_with_name(history_session_id());
             }
-            wcstring result;
-            if (history) history->get_string_representation(&result, ARRAY_SEP_STR);
+            wcstring_list_t result;
+            if (history) history->get_history(result);
             return env_var_t(L"history", result);
         } else if (key == L"status") {
             return env_var_t(L"status", to_string(proc_get_last_status()));

src/fish_tests.cpp

@@ -3079,12 +3079,12 @@ void history_tests_t::test_history_merge(void) {
     }
 
     // Everyone should also have items in the same order (#2312)
-    wcstring string_rep;
-    hists[0]->get_string_representation(&string_rep, L"\n");
+    wcstring_list_t hist_vals1;
+    hists[0]->get_history(hist_vals1);
     for (size_t i = 0; i < count; i++) {
-        wcstring string_rep2;
-        hists[i]->get_string_representation(&string_rep2, L"\n");
-        do_test(string_rep == string_rep2);
+        wcstring_list_t hist_vals2;
+        hists[i]->get_history(hist_vals2);
+        do_test(hist_vals1 == hist_vals2);
     }
 
     // Add some more per-history items.

src/history.cpp

@@ -842,15 +842,12 @@ void history_t::set_valid_file_paths(const wcstring_list_t &valid_file_paths,
     }
 }
 
-void history_t::get_string_representation(wcstring *result, const wcstring &separator) {
+void history_t::get_history(wcstring_list_t &result) {
     scoped_lock locker(lock);
 
-    bool first = true;
-
-    std::unordered_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;
+    std::unordered_set<wcstring> seen;
 
     // 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. See
@@ -863,12 +860,7 @@ void history_t::get_string_representation(wcstring *result, const wcstring &sepa
             continue;
         }
 
-        // Skip duplicates.
-        if (!seen.insert(iter->str()).second) continue;
-
-        if (!first) result->append(separator);
-        result->append(iter->str());
-        first = false;
+        if (seen.insert(iter->str()).second) result.push_back(iter->str());
     }
 
     // Append old items.
@@ -879,12 +871,7 @@ void history_t::get_string_representation(wcstring *result, const wcstring &sepa
         const history_item_t item =
             decode_item(mmap_start + offset, mmap_length - offset, mmap_type);
 
-        // Skip duplicates.
-        if (!seen.insert(item.str()).second) continue;
-
-        if (!first) result->append(separator);
-        result->append(item.str());
-        first = false;
+        if (seen.insert(item.str()).second) result.push_back(item.str());
     }
 }
 

src/history.h

@@ -257,9 +257,9 @@ class history_t {
     // Incorporates the history of other shells into this history.
     void incorporate_external_changes();
 
-    // Gets all the history into a string. This is intended for the $history environment variable.
+    // Gets all the history into a list. This is intended for the $history environment variable.
     // This may be long!
-    void get_string_representation(wcstring *result, const wcstring &separator);
+    void get_history(wcstring_list_t &result);
 
     // 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);

tests/history.expect

@@ -30,7 +30,7 @@ expect_prompt
 send "echo start1; builtin history; echo end1\r"
 expect_prompt -re {start1\r\nend1\r\n} {
     puts "empty history detected as expected"
-} unmatched {
+} timeout {
     puts stderr "empty history not detected as expected"
 }
 
@@ -39,7 +39,7 @@ expect_prompt -re {start1\r\nend1\r\n} {
 send "echo start2; builtin history; echo end2\r"
 expect_prompt -re {start2\r\necho start1; builtin history; echo end1\r\nend2\r\n} {
     puts "first history command detected as expected"
-} unmatched {
+} timeout {
     puts stderr "first history command not detected as expected"
 }
 
@@ -53,7 +53,7 @@ expect_prompt -re {start2\r\necho start1; builtin history; echo end1\r\nend2\r\n
 send "history search echo start\r"
 expect_prompt -re {\r\necho start1.*\r\necho start2} {
     puts "history function explicit search succeeded"
-} unmatched {
+} timeout {
     puts stderr "history function explicit search failed"
 }
 
@@ -62,7 +62,7 @@ expect_prompt -re {\r\necho start1.*\r\necho start2} {
 send "history -p 'echo start'\r"
 expect_prompt -re {\r\necho start2.*\r\necho start1} {
     puts "history function implicit search succeeded"
-} unmatched {
+} timeout {
     puts stderr "history function implicit search failed"
 }
 
@@ -71,7 +71,7 @@ expect_prompt -re {\r\necho start2.*\r\necho start1} {
 send "history search --show-time='# %F %T%n' --prefix 'echo start'\r"
 expect_prompt -re {\r\n# \d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d\r\necho start2; .*\r\n# \d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d\r\necho start1; } {
     puts "history function implicit search with timestamps succeeded"
-} unmatched {
+} timeout {
     puts stderr "history function implicit search with timestamps failed"
 }
 
@@ -90,14 +90,14 @@ expect_prompt
 send "history search --exact 'echo goodbye'\r"
 expect_prompt -re {\r\necho goodbye\r\n} {
     puts "history function explicit exact search 'echo goodbye' succeeded"
-} unmatched {
+} timeout {
     puts stderr "history function explicit exact search 'echo goodbye' failed"
 }
 
 send "history search --exact 'echo hello'\r"
 expect_prompt -re {\r\necho hello\r\n} {
     puts "history function explicit exact search 'echo hello' succeeded"
-} unmatched {
+} timeout {
     puts stderr "history function explicit exact search 'echo hello' failed"
 }
 
@@ -106,7 +106,7 @@ expect_prompt -re {\r\necho hello\r\n} {
 send "history search --exact 'echo hell'\r"
 expect_prompt -re {history search --exact 'echo hell'\r\n} {
     puts "history function explicit exact search 'echo hell' succeeded"
-} unmatched {
+} timeout {
     puts stderr "history function explicit exact search 'echo hell' failed"
 }
 
@@ -117,7 +117,7 @@ expect -re {history delete -e -C 'echo hello'\r\n}
 send "echo count hello (history search -e -C 'echo hello' | wc -l | string trim)\r"
 expect -re {\r\ncount hello 0\r\n} {
     puts "history function explicit exact delete 'echo hello' succeeded"
-} unmatched {
+} timeout {
     puts stderr "history function explicit exact delete 'echo hello' failed"
 }
 
@@ -139,13 +139,21 @@ expect -re {Deleting history entry 1: "echo hello AGAIN"\r\n}
 send "echo count AGAIN (history search -e -C 'echo hello AGAIN' | wc -l | string trim)\r"
 expect -re {\r\ncount AGAIN 0\r\n} {
     puts "history function explicit prefix delete 'echo hello AGAIN' succeeded"
-} unmatched {
+} timeout {
     puts stderr "history function explicit prefix delete 'echo hello AGAIN' failed"
 }
 
 send "echo count again (history search -e -C 'echo hello again' | wc -l | string trim)\r"
 expect -re {\r\ncount again 1\r\n} {
     puts "history function explicit exact search 'echo hello again' succeeded"
-} unmatched {
+} timeout {
     puts stderr "history function explicit exact search 'echo hello again' failed"
 }
+
+# Verify that the $history var has the expected content.
+send "echo history2=\$history\[2\]\r"
+expect -re {\r\nhistory2=echo count AGAIN .*\r\n} {
+    puts "history\[2\] had the correct data"
+} timeout {
+    puts stderr "history\[2\] had the wrong data"
+}

tests/history.expect.out

@@ -9,3 +9,4 @@ history function explicit exact search 'echo hell' succeeded
 history function explicit exact delete 'echo hello' succeeded
 history function explicit prefix delete 'echo hello AGAIN' succeeded
 history function explicit exact search 'echo hello again' succeeded
+history[2] had the correct data