make it hard to use history command incorrectly

Fixes #3307

share/functions/history.fish

@@ -63,7 +63,7 @@ function history --description "display or manipulate interactive command histor
         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 must specify at least one search term when deleting entries") >&2
+                printf (_ "You must specify at least one search term when deleting entries\n") >&2
                 return 1
             end
 
@@ -118,9 +118,19 @@ function history --description "display or manipulate interactive command histor
             end
 
         case save
+            if test -n "$search_mode"
+                or test -n "$with_time"
+                printf (_ "history: you cannot use any options with %s command\n") save >&2
+                return 1
+            end
             builtin history --save -- $argv
 
         case merge
+            if test -n "$search_mode"
+                or test -n "$with_time"
+                printf (_ "history: you cannot use any options with %s command\n") merge >&2
+                return 1
+            end
             builtin history --merge -- $argv
 
         case help
@@ -128,11 +138,19 @@ function history --description "display or manipulate interactive command histor
 
         case clear
             # Erase the entire history.
-            read --local --prompt "echo 'Are you sure you want to clear history? (y/n) '" choice
-            if test "$choice" = "y"
-                or test "$choice" = "yes"
+            if test -n "$search_mode"
+                or test -n "$with_time"
+                printf (_ "history: you cannot use any options with %s command\n") clear >&2
+                return 1
+            end
+
+            printf (_ "If you enter 'yes' your entire interactive command history will be erased\n")
+            read --local --prompt "echo 'Are you sure you want to clear history? (yes/no) '" choice
+            if test "$choice" = "yes"
                 builtin history --clear -- $argv
-                and echo "History cleared!"
+                and printf (_ "Command history cleared!")
+            else
+                printf (_ "You did not say 'yes' so I will not clear your command history\n")
             end
     end
 end

src/builtin.cpp

@@ -2826,7 +2826,15 @@ static bool set_hist_cmd(wchar_t *const cmd, hist_cmd_t *hist_cmd, hist_cmd_t su
     return true;
 }
 
-#define CHECK_FOR_UNEXPECTED_HIST_ARGS()                                                 \
+#define CHECK_FOR_UNEXPECTED_HIST_OPTIONS(hist_cmd)                                             \
+    if (history_search_type_defined || with_time) {                                             \
+        streams.err.append_format(_(L"history: you cannot use any options with %ls command\n"), \
+                                  hist_cmd_to_string(hist_cmd).c_str());                        \
+        status = STATUS_BUILTIN_ERROR;                                                          \
+        break;                                                                                  \
+    }
+
+#define CHECK_FOR_UNEXPECTED_HIST_ARGS(hist_cmd)                                         \
     if (args.size() != 0) {                                                              \
         streams.err.append_format(BUILTIN_ERR_ARG_COUNT, cmd,                            \
                                   hist_cmd_to_string(hist_cmd).c_str(), 0, args.size()); \
@@ -2837,7 +2845,6 @@ static bool set_hist_cmd(wchar_t *const cmd, hist_cmd_t *hist_cmd, hist_cmd_t su
 /// Manipulate history of interactive commands executed by the user.
 static int builtin_history(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     wchar_t *cmd = argv[0];
-    ;
     int argc = builtin_count_args(argv);
     hist_cmd_t hist_cmd = HIST_NOOP;
     history_search_type_t search_type = (history_search_type_t)-1;
@@ -2967,18 +2974,21 @@ static int builtin_history(parser_t &parser, io_streams_t &streams, wchar_t **ar
             break;
         }
         case HIST_CLEAR: {
-            CHECK_FOR_UNEXPECTED_HIST_ARGS();
+            CHECK_FOR_UNEXPECTED_HIST_OPTIONS(hist_cmd)
+            CHECK_FOR_UNEXPECTED_HIST_ARGS(hist_cmd)
             history->clear();
             history->save();
             break;
         }
         case HIST_MERGE: {
-            CHECK_FOR_UNEXPECTED_HIST_ARGS();
+            CHECK_FOR_UNEXPECTED_HIST_OPTIONS(hist_cmd)
+            CHECK_FOR_UNEXPECTED_HIST_ARGS(hist_cmd)
             history->incorporate_external_changes();
             break;
         }
         case HIST_SAVE: {
-            CHECK_FOR_UNEXPECTED_HIST_ARGS();
+            CHECK_FOR_UNEXPECTED_HIST_OPTIONS(hist_cmd)
+            CHECK_FOR_UNEXPECTED_HIST_ARGS(hist_cmd)
             history->save();
             break;
         }

tests/history.err

@@ -0,0 +1,7 @@
+history: you cannot use any options with clear command
+history: you cannot use any options with merge command
+history: save command expected 0 args, got 1
+history: you cannot use any options with save command
+history: you cannot use any options with clear command
+history: merge command expected 0 args, got 1
+history: clear command expected 0 args, got 2

tests/history.in

@@ -0,0 +1,13 @@
+# Verify that specifying unexpected options or arguments results in an error.
+# First with the history function.
+history --clear --contains
+history --merge -t
+history --save xyz
+# Now with the history builtin.
+builtin history --save --prefix
+builtin history --clear --with-time
+builtin history --merge xyz
+builtin history --clear abc def
+
+# Now do a history command that should succeed.
+builtin history --merge