Continue refactoring env_universal_t

Previously an instance of env_universal_t had to be created with a file
path. Switch to allowing it to be created as empty, and later initialized
with the file path. This will help simplify the case where universal
variables are not used; they may simply be not initialized and so just
appear empty.

src/env_universal_common.cpp

@@ -252,11 +252,6 @@ static wcstring encode_serialized(const wcstring_list_t &vals) {
     return join_strings(vals, UVAR_ARRAY_SEP);
 }
 
-env_universal_t::env_universal_t(wcstring path, bool load_legacy)
-    : vars_path_(std::move(path)), load_legacy_(load_legacy) {}
-
-env_universal_t::env_universal_t() : env_universal_t(default_vars_path(), true /* load_legacy */) {}
-
 maybe_t<env_var_t> env_universal_t::get(const wcstring &name) const {
     auto where = vars.find(name);
     if (where != vars.end()) return where->second;
@@ -490,8 +485,11 @@ bool env_universal_t::move_new_vars_file_into_place(const wcstring &src, const w
     return ret == 0;
 }
 
-void env_universal_t::initialize(callback_data_list_t &callbacks) {
+void env_universal_t::initialize_at_path(callback_data_list_t &callbacks, wcstring path,
-    if (vars_path_.empty()) return;
+                                         bool migrate_legacy) {
+    if (path.empty()) return;
+    assert(!initialized() && "Already initialized");
+    vars_path_ = std::move(path);
     scoped_lock locker(lock);
 
     if (load_from_path(vars_path_, callbacks)) {
@@ -499,7 +497,7 @@ void env_universal_t::initialize(callback_data_list_t &callbacks) {
         return;
     }
 
-    if (errno == ENOENT && load_legacy_) {
+    if (errno == ENOENT && migrate_legacy) {
         // We failed to load, because the file was not found. Attempt to load from our legacy paths.
         if (auto dir = default_vars_path_directory()) {
             for (const wcstring &path : get_legacy_paths(*dir)) {
@@ -518,6 +516,10 @@ void env_universal_t::initialize(callback_data_list_t &callbacks) {
     }
 }
 
+void env_universal_t::initialize(callback_data_list_t &callbacks) {
+    this->initialize_at_path(callbacks, default_vars_path(), true /* migrate legacy */);
+}
+
 autoclose_fd_t env_universal_t::open_temporary_file(const wcstring &directory, wcstring *out_path) {
     // Create and open a temporary file for writing within the given directory. Try to create a
     // temporary file, up to 10 times. We don't use mkstemps because we want to open it CLO_EXEC.
@@ -631,7 +633,7 @@ bool env_universal_t::open_and_acquire_lock(const wcstring &path, autoclose_fd_t
 // Returns true if modified variables were written, false if not. (There may still be variable
 // changes due to other processes on a false return).
 bool env_universal_t::sync(callback_data_list_t &callbacks) {
-    if (vars_path_.empty()) return false;
+    if (!initialized()) return false;
 
     FLOGF(uvar_file, L"universal log sync");
     scoped_lock locker(lock);

src/env_universal_common.h

@@ -40,13 +40,8 @@ bool get_hostname_identifier(wcstring &result);
 /// Class representing universal variables.
 class env_universal_t {
    public:
-    // Construct referencing a path \p path. If the path is empty, then the uvars will be empty as
+    // Construct an empty universal variables.
-    // well.
+    env_universal_t() = default;
-    // If \p load_legacy is true, then attempt to load from legacy paths as well.
-    explicit env_universal_t(wcstring path, bool load_legacy = false);
-
-    // Construct with the default path, loading from legacy paths.
-    env_universal_t();
 
     // Get the value of the variable with the specified name.
     maybe_t<env_var_t> get(const wcstring &name) const;
@@ -66,9 +61,15 @@ class env_universal_t {
     /// Get a view on the universal variable table.
     const var_table_t &get_table() const { return vars; }
 
-    /// Loads variables at the correct path, optionally migrating from a legacy path.
+    /// Initialize this uvars for the default path, migrating legacy variables.
+    /// This should be called at most once on any given instance.
     void initialize(callback_data_list_t &callbacks);
 
+    /// Initialize a this uvars for a given path.
+    /// This is exposed for testing only.
+    void initialize_at_path(callback_data_list_t &callbacks, wcstring path,
+                            bool migrate_legacy = false);
+
     /// Reads and writes variables at the correct path. Returns true if modified variables were
     /// written.
     bool sync(callback_data_list_t &callbacks);
@@ -91,11 +92,8 @@ class env_universal_t {
     uint64_t get_export_generation() const;
 
    private:
-    // Path that we save to.
+    // Path that we save to. This is set in initialize(). If empty, initialize has not been called.
-    const wcstring vars_path_;
+    wcstring vars_path_;
-
-    // Whether to load from legacy paths.
-    const bool load_legacy_;
 
     // The table of variables.
     var_table_t vars;
@@ -111,6 +109,12 @@ class env_universal_t {
     // fish wrote the uvars contents.
     bool ok_to_save{true};
 
+    // File id from which we last read.
+    file_id_t last_read_file = kInvalidFileID;
+
+    /// \return whether we are initialized.
+    bool initialized() const { return !vars_path_.empty(); }
+
     mutable std::mutex lock;
     bool load_from_path(const wcstring &path, callback_data_list_t &callbacks);
     void load_from_fd(int fd, callback_data_list_t &callbacks);
@@ -124,9 +128,6 @@ class env_universal_t {
     bool write_to_fd(int fd, const wcstring &path);
     bool move_new_vars_file_into_place(const wcstring &src, const wcstring &dst);
 
-    // File id from which we last read.
-    file_id_t last_read_file = kInvalidFileID;
-
     // Given a variable table, generate callbacks representing the difference between our vars and
     // the new vars. Also update our exports generation count as necessary.
     void generate_callbacks_and_update_exports(const var_table_t &new_vars,

src/fish_tests.cpp

@@ -60,6 +60,7 @@
 #include "input_common.h"
 #include "io.h"
 #include "iothread.h"
+#include "kill.h"
 #include "lru.h"
 #include "maybe.h"
 #include "operation_context.h"
@@ -83,7 +84,6 @@
 #include "wcstringutil.h"
 #include "wildcard.h"
 #include "wutil.h"  // IWYU pragma: keep
-#include "kill.h"
 
 static const char *const *s_arguments;
 static int s_test_run_count = 0;
@@ -3763,7 +3763,8 @@ static void test_undo() {
 
 static int test_universal_helper(int x) {
     callback_data_list_t callbacks;
-    env_universal_t uvars(UVARS_TEST_PATH);
+    env_universal_t uvars;
+    uvars.initialize_at_path(callbacks, UVARS_TEST_PATH);
     for (int j = 0; j < UVARS_PER_THREAD; j++) {
         const wcstring key = format_string(L"key_%d_%d", x, j);
         const wcstring val = format_string(L"val_%d_%d", x, j);
@@ -3793,9 +3794,9 @@ static void test_universal() {
     }
     iothread_drain_all();
 
-    env_universal_t uvars(UVARS_TEST_PATH);
+    env_universal_t uvars;
     callback_data_list_t callbacks;
-    uvars.initialize(callbacks);
+    uvars.initialize_at_path(callbacks, UVARS_TEST_PATH);
     for (int i = 0; i < threads; i++) {
         for (int j = 0; j < UVARS_PER_THREAD; j++) {
             const wcstring key = format_string(L"key_%d_%d", i, j);
@@ -3893,8 +3894,10 @@ static void test_universal_callbacks() {
     say(L"Testing universal callbacks");
     if (system("mkdir -p test/fish_uvars_test/")) err(L"mkdir failed");
     callback_data_list_t callbacks;
-    env_universal_t uvars1(UVARS_TEST_PATH);
+    env_universal_t uvars1;
-    env_universal_t uvars2(UVARS_TEST_PATH);
+    env_universal_t uvars2;
+    uvars1.initialize_at_path(callbacks, UVARS_TEST_PATH);
+    uvars2.initialize_at_path(callbacks, UVARS_TEST_PATH);
 
     env_var_t::env_var_flags_t noflags = 0;
 
@@ -3974,11 +3977,12 @@ static void test_universal_ok_to_save() {
     do_test(before_id != kInvalidFileID && "UVARS_TEST_PATH should be readable");
 
     callback_data_list_t cbs;
-    env_universal_t uvars(UVARS_TEST_PATH);
+    env_universal_t uvars;
-    do_test(uvars.is_ok_to_save() && "Should be OK to save before sync");
+    uvars.initialize_at_path(cbs, UVARS_TEST_PATH);
+    do_test(!uvars.is_ok_to_save() && "Should not be OK to save");
     uvars.sync(cbs);
     cbs.clear();
-    do_test(!uvars.is_ok_to_save() && "Should no longer be OK to save");
+    do_test(!uvars.is_ok_to_save() && "Should still not be OK to save");
     uvars.set(L"SOMEVAR", env_var_t{wcstring{L"SOMEVALUE"}, 0});
     uvars.sync(cbs);