fix echo -h

In addition to fixing `echo -h` this includes some debugging related
cleanups I made while investigating the issue.

Fixes #4120

CHANGELOG.md

@@ -12,6 +12,7 @@
 - Improved completions for `killall` (#4052), `ln` (#4090) and `zypper` (#4079).
 - Implemented `string lower` and `string upper` (#4080).
 - `help` can now open the tutorial.
+- `echo -h` now correctly echoes `-h` (#4120).
 
 ---
 

src/builtin.cpp

@@ -474,11 +474,10 @@ void builtin_destroy() {}
 /// Is there a builtin command with the given name?
 bool builtin_exists(const wcstring &cmd) { return static_cast<bool>(builtin_lookup(cmd)); }
 
-/// If builtin takes care of printing help itself
+/// Is the command a keyword or a builtin we need to special-case the handling of `-h` and `--help`.
 static const wcstring_list_t help_builtins({L"for", L"while", L"function", L"if", L"end", L"switch",
                                             L"case", L"count", L"printf"});
-static bool builtin_handles_help(const wchar_t *cmd) {
-    CHECK(cmd, 0);
+static bool cmd_needs_help(const wchar_t *cmd) {
     return contains(help_builtins, cmd);
 }
 
@@ -488,14 +487,16 @@ int builtin_run(parser_t &parser, const wchar_t *const *argv, io_streams_t &stre
     UNUSED(streams);
     if (argv == NULL || argv[0] == NULL) return STATUS_INVALID_ARGS;
 
-    const builtin_data_t *data = builtin_lookup(argv[0]);
-    if (argv[1] != NULL && !builtin_handles_help(argv[0]) && argv[2] == NULL &&
-        parse_util_argument_is_help(argv[1], 0)) {
+    // We can be handed a keyword by the parser as if it was a command. This happens when the user
+    // follows the keyword by `-h` or `--help`. Since it isn't really a builtin command we need to
+    // handle displaying help for it here.
+    if (argv[1] && !argv[2] && parse_util_argument_is_help(argv[1]) && cmd_needs_help(argv[0])) {
         builtin_print_help(parser, streams, argv[0], streams.out);
         return STATUS_CMD_OK;
     }
 
-    if (data != NULL) {
+    const builtin_data_t *data = builtin_lookup(argv[0]);
+    if (data) {
         // Warning: layering violation and naughty cast. The code originally had a much more
         // complicated solution to achieve exactly the same result: lie about the constness of argv.
         // Some of the builtins we call do mutate the array via their calls to wgetopt() which could

src/builtin_echo.cpp

@@ -13,7 +13,6 @@
 #include "wutil.h"  // IWYU pragma: keep
 
 struct echo_cmd_opts_t {
-    bool print_help = false;
     bool print_newline = true;
     bool print_spaces = true;
     bool interpret_special_chars = false;

src/event.cpp

@@ -247,7 +247,7 @@ static wcstring event_desc_compact(const event_t &event) {
 void event_add_handler(const event_t &event) {
     if (debug_level >= 3) {
         wcstring desc = event_desc_compact(event);
-        debug(3, "register: %ls\n", desc.c_str());
+        debug(3, "register: %ls", desc.c_str());
     }
 
     shared_ptr<event_t> e = std::make_shared<event_t>(event);
@@ -262,7 +262,7 @@ void event_add_handler(const event_t &event) {
 void event_remove(const event_t &criterion) {
     if (debug_level >= 3) {
         wcstring desc = event_desc_compact(criterion);
-        debug(3, "unregister: %ls\n", desc.c_str());
+        debug(3, "unregister: %ls", desc.c_str());
     }
 
     event_list_t::iterator iter = s_event_handlers.begin();

src/exec.cpp

@@ -1064,7 +1064,7 @@ void exec_job(parser_t &parser, job_t *j) {
                         // safe_launch_process _never_ returns...
                         DIE("safe_launch_process should not have returned");
                     } else {
-                        debug(2, L"Fork #%d, pid %d: external command '%s' from '%ls'\n",
+                        debug(2, L"Fork #%d, pid %d: external command '%s' from '%ls'",
                               g_fork_count, pid, p->argv0(), file ? file : L"<no file>");
                         if (pid < 0) {
                             job_mark_process_as_failed(j, p);

src/iothread.cpp

@@ -121,7 +121,7 @@ static void *iothread_worker(void *unused) {
     UNUSED(unused);
     struct spawn_request_t req;
     while (dequeue_spawn_request(&req)) {
-        debug(5, "pthread %p dequeued\n", this_thread());
+        debug(5, "pthread %p dequeued", this_thread());
 
         // Perform the work
         req.handler();
@@ -146,7 +146,7 @@ static void *iothread_worker(void *unused) {
     int new_thread_count = --s_spawn_requests.acquire().value.thread_count;
     assert(new_thread_count >= 0);
 
-    debug(5, "pthread %p exiting\n", this_thread());
+    debug(5, "pthread %p exiting", this_thread());
     // We're done.
     return NULL;
 }
@@ -168,7 +168,7 @@ static void iothread_spawn() {
 
     // We will never join this thread.
     DIE_ON_FAILURE(pthread_detach(thread));
-    debug(5, "pthread %p spawned\n", (void *)(intptr_t)thread);
+    debug(5, "pthread %p spawned", (void *)(intptr_t)thread);
     // Restore our sigmask.
     DIE_ON_FAILURE(pthread_sigmask(SIG_SETMASK, &saved_set, NULL));
 }

src/output.cpp

@@ -168,7 +168,7 @@ void set_color(rgb_color_t c, rgb_color_t c2) {
 #if 0
     wcstring tmp = c.description();
     wcstring tmp2 = c2.description();
-    debug(3, "set_color %ls : %ls\n", tmp.c_str(), tmp2.c_str());
+    debug(3, "set_color %ls : %ls", tmp.c_str(), tmp2.c_str());
 #endif
     ASSERT_IS_MAIN_THREAD();
     if (!cur_term) return;

src/parse_execution.cpp

@@ -877,9 +877,7 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process(
         process_type = function_exists(L"cd") ? INTERNAL_FUNCTION : INTERNAL_BUILTIN;
     } else {
         const globspec_t glob_behavior = (cmd == L"set" || cmd == L"count") ? nullglob : failglob;
-        // Form the list of arguments. The command is the first argument. TODO: count hack, where we
-        // treat 'count --help' as different from 'count $foo' that expands to 'count --help'. fish
-        // 1.x never successfully did this, but it tried to!
+        // Form the list of arguments. The command is the first argument.
         parse_execution_result_t arg_result =
             this->determine_arguments(statement, &argument_list, glob_behavior);
         if (arg_result != parse_execution_success) {

src/parse_productions.cpp

@@ -436,7 +436,7 @@ const production_element_t *parse_productions::production_for_token(parse_token_
                                                                     const parse_token_t &input1,
                                                                     const parse_token_t &input2,
                                                                     parse_node_tag_t *out_tag) {
-    debug(5, "Resolving production for %ls with input token <%ls>\n",
+    debug(5, "Resolving production for %ls with input token <%ls>",
           token_type_description(node_type), input1.describe().c_str());
 
     // Fetch the function to resolve the list of productions.
@@ -504,7 +504,7 @@ const production_element_t *parse_productions::production_for_token(parse_token_
 
     const production_element_t *result = resolver(input1, input2, out_tag);
     if (result == NULL) {
-        debug(5, "Node type '%ls' has no production for input '%ls' (in %s)\n",
+        debug(5, "Node type '%ls' has no production for input '%ls' (in %s)",
               token_type_description(node_type), input1.describe().c_str(), __FUNCTION__);
     }
 

src/parse_util.cpp

@@ -754,13 +754,8 @@ static int parser_is_pipe_forbidden(const wcstring &word) {
     return contains(forbidden_pipe_commands, word);
 }
 
-bool parse_util_argument_is_help(const wchar_t *s, int min_match) {
-    CHECK(s, 0);
-    size_t len = wcslen(s);
-
-    min_match = maxi(min_match, 3);  //!OCLINT(parameter reassignment)
-
-    return wcscmp(L"-h", s) == 0 || (len >= (size_t)min_match && (wcsncmp(L"--help", s, len) == 0));
+bool parse_util_argument_is_help(const wchar_t *s) {
+    return wcscmp(L"-h", s) == 0 || wcscmp(L"--help", s) == 0;
 }
 
 /// Check if the first argument under the given node is --help.
@@ -773,7 +768,7 @@ static bool first_argument_is_help(const parse_node_tree_t &node_tree, const par
         // Check the first argument only.
         const parse_node_t &arg = *arg_nodes.at(0);
         const wcstring first_arg_src = arg.get_source(src);
-        is_help = parse_util_argument_is_help(first_arg_src.c_str(), 3);
+        is_help = parse_util_argument_is_help(first_arg_src.c_str());
     }
     return is_help;
 }

src/parse_util.h

@@ -99,12 +99,7 @@ size_t parse_util_get_offset(const wcstring &str, int line, long line_offset);
 wcstring parse_util_unescape_wildcards(const wcstring &in);
 
 /// Checks if the specified string is a help option.
-///
-/// \param s the string to test
-/// \param min_match is the minimum number of characters that must match in a long style option,
-/// i.e. the longest common prefix between --help and any other option. If less than 3, 3 will be
-/// assumed.
-bool parse_util_argument_is_help(const wchar_t *s, int min_match);
+bool parse_util_argument_is_help(const wchar_t *s);
 
 /// Calculates information on the parameter at the specified index.
 ///

src/proc.cpp

@@ -718,7 +718,7 @@ static int select_try(job_t *j) {
             // fwprintf( stderr, L"fd %d on job %ls\n", fd, j->command );
             FD_SET(fd, &fds);
             maxfd = maxi(maxfd, fd);
-            debug(3, L"select_try on %d\n", fd);
+            debug(3, L"select_try on %d", fd);
         }
     }
 
@@ -731,7 +731,7 @@ static int select_try(job_t *j) {
 
         retval = select(maxfd + 1, &fds, 0, 0, &tv);
         if (retval == 0) {
-            debug(3, L"select_try hit timeout\n");
+            debug(3, L"select_try hit timeout");
         }
         return retval > 0;
     }
@@ -755,7 +755,7 @@ static void read_try(job_t *j) {
     }
 
     if (buff) {
-        debug(3, L"proc::read_try('%ls')\n", j->command_wcstr());
+        debug(3, L"proc::read_try('%ls')", j->command_wcstr());
         while (1) {
             char b[BUFFER_SIZE];
             long l;