From 0e1f5108aee0d1c1d2c32ae8f47db62d3a864b19 Mon Sep 17 00:00:00 2001
From: Fabian Homborg <FHomborg@gmail.com>
Date: Fri, 9 Jul 2021 21:20:58 +0200
Subject: [PATCH] string: Allow `collect --allow-empty` to avoid empty ellision
 (#8054)

* string: Allow `collect --no-empty` to avoid empty ellision

Currently we still have that issue where

    test -n (thing | string collect)

can return true if `thing` doesn't print anything, because the
collected argument will still be removed.

So, what we do is allow `--no-empty` to be used, in which case we
print one empty argument.

This means

    test -n (thing | string collect -n)

can now be safely used.

"no-empty" isn't the best name for this flag, but string's design
really incentivizes reusing names, and it's not *terrible*.

* Switch to `--allow-empty`

`--no-empty` does the exact opposite for `string split` and split0.

Since `-a`/`--allow-empty` already exists, use it.
---
 doc_src/cmds/string-collect.rst |  7 ++++++-
 src/builtin_string.cpp          |  9 +++++++++
 tests/checks/string.fish        | 12 ++++++++++++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/doc_src/cmds/string-collect.rst b/doc_src/cmds/string-collect.rst
index dd3f31e1b..e3e0b5f31 100644
--- a/doc_src/cmds/string-collect.rst
+++ b/doc_src/cmds/string-collect.rst
@@ -8,7 +8,7 @@ Synopsis
 
 ::
 
-    string collect [(-N | --no-trim-newlines)] [STRING...]
+    string collect [(-a | --allow-empty)] [(-N | --no-trim-newlines)] [STRING...]
 
 .. END SYNOPSIS
 
@@ -23,6 +23,8 @@ If invoked with multiple arguments instead of input, ``string collect`` preserve
 
 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)``.
 
+With ``--allow-empty``, ``string collect`` always prints one (empty) argument. This can be used to prevent an argument from disappearing.
+
 .. END DESCRIPTION
 
 Examples
@@ -43,4 +45,7 @@ Examples
     three
     "
 
+    >_ echo foo(true | string collect --allow-empty)bar
+    foobar
+
 .. END EXAMPLES
diff --git a/src/builtin_string.cpp b/src/builtin_string.cpp
index e913ae7a0..c62b1754b 100644
--- a/src/builtin_string.cpp
+++ b/src/builtin_string.cpp
@@ -1527,6 +1527,7 @@ static int string_split0(parser_t &parser, io_streams_t &streams, int argc, cons
 
 static int string_collect(parser_t &parser, io_streams_t &streams, int argc, const wchar_t **argv) {
     options_t opts;
+    opts.allow_empty_valid = true;
     opts.no_trim_newlines_valid = true;
     int optind;
     int retval = parse_opts(&opts, &optind, 0, argc, argv, parser, streams);
@@ -1546,6 +1547,14 @@ static int string_collect(parser_t &parser, io_streams_t &streams, int argc, con
         appended += len;
     }
 
+    // If we haven't printed anything and "no_empty" is set,
+    // print something empty. Helps with empty ellision:
+    // echo (true | string collect --allow-empty)"bar"
+    // prints "bar".
+    if (opts.allow_empty && appended == 0) {
+        streams.out.append_with_separation(L"", 0, separation_type_t::explicitly);
+    }
+
     return appended > 0 ? STATUS_CMD_OK : STATUS_CMD_ERROR;
 }
 
diff --git a/tests/checks/string.fish b/tests/checks/string.fish
index f08cd9bfc..ce1ef7f1d 100644
--- a/tests/checks/string.fish
+++ b/tests/checks/string.fish
@@ -667,6 +667,18 @@ string collect -N '' >/dev/null; and echo unexpected success; or echo expected f
 string collect \n\n >/dev/null; and echo unexpected success; or echo expected failure
 # CHECK: expected failure
 
+echo "foo"(true | string collect --allow-empty)"bar"
+# CHECK: foobar
+test -z (string collect)
+and echo Nothing
+# CHECK: Nothing
+test -n (string collect)
+and echo Something
+# CHECK: Something
+test -n (string collect -a)
+or echo No, actually nothing
+# CHECK: No, actually nothing
+
 # string collect in functions
 # This function outputs some newline-separated content, and some
 # explicitly un-separated content.