implement helper functions for fish script vars

This is the first step in implementing a better abstraction for handling
fish script vars in the C++ code. It implements a new function (with two
signatures) to provide a standard method for construct the flag string
representation of a fish script array.

Partial fix for fish-shell#4200

src/builtin_set.cpp

@@ -45,12 +45,8 @@ static int is_path_variable(const wchar_t *env) { return contains(path_variables
 
 /// Call env_set. If this is a path variable, e.g. PATH, validate the elements. On error, print a
 /// description of the problem to stderr.
-static int my_env_set(const wchar_t *key, const wcstring_list_t &val, int scope,
+static int my_env_set(const wchar_t *key, const wcstring_list_t &list, int scope,
                       io_streams_t &streams) {
-    size_t i;
-    int retcode = 0;
-    const wchar_t *val_str = NULL;
-
     if (is_path_variable(key)) {
         // Fix for https://github.com/fish-shell/fish-shell/issues/199 . Return success if any path
         // setting succeeds.
@@ -70,8 +66,8 @@ static int my_env_set(const wchar_t *key, const wcstring_list_t &val, int scope,
         if (!existing_variable.missing_or_empty())
             tokenize_variable_array(existing_variable, existing_values);
 
-        for (i = 0; i < val.size(); i++) {
-            const wcstring &dir = val.at(i);
+        for (size_t i = 0; i < list.size(); i++) {
+            const wcstring &dir = list.at(i);
             if (!string_prefixes_string(L"/", dir) || contains(existing_values, dir)) {
                 any_success = true;
                 continue;
@@ -109,44 +105,38 @@ static int my_env_set(const wchar_t *key, const wcstring_list_t &val, int scope,
 
         // Fail at setting the path if we tried to set it to something non-empty, but it wound up
         // empty.
-        if (!val.empty() && !any_success) {
-            return 1;
-        }
-    }
-
-    wcstring sb;
-    if (!val.empty()) {
-        for (i = 0; i < val.size(); i++) {
-            sb.append(val[i]);
-            if (i < val.size() - 1) {
-                sb.append(ARRAY_SEP_STR);
-            }
+        if (!list.empty() && !any_success) {
+            return STATUS_CMD_ERROR;
         }
-        val_str = sb.c_str();
     }
 
-    switch (env_set(key, val_str, scope | ENV_USER)) {
+    // We don't check `val->empty()` because an array var with a single empty string will be
+    // "empty". A truly empty array var is set to the special value `ENV_NULL`.
+    auto val = list_to_array_val(list);
+    int retcode = env_set(key, *val == ENV_NULL ? NULL : val->c_str(), scope | ENV_USER);
+    switch (retcode) {
         case ENV_OK: {
+            retcode = STATUS_CMD_OK;
             break;
         }
         case ENV_PERM: {
             streams.err.append_format(_(L"%ls: Tried to change the read-only variable '%ls'\n"),
                                       L"set", key);
-            retcode = 1;
+            retcode = STATUS_CMD_ERROR;
             break;
         }
         case ENV_SCOPE: {
             streams.err.append_format(
                 _(L"%ls: Tried to set the special variable '%ls' with the wrong scope\n"), L"set",
                 key);
-            retcode = 1;
+            retcode = STATUS_CMD_ERROR;
             break;
         }
         case ENV_INVALID: {
             streams.err.append_format(
                 _(L"%ls: Tried to set the special variable '%ls' to an invalid value\n"), L"set",
                 key);
-            retcode = 1;
+            retcode = STATUS_CMD_ERROR;
             break;
         }
         default: {

src/common.cpp

@@ -1633,19 +1633,6 @@ int common_get_width() { return get_current_winsize().ws_col; }
 
 int common_get_height() { return get_current_winsize().ws_row; }
 
-void tokenize_variable_array(const wcstring &val, std::vector<wcstring> &out) {
-    size_t pos = 0, end = val.size();
-    while (pos <= end) {
-        size_t next_pos = val.find(ARRAY_SEP, pos);
-        if (next_pos == wcstring::npos) {
-            next_pos = end;
-        }
-        out.resize(out.size() + 1);
-        out.back().assign(val, pos, next_pos - pos);
-        pos = next_pos + 1;  // skip the separator, or skip past the end
-    }
-}
-
 bool string_prefixes_string(const wchar_t *proposed_prefix, const wcstring &value) {
     size_t prefix_size = wcslen(proposed_prefix);
     return prefix_size <= value.size() && value.compare(0, prefix_size, proposed_prefix) == 0;

src/common.h

@@ -12,9 +12,6 @@
 #include <string.h>
 #include <termios.h>
 #include <wchar.h>
-#ifdef HAVE_SYS_IOCTL_H
-#include <sys/ioctl.h>
-#endif
 
 #include <memory>
 #include <sstream>
@@ -738,12 +735,6 @@ void common_handle_winch(int signal);
 /// Write the given paragraph of output, redoing linebreaks to fit the current screen.
 wcstring reformat_for_screen(const wcstring &msg);
 
-/// Tokenize the specified string into the specified wcstring_list_t.
-///
-/// \param val the input string. The contents of this string is not changed.
-/// \param out the list in which to place the elements.
-void tokenize_variable_array(const wcstring &val, wcstring_list_t &out);
-
 /// Make sure the specified direcotry exists. If needed, try to create it and any currently not
 /// existing parent directories.
 ///

src/env.cpp

@@ -961,7 +961,7 @@ int env_set(const wcstring &key, const wchar_t *val, env_mode_flags_t var_mode)
         return ENV_INVALID;
     }
 
-    // Zero element arrays are internaly not coded as null but as this placeholder string.
+    // Zero element arrays are internally not coded as an empty string but this placeholder string.
     if (!val) {
         val = ENV_NULL;  //!OCLINT(parameter reassignment)
     }
@@ -1522,10 +1522,55 @@ const wchar_t *const env_vars_snapshot_t::highlighting_keys[] = {L"PATH", L"CDPA
 
 const wchar_t *const env_vars_snapshot_t::completing_keys[] = {L"PATH", L"CDPATH", NULL};
 
-wcstring *list_to_array_val(const wcstring_list_t &list) {
-    return new wcstring();
+// The next set of functions convert between a flat string and an actual list of strings using
+// the encoding employed by fish internally for "arrays".
+
+std::unique_ptr<wcstring> list_to_array_val(const wcstring_list_t &list) {
+    auto val = std::unique_ptr<wcstring>(new wcstring());
+
+    if (list.size() == 0) {
+        // Zero element arrays are internally encoded as this placeholder string.
+        val->append(ENV_NULL);
+    } else {
+        for (auto it : list) {
+            if (!val->empty()) val->push_back(ARRAY_SEP);
+            val->append(it);
+        }
+    }
+
+    return val;
 }
 
-wcstring *list_to_array_val(const wchar_t **list) {
-    return new wcstring();
+std::unique_ptr<wcstring> list_to_array_val(const wchar_t **list) {
+    auto val = std::unique_ptr<wcstring>(new wcstring());
+
+    if (!*list) {
+        // Zero element arrays are internally encoded as this placeholder string.
+        val->append(ENV_NULL);
+    } else {
+        for (auto it = list; *it; it++) {
+            if (it != list) val->push_back(ARRAY_SEP);
+            val->append(*it);
+        }
+    }
+
+    return val;
+}
+
+void tokenize_variable_array(const wcstring &val, wcstring_list_t &out) {
+    out.clear();  // ensure the output var is empty -- this will normally be a no-op
+
+    // Zero element arrays are internally encoded as this placeholder string.
+    if (val == ENV_NULL) return;
+
+    size_t pos = 0, end = val.size();
+    while (pos <= end) {
+        size_t next_pos = val.find(ARRAY_SEP, pos);
+        if (next_pos == wcstring::npos) {
+            next_pos = end;
+        }
+        out.resize(out.size() + 1);
+        out.back().assign(val, pos, next_pos - pos);
+        pos = next_pos + 1;  // skip the separator, or skip past the end
+    }
 }

src/env.h

@@ -16,7 +16,7 @@ extern bool curses_initialized;
 
 /// Character for separating two array elements. We use 30, i.e. the ascii record separator since
 /// that seems logical.
-#define ARRAY_SEP ((wchar_t)(0x1e))
+#define ARRAY_SEP (wchar_t)0x1e
 
 /// String containing the character for separating two array elements.
 #define ARRAY_SEP_STR L"\x1e"
@@ -212,6 +212,12 @@ extern bool term_has_xn;  // does the terminal have the "eat_newline_glitch"
 bool term_supports_setting_title();
 
 /// Returns the fish internal representation for an array of strings.
-wcstring *list_to_array_val(const wcstring_list_t &list);
-wcstring *list_to_array_val(const wchar_t **list);
+std::unique_ptr<wcstring> list_to_array_val(const wcstring_list_t &list);
+std::unique_ptr<wcstring> list_to_array_val(const wchar_t **list);
+
+/// Tokenize the specified string into the specified wcstring_list_t.
+///
+/// \param val the input string. The contents of this string is not changed.
+/// \param out the list in which to place the elements.
+void tokenize_variable_array(const wcstring &val, wcstring_list_t &out);
 #endif

src/fish.cpp

@@ -424,19 +424,12 @@ int main(int argc, char **argv) {
                 // OK to not do this atomically since we cannot have gone multithreaded yet.
                 set_cloexec(fd);
 
-                if (*(argv + my_optind)) {
-                    wcstring sb;
-                    char **ptr;
-                    int i;
-                    for (i = 1, ptr = argv + my_optind; *ptr; i++, ptr++) {
-                        if (i != 1) sb.append(ARRAY_SEP_STR);
-                        sb.append(str2wcstring(*ptr));
-                    }
-
-                    env_set(L"argv", sb.c_str(), 0);
-                } else {
-                    env_set(L"argv", NULL, 0);
+                wcstring_list_t list;
+                for (char **ptr = argv + my_optind; *ptr; ptr++) {
+                    list.push_back(str2wcstring(*ptr));
                 }
+                auto val = list_to_array_val(list);
+                env_set(L"argv", *val == ENV_NULL ? NULL : val->c_str(), 0);
 
                 const wcstring rel_filename = str2wcstring(file);
 

src/fish_tests.cpp

@@ -940,6 +940,57 @@ static void test_parse_util_cmdsubst_extent() {
     }
 }
 
+/// Verify the behavior of the `list_to_aray_val()` family of functions.
+static void test_list_to_array() {
+    auto list = wcstring_list_t();
+    auto val = list_to_array_val(list);
+    if (*val != ENV_NULL) {
+        err(L"test_list_to_array failed on line %lu", __LINE__);
+    }
+
+    list.push_back(L"abc");
+    val = list_to_array_val(list);
+    if (*val != list[0]) {
+        err(L"test_list_to_array failed on line %lu", __LINE__);
+    }
+
+    list.push_back(L"def");
+    val = list_to_array_val(list);
+    if (*val != L"abc" ARRAY_SEP_STR L"def") {
+        err(L"test_list_to_array failed on line %lu", __LINE__);
+    }
+
+    list.push_back(L"ghi");
+    val = list_to_array_val(list);
+    if (*val != L"abc" ARRAY_SEP_STR L"def" ARRAY_SEP_STR L"ghi") {
+        err(L"test_list_to_array failed on line %lu", __LINE__);
+    }
+
+    const wchar_t *array1[] = {NULL};
+    val = list_to_array_val(array1);
+    if (*val != ENV_NULL) {
+        err(L"test_list_to_array failed on line %lu", __LINE__);
+    }
+
+    const wchar_t *array2[] = {L"abc", NULL};
+    val = list_to_array_val(array2);
+    if (wcscmp(val->c_str(), L"abc") != 0) {
+        err(L"test_list_to_array failed on line %lu", __LINE__);
+    }
+
+    const wchar_t *array3[] = {L"abc", L"def", NULL};
+    val = list_to_array_val(array3);
+    if (wcscmp(val->c_str(), L"abc" ARRAY_SEP_STR L"def") != 0) {
+        err(L"test_list_to_array failed on line %lu", __LINE__);
+    }
+
+    const wchar_t *array4[] = {L"abc", L"def", L"ghi", NULL};
+    val = list_to_array_val(array4);
+    if (wcscmp(val->c_str(), L"abc" ARRAY_SEP_STR L"def" ARRAY_SEP_STR L"ghi") != 0) {
+        err(L"test_list_to_array failed on line %lu", __LINE__);
+    }
+}
+
 static struct wcsfilecmp_test {
     const wchar_t *str1;
     const wchar_t *str2;
@@ -997,6 +1048,7 @@ static void test_wcsfilecmp() {
 
 static void test_utility_functions() {
     say(L"Testing utility functions");
+    test_list_to_array();
     test_wcsfilecmp();
     test_parse_util_cmdsubst_extent();
 }

src/history.h

@@ -258,8 +258,8 @@ class history_t {
     // Incorporates the history of other shells into this history.
     void incorporate_external_changes();
 
-    // Gets all the history into a string with ARRAY_SEP_STR. This is intended for the $history
-    // environment variable. This may be long!
+    // Gets all the history into a string. This is intended for the $history environment variable.
+    // This may be long!
     void get_string_representation(wcstring *result, const wcstring &separator);
 
     // Sets the valid file paths for the history item with the given identifier.

src/path.cpp

@@ -9,6 +9,7 @@
 #include <unistd.h>
 #include <wchar.h>
 
+#include <memory>
 #include <string>
 #include <type_traits>
 #include <vector>
@@ -56,7 +57,7 @@ static bool path_get_path_core(const wcstring &cmd, wcstring *out_path,
         // for the fish programs. Possibly with a duplicate dir if PREFIX is empty, "/", "/usr" or
         // "/usr/". If the PREFIX duplicates /bin or /usr/bin that is harmless other than a trivial
         // amount of time testing a path we've already tested.
-        bin_path = L"/bin" ARRAY_SEP_STR L"/usr/bin" ARRAY_SEP_STR PREFIX L"/bin";
+        bin_path = *list_to_array_val(wcstring_list_t({L"/bin", L"/usr/bin", PREFIX L"/bin"}));
     }
 
     std::vector<wcstring> pathsv;