From 292475148850512fd0649b7e83862348ff11b448 Mon Sep 17 00:00:00 2001
From: ridiculousfish <corydoras@ridiculousfish.com>
Date: Sat, 25 May 2019 17:15:50 -0700
Subject: [PATCH] Correct priority of universal and global variable setting

When setting a variable without a specified scope, we should give priority
to an existing local or global above an existing universal variable with
the same name.

In 16fd7804846c0aeca926f207eb0e2ae86f819931 there was a regression that
made universal variables have priority.

Fixes #5883
---
 src/env.cpp     | 15 +++++++++------
 tests/test3.err |  3 +++
 tests/test3.in  | 16 ++++++++++++++++
 tests/test3.out |  8 ++++++++
 4 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/src/env.cpp b/src/env.cpp
index 4eccc060f..6aa1fcd5d 100644
--- a/src/env.cpp
+++ b/src/env.cpp
@@ -1087,16 +1087,19 @@ int env_stack_impl_t::set(const wcstring &key, env_mode_flags_t mode, wcstring_l
         } else {
             DIE("Unknown scope");
         }
+    } else if (env_node_ref_t node = find_in_chain(locals_, key)) {
+        // Existing local variable.
+        set_in_node(node, key, std::move(val), flags);
+    } else if (env_node_ref_t node = find_in_chain(globals_, key)) {
+        // Existing global variable.
+        set_in_node(node, key, std::move(val), flags);
     } else if (uvars() && uvars()->get(key)) {
-        // Modifying an existing universal variable.
+        // Existing universal variable.
         set_universal(key, std::move(val), query);
         *out_needs_uvar_sync = true;
     } else {
-        // The user did not request any scope.
-        // Find the scope.
-        env_node_ref_t node = find_in_chain(locals_, key);
-        if (!node) node = find_in_chain(globals_, key);
-        if (!node) node = resolve_unspecified_scope();
+        // Unspecified scope with no existing variables.
+        auto node = resolve_unspecified_scope();
         assert(node && "Should always resolve some scope");
         set_in_node(node, key, std::move(val), flags);
     }
diff --git a/tests/test3.err b/tests/test3.err
index 1d1eafdb3..5fc2c0e09 100644
--- a/tests/test3.err
+++ b/tests/test3.err
@@ -8,3 +8,6 @@ fish: for: Variable name 'a,b' is not valid. See `help identifiers`.
 
 for a,b in y 1 z 3; echo $a,$b; end
     ^
+
+####################
+# Global vs Universal Unspecified Scopes
diff --git a/tests/test3.in b/tests/test3.in
index 53f637f76..6ee7f5365 100644
--- a/tests/test3.in
+++ b/tests/test3.in
@@ -330,4 +330,20 @@ logmsg Variable names in other commands
 # Test invalid variable names in loops (#5800)
 for a,b in y 1 z 3; echo $a,$b; end
 
+logmsg Global vs Universal Unspecified Scopes
+set -U __fish_test_global_vs_universal universal
+echo "global-vs-universal 1: $__fish_test_global_vs_universal"
+
+set -g __fish_test_global_vs_universal global
+echo "global-vs-universal 2: $__fish_test_global_vs_universal"
+
+set __fish_test_global_vs_universal global2
+echo "global-vs-universal 3: $__fish_test_global_vs_universal"
+
+set -e -g __fish_test_global_vs_universal
+echo "global-vs-universal 4: $__fish_test_global_vs_universal"
+
+set -e -U __fish_test_global_vs_universal
+echo "global-vs-universal 5: $__fish_test_global_vs_universal"
+
 true
diff --git a/tests/test3.out b/tests/test3.out
index 07c15bef4..b8c6b861a 100644
--- a/tests/test3.out
+++ b/tests/test3.out
@@ -58,3 +58,11 @@ a:b a b
 
 ####################
 # Variable names in other commands
+
+####################
+# Global vs Universal Unspecified Scopes
+global-vs-universal 1: universal
+global-vs-universal 2: global
+global-vs-universal 3: global2
+global-vs-universal 4: universal
+global-vs-universal 5: