Large set of changes to how PATH is handled. Changed fish to no longer modify PATH in share/config.fish. Introduced variable fish_user_paths, and a glue function __fish_reconstruct_path that splices together PATH with fish_user_paths. Changed fish to no longer validate changes to PATH unless the paths are new (i.e. don't recheck what's already there). Modified certain sets to store const wchar_t instead of wcstring to save a few allocations.

https://github.com/fish-shell/fish-shell/issues/527

builtin_set.cpp

@@ -67,15 +67,26 @@ static int my_env_set(const wchar_t *key, const wcstring_list_t &val, int scope)
         /* Fix for https://github.com/fish-shell/fish-shell/issues/199 . Return success if any path setting succeeds. */
         bool any_success = false;
 
+        /* 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);
+        if (! existing_variable.missing_or_empty())
+            tokenize_variable_array(existing_variable, existing_values);
+
         for (i=0; i< val.size() ; i++)
         {
+            const wcstring &dir = val.at(i);
+            if (list_contains_string(existing_values, dir))
+            {
+                any_success = true;
+                continue;
+            }
+
             bool show_perror = false;
             int show_hint = 0;
             bool error = false;
 
             struct stat buff;
-            const wchar_t *dir = val[i].c_str();
-
             if (wstat(dir, &buff))
             {
                 error = true;
@@ -93,9 +104,8 @@ static int my_env_set(const wchar_t *key, const wcstring_list_t &val, int scope)
             }
             else
             {
-                const wchar_t *colon;
+                append_format(stderr_buffer, _(BUILTIN_SET_PATH_ERROR), L"set", dir.c_str(), key);
-                append_format(stderr_buffer, _(BUILTIN_SET_PATH_ERROR), L"set", dir, key);
+                const wchar_t *colon = wcschr(dir.c_str(), L':');
-                colon = wcschr(dir, L':');
 
                 if (colon && *(colon+1))
                 {
@@ -111,7 +121,7 @@ static int my_env_set(const wchar_t *key, const wcstring_list_t &val, int scope)
 
             if (show_hint)
             {
-                append_format(stderr_buffer, _(BUILTIN_SET_PATH_HINT), L"set", key, key, wcschr(dir, L':')+1);
+                append_format(stderr_buffer, _(BUILTIN_SET_PATH_HINT), L"set", key, key, wcschr(dir.c_str(), L':')+1);
             }
 
         }

builtin_set_color.cpp

@@ -163,7 +163,7 @@ static int builtin_set_color(parser_t &parser, wchar_t **argv)
         append_format(stderr_buffer, _("%s: Unknown color '%s'\n"), argv[0], bgcolor);
         return STATUS_BUILTIN_ERROR;
     }
-    
+
     /* Make sure that the term exists */
     if (cur_term == NULL && setupterm(0, STDOUT_FILENO, 0) == ERR)
     {
@@ -183,7 +183,7 @@ static int builtin_set_color(parser_t &parser, wchar_t **argv)
 
     /* Save old output function so we can restore it */
     int (* const saved_writer_func)(char) = output_get_writer();
-    
+
     /* Set our output function, which writes to a std::string */
     builtin_set_color_output.clear();
     output_set_writer(set_color_builtin_outputter);
@@ -229,14 +229,14 @@ static int builtin_set_color(parser_t &parser, wchar_t **argv)
             write_background_color(index_for_color(bg));
         }
     }
-    
+
     /* Restore saved writer function */
     output_set_writer(saved_writer_func);
-    
+
     /* Output the collected string */
     std::string local_output;
     std::swap(builtin_set_color_output, local_output);
     stdout_buffer.append(str2wcstring(local_output));
-    
+
     return STATUS_BUILTIN_OK;
 }

env.cpp

@@ -130,7 +130,13 @@ struct env_node_t
     struct env_node_t *next;
 
 
-    env_node_t() : new_scope(false), exportv(false), next(NULL) { }
+    env_node_t() : new_scope(false), exportv(false) { }
+
+    /* Returns a pointer to the given entry if present, or NULL. */
+    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);
 };
 
 class variable_entry_t
@@ -141,15 +147,11 @@ class variable_entry_t
 
 static pthread_mutex_t env_lock = PTHREAD_MUTEX_INITIALIZER;
 
-/**
+/** Top node on the function stack */
-   Top node on the function stack
+static env_node_t *top = NULL;
-*/
-static env_node_t *top=0;
 
-/**
+/** Bottom node on the function stack */
-   Bottom node on the function stack
+static env_node_t *global_env = NULL;
-*/
-static env_node_t *global_env = 0;
 
 
 /**
@@ -157,33 +159,41 @@ static env_node_t *global_env = 0;
 */
 static var_table_t *global;
 
-/**
+/* Helper class for storing constant strings, without needing to wrap them in a wcstring */
-   Table of variables that may not be set using the set command.
+
-*/
+/* Comparer for const string set */
-static std::set<wcstring> env_read_only;
+struct const_string_set_comparer
+{
+    bool operator()(const wchar_t *a, const wchar_t *b)
+    {
+        return wcscmp(a, b) < 0;
+    }
+};
+typedef std::set<const wchar_t *, const_string_set_comparer> const_string_set_t;
+
+/** Table of variables that may not be set using the set command. */
+static const_string_set_t env_read_only;
 
 static bool is_read_only(const wcstring &key)
 {
-    return env_read_only.find(key) != env_read_only.end();
+    return env_read_only.find(key.c_str()) != env_read_only.end();
 }
 
 /**
    Table of variables whose value is dynamically calculated, such as umask, status, etc
 */
-static std::set<wcstring> env_electric;
+static const_string_set_t env_electric;
 
 static bool is_electric(const wcstring &key)
 {
-    return env_electric.find(key) != env_electric.end();
+    return env_electric.find(key.c_str()) != env_electric.end();
 }
 
-
 /**
    Exported variable array used by execv
 */
 static null_terminated_array_t<char> export_array;
 
-
 /**
    Flag for checking if we need to regenerate the exported variable
    array
@@ -194,12 +204,6 @@ static void mark_changed_exported()
     has_changed_exported = true;
 }
 
-/**
-   This string is used to store the value of dynamically
-   generated variables, such as history.
-*/
-static wcstring dyn_var;
-
 /**
    List of all locale variable names
 */
@@ -217,6 +221,23 @@ static const wchar_t * const locale_variable[] =
 };
 
 
+const var_entry_t *env_node_t::find_entry(const wcstring &key)
+{
+    const var_entry_t *result = NULL;
+    var_table_t::const_iterator where = env.find(key);
+    if (where != env.end())
+    {
+        result = &where->second;
+    }
+    return result;
+}
+
+env_node_t *env_node_t::next_scope_to_search(void)
+{
+    return this->new_scope ? global_env : this->next;
+}
+
+
 /**
    When fishd isn't started, this function is provided to
    env_universal as a callback, it tries to start up fishd. It's
@@ -376,7 +397,7 @@ static void universal_callback(fish_message_type_t type,
                                const wchar_t *name,
                                const wchar_t *val)
 {
-    const wchar_t *str=0;
+    const wchar_t *str = NULL;
 
     switch (type)
     {
@@ -417,37 +438,32 @@ static void universal_callback(fish_message_type_t type,
 */
 static void setup_path()
 {
-    size_t i;
-    int j;
-    wcstring_list_t lst;
-
     const wchar_t *path_el[] =
     {
         L"/bin",
         L"/usr/bin",
-        PREFIX L"/bin",
+        NULL
-        0
+    };
-    }
-    ;
 
     env_var_t path = env_get_string(L"PATH");
 
-    if (!path.missing())
+    wcstring_list_t lst;
+    if (! path.missing())
     {
         tokenize_variable_array(path, lst);
     }
 
-    for (j=0; path_el[j]; j++)
+    for (size_t j=0; path_el[j] != NULL; j++)
     {
 
-        int has_el=0;
+        bool has_el = false;
 
-        for (i=0; i<lst.size(); i++)
+        for (size_t i=0; i<lst.size(); i++)
         {
-            wcstring el = lst.at(i);
+            const wcstring &el = lst.at(i);
             size_t len = el.size();
 
-            while ((len > 0) && (el[len-1]==L'/'))
+            while ((len > 0) && (el.at(len-1)==L'/'))
             {
                 len--;
             }
@@ -455,11 +471,12 @@ static void setup_path()
             if ((wcslen(path_el[j]) == len) &&
                     (wcsncmp(el.c_str(), path_el[j], len)==0))
             {
-                has_el = 1;
+                has_el = true;
+                break;
             }
         }
 
-        if (!has_el)
+        if (! has_el)
         {
             wcstring buffer;
 
@@ -470,8 +487,8 @@ static void setup_path()
                 buffer += path;
             }
 
-            buffer += ARRAY_SEP_STR;
+            buffer.append(ARRAY_SEP_STR);
-            buffer += path_el[j];
+            buffer.append(path_el[j]);
 
             env_set(L"PATH", buffer.empty()?NULL:buffer.c_str(), ENV_GLOBAL | ENV_EXPORT);
 
@@ -542,8 +559,6 @@ static bool variable_can_be_array(const wcstring &key)
 
 void env_init(const struct config_paths_t *paths /* or NULL */)
 {
-    char **p;
-
     /*
       env_read_only variables can not be altered directly by the user
     */
@@ -594,7 +609,7 @@ void env_init(const struct config_paths_t *paths /* or NULL */)
     /*
       Import environment variables
     */
-    for (p=environ?environ:__environ; p && *p; p++)
+    for (char **p = (environ ? environ : __environ); p && *p; p++)
     {
         const wcstring key_and_val = str2wcstring(*p); //like foo=bar
         size_t eql = key_and_val.find(L'=');
@@ -719,19 +734,12 @@ static env_node_t *env_get_node(const wcstring &key)
     env_node_t *env = top;
     while (env != NULL)
     {
-        if (env->env.find(key) != env->env.end())
+        if (env->find_entry(key) != NULL)
         {
             break;
         }
 
-        if (env->new_scope)
+        env = env->next_scope_to_search();
-        {
-            env = global_env;
-        }
-        else
-        {
-            env = env->next;
-        }
     }
     return env;
 }
@@ -1079,28 +1087,20 @@ env_var_t env_get_string(const wcstring &key)
 
             while (env != NULL)
             {
-                var_table_t::iterator result =  env->env.find(key);
+                const var_entry_t *entry = env->find_entry(key);
-                if (result != env->env.end())
+                if (entry != NULL)
                 {
-                    const var_entry_t &res = result->second;
+                    if (entry->val == ENV_NULL)
-                    if (res.val == ENV_NULL)
                     {
                         return env_var_t::missing_var();
                     }
                     else
                     {
-                        return res.val;
+                        return entry->val;
                     }
                 }
 
-                if (env->new_scope)
+                env = env->next_scope_to_search();
-                {
-                    env = global_env;
-                }
-                else
-                {
-                    env = env->next;
-                }
             }
         }
 
@@ -1180,14 +1180,7 @@ bool env_exist(const wchar_t *key, int mode)
             if (mode & ENV_LOCAL)
                 break;
 
-            if (env->new_scope)
+            env = env->next_scope_to_search();
-            {
-                env = global_env;
-            }
-            else
-            {
-                env = env->next;
-            }
         }
     }
 

env.h

@@ -172,9 +172,8 @@ public:
 
 
 };
-/**
+
- Gets the variable with the specified name, or an empty string if it does not exist.
+/** 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);
 
 /**

env_universal_common.cpp

@@ -425,7 +425,7 @@ static int read_byte(connection_t *src)
     if (src->buffer_consumed >= src->read_buffer.size())
     {
         char local[ENV_UNIVERSAL_BUFFER_SIZE];
-        
+
         ssize_t res = read(src->fd, local, sizeof local);
 
 //    debug(4, L"Read chunk '%.*s'", res, src->buffer );
@@ -600,7 +600,7 @@ static void parse_message(wchar_t *msg,
         {
             wchar_t *val;
             const wcstring key(name, tmp - name);
-            
+
             val = tmp+1;
             val = unescape(val, 0);
 

env_universal_common.h

@@ -78,17 +78,17 @@ typedef std::queue<message_t *> message_queue_t;
 */
 class connection_t
 {
-    private:
+private:
     /* No assignment */
     connection_t &operator=(const connection_t &);
-    
+
-    public:
+public:
-    
+
     /**
        The file descriptor this socket lives on
     */
     int fd;
-    
+
     /**
        Queue of unsent messages
     */
@@ -113,7 +113,7 @@ class connection_t
        Number of bytes that have already been consumed.
     */
     size_t buffer_consumed;
-    
+
     /* Constructor */
     connection_t(int input_fd);
 };

fishd.cpp

@@ -1030,7 +1030,7 @@ int main(int argc, char ** argv)
             }
         }
 
-        for (connection_list_t::iterator iter = connections.begin(); iter != connections.end(); )
+        for (connection_list_t::iterator iter = connections.begin(); iter != connections.end();)
         {
             if (iter->killme)
             {

reader.cpp

@@ -3101,18 +3101,18 @@ const wchar_t *reader_readline()
                     const wchar_t *buff = data->command_line.c_str();
                     const wchar_t *end = &buff[data->buff_pos];
                     const wchar_t *begin = end;
-                    
+
                     /* Make sure we delete at least one character (see #580) */
                     begin--;
-                    
+
                     /* Delete until we hit a newline, or the beginning of the string */
                     while (begin > buff  && *begin != L'\n')
                         begin--;
-                    
+
                     /* If we landed on a newline, don't delete it */
                     if (*begin == L'\n')
                         begin++;
-                    
+
                     assert(end >= begin);
                     size_t len = maxi<size_t>(end-begin, 1);
                     begin = end - len;

share/config.fish

@@ -57,38 +57,32 @@ if test -d /usr/xpg4/bin
 	end
 end
 
-#
+# Add a handler for when fish_user_path changes, so we can apply the same changes to PATH
-# Add a few common directories to path, if they exists. Note that pure
+# Invoke it immediately to apply the current value of fish_user_path
-# console programs like makedep sometimes live in /usr/X11R6/bin, so we
+function __fish_reconstruct_path -d "Update PATH when fish_user_paths changes" --on-variable fish_user_paths
-# want this even for text-only terminals.
+        set -l local_path $PATH
-#
+        set -l x
-
+        for x in $__fish_added_user_paths
-set -l path_list /bin /usr/bin /usr/X11R6/bin /usr/local/bin $__fish_bin_dir
+                if set -l idx (contains --index $x $local_path)
-
+                        set -e local_path[$idx]
-# Root should also have the sbin directories in the path
+                end
-switch $USER
-	case root
-	set path_list $path_list /sbin /usr/sbin /usr/local/sbin
-end
-
-#
-# It's desirable to only modify PATH once, because fish will check it for validity,
-# which performs disk I/O. Construct the new PATH locally.
-#
-
-set -l path_under_construction $PATH
-for i in $path_list
-	if begin ; not contains $i $path_under_construction ; and test -d $i ; end
-		set path_under_construction $path_under_construction $i
 	end
-end
 
-set PATH $path_under_construction
+        set -e __fish_added_user_paths
+        for x in $fish_user_paths
+                if not contains $x $local_path
+                        set local_path $local_path $x
+                        set -g __fish_added_user_paths $__fish_added_user_paths $x
+                end
+        end
+        set -xg PATH $local_path
+end
+__fish_reconstruct_path
 
 #
 # Launch debugger on SIGTRAP
 #
-function fish_sigtrap_handler --on-signal TRAP --no-scope-shadowing --description "Signal handler for the TRAP signal. Lanches a debug prompt."
+function fish_sigtrap_handler --on-signal TRAP --no-scope-shadowing --description "Signal handler for the TRAP signal. Launches a debug prompt."
 	breakpoint
 end