make kill/pkill completions more robust (#3200)

Someone running fish in an unusual locale reported that an `assert()` was
firing when they typed `pkill c`. I traced it to two bugs. First, the
__fish_make_completion_signals command was producing a weird result. Second,
the builtin `complete` command wasn't adequately verifying its arguments.

Fixes #3129

share/functions/__fish_make_completion_signals.fish

@@ -1,17 +1,31 @@
 function __fish_make_completion_signals --description 'Make list of kill signals for completion'
-    set -q __kill_signals; and return 0
+    set -q __kill_signals
+    and return 0
 
-    if kill -L ^/dev/null >/dev/null
+    # Some systems use the GNU coreutils kill command where `kill -L` produces an extended table
+    # format that looks like this:
+    #
+    #  1 HUP    Hangup: 1
+    #  2 INT    Interrupt: 2
+    #
+    # The procps `kill -L` produces a more compact table. We can distinguish the two cases by
+    # testing whether it supports `kill -t`; in which case it is the coreutils `kill` command.
+    if kill -t ^/dev/null >/dev/null
+        # Posix systems print out the name of a signal using 'kill -l SIGNUM'.
+        complete -c kill -s l --description "List names of available signals"
+        for i in (seq 31)
+            set -g __kill_signals $__kill_signals $i" "(kill -l $i | tr '[:lower:]' '[:upper:]')
+        end
+    else
         # Debian and some related systems use 'kill -L' to write out a numbered list
         # of signals. Use this to complete on both number _and_ on signal name.
         complete -c kill -s L --description "List codes and names of available signals"
-        set -g __kill_signals (kill -L | sed -e 's/\([0-9][0-9]*\)  *\([A-Z,0-9][A-Z,0-9]*\)/\1 \2\n/g;s/ +/ /g' | sed -e 's/^ \+//' | __fish_sgrep -E '^[^ ]+')
+        set -g __kill_signals
-    else
+        kill -L | sed -e 's/^ //; s/  */ /g; y/ /\n/' | while read -l signo
-        # Posix systems print out the name of a signal using 'kill -l
+            test -z "$signo"
-        # SIGNUM', so we use this instead.
+            and break  # the sed above produces one blank line at the end
-        complete -c kill -s l --description "List names of available signals"
+            read -l signame
-        for i in (seq 31)
+            set -g __kill_signals $__kill_signals "$signo $signame"
-            set -g __kill_signals $__kill_signals $i" "(kill -l $i)
         end
     end
 end

src/builtin_complete.cpp

@@ -208,6 +208,10 @@ int builtin_complete(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
             }
             case 'd': {
                 desc = w.woptarg;
+                if (w.woptarg[0] == '\0') {
+                    streams.err.append_format(L"%ls: -d requires a non-empty string\n", argv[0]);
+                    res = true;
+                }
                 break;
             }
             case 'u': {
@@ -220,14 +224,26 @@ int builtin_complete(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
             }
             case 's': {
                 short_opt.append(w.woptarg);
+                if (w.woptarg[0] == '\0') {
+                    streams.err.append_format(L"%ls: -s requires a non-empty string\n", argv[0]);
+                    res = true;
+                }
                 break;
             }
             case 'l': {
                 gnu_opt.push_back(w.woptarg);
+                if (w.woptarg[0] == '\0') {
+                    streams.err.append_format(L"%ls: -l requires a non-empty string\n", argv[0]);
+                    res = true;
+                }
                 break;
             }
             case 'o': {
                 old_opt.push_back(w.woptarg);
+                if (w.woptarg[0] == '\0') {
+                    streams.err.append_format(L"%ls: -o requires a non-empty string\n", argv[0]);
+                    res = true;
+                }
                 break;
             }
             case 'a': {

tests/test6.err

@@ -0,0 +1,4 @@
+complete: -o requires a non-empty string
+complete: -d requires a non-empty string
+complete: -l requires a non-empty string
+complete: -s requires a non-empty string

tests/test6.in

@@ -1,5 +1,12 @@
 # vim: set filetype=fish:
 
+# Regression test for issue #3129. In previous versions these statements would
+# cause an `assert()` to fire thus killing the shell.
+complete -c pkill -o ''
+complete -c pkill -d ''
+complete -c pkill -l ''
+complete -c pkill -s ''
+
 # Test that conditions that add or remove completions don't deadlock, etc.
 # We actually encountered some case that was effectively like this (Issue 2 in github)