Stop exporting empty variables as ENV_NULL

Localize the encoding of empty variables as ENV_NULL into the universal
variables component, and ensure they are not exported as ENV_NULL.

Fixes #4846

src/common.cpp

@@ -1791,6 +1791,33 @@ bool contains(const wcstring_list_t &list, const wcstring &str) {
     return std::find(list.begin(), list.end(), str) != list.end();
 }
 
+wcstring_list_t split_string(const wcstring &val, wchar_t sep) {
+    wcstring_list_t out;
+    size_t pos = 0, end = val.size();
+    while (pos <= end) {
+        size_t next_pos = val.find(sep, pos);
+        if (next_pos == wcstring::npos) {
+            next_pos = end;
+        }
+        out.emplace_back(val, pos, next_pos - pos);
+        pos = next_pos + 1;  // skip the separator, or skip past the end
+    }
+    return out;
+}
+
+wcstring join_strings(const wcstring_list_t &vals, wchar_t sep) {
+    wcstring result;
+    bool first = true;
+    for (const wcstring &s : vals) {
+        if (!first) {
+            result.push_back(sep);
+        }
+        result.append(s);
+        first = false;
+    }
+    return result;
+}
+
 int create_directory(const wcstring &d) {
     bool ok = false;
     struct stat buf;

src/common.h

@@ -326,6 +326,12 @@ bool string_suffixes_string(const wchar_t *proposed_suffix, const wcstring &valu
 bool string_prefixes_string_case_insensitive(const wcstring &proposed_prefix,
                                              const wcstring &value);
 
+/// Split a string by a separator character.
+wcstring_list_t split_string(const wcstring &val, wchar_t sep);
+
+/// Join a list of strings by a separator character.
+wcstring join_strings(const wcstring_list_t &vals, wchar_t sep);
+
 enum fuzzy_match_type_t {
     // We match the string exactly: FOOBAR matches FOOBAR.
     fuzzy_match_exact = 0,

src/env.cpp

@@ -109,30 +109,6 @@ static const wcstring_list_t curses_variables({L"TERM", L"TERMINFO", L"TERMINFO_
 static void init_locale();
 static void init_curses();
 
-/// This is used to convert a serialized env_var_t back into a list. It is used when reading legacy
-/// (fish 2.x) encoded vars stored in the universal variable file and the environment.
-static wcstring_list_t tokenize_variable_array(const wcstring &val) {
-    // Zero element arrays are externally encoded as this placeholder string.
-    if (val == ENV_NULL) return {};
-
-    wcstring_list_t 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.emplace_back(val, pos, next_pos - pos);
-        pos = next_pos + 1;  // skip the separator, or skip past the end
-    }
-    return out;
-}
-
-/// This is used to convert a serialized env_var_t back into a list.
-wcstring_list_t decode_serialized(const wcstring &s) {
-    return tokenize_variable_array(s);
-}
-
 // Struct representing one level in the function variable stack.
 // Only our variable stack should create and destroy these
 class env_node_t {
@@ -916,14 +892,8 @@ void env_init(const struct config_paths_t *paths /* or NULL */) {
             key.assign(key_and_val, 0, eql);
             if (is_read_only(key) || is_electric(key)) continue;
             val.assign(key_and_val, eql + 1, wcstring::npos);
-            if (variable_is_colon_delimited_var(key)) {
-                std::replace(val.begin(), val.end(), L':', ARRAY_SEP);
-                wcstring_list_t values = decode_serialized(val);
-                env_set(key, ENV_EXPORT | ENV_GLOBAL, values);
-            } else {
-                wcstring_list_t values = decode_serialized(val);
-                env_set(key, ENV_EXPORT | ENV_GLOBAL, values);
-            }
+            wchar_t sep = variable_is_colon_delimited_var(key) ? L':' : ARRAY_SEP;
+            env_set(key, ENV_EXPORT | ENV_GLOBAL, split_string(val, sep));
         }
     }
 
@@ -1322,16 +1292,8 @@ const wcstring_list_t &env_var_t::as_list() const { return vals; }
 /// Return a string representation of the var. At the present time this uses the legacy 2.x
 /// encoding.
 wcstring env_var_t::as_string() const {
-    if (this->vals.empty()) return wcstring(ENV_NULL);
-
     wchar_t sep = (flags & flag_colon_delimit) ? L':' : ARRAY_SEP;
-    auto it = this->vals.cbegin();
-    wcstring result(*it);
-    while (++it != vals.end()) {
-        result.push_back(sep);
-        result.append(*it);
-    }
-    return result;
+    return join_strings(vals, sep);
 }
 
 void env_var_t::to_list(wcstring_list_t &out) const {
@@ -1539,16 +1501,13 @@ static void export_func(const var_table_t &envs, std::vector<std::string> &out)
             // Replace ARRAY_SEP with colon.
             std::replace(vs.begin(), vs.end(), (char)ARRAY_SEP, ':');
         }
-
-        // Put a string on the vector.
-        out.push_back(std::string());
-        std::string &str = out.back();
+        // Create and append a string of the form ks=vs
+        std::string str;
         str.reserve(ks.size() + 1 + vs.size());
-
-        // Append our environment variable data to it.
         str.append(ks);
         str.append("=");
         str.append(vs);
+        out.push_back(std::move(str));
     }
 }
 

src/env.h

@@ -20,12 +20,6 @@ extern bool curses_initialized;
 /// that seems logical.
 #define ARRAY_SEP (wchar_t)0x1e
 
-/// String containing the character for separating two array elements.
-#define ARRAY_SEP_STR L"\x1e"
-
-/// Value denoting a null string.
-#define ENV_NULL L"\x1d"
-
 // Flags that may be passed as the 'mode' in env_set / env_get.
 enum {
     /// Default mode. Used with `env_get()` to indicate the caller doesn't care what scope the var
@@ -122,9 +116,6 @@ class env_var_t {
     bool operator!=(const env_var_t &var) const { return vals != var.vals; }
 };
 
-/// This is used to convert a serialized env_var_t back into a list.
-wcstring_list_t decode_serialized(const wcstring &s);
-
 /// Gets the variable with the specified name, or none() if it does not exist.
 maybe_t<env_var_t> env_get(const wcstring &key, env_mode_flags_t mode = ENV_DEFAULT);
 

src/env_universal_common.cpp

@@ -255,6 +255,21 @@ static bool append_file_entry(fish_message_type_t type, const wcstring &key_in,
     return success;
 }
 
+/// Encoding of a null string.
+static const wchar_t *ENV_NULL = L"\x1d";
+
+/// Decode a serialized universal variable value into a list.
+static wcstring_list_t decode_serialized(const wcstring &val) {
+    if (val == ENV_NULL) return {};
+    return split_string(val, ARRAY_SEP);
+}
+
+/// Decode a a list into a serialized universal variable value.
+static wcstring encode_serialized(const wcstring_list_t &vals) {
+    if (vals.empty()) return ENV_NULL;
+    return join_strings(vals, ARRAY_SEP);
+}
+
 env_universal_t::env_universal_t(wcstring path)
     : explicit_vars_path(std::move(path)), tried_renaming(false), last_read_file(kInvalidFileID) {}
 
@@ -448,8 +463,8 @@ bool env_universal_t::write_to_fd(int fd, const wcstring &path) {
         // variable; soldier on.
         const wcstring &key = iter->first;
         const env_var_t &var = iter->second;
-        append_file_entry(var.exports() ? SET_EXPORT : SET, key, var.as_string(), &contents,
-                          &storage);
+        append_file_entry(var.exports() ? SET_EXPORT : SET, key, encode_serialized(var.as_list()),
+                          &contents, &storage);
 
         // Go to next.
         ++iter;

tests/set.err

@@ -17,3 +17,6 @@ $argle bargle: invalid var name
 
 ####################
 # Setting local scope when no local scope of the var uses the closest scope
+
+####################
+# Exporting works

tests/set.in

@@ -54,3 +54,9 @@ begin
     set -l -a var6 mno
     set --show var6
 end
+
+logmsg Exporting works
+set -x TESTVAR0
+set -x TESTVAR1 a
+set -x TESTVAR2 a b
+env | grep TESTVAR | cat -v

tests/set.out

@@ -100,3 +100,9 @@ $var6[1]: length=3 value=|ghi|
 $var6[2]: length=3 value=|jkl|
 $var6: not set in universal scope
 
+
+####################
+# Exporting works
+TESTVAR0=
+TESTVAR1=a
+TESTVAR2=a^^b