fix bug in env_get() involving empty vars

My previous change to eliminate `class var_entry_t` caused me to notice
that `env_get()` turned a set but empty var into a missing var. Which
is wrong. Fixing that brought to light several other pieces of code that
were wrong as a consequence of the aforementioned bug.

Another step to fixing issue #4200.

Makefile.in

@@ -353,7 +353,7 @@ test: test-prep install-force test_low_level test_high_level
 # We want the various tests to run serially so their output doesn't mix
 # We can do that by adding ordering dependencies based on what goals are being used.
 #
-test_goals := test_low_level test_invocation test_fishscript test_interactive
+test_goals := test_low_level test_fishscript test_interactive test_invocation
 
 #
 # The following variables define targets that depend on the tests. If any more targets
@@ -374,7 +374,7 @@ test_low_level: fish_tests $(call filter_up_to,test_low_level,$(active_test_goal
 
 test_high_level: DESTDIR = $(PWD)/test/root/
 test_high_level: prefix = .
-test_high_level: test-prep install-force test_invocation test_fishscript test_interactive
+test_high_level: test-prep install-force test_fishscript test_interactive test_invocation
 .PHONY: test_high_level
 
 test_invocation: $(call filter_up_to,test_invocation,$(active_test_goals))

src/builtin_argparse.cpp

@@ -645,8 +645,7 @@ static void set_argparse_result_vars(argparse_cmd_opts_t &opts) {
 
         auto val = list_to_array_val(opt_spec->vals);
         if (opt_spec->short_flag_valid) {
-            env_set(var_name_prefix + opt_spec->short_flag, *val == ENV_NULL ? NULL : val->c_str(),
-                    ENV_LOCAL);
+            env_set(var_name_prefix + opt_spec->short_flag, val->c_str(), ENV_LOCAL);
         }
         if (!opt_spec->long_flag.empty()) {
             // We do a simple replacement of all non alphanum chars rather than calling
@@ -655,12 +654,12 @@ static void set_argparse_result_vars(argparse_cmd_opts_t &opts) {
             for (size_t pos = 0; pos < long_flag.size(); pos++) {
                 if (!iswalnum(long_flag[pos])) long_flag[pos] = L'_';
             }
-            env_set(var_name_prefix + long_flag, *val == ENV_NULL ? NULL : val->c_str(), ENV_LOCAL);
+            env_set(var_name_prefix + long_flag, val->c_str(), ENV_LOCAL);
         }
     }
 
     auto val = list_to_array_val(opts.argv);
-    env_set(L"argv", *val == ENV_NULL ? NULL : val->c_str(), ENV_LOCAL);
+    env_set(L"argv", val->c_str(), ENV_LOCAL);
 }
 
 /// The argparse builtin. This is explicitly not compatible with the BSD or GNU version of this

src/builtin_read.cpp

@@ -479,7 +479,7 @@ int builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
             split_about(buff.begin(), buff.end(), opts.delimiter.begin(), opts.delimiter.end(),
                         &splits, LONG_MAX);
             auto val = list_to_array_val(splits);
-            env_set(argv[0], *val == ENV_NULL ? NULL : val->c_str(), opts.place);
+            env_set(argv[0], val->c_str(), opts.place);
         }
     } else {  // not array
         if (!opts.have_delimiter) {

src/builtin_set.cpp

@@ -457,17 +457,15 @@ static int builtin_set_list(const wchar_t *cmd, set_cmd_opts_t &opts, int argc,
 
         if (!names_only) {
             env_var_t var = env_get(key, compute_scope(opts));
-            if (!var.missing()) {
+            if (!var.missing_or_empty()) {
                 bool shorten = false;
-                wcstring val = var.as_string();
+                wcstring val = expand_escape_variable(var);
                 if (opts.shorten_ok && val.length() > 64) {
                     shorten = true;
                     val.resize(60);
                 }
-
-                wcstring e_value = expand_escape_variable(val);
                 streams.out.append(L" ");
-                streams.out.append(e_value);
+                streams.out.append(val);
 
                 if (shorten) streams.out.push_back(ellipsis_char);
             }

src/common.cpp

@@ -957,7 +957,6 @@ static void escape_string_script(const wchar_t *orig_in, size_t in_len, wcstring
                 case L'\\':
                 case L'\'': {
                     need_escape = need_complex_escape = 1;
-                    // WTF if (escape_all) out += L'\\';
                     out += L'\\';
                     out += *in;
                     break;

src/env.cpp

@@ -1268,12 +1268,6 @@ env_var_t env_get(const wcstring &key, env_mode_flags_t mode) {
         while (env != NULL) {
             const env_var_t var = env->find_entry(key);
             if (!var.missing() && (var.exportv ? search_exported : search_unexported)) {
-                // The following statement is wrong because ENV_NULL is supposed to mean the var is
-                // set but has zero elements. Therefore we should not return missing_var. However,
-                // If this is changed to return `var` without the special-case then unit tests fail.
-                // Note that tokenize_variable_array() explicitly handles a var whose string
-                // representation contains only ENV_NULL.
-                if (var.as_string() == ENV_NULL) return env_var_t::missing_var();
                 return var;
             }
 

src/env.h

@@ -89,9 +89,9 @@ class env_var_t {
     env_var_t(const wchar_t *x) : val(x), is_missing(false), exportv(false) {}
     env_var_t() : val(L""), is_missing(false), exportv(false) {}
 
-    bool empty(void) const { return val.empty(); };
+    bool empty(void) const { return val.empty() || val == ENV_NULL; };
     bool missing(void) const { return is_missing; }
-    bool missing_or_empty(void) const { return missing() || val.empty(); }
+    bool missing_or_empty(void) const { return missing() || empty(); }
 
     const wchar_t *c_str(void) const;
     void to_list(wcstring_list_t &out) const;

src/expand.cpp

@@ -27,8 +27,10 @@
 
 #include <algorithm>
 #include <functional>
+#include <map>
 #include <memory>  // IWYU pragma: keep
 #include <type_traits>
+#include <utility>
 #include <vector>
 
 #include "common.h"
@@ -42,7 +44,6 @@
 #include "parse_util.h"
 #include "path.h"
 #include "proc.h"
-#include "util.h"
 #include "wildcard.h"
 #include "wutil.h"  // IWYU pragma: keep
 #ifdef KERN_PROCARGS2
@@ -169,15 +170,13 @@ static int is_quotable(const wchar_t *str) {
 static int is_quotable(const wcstring &str) { return is_quotable(str.c_str()); }
 
 wcstring expand_escape_variable(const env_var_t &var) {
-    wcstring_list_t lst;
     wcstring buff;
+    wcstring_list_t lst;
 
     var.to_list(lst);
-
-    size_t size = lst.size();
-    if (size == 0) {
-        buff.append(L"''");
-    } else if (size == 1) {
+    if (lst.size() == 0) {
+        ;  // empty list expands to nothing
+    } else if (lst.size() == 1) {
         const wcstring &el = lst.at(0);
 
         if (el.find(L' ') != wcstring::npos && is_quotable(el)) {
@@ -201,6 +200,7 @@ wcstring expand_escape_variable(const env_var_t &var) {
             }
         }
     }
+
     return buff;
 }
 
@@ -265,9 +265,7 @@ wcstring process_iterator_t::name_for_pid(pid_t pid) {
     }
 
     args = (char *)malloc(maxarg);
-    if (args == NULL) {  // cppcheck-suppress memleak
-        return result;
-    }
+    if (!args) return result;
 
     mib[0] = CTL_KERN;
     mib[1] = KERN_PROCARGS2;
@@ -1584,7 +1582,7 @@ bool fish_xdm_login_hack_hack_hack_hack(std::vector<std::string> *cmds, int argc
 }
 
 std::map<const wcstring, const wcstring> abbreviations;
-void update_abbr_cache(const wchar_t *op, const wcstring varname) {
+void update_abbr_cache(const wchar_t *op, const wcstring &varname) {
     wcstring abbr;
     if (!unescape_string(varname.substr(wcslen(L"_fish_abbr_")), &abbr, 0, STRING_STYLE_VAR)) {
         debug(1, L"Abbreviation var '%ls' is not correctly encoded, ignoring it.", varname.c_str());

src/expand.h

@@ -134,7 +134,7 @@ wcstring replace_home_directory_with_tilde(const wcstring &str);
 
 /// Abbreviation support. Expand src as an abbreviation, returning true if one was found, false if
 /// not. If result is not-null, returns the abbreviation by reference.
-void update_abbr_cache(const wchar_t *op, const wcstring varnam);
+void update_abbr_cache(const wchar_t *op, const wcstring &varname);
 bool expand_abbreviation(const wcstring &src, wcstring *output);
 
 // Terrible hacks

src/fish.cpp

@@ -421,7 +421,7 @@ int main(int argc, char **argv) {
                     list.push_back(str2wcstring(*ptr));
                 }
                 auto val = list_to_array_val(list);
-                env_set(L"argv", *val == ENV_NULL ? NULL : val->c_str(), 0);
+                env_set(L"argv", val->c_str(), ENV_DEFAULT);
 
                 const wcstring rel_filename = str2wcstring(file);