Merge branch 'fix_set_scopes' of github.com:kballard/fish-shell into kballard-fix_set_scopes

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);
 
@@ -157,6 +157,13 @@ static int my_env_set(const wchar_t *key, const wcstring_list_t &val, int scope)
             break;
         }
 
+        case ENV_SCOPE:
+        {
+            append_format(stderr_buffer, _(L"%ls: Tried to set the special variable '%ls' with the wrong scope\n"), L"set", key);
+            retcode=1;
+            break;
+        }
+
         case ENV_INVALID:
         {
             append_format(stderr_buffer, _(L"%ls: Unknown error"), L"set");
@@ -353,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;
@@ -519,7 +526,7 @@ static int builtin_set(parser_t &parser, wchar_t **argv)
     }
 
 
-    /* We can't both list and erase varaibles */
+    /* We can't both list and erase variables */
     if (erase && list)
     {
         append_format(stderr_buffer,
@@ -588,7 +595,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);
 
@@ -678,14 +685,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.
@@ -700,35 +699,44 @@ 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);
-
-        for (; woptind<argc; woptind++)
         {
-            if (!parse_index(indexes, argv[woptind], dest, result.size()))
-            {
-                builtin_print_help(parser, argv[0], stderr_buffer);
-                retcode = 1;
-                break;
-            }
+            tokenize_variable_array(dest_str, result);
+        }
+        else if (erase)
+        {
+            retcode = 1;
+        }
 
-            size_t idx_count = indexes.size();
-            size_t val_count = argc-woptind-1;
-
-            if (!erase)
+        if (!retcode)
+        {
+            for (; woptind<argc; woptind++)
             {
-                if (val_count < idx_count)
+                if (!parse_index(indexes, argv[woptind], dest, result.size()))
                 {
-                    append_format(stderr_buffer, _(BUILTIN_SET_ARG_COUNT), argv[0]);
                     builtin_print_help(parser, argv[0], stderr_buffer);
-                    retcode=1;
+                    retcode = 1;
                     break;
                 }
-                if (val_count == idx_count)
+
+                size_t idx_count = indexes.size();
+                size_t val_count = argc-woptind-1;
+
+                if (!erase)
                 {
-                    woptind++;
-                    break;
+                    if (val_count < idx_count)
+                    {
+                        append_format(stderr_buffer, _(BUILTIN_SET_ARG_COUNT), argv[0]);
+                        builtin_print_help(parser, argv[0], stderr_buffer);
+                        retcode=1;
+                        break;
+                    }
+                    if (val_count == idx_count)
+                    {
+                        woptind++;
+                        break;
+                    }
                 }
             }
         }

doc_src/set.txt

@@ -27,13 +27,13 @@ The following options control variable scope:
 - <code>-l</code> or <code>--local</code> forces the specified shell variable to be given a scope that is local to the current block, even if a variable with the given name exists and is non-local
 - <code>-g</code> or <code>--global</code> causes the specified shell variable to be given a global scope. Non-global variables disappear when the block they belong to ends
 - <code>-U</code> or <code>--universal</code> causes the specified shell variable to be given a universal scope. If this option is supplied, the variable will be shared between all the current users fish instances on the current computer, and will be preserved across restarts of the shell.
-- <code>-n</code> or <code>--names</code> List only the names of all defined variables, not their value
 - <code>-x</code> or <code>--export</code> causes the specified shell variable to be exported to child processes (making it an "environment variable")
 - <code>-u</code> or <code>--unexport</code> causes the specified shell variable to NOT be exported to child processes
 
 The following options are available:
 - <code>-e</code> or <code>--erase</code> causes the specified shell variable to be erased
 - <code>-q</code> or <code>--query</code> test if the specified variable names are defined. Does not output anything, but the builtins exit status is the number of variables specified that were not defined.
+- <code>-n</code> or <code>--names</code> List only the names of all defined variables, not their value
 - <code>-L</code> or <code>--long</code> do not abbreviate long values when printing set variables
 
 If a variable is set to more than one value, the variable will be an
@@ -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
-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.
+slices of the array variable will be erased.
 
 \c set requires all options to come before any
 other arguments. For example, <code>set flags -l</code> will have

env.cpp

@@ -490,6 +490,8 @@ void env_init(const struct config_paths_t *paths /* or NULL */)
     env_electric.insert(L"history");
     env_electric.insert(L"status");
     env_electric.insert(L"umask");
+    env_electric.insert(L"COLUMNS");
+    env_electric.insert(L"LINES");
 
     top = new env_node_t;
     global_env = top;
@@ -510,11 +512,13 @@ void env_init(const struct config_paths_t *paths /* or NULL */)
         if (eql == wcstring::npos)
         {
             // no equals found
-            env_set(key_and_val, L"", ENV_EXPORT);
+            if (is_read_only(key_and_val) || is_electric(key_and_val)) continue;
+            env_set(key_and_val, L"", ENV_EXPORT | ENV_GLOBAL);
         }
         else
         {
             wcstring key = key_and_val.substr(0, eql);
+            if (is_read_only(key) || is_electric(key)) continue;
             wcstring val = key_and_val.substr(eql + 1);
             if (variable_can_be_array(val))
             {
@@ -589,6 +593,14 @@ void env_init(const struct config_paths_t *paths /* or NULL */)
 
     /* Set fish_bind_mode to "default" */
     env_set(FISH_BIND_MODE_VAR, DEFAULT_BIND_MODE, ENV_GLOBAL);
+
+    /*
+      Now that the global scope is fully initialized, add a toplevel local
+      scope. This same local scope will persist throughout the lifetime of the
+      fish process, and it will ensure that `set -l` commands run at the
+      command-line don't affect the global scope.
+    */
+    env_push(false);
 }
 
 /**
@@ -630,6 +642,15 @@ int env_set(const wcstring &key, const wchar_t *val, int var_mode)
         }
     }
 
+    if ((var_mode & (ENV_LOCAL | ENV_UNIVERSAL)) && (is_read_only(key) || is_electric(key)))
+    {
+        return ENV_SCOPE;
+    }
+    if ((var_mode & ENV_EXPORT) && is_electric(key))
+    {
+        return ENV_SCOPE;
+    }
+
     if ((var_mode & ENV_USER) && is_read_only(key))
     {
         return ENV_PERM;
@@ -923,82 +944,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 is_main = is_main_thread();
-    if (key == L"history" && is_main)
-    {
-        env_var_t result;
+    const bool has_scope = mode & (ENV_LOCAL | ENV_GLOBAL | ENV_UNIVERSAL);
+    const bool search_local = !has_scope || (mode & ENV_LOCAL);
+    const bool search_global = !has_scope || (mode & ENV_GLOBAL);
+    const bool search_universal = !has_scope || (mode & ENV_UNIVERSAL);
 
-        history_t *history = reader_get_history();
-        if (! history)
+    const bool search_exported = (mode & ENV_EXPORT) || !(mode & ENV_UNEXPORT);
+    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");
-        }
-        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_var_t result;
 
-            env_node_t *env = top;
-            wcstring result;
-
-            while (env != NULL)
+            history_t *history = reader_get_history();
+            if (! history)
             {
-                const var_entry_t *entry = env->find_entry(key);
-                if (entry != NULL)
-                {
-                    if (entry->val == ENV_NULL)
-                    {
-                        return env_var_t::missing_var();
-                    }
-                    else
-                    {
-                        return entry->val;
-                    }
-                }
+                history = &history_t::history_with_name(L"fish");
+            }
+            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());
+        }
+        // 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)
-           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();
-        }
+    if (!search_universal) return env_var_t::missing_var();
 
-        env_var_t env_var = uvars() ?  uvars()->get(key) : env_var_t::missing_var();
-        if (env_var == ENV_NULL)
+    /* Another big hack - only do a universal barrier on the main thread (since it can change variable values)
+        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)
@@ -1007,59 +1053,54 @@ bool env_exist(const wchar_t *key, int mode)
 
     CHECK(key, false);
 
-    /*
-      Read only variables all exist, and they are all global. A local
-      version can not exist.
-    */
-    if (!(mode & ENV_LOCAL) && !(mode & ENV_UNIVERSAL))
+    const bool has_scope = mode & (ENV_LOCAL | ENV_GLOBAL | ENV_UNIVERSAL);
+    const bool test_local = !has_scope || (mode & ENV_LOCAL);
+    const bool test_global = !has_scope || (mode & ENV_GLOBAL);
+    const bool test_universal = !has_scope || (mode & ENV_UNIVERSAL);
+
+    const bool test_exported = (mode & ENV_EXPORT) || !(mode & ENV_UNEXPORT);
+    const bool test_unexported = (mode & ENV_UNEXPORT) || !(mode & ENV_EXPORT);
+
+    if (is_electric(key))
     {
-        if (is_read_only(key) || is_electric(key))
+        /*
+        Electric variables all exist, and they are all global. A local or
+        universal version can not exist. They are also never exported.
+        */
+        if (test_global && test_unexported)
         {
-            //Such variables are never exported
-            if (mode & ENV_EXPORT)
-            {
-                return false;
-            }
-            else if (mode & ENV_UNEXPORT)
-            {
-                return true;
-            }
             return true;
         }
+        return false;
     }
 
-    if (!(mode & ENV_UNIVERSAL))
+    if (test_local || test_global)
     {
-        env = (mode & ENV_GLOBAL)?global_env:top;
+        env = test_local ? top : global_env;
 
-        while (env != 0)
+        while (env)
         {
             var_table_t::iterator result = env->env.find(key);
 
             if (result != env->env.end())
             {
                 const var_entry_t &res = result->second;
-
-                if (mode & ENV_EXPORT)
-                {
-                    return res.exportv;
-                }
-                else if (mode & ENV_UNEXPORT)
-                {
-                    return ! res.exportv;
-                }
-
-                return true;
+                return res.exportv ? test_exported : test_unexported;
             }
 
-            if (mode & ENV_LOCAL)
-                break;
-
-            env = env->next_scope_to_search();
+            if (has_scope)
+            {
+                if (!test_global || env == global_env) break;
+                env = global_env;
+            }
+            else
+            {
+                env = env->next_scope_to_search();
+            }
         }
     }
 
-    if (!(mode & ENV_LOCAL) && !(mode & ENV_GLOBAL))
+    if (test_universal)
     {
         if (! get_proc_had_barrier())
         {
@@ -1069,16 +1110,7 @@ bool env_exist(const wchar_t *key, int mode)
 
         if (uvars() && ! uvars()->get(key).missing())
         {
-            if (mode & ENV_EXPORT)
-            {
-                return uvars()->get_export(key);
-            }
-            else if (mode & ENV_UNEXPORT)
-            {
-                return ! uvars()->get_export(key);
-            }
-
-            return 1;
+            return uvars()->get_export(key) ? test_exported : test_unexported;
         }
     }
 
@@ -1233,13 +1265,6 @@ wcstring_list_t env_get_names(int flags)
         {
             result.insert(result.end(), env_electric.begin(), env_electric.end());
         }
-
-        if (show_exported)
-        {
-            result.push_back(L"COLUMNS");
-            result.push_back(L"LINES");
-        }
-
     }
 
     if (show_universal && uvars())

env.h

@@ -49,6 +49,7 @@
 enum
 {
     ENV_PERM = 1,
+    ENV_SCOPE,
     ENV_INVALID
 }
 ;
@@ -82,6 +83,7 @@ void env_init(const struct config_paths_t *paths = NULL);
    The current error codes are:
 
    * ENV_PERM, can only be returned when setting as a user, e.g. ENV_USER is set. This means that the user tried to change a read-only variable.
+   * ENV_SCOPE, the variable cannot be set in the given scope. This applies to readonly/electric variables set from the local or universal scopes, or set as exported.
    * ENV_INVALID, the variable name or mode was invalid
 */
 
@@ -167,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