From 47ff060b896a70295bfa28937fba9081ef9d8f47 Mon Sep 17 00:00:00 2001
From: Fabian Homborg <FHomborg@gmail.com>
Date: Tue, 26 Feb 2019 19:50:53 +0100
Subject: [PATCH] string: Fix split0 return status

It turns out that `string split0` didn't actually ever do any
splitting. The arg_iterator_t already split stdin on NUL, and split0 just
performed an additional search that could never succeed (since
arguments from argv already can't contain NUL).

Let the arg_iterator_t not perform any splitting if asked, and then
let split0 split in 0.

One slight wart is that split0 ignores a trailing NUL, which normal
split doesn't.

Fixes #5701.
---
 CHANGELOG.md           |  1 +
 src/builtin_string.cpp | 22 ++++++++++++++--------
 tests/string.in        |  3 +++
 tests/string.out       |  3 +++
 4 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 07fbf1784..c062e66d8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@
 ## Notable fixes and improvements
 - Fixed infinite recursion triggered if a custom `fish_title` function calls `read` interactively
 - Add `$pipestatus` support
+- `string split0` now returns 0 if it split something (#5701).
 
 ### Syntax changes and new commands
 - None yet.
diff --git a/src/builtin_string.cpp b/src/builtin_string.cpp
index ad945fdef..8b44f452e 100644
--- a/src/builtin_string.cpp
+++ b/src/builtin_string.cpp
@@ -75,8 +75,8 @@ class arg_iterator_t {
     int argidx_;
     // If not using argv, a string to store bytes that have been read but not yet returned.
     std::string buffer_;
-    // If set, when reading from a stream, split on zeros instead of newlines.
-    const bool split0_;
+    // If set, when reading from a stream, split on newlines.
+    const bool split_;
     // Backing storage for the next() string.
     wcstring storage_;
     const io_streams_t &streams_;
@@ -85,10 +85,9 @@ class arg_iterator_t {
     /// not. On true, the string is stored in storage_.
     bool get_arg_stdin() {
         assert(string_args_from_stdin(streams_) && "should not be reading from stdin");
-        // Read in chunks from fd until buffer has a line (or zero if split0_ is set).
-        const char sep = split0_ ? '\0' : '\n';
+        // Read in chunks from fd until buffer has a line (or the end if split_ is unset).
         size_t pos;
-        while ((pos = buffer_.find(sep)) == std::string::npos) {
+        while (!split_ || (pos = buffer_.find('\n')) == std::string::npos) {
             char buf[STRING_CHUNK_SIZE];
             long n = read_blocked(streams_.stdin_fd, buf, STRING_CHUNK_SIZE);
             if (n == 0) {
@@ -118,8 +117,8 @@ class arg_iterator_t {
 
    public:
     arg_iterator_t(const wchar_t *const *argv, int argidx, const io_streams_t &streams,
-                   bool split0 = false)
-        : argv_(argv), argidx_(argidx), split0_(split0), streams_(streams) {}
+                   bool split = true)
+        : argv_(argv), argidx_(argidx), split_(split), streams_(streams) {}
 
     const wcstring *nextstr() {
         if (string_args_from_stdin(streams_)) {
@@ -1073,7 +1072,7 @@ static int string_split_maybe0(parser_t &parser, io_streams_t &streams, int argc
 
     wcstring_list_t splits;
     size_t arg_count = 0;
-    arg_iterator_t aiter(argv, optind, streams, is_split0);
+    arg_iterator_t aiter(argv, optind, streams, !is_split0);
     while (const wcstring *arg = aiter.nextstr()) {
         if (opts.right) {
             split_about(arg->rbegin(), arg->rend(), sep.rbegin(), sep.rend(), &splits, opts.max,
@@ -1095,6 +1094,13 @@ static int string_split_maybe0(parser_t &parser, io_streams_t &streams, int argc
 
     const size_t split_count = splits.size();
     if (!opts.quiet) {
+        if (is_split0 && splits.size()) {
+            // split0 ignores a trailing \0, so a\0b\0 is two elements.
+            // In contrast to split, where a\nb\n is three - "a", "b" and "".
+            //
+            // Remove the last element if it is empty.
+            if (splits.back().empty()) splits.pop_back();
+        }
         auto &buff = streams.out.buffer();
         for (const wcstring &split : splits) {
             buff.append(split, separation_type_t::explicitly);
diff --git a/tests/string.in b/tests/string.in
index 74d60ed9a..e15f33ee5 100644
--- a/tests/string.in
+++ b/tests/string.in
@@ -368,6 +368,9 @@ count (echo -ne 'abc\x00def\x00ghi\x00\x00' | string split0)
 count (echo -ne 'abc\x00def\x00ghi' | string split0)
 count (echo -ne 'abc\ndef\x00ghi\x00' | string split0)
 count (echo -ne 'abc\ndef\nghi' | string split0)
+# #5701 - split0 always returned 1
+echo -ne 'a\x00b' | string split0
+and echo Split something
 
 logmsg string join0
 set tmp beta alpha\ngamma
diff --git a/tests/string.out b/tests/string.out
index a8a8ecf2f..2fdec3025 100644
--- a/tests/string.out
+++ b/tests/string.out
@@ -463,6 +463,9 @@ a\x00g
 3
 2
 1
+a
+b
+Split something
 
 ####################
 # string join0