Implement deletion of universal variables. Add tests for them.

env_universal.cpp

@@ -277,11 +277,6 @@ static void reconnect()
     }
 }
 
-void env_universal_read_from_file()
-{
-    
-}
-
 void env_universal_init(wchar_t * p,
                         wchar_t *u,
                         void (*sf)(),
@@ -291,8 +286,8 @@ void env_universal_init(wchar_t * p,
     {
         external_callback = cb;
         env_universal_common_init(&callback);
-        env_universal_read_from_file();
         s_env_univeral_inited = true;
+        env_universal_common_sync();
     }
     else
     {

env_universal_common.cpp

@@ -647,6 +647,33 @@ void env_universal_t::enqueue_all(connection_t *c) const
     enqueue_all_internal(c);
 }
 
+void env_universal_t::clear_values_for_unmodified_keys()
+{
+    // Little optimization
+    if (modified.empty())
+    {
+        // Nothing modified, clear all values
+        vars.clear();
+        return;
+    }
+    
+    var_table_t::iterator iter = vars.begin();
+    while (iter != vars.end())
+    {
+        const wcstring &key = iter->first;
+        if (modified.find(key) != modified.end())
+        {
+            // It's modified, don't erase the value
+            ++iter;
+        }
+        else
+        {
+            // Unmodified, clear the value from the map
+            vars.erase(iter++);
+        }
+    }
+}
+
 void env_universal_t::load_from_fd(int fd, callback_data_list_t *callbacks)
 {
     ASSERT_IS_LOCKED(lock);
@@ -655,6 +682,9 @@ void env_universal_t::load_from_fd(int fd, callback_data_list_t *callbacks)
     const file_id_t current_file = file_id_for_fd(fd);
     if (current_file != last_read_file)
     {
+        /* Erase all non-modified keys. Some of these may have been deleted in the file, and the only way we can tell is by their absence in the file. */
+        this->clear_values_for_unmodified_keys();
+        
         connection_t c(fd);
         /* Read from the file. Do not destroy the connection; the caller is responsible for closing the fd. */
         this->read_message_internal(&c, callbacks);
@@ -665,14 +695,18 @@ void env_universal_t::load_from_fd(int fd, callback_data_list_t *callbacks)
 bool env_universal_t::load_from_path(const wcstring &path, callback_data_list_t *callbacks)
 {
     ASSERT_IS_LOCKED(lock);
-    /* OK to not use CLO_EXEC here because fishd is single threaded */
     bool result = false;
-    int fd = wopen_cloexec(path, O_RDONLY);
-    if (fd >= 0)
+    /* Skip the file if it has not changed. This is the primary mechanism by which we elide expensive re-syncing. */
+    const file_id_t current_file = file_id_for_path(path);
+    if (current_file != last_read_file)
     {
-        this->load_from_fd(fd, callbacks);
-        close(fd);
-        result = true;
+        int fd = wopen_cloexec(path, O_RDONLY);
+        if (fd >= 0)
+        {
+            this->load_from_fd(fd, callbacks);
+            close(fd);
+            result = true;
+        }
     }
     return result;
 }

env_universal_common.h

@@ -227,6 +227,7 @@ class env_universal_t
     
     void set_internal(const wcstring &key, const wcstring &val, bool exportv, bool overwrite);
     void remove_internal(const wcstring &name, bool overwrite);
+    void clear_values_for_unmodified_keys();
     
     /* Functions concerned with saving */
     bool open_and_acquire_lock(const wcstring &path, int *out_fd);

fish_tests.cpp

@@ -2160,7 +2160,7 @@ static void test_input()
     }
 }
 
-#define UVARS_PER_THREAD 8
+#define UVARS_PER_THREAD 16
 
 static int test_universal_helper(int *x)
 {
@@ -2176,8 +2176,14 @@ static int test_universal_helper(int *x)
             err(L"Failed to sync universal variables");
         }
         fputc('.', stderr);
-        fflush(stderr);
     }
+    
+    /* Delete the first key */
+    const wcstring key = format_string(L"key_%d_%d", *x, 0);
+    uvars.remove(key);
+    uvars.sync(NULL);
+    fputc('.', stderr);
+    
     return 0;
 }
 
@@ -2204,11 +2210,21 @@ static void test_universal()
         for (int j=0; j < UVARS_PER_THREAD; j++)
         {
             const wcstring key = format_string(L"key_%d_%d", i, j);
-            const wcstring val = format_string(L"val_%d_%d", i, j);
-            const env_var_t var = uvars.get(key);
-            if (var != val)
+            env_var_t expected_val;
+            if (j == 0)
             {
-                err(L"Wrong value for key %ls: %ls vs %ls\n", key.c_str(), val.c_str(), var.missing() ? L"<missing>" : var.c_str());
+                expected_val = env_var_t::missing_var();
+            }
+            else
+            {
+                expected_val = format_string(L"val_%d_%d", i, j);
+            }
+            const env_var_t var = uvars.get(key);
+            if (var != expected_val)
+            {
+                const wchar_t *missing = L"<missing>";
+                
+                err(L"Wrong value for key %ls: expected %ls, got %ls\n", key.c_str(), expected_val.missing() ? missing : expected_val.c_str(), var.missing() ? missing : var.c_str());
             }
         }
     }