Reimplement the whole variable stack

The variable stack is a mess - confused locking, surprising callouts, and
unclear division of labor. Just reimplement the whole thing.

src/common.h

@@ -607,15 +607,15 @@ typedef std::lock_guard<std::recursive_mutex> scoped_rlock;
 // Or for simple cases:
 //   name.acquire().value = "derp"
 //
-template <typename DATA>
+template <typename Data>
 class acquired_lock {
     std::unique_lock<std::mutex> lock;
-    acquired_lock(std::mutex &lk, DATA *v) : lock(lk), value(v) {}
+    acquired_lock(std::mutex &lk, Data *v) : lock(lk), value(v) {}
 
     template <typename T>
     friend class owning_lock;
 
-    DATA *value;
+    Data *value;
 
    public:
     // No copying, move construction only
@@ -624,10 +624,14 @@ class acquired_lock {
     acquired_lock(acquired_lock &&) = default;
     acquired_lock &operator=(acquired_lock &&) = default;
 
-    DATA *operator->() { return value; }
-    const DATA *operator->() const { return value; }
-    DATA &operator*() { return *value; }
-    const DATA &operator*() const { return *value; }
+    Data *operator->() { return value; }
+    const Data *operator->() const { return value; }
+    Data &operator*() { return *value; }
+    const Data &operator*() const { return *value; }
+
+    /// Create from a global lock.
+    /// This is used in weird cases where a global lock protects more than one piece of data.
+    static acquired_lock from_global(std::mutex &lk, Data *v) { return acquired_lock{lk, v}; }
 };
 
 // A lock that owns a piece of data

src/env.h

@@ -193,78 +193,30 @@ class null_environment_t : public environment_t {
     wcstring_list_t get_names(int flags) const override;
 };
 
-/// An environment stack of scopes.
-/// The base implementation provides read-only access.
-struct var_stack_t;
-class env_node_t;
-class env_scoped_t : public environment_t {
-   private:
-    std::unique_ptr<var_stack_t> vars_;
-
-    /// A struct wrapping up parser-local variables. These are conceptually variables that differ in
-    /// different fish internal processes.
-    struct perproc_data_t {
-        wcstring pwd;
-    };
-
-    /// The per-process data.
-    perproc_data_t perproc_data_{};
-
-   protected:
-    var_stack_t &vars_stack();
-    const var_stack_t &vars_stack() const;
-
-    explicit env_scoped_t(std::unique_ptr<var_stack_t> vars_);
-    env_scoped_t();
-    env_scoped_t(env_scoped_t &&);
-
-    /// Read-write access to the perproc data.
-    perproc_data_t &perproc_data() { return perproc_data_; }
-
-   public:
-    /// Read-only access to the perproc data.
-    const perproc_data_t &perproc_data() const { return perproc_data_; }
-
-    /// Gets the variable with the specified name, or none() if it does not exist.
-    maybe_t<env_var_t> get(const wcstring &key, env_mode_flags_t mode = ENV_DEFAULT) const override;
-
-    /// Returns all variable names.
-    wcstring_list_t get_names(int flags) const override;
-
-    /// Snapshot this environment. This means returning a read-only copy. Local variables are copied
-    /// but globals are shared (i.e. changes to global will be visible to this snapshot). This
-    /// returns a shared_ptr for convenience, since the most common reason to snapshot is because
-    /// you want to read from another thread.
-    std::shared_ptr<environment_t> snapshot() const;
-
-    ~env_scoped_t() override;
-};
-
-/// A mutable env_scoped_t, that allows scopes to be pushed and popped.
-class env_stack_t final : public env_scoped_t {
+/// A mutable environment which allows scopes to be pushed and popped.
+class env_stack_impl_t;
+class env_stack_t final : public environment_t {
     friend class parser_t;
 
-    int set_internal(const wcstring &key, env_mode_flags_t var_mode, wcstring_list_t val);
+    /// The implementation. Do not access this directly.
+    const std::unique_ptr<env_stack_impl_t> impl_;
 
-    bool try_remove(const std::shared_ptr<env_node_t> &n, const wcstring &key, int var_mode);
-    std::shared_ptr<env_node_t> get_node(const wcstring &key);
+    /// All environment stacks are guarded by a global lock.
+    acquired_lock<env_stack_impl_t> acquire_impl();
+    acquired_lock<const env_stack_impl_t> acquire_impl() const;
 
-    maybe_t<env_var_t> get_perproc(const wcstring &key) const;
+    explicit env_stack_t(std::unique_ptr<env_stack_impl_t> impl);
 
-    static env_stack_t make_principal();
-
-    using env_scoped_t::env_scoped_t;
+   public:
     ~env_stack_t() override;
     env_stack_t(env_stack_t &&);
 
-    /// Given that we have determined that \p key is an unspecial key, set the variable in the right
-    /// scope in the variable stack.
-    void set_scoped_internal(const wcstring &key, env_mode_flags_t var_mode, wcstring_list_t val);
+    /// Implementation of environment_t.
+    maybe_t<env_var_t> get(const wcstring &key, env_mode_flags_t mode = ENV_DEFAULT) const override;
 
-    /// Set the pwd.
-    int set_pwd(wcstring_list_t pwds);
+    /// Implementation of environment_t.
+    wcstring_list_t get_names(int flags) const override;
 
-   public:
     /// Sets the variable with the specified name to the given values.
     int set(const wcstring &key, env_mode_flags_t mode, wcstring_list_t vals);
 
@@ -297,8 +249,14 @@ class env_stack_t final : public env_scoped_t {
     /// \return true if something changed, false otherwise.
     bool universal_barrier();
 
-    /// Returns an array containing all exported variables in a format suitable for execv
-    const char *const *export_arr();
+    /// Returns an array containing all exported variables in a format suitable for execv.
+    std::shared_ptr<const null_terminated_array_t<char>> export_arr();
+
+    /// Snapshot this environment. This means returning a read-only copy. Local variables are copied
+    /// but globals are shared (i.e. changes to global will be visible to this snapshot). This
+    /// returns a shared_ptr for convenience, since the most common reason to snapshot is because
+    /// you want to read from another thread.
+    std::shared_ptr<environment_t> snapshot() const;
 
     /// Update the termsize variable.
     void set_termsize();

src/exec.cpp

@@ -169,7 +169,8 @@ static void launch_process_nofork(env_stack_t &vars, process_t *p) {
     null_terminated_array_t<char> argv_array;
     convert_wide_array_to_narrow(p->get_argv_array(), &argv_array);
 
-    const char *const *envv = vars.export_arr();
+    auto export_vars = vars.export_arr();
+    const char *const *envv = export_vars->get();
     char *actual_cmd = wcs2str(p->actual_cmd);
 
     // Ensure the terminal modes are what they were before we changed them.
@@ -693,8 +694,10 @@ static bool exec_external_command(env_stack_t &vars, const std::shared_ptr<job_t
     // (/dev/tty?).
     make_fd_blocking(STDIN_FILENO);
 
+    auto export_arr = vars.export_arr();
+    ;
     const char *const *argv = argv_array.get();
-    const char *const *envv = vars.export_arr();
+    const char *const *envv = export_arr->get();
 
     std::string actual_cmd_str = wcs2string(p->actual_cmd);
     const char *actual_cmd = actual_cmd_str.c_str();