From d5f3a09ce932779346d521afe1ba0ffdcb1cbfa9 Mon Sep 17 00:00:00 2001
From: ridiculousfish <corydoras@ridiculousfish.com>
Date: Fri, 23 Oct 2015 15:15:39 -0700
Subject: [PATCH] Make 'set -ql' search up to function scope

Previously 'set -ql' would only look for variables in the
immediate local scope. This was not very useful. It's also
arguably surprising, since a 'set -l' in a function, followed
by a 'set -ql' in a child block, would fail. There was also no
way to check for a function-scoped variable, since omitting the
scope would also pull in global variables.

We could revisit this and introduce an explicit function scope.

Fixes #2502
---
 src/env.cpp     | 37 ++++++++++++++++++-------------------
 tests/test4.in  | 11 +++++++++++
 tests/test4.out |  2 ++
 3 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/src/env.cpp b/src/env.cpp
index 4f2004e22..3c73fb076 100644
--- a/src/env.cpp
+++ b/src/env.cpp
@@ -93,7 +93,8 @@ struct env_node_t
     const var_entry_t *find_entry(const wcstring &key);
 
     /* Returns the next scope to search in order, respecting the new_scope flag, or NULL if we're done. */
-    env_node_t *next_scope_to_search(void);
+    env_node_t *next_scope_to_search();
+    const env_node_t *next_scope_to_search() const;
 };
 
 class variable_entry_t
@@ -195,11 +196,17 @@ const var_entry_t *env_node_t::find_entry(const wcstring &key)
     return result;
 }
 
-env_node_t *env_node_t::next_scope_to_search(void)
+env_node_t *env_node_t::next_scope_to_search()
 {
     return this->new_scope ? global_env : this->next;
 }
 
+const env_node_t *env_node_t::next_scope_to_search() const
+{
+    return this->new_scope ? global_env : this->next;
+}
+
+
 /**
    Return the current umask value.
 */
@@ -1010,8 +1017,6 @@ env_var_t env_get_string(const wcstring &key, env_mode_flags_t mode)
 
 bool env_exist(const wchar_t *key, env_mode_flags_t mode)
 {
-    env_node_t *env;
-
     CHECK(key, false);
 
     const bool has_scope = mode & (ENV_LOCAL | ENV_GLOBAL | ENV_UNIVERSAL);
@@ -1037,27 +1042,21 @@ bool env_exist(const wchar_t *key, env_mode_flags_t mode)
 
     if (test_local || test_global)
     {
-        env = test_local ? top : global_env;
-
-        while (env)
+        const env_node_t *env = test_local ? top : global_env;
+        while (env != NULL)
         {
-            var_table_t::iterator result = env->env.find(key);
-
+            if (env == global_env && ! test_global)
+            {
+                break;
+            }
+            
+            var_table_t::const_iterator result = env->env.find(key);
             if (result != env->env.end())
             {
                 const var_entry_t &res = result->second;
                 return res.exportv ? test_exported : test_unexported;
             }
-
-            if (has_scope)
-            {
-                if (!test_global || env == global_env) break;
-                env = global_env;
-            }
-            else
-            {
-                env = env->next_scope_to_search();
-            }
+            env = env->next_scope_to_search();
         }
     }
 
diff --git a/tests/test4.in b/tests/test4.in
index f207e6b79..6d5014553 100644
--- a/tests/test4.in
+++ b/tests/test4.in
@@ -177,3 +177,14 @@ true ; set -NOT_AN_OPTION 2> /dev/null ; echo 9 $status # no passthrough
 false ; set foo (echo A; true) ; echo 10 $status $foo
 true ; set foo (echo B; false) ; echo 11 $status $foo
 true
+
+echo "Verify set -ql behavior" # see 2502
+function setql_check
+  set -l setql_foo val
+  if set -ql setql_foo
+    echo Pass
+  else
+    echo Fail
+  end
+end
+setql_check
diff --git a/tests/test4.out b/tests/test4.out
index 148d870c4..f38fa96f9 100644
--- a/tests/test4.out
+++ b/tests/test4.out
@@ -34,3 +34,5 @@ Verify that set passes through exit status, except when passed -n or -q or -e
 9 1
 10 0 A
 11 1 B
+Verify set -ql behavior
+Pass