Do not import vars that are equivalent to a universal exported var

Universal exported variables (created by `set -xU`) used to show up
both as universal and global variable in child instances of fish.

As a result, when changing an exported universal variable, the
new value would only be visible after a new login (or deleting the
variable from global scope in each fish instance).

Additionally, something like `set -xU EDITOR vim -g` would be imported
into the global scope as a single word resulting in failures to
execute $EDITOR in fish.

We cannot simply give precedence to universal variables, because
another process might have exported the same variable.  Instead, we
only skip importing a variable when it is equivalent to an exported
universal variable with the same name.  We compare their values after
joining with spaces, hence skipping those imports does not change the
environment fish passes to its children. Only the representation in
fish is changed from `"vim -g"` to `vim -g`.

Closes #5258.
This eliminates the issue #5348 for universal variables.

CHANGELOG.md

@@ -13,6 +13,7 @@
 - The fish manual, tutorial and FAQ are now available in `man` format as `fish-doc`, `fish-tutorial` and `fish-faq` respectively (#5521).
 - Local values for `fish_complete_path` and `fish_function_path` are now ignored; only their global values are respected.
 - Empty universal variables may now be exported (#5992).
+- Exported universal variables are no longer imported into the global scope, preventing shadowing. This makes it easier to change such variables for all fish sessions and avoids breakage when the value is a list of multiple elements (#5258).
 - A bug where local variables would not be exported to functions has been fixed (#6153).
 - A bug where `string split` would be drop empty strings if the output was only empty strings has been fixed (#5987).
 - `switch` now allows arguments that expand to nothing, like empty variables (#5677).

src/env.cpp

@@ -405,6 +405,19 @@ void env_init(const struct config_paths_t *paths /* or NULL */) {
     callback_data_list_t callbacks;
     s_universal_variables->initialize(callbacks);
     env_universal_callbacks(&env_stack_t::principal(), callbacks);
+
+    // Do not import variables that have the same name and value as
+    // an exported universal variable. See issues #5258 and #5348.
+    for (const auto &kv : uvars()->get_table()) {
+        const wcstring &name = kv.first;
+        const env_var_t &uvar = kv.second;
+        if (!uvar.exports()) continue;
+        // Look for a global exported variable with the same name.
+        maybe_t<env_var_t> global = vars.globals().get(name, ENV_GLOBAL | ENV_EXPORT);
+        if (global && uvar.as_string() == global->as_string()) {
+            vars.globals().remove(name, ENV_GLOBAL | ENV_EXPORT);
+        }
+    }
 }
 
 static int set_umask(const wcstring_list_t &list_val) {

src/env_universal_common.h

@@ -111,6 +111,9 @@ class env_universal_t {
     // Gets variable names.
     wcstring_list_t get_names(bool show_exported, bool show_unexported) const;
 
+    /// Get a view on the universal variable table.
+    const var_table_t &get_table() const { return vars; }
+
     /// Loads variables at the correct path, optionally migrating from a legacy path.
     bool initialize(callback_data_list_t &callbacks);
 

tests/checks/set.fish

@@ -462,4 +462,16 @@ begin
     # CHECK: inner
 end
 
+# Skip importing universal variables (#5258)
+while set -q EDITOR; set -e EDITOR; end
+set -Ux EDITOR emacs -nw
+# CHECK: $EDITOR: not set in global scope
+# CHECK: $EDITOR: set in universal scope, exported, with 2 elements
+$FISH -c 'set -S EDITOR' | string match -r -e 'global|universal'
+
+# When the variable has been changed outside of fish we accept it.
+# CHECK: $EDITOR: set in global scope, exported, with 1 elements
+# CHECK: $EDITOR: set in universal scope, exported, with 2 elements
+sh -c "EDITOR='vim -g' $FISH -c "'\'set -S EDITOR\'' | string match -r -e 'global|universal'
+
 true