modify read to require at least one var

Fixes #4220

doc_src/read.txt

@@ -2,12 +2,12 @@
 
 \subsection read-synopsis Synopsis
 \fish{synopsis}
-read [OPTIONS] [VARIABLES...]
+read [OPTIONS] VARIABLES...
 \endfish
 
 \subsection read-description Description
 
-`read` reads from standard input and stores the result in one or more shell variables. By default it reads one line terminated by a newline but options are available to read up to a null character and to limit each "line" to a maximum number of characters.
+`read` reads from standard input and stores the result in one or more shell variables. By default it reads one line terminated by a newline but options are available to read up to a null character and to limit each "line" to a maximum number of characters. At least one variable name must be given. This command does not default to populating a var named `REPLY` like other shells do.
 
 The following options are available:
 

src/builtin.h

@@ -51,8 +51,8 @@ enum { COMMAND_NOT_BUILTIN, BUILTIN_REGULAR, BUILTIN_FUNCTION };
 #define BUILTIN_ERR_UNKNOWN _(L"%ls: Unknown option '%ls'\n")
 
 /// Error message for unexpected args.
-#define BUILTIN_ERR_ARG_COUNT1 _(L"%ls: expected %d args, got %d\n")
+#define BUILTIN_ERR_ARG_COUNT1 _(L"%ls: Expected %d args, got %d\n")
-#define BUILTIN_ERR_ARG_COUNT2 _(L"%ls: %ls expected %d args, got %d\n")
+#define BUILTIN_ERR_ARG_COUNT2 _(L"%ls %ls: Expected %d args, got %d\n")
 #define BUILTIN_ERR_MIN_ARG_COUNT1 _(L"%ls: Expected at least %d args, got only %d\n")
 #define BUILTIN_ERR_MAX_ARG_COUNT1 _(L"%ls: Expected at most %d args, got %d\n")
 

src/builtin_read.cpp

@@ -19,7 +19,6 @@
 #include "complete.h"
 #include "env.h"
 #include "event.h"
-#include "expand.h"
 #include "fallback.h"  // IWYU pragma: keep
 #include "highlight.h"
 #include "history.h"
@@ -320,23 +319,9 @@ static int read_one_char_at_a_time(int fd, wcstring &buff, int nchars, bool spli
     return exit_res;
 }
 
-/// The read builtin. Reads from stdin and stores the values in environment variables.
+/// Validate the arguments given to `read` and provide defaults where needed.
-int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
+static int validate_read_args(const wchar_t *cmd, read_cmd_opts_t &opts, int argc,
-    wchar_t *cmd = argv[0];
+                              const wchar_t *const *argv, parser_t &parser, io_streams_t &streams) {
-    int argc = builtin_count_args(argv);
-    wcstring buff;
-    int exit_res = STATUS_CMD_OK;
-    read_cmd_opts_t opts;
-
-    int optind;
-    int retval = parse_cmd_opts(opts, &optind, argc, argv, parser, streams);
-    if (retval != STATUS_CMD_OK) return retval;
-
-    if (opts.print_help) {
-        builtin_print_help(parser, streams, cmd, streams.out);
-        return STATUS_CMD_OK;
-    }
-
     if (opts.prompt && opts.prompt_str) {
         streams.err.append_format(_(L"%ls: You can't specify both -p and -P\n"), cmd);
         builtin_print_help(parser, streams, cmd, streams.err);
@@ -364,15 +349,18 @@ int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         return STATUS_INVALID_ARGS;
     }
 
-    if (opts.array && optind + 1 != argc) {
+    if (!opts.array && argc < 1) {
-        streams.err.append_format(_(L"%ls: --array option requires a single variable name.\n"),
+        streams.err.append_format(BUILTIN_ERR_MIN_ARG_COUNT1, cmd, 1, argc);
-                                  cmd);
+        return STATUS_INVALID_ARGS;
-        builtin_print_help(parser, streams, cmd, streams.err);
+    }
+
+    if (opts.array && argc != 1) {
+        streams.err.append_format(BUILTIN_ERR_ARG_COUNT1, cmd, 1, argc);
         return STATUS_INVALID_ARGS;
     }
 
     // Verify all variable names.
-    for (int i = optind; i < argc; i++) {
+    for (int i = 0; i < argc; i++) {
         if (!valid_var_name(argv[i])) {
             streams.err.append_format(BUILTIN_ERR_VARNAME, cmd, argv[i]);
             builtin_print_help(parser, streams, cmd, streams.err);
@@ -380,6 +368,31 @@ int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         }
     }
 
+    return STATUS_CMD_OK;
+}
+
+/// The read builtin. Reads from stdin and stores the values in environment variables.
+int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
+    wchar_t *cmd = argv[0];
+    int argc = builtin_count_args(argv);
+    wcstring buff;
+    int exit_res = STATUS_CMD_OK;
+    read_cmd_opts_t opts;
+
+    int optind;
+    int retval = parse_cmd_opts(opts, &optind, argc, argv, parser, streams);
+    if (retval != STATUS_CMD_OK) return retval;
+    argc -= optind;
+    argv += optind;
+
+    if (opts.print_help) {
+        builtin_print_help(parser, streams, cmd, streams.out);
+        return STATUS_CMD_OK;
+    }
+
+    retval = validate_read_args(cmd, opts, argc, argv, parser, streams);
+    if (retval != STATUS_CMD_OK) return retval;
+
     // TODO: Determine if the original set of conditions for interactive reads should be reinstated:
     // if (isatty(0) && streams.stdin_fd == STDIN_FILENO && !split_null) {
     int stream_stdin_is_a_tty = isatty(streams.stdin_fd);
@@ -395,12 +408,10 @@ int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         exit_res = read_one_char_at_a_time(streams.stdin_fd, buff, opts.nchars, opts.split_null);
     }
 
-    if (optind == argc || exit_res != STATUS_CMD_OK) {
+    if (exit_res != STATUS_CMD_OK) {
-        // Define the var without any data. We do this because when this happens we want the user to
+        // Define the var(s) without any data. We do this because when this happens we want the user
-        // be able to use the var but have it expand to nothing.
+        // to be able to use the var but have it expand to nothing.
-        //
+        for (int i = 0; i < argc; i++) env_set(argv[i], NULL, opts.place);
-        // TODO: For fish 3.0 we should mandate at least one var name.
-        if (argv[optind]) env_set(argv[optind], NULL, opts.place);
         return exit_res;
     }
 
@@ -417,21 +428,22 @@ int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
                     *out = *it;
                     out += 2;
                 }
-                env_set(argv[optind], chars.c_str(), opts.place);
+                env_set(argv[0], chars.c_str(), opts.place);
             } else {
-                env_set(argv[optind], NULL, opts.place);
+                env_set(argv[0], NULL, opts.place);
             }
-        } else {  // not array
+        } else {  // not array mode
+            int i = 0;
             size_t j = 0;
-            for (; optind + 1 < argc; ++optind) {
+            for (; i + 1 < argc; ++i) {
                 if (j < bufflen) {
                     wchar_t buffer[2] = {buff[j++], 0};
-                    env_set(argv[optind], buffer, opts.place);
+                    env_set(argv[i], buffer, opts.place);
                 } else {
-                    env_set(argv[optind], L"", opts.place);
+                    env_set(argv[i], L"", opts.place);
                 }
             }
-            if (optind < argc) env_set(argv[optind], &buff[j], opts.place);
+            if (i < argc) env_set(argv[i], &buff[j], opts.place);
         }
     } else if (opts.array) {
         wcstring tokens;
@@ -444,15 +456,13 @@ int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
             tokens.append(buff, loc.first, loc.second);
             empty = false;
         }
-        env_set(argv[optind], empty ? NULL : tokens.c_str(), opts.place);
+        env_set(argv[0], empty ? NULL : tokens.c_str(), opts.place);
     } else {  // not array
         wcstring_range loc = wcstring_range(0, 0);
-
+        for (int i = 0; i < argc; i++) {
-        while (optind < argc) {
+            loc = wcstring_tok(buff, (i + 1 < argc) ? ifs : wcstring(), loc);
-            loc = wcstring_tok(buff, (optind + 1 < argc) ? ifs : wcstring(), loc);
+            env_set(argv[i], loc.first == wcstring::npos ? L"" : &buff.c_str()[loc.first],
-            env_set(argv[optind], loc.first == wcstring::npos ? L"" : &buff.c_str()[loc.first],
                     opts.place);
-            ++optind;
         }
     }
 

tests/history.err

@@ -4,8 +4,8 @@ history: you cannot use any options with the merge command
 history: save expected 0 args, got 1
 history: you cannot use any options with the save command
 history: you cannot use any options with the clear command
-history: merge expected 0 args, got 1
+history merge: Expected 0 args, got 1
-history: clear expected 0 args, got 2
+history clear: Expected 0 args, got 2
 
 history: you cannot use any options with the clear command
 history: you cannot use any options with the merge command
@@ -17,7 +17,7 @@ history: Invalid combination of options,
 you cannot do both 'search' and 'merge' in the same invocation
 history: you cannot use any options with the save command
 history: you cannot use any options with the clear command
-history: merge expected 0 args, got 1
+history merge: Expected 0 args, got 1
-history: clear expected 0 args, got 2
+history clear: Expected 0 args, got 2
 history: you cannot use any options with the save command
 history: you cannot use any options with the merge command

tests/read.err

@@ -0,0 +1,5 @@
+# Read with no vars is an error
+read: Expected at least 1 args, got only 0
+# Read with -a and anything other than exactly on var name is an error
+read: Expected 1 args, got 0
+read: Expected 1 args, got 2

tests/read.in

@@ -2,6 +2,18 @@
 #
 # Test read builtin and IFS.
 #
+
+###########
+# Start by testing that invocation errors are handled correctly.
+echo '# Read with no vars is an error' >&2
+read
+echo '# Read with -a and anything other than exactly on var name is an error' >&2
+read -a
+read -a v1 v2
+read -a v1
+
+###########
+# Verify correct behavior of subcommands and splitting of input.
 count (echo one\ntwo)
 set -l IFS \t
 count (echo one\ntwo)
@@ -25,6 +37,7 @@ function print_vars --no-scope-shadowing
 end
 
 echo
+echo '# Test splitting input'
 echo 'hello there' | read -l one two
 print_vars one two
 echo 'hello there' | read -l one
@@ -41,6 +54,7 @@ echo -n 'a' | read -l one
 echo "$status $one"
 
 echo
+echo '# Test splitting input with IFS empty'
 set -l IFS
 echo 'hello' | read -l one
 print_vars one

tests/read.out

@@ -11,6 +11,7 @@ two]
 two
 ]
 
+# Test splitting input
 1 'hello' 1 'there'
 1 'hello there'
 1 ''
@@ -19,6 +20,7 @@ two
 1 'foo' 1 'bar' 1 '  baz'
 0 a
 
+# Test splitting input with IFS empty
 1 'hello'
 1 'h' 1 'ello'
 1 'h' 1 'e' 1 'llo'