Invert the flag for string collect

Instead of requiring a flag to enable newline trimming, invert it so the
flag (now `--no-trim-newlines`) disables newline trimming. This way our
default behavior matches that of sh's `"$(cmd)"`.

Also change newline trimming to trim all newlines instead of just one,
again to match sh's behavior.

CHANGELOG.md

@@ -15,7 +15,7 @@
 - The `--debug` option has been extended to allow specifying categories. Categories may be listed via `fish --print-debug-categories`.
 - `string replace` had an additional round of escaping in the replacement (not the match!), so escaping backslashes would require `string replace -ra '([ab])' '\\\\\\\$1' a`. A new feature flag `string-replace-fewer-backslashes` can be used to disable this, so that it becomes `string replace -ra '([ab])' '\\\\$1' a` (#5556).
 - Some parser errors did not set `$status` to non-zero. This has been corrected (b2a1da602f79878f4b0adc4881216c928a542608).
-- `string` has a new `collect` subcommand that disables newline-splitting on its input. This is meant to be used as the end of a command substitution pipeline to produce a single output argument potentially containing newlines, such as `set contents (cat filename | string collect)`. It also supports a `--trim-newline` flag to trim a single trailing newline from the output (#159).
+- `string` has a new `collect` subcommand that disables newline-splitting on its input. This is meant to be used as the end of a command substitution pipeline to produce a single output argument potentially containing internal newlines, such as `set output (some-cmd | string collect)`. Any trailing newlines are trimmed, just like `"$(cmd)"` substitution in sh. It also supports a `--no-trim-newlines` flag to disable trailing newline trimming, which may be useful when doing something like `set contents (cat filename | string collect -N)` (#159).
 
 ### Syntax changes and new commands
 - Brace expansion now only takes place if the braces include a "," or a variable expansion, so things like `git reset HEAD@{0}` now work (#5869).

share/completions/string.fish

@@ -14,7 +14,7 @@ complete -x -c string -n 'test (count (commandline -opc)) -ge 2; and string matc
 complete -f -c string -n 'test (count (commandline -opc)) -ge 2; and string match -qr split0\?\$ -- (commandline -opc)[2]' -s r -l right -d "Split right-to-left"
 complete -f -c string -n 'test (count (commandline -opc)) -ge 2; and string match -qr split0\?\$ -- (commandline -opc)[2]' -s n -l no-empty -d "Empty results excluded"
 complete -f -c string -n "test (count (commandline -opc)) -lt 2" -a "collect"
-complete -f -c string -n 'test (count (commandline -opc)) -ge 2; and string match -qr collect\$ -- (commandline -opc)[2]' -s n -l trim-newline -d "Remove trailing newline"
+complete -f -c string -n 'test (count (commandline -opc)) -ge 2; and string match -qr collect\$ -- (commandline -opc)[2]' -s N -l no-trim-newlines -d "Don't trim trailing newlines"
 
 complete -f -c string -n "test (count (commandline -opc)) -lt 2" -a "join"
 complete -f -c string -n "test (count (commandline -opc)) -lt 2" -a "join0"

sphinx_doc_src/cmds/string.rst

@@ -6,7 +6,7 @@ string - manipulate strings
 Synopsis
 --------
 
-``string collect [(-n | --trim-newline)] [STRING...]``
+``string collect [(-N | --no-trim-newlines)] [STRING...]``
 
 ``string escape [(-n | --no-quoted)] [--style=xxx] [STRING...]``
 
@@ -53,13 +53,13 @@ The following subcommands are available.
 "collect" subcommand
 --------------------
 
-``string collect [(-n | --trim-newline)] [STRING...]``
+``string collect [(-N | --no-trim-newlines)] [STRING...]``
 
 ``string collect`` collects its input into a single output argument, without splitting the output when used in a command substitution. This is useful when trying to collect multiline output from another command into a variable. Exit status: 0 if any output argument is non-empty, or 1 otherwise.
 
 If invoked with multiple arguments instead of input, ``string collect`` preserves each argument separately, where the number of output arguments is equal to the number of arguments given to ``string collect``.
 
-``--trim-newline`` trims a single trailing newline off of each output argument. This is useful when collecting the output from another command as the trailing newline is frequently not desired.
+Any trailing newlines on the input are trimmed, just as with ``"$(cmd)"`` substitution in sh. ``--no-trim-newlines`` can be used to disable this behavior, which may be useful when running a command such as ``set contents (cat filename | string collect -N)``.
 
 "escape" and "unescape" subcommands
 -----------------------------------
@@ -352,7 +352,7 @@ Examples
     three
     "
     
-    >_ echo \"(ech one\ntwo\nthree | string collect -n)\"
+    >_ echo \"(echo one\ntwo\nthree | string collect -N)\"
     "one
     two
     three"

src/builtin_string.cpp

@@ -157,7 +157,7 @@ typedef struct {  //!OCLINT(too many fields)
     bool start_valid = false;
     bool style_valid = false;
     bool no_empty_valid = false;
-    bool trim_newline_valid = false;
+    bool no_trim_newlines_valid = false;
 
     bool all = false;
     bool entire = false;
@@ -172,7 +172,7 @@ typedef struct {  //!OCLINT(too many fields)
     bool regex = false;
     bool right = false;
     bool no_empty = false;
-    bool trim_newline = false;
+    bool no_trim_newlines = false;
 
     long count = 0;
     long length = 0;
@@ -216,6 +216,9 @@ static int handle_flag_N(wchar_t **argv, parser_t &parser, io_streams_t &streams
     if (opts->no_newline_valid) {
         opts->no_newline = true;
         return STATUS_CMD_OK;
+    } else if (opts->no_trim_newlines_valid) {
+        opts->no_trim_newlines = true;
+        return STATUS_CMD_OK;
     }
     string_unknown_option(parser, streams, argv[0], argv[w.woptind - 1]);
     return STATUS_INVALID_ARGS;
@@ -332,9 +335,6 @@ static int handle_flag_n(wchar_t **argv, parser_t &parser, io_streams_t &streams
     } else if (opts->no_empty_valid) {
         opts->no_empty = true;
         return STATUS_CMD_OK;
-    } else if (opts->trim_newline_valid) {
-        opts->trim_newline = true;
-        return STATUS_CMD_OK;
     }
     string_unknown_option(parser, streams, argv[0], argv[w.woptind - 1]);
     return STATUS_INVALID_ARGS;
@@ -413,7 +413,7 @@ static wcstring construct_short_opts(options_t *opts) {  //!OCLINT(high npath co
     if (opts->right_valid) short_opts.append(L"r");
     if (opts->start_valid) short_opts.append(L"s:");
     if (opts->no_empty_valid) short_opts.append(L"n");
-    if (opts->trim_newline_valid) short_opts.append(L"n");
+    if (opts->no_trim_newlines_valid) short_opts.append(L"N");
     return short_opts;
 }
 
@@ -430,7 +430,7 @@ static const struct woption long_options[] = {
     {L"no-newline", no_argument, NULL, 'N'},  {L"no-quoted", no_argument, NULL, 'n'},
     {L"quiet", no_argument, NULL, 'q'},       {L"regex", no_argument, NULL, 'r'},
     {L"right", no_argument, NULL, 'r'},       {L"start", required_argument, NULL, 's'},
-    {L"style", required_argument, NULL, 1},   {L"trim-newline", no_argument, NULL, 'n'},
+    {L"style", required_argument, NULL, 1},   {L"no-trim-newlines", no_argument, NULL, 'N'},
     {NULL, 0, NULL, 0}};
 
 static const std::unordered_map<char, decltype(*handle_flag_N)> flag_to_function = {
@@ -1134,7 +1134,7 @@ static int string_split0(parser_t &parser, io_streams_t &streams, int argc, wcha
 
 static int string_collect(parser_t &parser, io_streams_t &streams, int argc, wchar_t **argv) {
     options_t opts;
-    opts.trim_newline_valid = true;
+    opts.no_trim_newlines_valid = true;
     int optind;
     int retval = parse_opts(&opts, &optind, 0, argc, argv, parser, streams);
     if (retval != STATUS_CMD_OK) return retval;
@@ -1142,12 +1142,14 @@ static int string_collect(parser_t &parser, io_streams_t &streams, int argc, wch
     auto &buff = streams.out.buffer();
     arg_iterator_t aiter(argv, optind, streams, /* don't split */ false);
     while (const wcstring *arg = aiter.nextstr()) {
-        auto end = arg->cend();
-        if (opts.trim_newline && !arg->empty() && arg->back() == L'\n') {
-            --end;
+        auto begin = arg->cbegin(), end = arg->cend();
+        if (!opts.no_trim_newlines) {
+            while (end > begin && *(end-1) == L'\n') {
+                --end;
+            }
         }
 
-        buff.append(arg->cbegin(), end, separation_type_t::explicitly);
+        buff.append(begin, end, separation_type_t::explicitly);
     }
 
     return buff.size() > 0 ? STATUS_CMD_OK : STATUS_CMD_ERROR;

tests/string.in

@@ -396,18 +396,18 @@ logmsg string collect
 count (echo one\ntwo\nthree\nfour | string collect)
 count (echo one | string collect)
 echo [(echo one\ntwo\nthree | string collect)]
-echo [(echo one\ntwo\nthree | string collect -n)]
+echo [(echo one\ntwo\nthree | string collect -N)]
 printf '[%s]\n' (string collect one\n\n two\n)
-printf '[%s]\n' (string collect -n one\n\n two\n)
-printf '[%s]\n' (string collect --trim-newline one\n\n two\n)
+printf '[%s]\n' (string collect -N one\n\n two\n)
+printf '[%s]\n' (string collect --no-trim-newlines one\n\n two\n)
 # string collect returns 0 when it has any output, otherwise 1
 string collect >/dev/null; and echo unexpected success; or echo expected failure
 echo -n | string collect >/dev/null; and echo unexpected success; or echo expected failure
-echo | string collect >/dev/null; and echo expected success; or echo unexpected failure
-echo | string collect -n >/dev/null; and echo unexpected success; or echo expected failure
+echo | string collect -N >/dev/null; and echo expected success; or echo unexpected failure
+echo | string collect >/dev/null; and echo unexpected success; or echo expected failure
 string collect a >/dev/null; and echo expected success; or echo unexpected failure
-string collect '' >/dev/null; and echo unexpected success; or echo expected failure
-string collect -n \n >/dev/null; and echo unexpected success; or echo expected failure
+string collect -N '' >/dev/null; and echo unexpected success; or echo expected failure
+string collect \n\n >/dev/null; and echo unexpected success; or echo expected failure
 
 logmsg string collect in functions
 # This function outputs some newline-separated content, and some

tests/string.out

@@ -490,22 +490,23 @@ Split something
 1
 [one
 two
-three
-]
+three]
 [one
 two
-three]
+three
+]
+[one]
+[two]
 [one
 
 ]
 [two
 ]
 [one
+
 ]
-[two]
-[one
+[two
 ]
-[two]
 expected failure
 expected failure
 expected success