Add an optional mode to env_get_string()

The mode restricts the scope in which the variable is searched for.

Use this new restricted scope functionality in the `set` builtin. This
fixes `set -g` to not show local shadowing variable values, and also
allows for scoped erasing of slices.

builtin_set.cpp

@@ -69,7 +69,7 @@ static int my_env_set(const wchar_t *key, const wcstring_list_t &val, int scope)
 
         /* Don't bother validating (or complaining about) values that are already present */
         wcstring_list_t existing_values;
-        const env_var_t existing_variable = env_get_string(key);
+        const env_var_t existing_variable = env_get_string(key, scope);
         if (! existing_variable.missing_or_empty())
             tokenize_variable_array(existing_variable, existing_values);
 
@@ -360,7 +360,7 @@ static void print_variables(int include_values, int esc, bool shorten_ok, int sc
 
         if (include_values)
         {
-            env_var_t value = env_get_string(key);
+            env_var_t value = env_get_string(key, scope);
             if (!value.missing())
             {
                 int shorten = 0;
@@ -606,7 +606,7 @@ static int builtin_set(parser_t &parser, wchar_t **argv)
                 wcstring_list_t result;
                 size_t j;
 
-                env_var_t dest_str = env_get_string(dest);
+                env_var_t dest_str = env_get_string(dest, scope);
                 if (! dest_str.missing())
                     tokenize_variable_array(dest_str, result);
 
@@ -696,14 +696,6 @@ static int builtin_set(parser_t &parser, wchar_t **argv)
         return 1;
     }
 
-    if (slice && erase && (scope != ENV_USER))
-    {
-        free(dest);
-        append_format(stderr_buffer, _(L"%ls: Can not specify scope when erasing array slice\n"), argv[0]);
-        builtin_print_help(parser, argv[0], stderr_buffer);
-        return 1;
-    }
-
     /*
       set assignment can work in two modes, either using slices or
       using the whole array. We detect which mode is used here.
@@ -718,7 +710,7 @@ static int builtin_set(parser_t &parser, wchar_t **argv)
         std::vector<long> indexes;
         wcstring_list_t result;
 
-        const env_var_t dest_str = env_get_string(dest);
+        const env_var_t dest_str = env_get_string(dest, scope);
         if (! dest_str.missing())
             tokenize_variable_array(dest_str, result);
 

doc_src/set.txt

@@ -65,11 +65,7 @@ to the scoping rules for variables:
 In query mode, the scope to be examined can be specified.
 
 In erase mode, if variable indices are specified, only the specified
-slices of the array variable will be erased. When erasing an entire
+slices of the array variable will be erased.
-variable (i.e. no slicing), the scope of the variable to be erased can
-be specified. That way, a global variable can be erased even if a
-local variable with the same name exists. Scope can not be specified
-when erasing a slice of an array. The innermost scope is always used.
 
 \c set requires all options to come before any
 other arguments. For example, <code>set flags -l</code> will have

env.cpp

@@ -936,82 +936,107 @@ const wchar_t *env_var_t::c_str(void) const
     return wcstring::c_str();
 }
 
-env_var_t env_get_string(const wcstring &key)
+env_var_t env_get_string(const wcstring &key, int mode)
 {
-    /* Big hack...we only allow getting the history on the main thread. Note that history_t may ask for an environment variable, so don't take the lock here (we don't need it) */
+    const bool has_scope = mode & (ENV_LOCAL | ENV_GLOBAL | ENV_UNIVERSAL);
-    const bool is_main = is_main_thread();
+    const bool search_local = !has_scope || (mode & ENV_LOCAL);
-    if (key == L"history" && is_main)
+    const bool search_global = !has_scope || (mode & ENV_GLOBAL);
-    {
+    const bool search_universal = !has_scope || (mode & ENV_UNIVERSAL);
-        env_var_t result;
 
-        history_t *history = reader_get_history();
+    const bool search_exported = (mode & ENV_EXPORT) || !(mode & ENV_UNEXPORT);
-        if (! history)
+    const bool search_unexported = (mode & ENV_UNEXPORT) || !(mode & ENV_EXPORT);
+
+    /* Make the assumption that electric keys can't be shadowed elsewhere, since we currently block that in env_set() */
+    if (is_electric(key))
+    {
+        if (!search_global) return env_var_t::missing_var();
+        /* Big hack...we only allow getting the history on the main thread. Note that history_t may ask for an environment variable, so don't take the lock here (we don't need it) */
+        if (key == L"history" && is_main_thread())
         {
-            history = &history_t::history_with_name(L"fish");
+            env_var_t result;
-        }
-        if (history)
-            history->get_string_representation(result, ARRAY_SEP_STR);
-        return result;
-    }
-    else if (key == L"COLUMNS")
-    {
-        return to_string(common_get_width());
-    }
-    else if (key == L"LINES")
-    {
-        return to_string(common_get_height());
-    }
-    else if (key == L"status")
-    {
-        return to_string(proc_get_last_status());
-    }
-    else if (key == L"umask")
-    {
-        return format_string(L"0%0.3o", get_umask());
-    }
-    else
-    {
-        {
-            /* Lock around a local region */
-            scoped_lock lock(env_lock);
 
-            env_node_t *env = top;
+            history_t *history = reader_get_history();
-            wcstring result;
+            if (! history)
-
-            while (env != NULL)
             {
-                const var_entry_t *entry = env->find_entry(key);
+                history = &history_t::history_with_name(L"fish");
-                if (entry != NULL)
+            }
-                {
+            if (history)
-                    if (entry->val == ENV_NULL)
+                history->get_string_representation(result, ARRAY_SEP_STR);
-                    {
+            return result;
-                        return env_var_t::missing_var();
+        }
-                    }
+        else if (key == L"COLUMNS")
-                    else
+        {
-                    {
+            return to_string(common_get_width());
-                        return entry->val;
+        }
-                    }
+        else if (key == L"LINES")
-                }
+        {
+            return to_string(common_get_height());
+        }
+        else if (key == L"status")
+        {
+            return to_string(proc_get_last_status());
+        }
+        else if (key == L"umask")
+        {
+            return format_string(L"0%0.3o", get_umask());
+        }
+        // we should never get here unless the electric var list is out of sync
+    }
 
+    if (search_local || search_global) {
+        /* Lock around a local region */
+        scoped_lock lock(env_lock);
+
+        env_node_t *env = search_local ? top : global_env;
+        wcstring result;
+
+        while (env != NULL)
+        {
+            const var_entry_t *entry = env->find_entry(key);
+            if (entry != NULL && (entry->exportv ? search_exported : search_unexported))
+            {
+                if (entry->val == ENV_NULL)
+                {
+                    return env_var_t::missing_var();
+                }
+                else
+                {
+                    return entry->val;
+                }
+            }
+
+            if (has_scope)
+            {
+                if (!search_global || env == global_env) break;
+                env = global_env;
+            }
+            else
+            {
                 env = env->next_scope_to_search();
             }
         }
+    }
 
-        /* Another big hack - only do a universal barrier on the main thread (since it can change variable values)
+    if (!search_universal) return env_var_t::missing_var();
-           Make sure we do this outside the env_lock because it may itself call env_get_string */
-        if (is_main && ! get_proc_had_barrier())
-        {
-            set_proc_had_barrier(true);
-            env_universal_barrier();
-        }
 
-        env_var_t env_var = uvars() ?  uvars()->get(key) : env_var_t::missing_var();
+    /* Another big hack - only do a universal barrier on the main thread (since it can change variable values)
-        if (env_var == ENV_NULL)
+        Make sure we do this outside the env_lock because it may itself call env_get_string */
+    if (is_main_thread() && ! get_proc_had_barrier())
+    {
+        set_proc_had_barrier(true);
+        env_universal_barrier();
+    }
+
+    if (uvars())
+    {
+        env_var_t env_var = uvars()->get(key);
+        if (env_var == ENV_NULL || !(uvars()->get_export(key) ? search_exported : search_unexported))
         {
             env_var = env_var_t::missing_var();
         }
         return env_var;
     }
+    return env_var_t::missing_var();
 }
 
 bool env_exist(const wchar_t *key, int mode)

env.h

@@ -169,8 +169,13 @@ public:
 
 };
 
-/** Gets the variable with the specified name, or env_var_t::missing_var if it does not exist. */
+/**
-env_var_t env_get_string(const wcstring &key);
+   Gets the variable with the specified name, or env_var_t::missing_var if it does not exist.
+
+   \param key The name of the variable to get
+   \param mode An optional scope to search in. All scopes are searched if unset
+*/
+env_var_t env_get_string(const wcstring &key, int mode = 0);
 
 /**
    Returns true if the specified key exists. This can't be reliably done