From 4a7aa98e9349426712f45cb403c9b359c9ef319f Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Wed, 19 Jul 2017 20:22:34 -0700 Subject: [PATCH] modify `read` to require at least one var Fixes #4220 --- doc_src/read.txt | 4 +- src/builtin.h | 4 +- src/builtin_read.cpp | 94 ++++++++++++++++++++++++-------------------- tests/history.err | 8 ++-- tests/read.err | 5 +++ tests/read.in | 14 +++++++ tests/read.out | 2 + 7 files changed, 81 insertions(+), 50 deletions(-) diff --git a/doc_src/read.txt b/doc_src/read.txt index 0f1b0589a..f8a0ad618 100644 --- a/doc_src/read.txt +++ b/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: diff --git a/src/builtin.h b/src/builtin.h index 24d60f3c0..6f7e96d11 100644 --- a/src/builtin.h +++ b/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_COUNT2 _(L"%ls: %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_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") diff --git a/src/builtin_read.cpp b/src/builtin_read.cpp index 895184d4c..7622bb768 100644 --- a/src/builtin_read.cpp +++ b/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. -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; - - if (opts.print_help) { - builtin_print_help(parser, streams, cmd, streams.out); - return STATUS_CMD_OK; - } - +/// Validate the arguments given to `read` and provide defaults where needed. +static int validate_read_args(const wchar_t *cmd, read_cmd_opts_t &opts, int argc, + const wchar_t *const *argv, parser_t &parser, io_streams_t &streams) { 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) { - streams.err.append_format(_(L"%ls: --array option requires a single variable name.\n"), - cmd); - builtin_print_help(parser, streams, cmd, streams.err); + if (!opts.array && argc < 1) { + streams.err.append_format(BUILTIN_ERR_MIN_ARG_COUNT1, cmd, 1, argc); + return STATUS_INVALID_ARGS; + } + + 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) { - // Define the var without any data. We do this because when this happens we want the user to - // be able to use the var but have it expand to nothing. - // - // TODO: For fish 3.0 we should mandate at least one var name. - if (argv[optind]) env_set(argv[optind], NULL, opts.place); + if (exit_res != STATUS_CMD_OK) { + // Define the var(s) without any data. We do this because when this happens we want the user + // 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); 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); - - while (optind < argc) { - loc = wcstring_tok(buff, (optind + 1 < argc) ? ifs : wcstring(), loc); - env_set(argv[optind], loc.first == wcstring::npos ? L"" : &buff.c_str()[loc.first], + for (int i = 0; i < argc; i++) { + loc = wcstring_tok(buff, (i + 1 < argc) ? ifs : wcstring(), loc); + env_set(argv[i], loc.first == wcstring::npos ? L"" : &buff.c_str()[loc.first], opts.place); - ++optind; } } diff --git a/tests/history.err b/tests/history.err index abce1dfc6..b5bc875eb 100644 --- a/tests/history.err +++ b/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: clear expected 0 args, got 2 +history merge: Expected 0 args, got 1 +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: clear expected 0 args, got 2 +history merge: Expected 0 args, got 1 +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 diff --git a/tests/read.err b/tests/read.err index e69de29bb..457658c44 100644 --- a/tests/read.err +++ b/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 diff --git a/tests/read.in b/tests/read.in index 236c2ede8..8c14f0383 100644 --- a/tests/read.in +++ b/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 diff --git a/tests/read.out b/tests/read.out index 015bb414e..94301d087 100644 --- a/tests/read.out +++ b/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'