fix history --delete regression

The recent change to reconcile the history builtin command and function
broke an undocumented behavior of `history --delete`. This change
reinstates that behavior. It also adds an explicit `--exact` search mode
for the `--search` and `--delete` subcommands.

Fixes #3270

doc_src/history.txt

@@ -2,8 +2,8 @@
 
 \subsection history-synopsis Synopsis
 \fish{synopsis}
-history ( -s | --search ) [ -t | --with-time ] [ -p | --prefix | -c | --contains ] [ "search string"... ]
-history ( -d | --delete ) [ -t | --with-time ] [ -p | --prefix | -c | --contains ] "search string"...
+history ( -s | --search ) [ -t | --with-time ] [ -e | --exact | -p | --prefix | -c | --contains ] [ "search string"... ]
+history ( -d | --delete ) [ -t | --with-time ] [ -e | --exact | -p | --prefix | -c | --contains ] "search string"...
 history ( -m | --merge )
 history ( -s | --save )
 history ( -l | --clear )
@@ -24,13 +24,15 @@ The following commands are available:
 
 - `-v` or `--save` saves all changes in the history file. The shell automatically saves the history file; this option is provided for internal use.
 
-- `-l` or `--clear` clears the history file. A prompt is displayed before the history is erased asking you to confirm you really want to clear all history.
+- `-l` or `--clear` clears the history file. A prompt is displayed before the history is erased asking you to confirm you really want to clear all history unless `builtin history` is used.
 
 The following options are available:
 
-- `-p` or `--prefix` searches or deletes items in the history that begin with the specified text string.
+- `-c` or `--contains` searches or deletes items in the history that contain the specified text string. This is the default for the `--search` flag. This is not currently supported by the `--delete` flag.
 
-- `-c` or `--contains` searches or deletes items in the history that contain the specified text string. This is the default.
+- `-e` or `--exact` searches or deletes items in the history that exactly match the specified text string. This is the default for the `--delete` flag.
+
+- `-p` or `--prefix` searches or deletes items in the history that begin with the specified text string. This is not currently supported by the `--delete` flag.
 
 - `-t` or `--with-time` prefixes the output of each displayed history entry with the time it was recorded in the format "%Y-%m-%d %H:%M:%S" in your local timezone.
 

share/functions/history.fish

@@ -3,7 +3,7 @@
 #
 function history --shadow-builtin --description "display or manipulate interactive command history"
     set -l cmd
-    set -l search_mode --contains
+    set -l search_mode
     set -l with_time
 
     # The "set cmd $cmd xyz" lines are to make it easy to detect if the user specifies more than one
@@ -28,6 +28,8 @@ function history --shadow-builtin --description "display or manipulate interacti
                 set search_mode --prefix
             case -c --contains
                 set search_mode --contains
+            case -e --exact
+                set search_mode --exact
             case --
                 set -e argv[1]
                 break
@@ -46,6 +48,9 @@ function history --shadow-builtin --description "display or manipulate interacti
 
     switch $cmd
         case search
+            test -z "$search_mode"
+            and set search_mode "--contains"
+
             if isatty stdout
                 set -l pager less
                 set -q PAGER
@@ -58,10 +63,18 @@ function history --shadow-builtin --description "display or manipulate interacti
         case delete  # Interactively delete history
             # TODO: Fix this to deal with history entries that have multiple lines.
             if not set -q argv[1]
-                printf "You have to specify at least one search term to find entries to delete" >&2
+                printf (_ "You must specify at least one search term when deleting entries") >&2
                 return 1
             end
 
+            test -z "$search_mode"
+            and set search_mode "--exact"
+
+            if test $search_mode = "--exact"
+                builtin history --delete $search_mode $argv
+                return
+            end
+
             # TODO: Fix this so that requesting history entries with a timestamp works:
             #   set -l found_items (builtin history --search $search_mode $with_time -- $argv)
             set -l found_items (builtin history --search $search_mode -- $argv)

src/builtin.cpp

@@ -2866,15 +2866,21 @@ static int builtin_history(parser_t &parser, io_streams_t &streams, wchar_t **ar
     ;
     int argc = builtin_count_args(argv);
     hist_cmd_t hist_cmd = HIST_NOOP;
-    history_search_type_t search_type = HISTORY_SEARCH_TYPE_CONTAINS;
+    history_search_type_t search_type = (history_search_type_t)-1;
+    bool history_search_type_defined = false;
     bool with_time = false;
 
-    static const struct woption long_options[] = {
-        {L"delete", no_argument, 0, 'd'},    {L"search", no_argument, 0, 's'},
-        {L"prefix", no_argument, 0, 'p'},    {L"contains", no_argument, 0, 'c'},
-        {L"save", no_argument, 0, 'v'},      {L"clear", no_argument, 0, 'l'},
-        {L"merge", no_argument, 0, 'm'},     {L"help", no_argument, 0, 'h'},
-        {L"with-time", no_argument, 0, 't'}, {0, 0, 0, 0}};
+    static const struct woption long_options[] = {{L"delete", no_argument, 0, 'd'},
+                                                  {L"search", no_argument, 0, 's'},
+                                                  {L"prefix", no_argument, 0, 'p'},
+                                                  {L"contains", no_argument, 0, 'c'},
+                                                  {L"save", no_argument, 0, 'v'},
+                                                  {L"clear", no_argument, 0, 'l'},
+                                                  {L"merge", no_argument, 0, 'm'},
+                                                  {L"help", no_argument, 0, 'h'},
+                                                  {L"with-time", no_argument, 0, 't'},
+                                                  {L"exact", no_argument, 0, 'e'},
+                                                  {0, 0, 0, 0}};
 
     history_t *history = reader_get_history();
     // Use the default history if we have none (which happens if invoked non-interactively, e.g.
@@ -2884,7 +2890,7 @@ static int builtin_history(parser_t &parser, io_streams_t &streams, wchar_t **ar
     int opt = 0;
     int opt_index = 0;
     wgetopter_t w;
-    while ((opt = w.wgetopt_long(argc, argv, L"+dspcvlmht", long_options, &opt_index)) != EOF) {
+    while ((opt = w.wgetopt_long(argc, argv, L"+despcvlmht", long_options, &opt_index)) != EOF) {
         switch (opt) {
             case 's': {
                 if (!set_hist_cmd(cmd, &hist_cmd, HIST_SEARCH, streams)) {
@@ -2918,10 +2924,17 @@ static int builtin_history(parser_t &parser, io_streams_t &streams, wchar_t **ar
             }
             case 'p': {
                 search_type = HISTORY_SEARCH_TYPE_PREFIX;
+                history_search_type_defined = true;
                 break;
             }
             case 'c': {
                 search_type = HISTORY_SEARCH_TYPE_CONTAINS;
+                history_search_type_defined = true;
+                break;
+            }
+            case 'e': {
+                search_type = HISTORY_SEARCH_TYPE_EXACT;
+                history_search_type_defined = true;
                 break;
             }
             case 't': {
@@ -2946,7 +2959,12 @@ static int builtin_history(parser_t &parser, io_streams_t &streams, wchar_t **ar
     // Everything after the flags is an argument for a subcommand (e.g., a search term).
     const wcstring_list_t args(argv + w.woptind, argv + argc);
 
+    // Establish appropriate defaults for unspecified options.
     if (hist_cmd == HIST_NOOP) hist_cmd = HIST_SEARCH;
+    if (!history_search_type_defined) {
+        if (hist_cmd == HIST_SEARCH) search_type = HISTORY_SEARCH_TYPE_CONTAINS;
+        if (hist_cmd == HIST_DELETE) search_type = HISTORY_SEARCH_TYPE_EXACT;
+    }
 
     int status = STATUS_BUILTIN_OK;
     switch (hist_cmd) {
@@ -2957,11 +2975,19 @@ static int builtin_history(parser_t &parser, io_streams_t &streams, wchar_t **ar
             break;
         }
         case HIST_DELETE: {
+            // TODO: Move this code to the history module and support the other search types. At
+            // this time we expect the non-exact deletions to be handled only by the history
+            // function's interactive delete feature.
+            if (search_type != HISTORY_SEARCH_TYPE_EXACT) {
+                streams.err.append_format(_(L"builtin history --delete only supports --exact\n"));
+                status = STATUS_BUILTIN_ERROR;
+                break;
+            }
             for (wcstring_list_t::const_iterator iter = args.begin(); iter != args.end(); ++iter) {
                 wcstring delete_string = *iter;
-                if (delete_string[0] == '"' && delete_string[delete_string.length() - 1] == '"')
+                if (delete_string[0] == '"' && delete_string[delete_string.length() - 1] == '"') {
                     delete_string = delete_string.substr(1, delete_string.length() - 2);
-
+                }
                 history->remove(delete_string);
             }
             break;

src/history.cpp

@@ -30,7 +30,6 @@
 #include "parse_tree.h"
 #include "path.h"
 #include "reader.h"
-#include "sanity.h"
 #include "signal.h"
 #include "wutil.h"  // IWYU pragma: keep
 
@@ -438,19 +437,17 @@ 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) const {
     switch (type) {
         case HISTORY_SEARCH_TYPE_CONTAINS: {
-            // We consider equal strings to NOT match a contains search (so that you don't have to
-            // see history equal to what you typed). The length check ensures that.
-            return contents.size() > term.size() && contents.find(term) != wcstring::npos;
+            return contents.find(term) != wcstring::npos;
         }
         case HISTORY_SEARCH_TYPE_PREFIX: {
             // We consider equal strings to match a prefix search, so that autosuggest will allow
             // suggesting what you've typed.
             return string_prefixes_string(term, contents);
         }
-        default: {
-            sanity_lose();
-            abort();
+        case HISTORY_SEARCH_TYPE_EXACT: {
+            return term == contents;
         }
+        default: { DIE("unexpected history_search_type_t value"); }
     }
 }
 

src/history.h

@@ -40,9 +40,11 @@ struct io_streams_t;
 typedef std::vector<wcstring> path_list_t;
 
 enum history_search_type_t {
-    // The history searches for strings containing the given string.
+    // Search for commands exactly matching the given string.
+    HISTORY_SEARCH_TYPE_EXACT = 1,
+    // Search for commands containing the given string.
     HISTORY_SEARCH_TYPE_CONTAINS,
-    // The history searches for strings starting with the given string.
+    // Search for commands starting with the given string.
     HISTORY_SEARCH_TYPE_PREFIX
 };
 

src/reader.cpp

@@ -2873,11 +2873,15 @@ const wchar_t *reader_readline(int nchars) {
                     data->history_search = history_search_t(*data->history, data->search_buff,
                                                             HISTORY_SEARCH_TYPE_CONTAINS);
 
-                    // Skip the autosuggestion as history unless it was truncated.
+                    // Always skip history entries that exactly match what has been typed so far.
+                    wcstring_list_t skip_list;
+                    skip_list.push_back(data->command_line.text);
                     const wcstring &suggest = data->autosuggestion;
                     if (!suggest.empty() && !data->screen.autosuggestion_is_truncated) {
-                        data->history_search.skip_matches(wcstring_list_t(&suggest, 1 + &suggest));
+                        // Also skip the autosuggestion in the history unless it was truncated.
+                        skip_list.push_back(suggest);
                     }
+                    data->history_search.skip_matches(skip_list);
                 }
 
                 switch (data->search_mode) {

tests/history.expect

@@ -83,3 +83,80 @@ expect_prompt -re {\r\n\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d  echo start1; builtin h
 } unmatched {
     puts stderr "history function implicit search with timestamps failed"
 }
+
+# ==========
+# Verify explicit searching for an exact command returns just that command.
+# returns the expected results.
+send "echo hello\r"
+expect_prompt
+send "echo goodbye\r"
+expect_prompt
+send "echo hello again\r"
+expect_prompt
+send "echo hello AGAIN\r"
+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 {
+    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 {
+    puts stderr "history function explicit exact search 'echo hello' failed"
+}
+
+# This is slightly subtle in that it shouldn't actually match anything between
+# the command we sent and the next prompt.
+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 {
+    puts stderr "history function explicit exact search 'echo hell' failed"
+}
+
+# ==========
+# Delete a single command we recently ran.
+send "history --delete 'echo hello'\r"
+expect_prompt -re {history --delete 'echo hello'\r\n} {
+    puts "history function explicit exact delete 'echo hello' succeeded"
+} unmatched {
+    puts stderr "history function explicit exact delete 'echo hello' failed"
+}
+
+# ==========
+# Interactively delete one of multiple matched commands. This verifies that we
+# delete the first entry matched by the prefix search (the most recent command
+# sent above that matches).
+send "history --delete -p 'echo hello'\r"
+expect -re {history --delete -p 'echo hello'\r\n}
+expect -re {\[1\] echo hello AGAIN\r\n}
+expect -re {\[2\] echo hello again\r\n\r\n}
+expect -re {Enter nothing to cancel.*\r\nEnter "all" to delete all the matching entries\.\r\n}
+expect -re {Delete which entries\? >}
+send "1\r"
+expect_prompt -re {Deleting history entry 1: "echo hello AGAIN"\r\n} {
+    puts "history function explicit prefix delete 'echo hello' succeeded"
+} unmatched {
+    puts stderr "history function explicit prefix delete 'echo hello' failed"
+}
+
+# Verify that the deleted history entry is gone and the other one that matched
+# the prefix search above is still there.
+send "history --search --exact 'echo hello again'\r"
+expect_prompt -re {\r\necho hello again\r\n} {
+    puts "history function explicit exact search 'echo hello again' succeeded"
+} unmatched {
+    puts stderr "history function explicit exact search 'echo hello again' failed"
+}
+
+send "history --search --exact 'echo hello AGAIN'\r"
+expect_prompt -re {\r\necho hello AGAIN\r\n} {
+    puts stderr "history function explicit exact search 'echo hello AGAIN' found the entry"
+} unmatched {
+    puts "history function explicit exact search 'echo hello AGAIN' failed to find the entry"
+}

tests/history.expect.out

@@ -4,3 +4,10 @@ invalid attempt at multiple history commands detected
 history function explicit search succeeded
 history function implicit search succeeded
 history function implicit search with timestamps succeeded
+history function explicit exact search 'echo goodbye' succeeded
+history function explicit exact search 'echo hello' succeeded
+history function explicit exact search 'echo hell' succeeded
+history function explicit exact delete 'echo hello' succeeded
+history function explicit prefix delete 'echo hello' succeeded
+history function explicit exact search 'echo hello again' succeeded
+history function explicit exact search 'echo hello AGAIN' failed to find the entry