Merge branch 'patch/service-check' into 'master'

Service: only emit changed signals when values change

See merge request GNOME/dconf!3

6 files changed, 449 insertions, 26 deletions

diff --git a/common/dconf-changeset.c b/common/dconf-changeset.c
index c80c88c..cea34c1 100644
--- a/common/dconf-changeset.c
+++ b/common/dconf-changeset.c
@@ -772,6 +772,87 @@ dconf_changeset_change (DConfChangeset *changeset,
 }
 
 /**
+ * dconf_changeset_filter_changes:
+ * @base: a database mode changeset
+ * @changes: a changeset
+ *
+ * Produces a changeset that contains all the changes in @changes that
+ * are not already present in @base
+ *
+ * If there are no such changes, %NULL is returned
+ *
+ * Applying the result to @base will yield the same result as applying
+ * @changes to @base
+ *
+ * Returns: (transfer full) (nullable): the minimal changes, or %NULL
+ *
+ * Since: 0.35.1
+ */
+DConfChangeset *
+dconf_changeset_filter_changes (DConfChangeset *base,
+                                DConfChangeset *changes)
+{
+  DConfChangeset *result = NULL;
+  GHashTableIter iter_changes;
+  gpointer key, val;
+
+  g_return_val_if_fail (base->is_database, NULL);
+
+  /* We create the list of changes by iterating the 'changes' changeset
+   * and noting any keys that are not in the 'base' changeset or do not
+   * have the same value in the 'base' changeset
+   *
+   * Note: because 'base' is a database changeset we don't have to
+   * worry about it containing NULL values (dir resets).
+   */
+  g_hash_table_iter_init (&iter_changes, changes->table);
+  while (g_hash_table_iter_next (&iter_changes, &key, &val))
+    {
+      GVariant *base_val = g_hash_table_lookup (base->table, key);
+
+      if (g_str_has_suffix (key, "/"))
+        {
+          // Path reset
+          gboolean reset_is_effective = FALSE;
+          GHashTableIter iter_base;
+          gpointer base_key = NULL;
+
+          g_return_val_if_fail (val == NULL, NULL);
+
+          // First we check whether there are any keys in base that would be reset
+          g_hash_table_iter_init (&iter_base, base->table);
+          while (g_hash_table_iter_next (&iter_base, &base_key, NULL))
+            if (g_str_has_prefix (base_key, key) && !g_str_equal (base_key, key))
+              {
+                reset_is_effective = TRUE;
+                break;
+              }
+
+          if (reset_is_effective)
+            {
+              if (!result)
+                result = dconf_changeset_new ();
+
+              dconf_changeset_set (result, key, val);
+            }
+        }
+      else if (base_val == NULL && val == NULL)
+        continue; // Resetting a key that wasn't set
+      else if (val == NULL || base_val == NULL || !g_variant_equal (val, base_val))
+        {
+          // Resetting an existing key, inserting a value under a key that was not
+          // set, or replacing an existing value with a different one.
+          if (!result)
+            result = dconf_changeset_new ();
+
+          dconf_changeset_set (result, key, val);
+        }
+    }
+
+  return result;
+}
+
+/**
  * dconf_changeset_diff:
  * @from: a database mode changeset
  * @to: a database mode changeset
@@ -793,7 +874,7 @@ DConfChangeset *
 dconf_changeset_diff (DConfChangeset *from,
                       DConfChangeset *to)
 {
-  DConfChangeset *changeset = NULL;
+  DConfChangeset *changeset;
   GHashTableIter iter;
   gpointer key, val;
 
@@ -806,8 +887,8 @@ dconf_changeset_diff (DConfChangeset *from,
    *
    * We create our list of changes in two steps:
    *
-   *   - iterate the 'to' changeset and note any keys that do not have
-   *     the same value in the 'from' changeset
+   *   - call dconf_changeset_filter_changes to find values from 'to'
+   *     which are not present in 'from' or hold different values to 'to'
    *
    *   - iterate the 'from' changeset and note any keys not present in
    *     the 'to' changeset, recording resets for them
@@ -817,19 +898,8 @@ dconf_changeset_diff (DConfChangeset *from,
    * Note: because 'from' and 'to' are database changesets we don't have
    * to worry about seeing NULL values or dirs.
    */
-  g_hash_table_iter_init (&iter, to->table);
-  while (g_hash_table_iter_next (&iter, &key, &val))
-    {
-      GVariant *from_val = g_hash_table_lookup (from->table, key);
-
-      if (from_val == NULL || !g_variant_equal (val, from_val))
-        {
-          if (!changeset)
-            changeset = dconf_changeset_new ();
 
-          dconf_changeset_set (changeset, key, val);
-        }
-    }
+  changeset = dconf_changeset_filter_changes (from, to);
 
   g_hash_table_iter_init (&iter, from->table);
   while (g_hash_table_iter_next (&iter, &key, &val))
diff --git a/common/dconf-changeset.h b/common/dconf-changeset.h
index b0ce450..6fe60f2 100644
--- a/common/dconf-changeset.h
+++ b/common/dconf-changeset.h
@@ -65,6 +65,9 @@ DConfChangeset *        dconf_changeset_deserialise                     (GVarian
 void                    dconf_changeset_change                          (DConfChangeset           *changeset,
                                                                          DConfChangeset           *changes);
 
+DConfChangeset *        dconf_changeset_filter_changes                  (DConfChangeset           *from,
+                                                                         DConfChangeset           *changes);
+
 DConfChangeset *        dconf_changeset_diff                            (DConfChangeset           *from,
                                                                          DConfChangeset           *to);
 
diff --git a/service/dconf-writer.c b/service/dconf-writer.c
index 26f66dd..4d054c8 100644
--- a/service/dconf-writer.c
+++ b/service/dconf-writer.c
@@ -130,21 +130,25 @@ dconf_writer_real_change (DConfWriter    *writer,
                           const gchar    *tag)
 {
   g_return_if_fail (writer->priv->uncommited_values != NULL);
+  DConfChangeset *effective_changeset = dconf_changeset_filter_changes (writer->priv->uncommited_values,
+                                                                        changeset);
 
-  dconf_changeset_change (writer->priv->uncommited_values, changeset);
-
-  if (tag)
+  if (effective_changeset)
     {
-      TaggedChange *change;
+      dconf_changeset_change (writer->priv->uncommited_values, effective_changeset);
+      if (tag)
+        {
+          TaggedChange *change;
 
-      change = g_slice_new (TaggedChange);
-      change->changeset = dconf_changeset_ref (changeset);
-      change->tag = g_strdup (tag);
+          change = g_slice_new (TaggedChange);
+          change->changeset = dconf_changeset_ref (effective_changeset);
+          change->tag = g_strdup (tag);
 
-      g_queue_push_tail (&writer->priv->uncommited_changes, change);
-    }
+          g_queue_push_tail (&writer->priv->uncommited_changes, change);
+        }
 
-  writer->priv->need_write = TRUE;
+      writer->priv->need_write = TRUE;
+    }
 }
 
 static gboolean
@@ -179,6 +183,8 @@ dconf_writer_real_commit (DConfWriter  *writer,
       close (invalidate_fd);
     }
 
+  writer->priv->need_write = FALSE;
+
   if (writer->priv->commited_values)
     dconf_changeset_unref (writer->priv->commited_values);
   writer->priv->commited_values = writer->priv->uncommited_values;
diff --git a/tests/changeset.c b/tests/changeset.c
index 5f046df..e3153e2 100644
--- a/tests/changeset.c
+++ b/tests/changeset.c
@@ -572,6 +572,145 @@ test_diff (void)
     }
 }
 
+static DConfChangeset *
+changeset_from_string (const gchar *string, gboolean is_database)
+{
+  GVariant *parsed;
+  DConfChangeset *changes, *parsed_changes;
+
+  if (is_database)
+    changes = dconf_changeset_new_database (NULL);
+  else
+    changes = dconf_changeset_new ();
+
+  if (string != NULL)
+    {
+      parsed = g_variant_parse (NULL, string, NULL, NULL, NULL);
+      parsed_changes = dconf_changeset_deserialise (parsed);
+      dconf_changeset_change (changes, parsed_changes);
+      dconf_changeset_unref (parsed_changes);
+      g_variant_unref (parsed);
+    }
+
+  return changes;
+}
+
+static gchar *
+string_from_changeset (DConfChangeset *changeset)
+{
+  GVariant *serialised;
+  gchar *string;
+
+  if (dconf_changeset_is_empty (changeset))
+    return NULL;
+
+  serialised = dconf_changeset_serialise (changeset);
+  string = g_variant_print (serialised, TRUE);
+  g_variant_unref (serialised);
+  return string;
+}
+
+static void
+call_filter_changes (const gchar *base_string,
+                     const gchar *changes_string,
+                     const gchar *expected)
+{
+  DConfChangeset *base, *changes, *filtered;
+  gchar *filtered_string = NULL;
+
+  base = changeset_from_string (base_string, TRUE);
+  changes = changeset_from_string (changes_string, FALSE);
+  filtered = dconf_changeset_filter_changes (base, changes);
+  if (filtered != NULL)
+    {
+      filtered_string = string_from_changeset (filtered);
+      dconf_changeset_unref (filtered);
+    }
+
+  g_assert_cmpstr (filtered_string, ==, expected);
+
+  dconf_changeset_unref (base);
+  dconf_changeset_unref (changes);
+  g_free (filtered_string);
+}
+
+static void
+test_filter_changes (void)
+{
+  /* These tests are mostly negative, since dconf_changeset_filter_changes
+   * is called from dconf_changeset_diff */
+
+  // Define test changesets as serialised g_variant strings
+  const gchar *empty = NULL;
+  const gchar *a1 = "{'/a': @mv <'value1'>}";
+  const gchar *a2 = "{'/a': @mv <'value2'>}";
+  const gchar *b2 = "{'/b': @mv <'value2'>}";
+  const gchar *a1b1 = "{'/a': @mv <'value1'>, '/b': @mv <'value1'>}";
+  const gchar *a1b2 = "{'/a': @mv <'value1'>, '/b': @mv <'value2'>}";
+  const gchar *a1r1 = "{'/a': @mv <'value1'>, '/r/c': @mv <'value3'>}";
+  const gchar *key_reset = "{'/a': @mv nothing}";
+  const gchar *root_reset = "{'/': @mv nothing}";
+  const gchar *partial_reset = "{'/r/': @mv nothing}";
+
+  /* an empty changeset would not change an empty database */
+  call_filter_changes (empty, empty, NULL);
+
+  /* an empty changeset would not change a database with values */
+  call_filter_changes (a1, empty, NULL);
+
+  /* a changeset would not change a database with the same values */
+  call_filter_changes (a1, a1, NULL);
+  call_filter_changes (a1b2, a1b2, NULL);
+
+  /* A non-empty changeset would change an empty database */
+  call_filter_changes (empty, a1, a1);
+
+  /* a changeset would change a database with the same keys but
+   * different values */
+  call_filter_changes (a1, a2, a2);
+  call_filter_changes (a1b1, a1b2, b2);
+
+  /* A changeset would change a database with disjoint values */
+  call_filter_changes (a1, b2, b2);
+
+  /* A changeset would change a database with some equal and some new
+   * values */
+  call_filter_changes (a1, a1b2, b2);
+
+  /* A changeset would not change a database with some equal and some
+   * new values */
+  call_filter_changes (a1b2, a1, NULL);
+
+  /* A root reset has an effect on a database with values */
+  call_filter_changes (a1, root_reset, root_reset);
+  call_filter_changes (a1b2, root_reset, root_reset);
+
+  /* A root reset would have no effect on an empty database */
+  call_filter_changes (empty, root_reset, NULL);
+
+  /* A key reset would have no effect on an empty database */
+  call_filter_changes (empty, key_reset, NULL);
+
+  /* A key reset would have no effect on a database with other keys */
+  call_filter_changes (b2, key_reset, NULL);
+
+  /* A key reset would have an effect on a database containing that
+   * key */
+  call_filter_changes (a1, key_reset, key_reset);
+  call_filter_changes (a1b1, key_reset, key_reset);
+
+  /* A partial reset would have no effect on an empty database */
+  call_filter_changes (empty, partial_reset, NULL);
+
+  /* A partial reset would have no effect on a database with other
+   * values */
+  call_filter_changes (a1, partial_reset, NULL);
+
+  /* A partial reset would have an effect on a database with some values
+   * under that path */
+  call_filter_changes (a1r1, partial_reset, partial_reset);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -584,6 +723,7 @@ main (int argc, char **argv)
   g_test_add_func ("/changeset/serialiser", test_serialiser);
   g_test_add_func ("/changeset/change", test_change);
   g_test_add_func ("/changeset/diff", test_diff);
+  g_test_add_func ("/changeset/filter", test_filter_changes);
 
   return g_test_run ();
 }
diff --git a/tests/test-dconf.py b/tests/test-dconf.py
index 6cd80a8..5e65884 100755
--- a/tests/test-dconf.py
+++ b/tests/test-dconf.py
@@ -516,7 +516,6 @@ class DBusTest(unittest.TestCase):
         # Lexicographically last value should win:
         self.assertEqual(dconf_read('/org/file'), '99')
 
-    @unittest.expectedFailure
     def test_redundant_disk_writes(self):
         """Redundant disk writes are avoided.
 
diff --git a/tests/writer.c b/tests/writer.c
index 955ba91..df33fc4 100644
--- a/tests/writer.c
+++ b/tests/writer.c
@@ -45,6 +45,29 @@ assert_n_warnings (guint expected_n_warnings)
   n_warnings = 0;
 }
 
+static guint64
+get_file_mtime_us (char *filename)
+{
+  GFile *file = g_file_new_for_path (filename);
+  GError *error = NULL;
+  GFileInfo *info = g_file_query_info (
+    file,
+    "time::*",
+    G_FILE_QUERY_INFO_NONE,
+    NULL,
+    &error);
+  if (!info)
+    {
+      printf ("failed with error %i: %s\n", error->code, error->message);
+      exit (1);
+    }
+
+  guint64 mtime_us =
+    g_file_info_get_attribute_uint64 (info, G_FILE_ATTRIBUTE_TIME_MODIFIED) * 1000000 +
+    g_file_info_get_attribute_uint32 (info, G_FILE_ATTRIBUTE_TIME_MODIFIED_USEC);
+  return mtime_us;
+}
+
 typedef struct
 {
   gchar *dconf_dir;  /* (owned) */
@@ -179,6 +202,182 @@ test_writer_begin_corrupt_file (Fixture       *fixture,
     }
 }
 
+/**
+ * Test that committing a write operation when no writes have been queued
+ * does not result in a database write.
+ */
+static void test_writer_commit_no_change (Fixture       *fixture,
+                                          gconstpointer  test_data)
+{
+  const char *db_name = "nonexistent";
+  g_autoptr(DConfWriter) writer = NULL;
+  DConfWriterClass *writer_class;
+  gboolean retval;
+  g_autoptr(GError) local_error = NULL;
+  g_autofree gchar *db_filename = g_build_filename (fixture->dconf_dir, db_name, NULL);
+
+  /* Create a writer. */
+  writer = DCONF_WRITER (dconf_writer_new (DCONF_TYPE_WRITER, db_name));
+  g_assert_nonnull (writer);
+  writer_class = DCONF_WRITER_GET_CLASS (writer);
+
+  /* Check the database doesn’t exist. */
+  g_assert_false (g_file_test (db_filename, G_FILE_TEST_EXISTS));
+
+  /* Begin transaction */
+  retval = writer_class->begin (writer, &local_error);
+  g_assert_no_error (local_error);
+  g_assert_true (retval);
+
+  /* Commit transaction */
+  retval = writer_class->commit (writer, &local_error);
+  g_assert_no_error (local_error);
+  g_assert_true (retval);
+
+  /* Check the database still doesn’t exist. */
+  g_assert_false (g_file_test (db_filename, G_FILE_TEST_EXISTS));
+
+  /* End transaction */
+  writer_class->end (writer);
+}
+
+/**
+ * Test that committing a write operation when writes that would not change
+ * the database have been queued does not result in a database write.
+ */
+static void test_writer_commit_empty_changes (Fixture       *fixture,
+                                              gconstpointer  test_data)
+{
+  const char *db_name = "nonexistent";
+  g_autoptr(DConfWriter) writer = NULL;
+  DConfWriterClass *writer_class;
+  gboolean retval;
+  g_autoptr(GError) local_error = NULL;
+  g_autofree gchar *db_filename = g_build_filename (fixture->dconf_dir, db_name, NULL);
+
+  /* Create a writer. */
+  writer = DCONF_WRITER (dconf_writer_new (DCONF_TYPE_WRITER, db_name));
+  g_assert_nonnull (writer);
+  writer_class = DCONF_WRITER_GET_CLASS (writer);
+
+  /* Check the database doesn’t exist. */
+  g_assert_false (g_file_test (db_filename, G_FILE_TEST_EXISTS));
+
+  /* Begin transaction */
+  retval = writer_class->begin (writer, &local_error);
+  g_assert_no_error (local_error);
+  g_assert_true (retval);
+
+  /* Make a redundant/empty change to the database */
+  DConfChangeset *changes = dconf_changeset_new();
+  writer_class->change (writer, changes, NULL);
+  g_assert_no_error (local_error);
+  g_assert_true (retval);
+
+  /* Commit transaction */
+  retval = writer_class->commit (writer, &local_error);
+  g_assert_no_error (local_error);
+  g_assert_true (retval);
+
+  /* Check the database still doesn't exist */
+  g_assert_false (g_file_test (db_filename, G_FILE_TEST_EXISTS));
+
+  /* End transaction */
+  writer_class->end (writer);
+}
+
+/**
+ * Test that committing a write operation when writes that would change
+ * the database have been queued does result in a database write.
+ */
+static void test_writer_commit_real_changes (Fixture       *fixture,
+                                             gconstpointer  test_data)
+{
+  const char *db_name = "nonexistent";
+  g_autoptr(DConfWriter) writer = NULL;
+  DConfWriterClass *writer_class;
+  DConfChangeset *changes;
+  gboolean retval;
+  g_autoptr(GError) local_error = NULL;
+  guint64 db_mtime_us;
+  g_autofree gchar *db_filename = g_build_filename (fixture->dconf_dir, db_name, NULL);
+
+  /* Create a writer. */
+  writer = DCONF_WRITER (dconf_writer_new (DCONF_TYPE_WRITER, db_name));
+  g_assert_nonnull (writer);
+  writer_class = DCONF_WRITER_GET_CLASS (writer);
+
+  /* Check the database doesn’t exist. */
+  g_assert_false (g_file_test (db_filename, G_FILE_TEST_EXISTS));
+
+  /* Begin transaction */
+  retval = writer_class->begin (writer, &local_error);
+  g_assert_no_error (local_error);
+  g_assert_true (retval);
+
+  /* Make a real change to the database */
+  changes = dconf_changeset_new();
+  dconf_changeset_set(changes, "/key", g_variant_new ("(s)", "value"));
+  writer_class->change (writer, changes, NULL);
+  g_assert_no_error (local_error);
+  g_assert_true (retval);
+
+  /* Commit transaction */
+  retval = writer_class->commit (writer, &local_error);
+  g_assert_no_error (local_error);
+  g_assert_true (retval);
+
+  /* Check the database now exists */
+  g_assert_true (g_file_test (db_filename, G_FILE_TEST_EXISTS));
+  db_mtime_us = get_file_mtime_us (db_filename);
+
+  /* End transaction */
+  writer_class->end (writer);
+
+  /* Begin a second transaction */
+  retval = writer_class->begin (writer, &local_error);
+  g_assert_no_error (local_error);
+  g_assert_true (retval);
+
+  /* Make a redundant/empty change to the database */
+  changes = dconf_changeset_new();
+  writer_class->change (writer, changes, NULL);
+  g_assert_no_error (local_error);
+  g_assert_true (retval);
+
+  /* Commit transaction */
+  retval = writer_class->commit (writer, &local_error);
+  g_assert_no_error (local_error);
+  g_assert_true (retval);
+
+  /* End transaction */
+  writer_class->end (writer);
+
+  /* Check that no extra write was done (even afer committing a real change) */
+  g_assert_cmpuint (db_mtime_us, ==, get_file_mtime_us (db_filename));
+  db_mtime_us = get_file_mtime_us (db_filename);
+
+  /* Begin a third transaction */
+  retval = writer_class->begin (writer, &local_error);
+  g_assert_no_error (local_error);
+  g_assert_true (retval);
+
+  /* Commit transaction (with no changes at all) */
+  retval = writer_class->commit (writer, &local_error);
+  g_assert_no_error (local_error);
+  g_assert_true (retval);
+
+  /* Check that no extra write was done (even afer committing a real change) */
+  g_assert_cmpuint (db_mtime_us, ==, get_file_mtime_us (db_filename));
+  db_mtime_us = get_file_mtime_us (db_filename);
+
+  /* End transaction */
+  writer_class->end (writer);
+
+  /* Clean up. */
+  g_assert_cmpint (g_unlink (db_filename), ==, 0);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -221,6 +420,12 @@ main (int argc, char **argv)
               test_writer_begin_corrupt_file, tear_down);
   g_test_add ("/writer/begin/corrupt-file/2", Fixture, &corrupt_file_data2, set_up,
               test_writer_begin_corrupt_file, tear_down);
+  g_test_add ("/writer/commit/redundant_change/0", Fixture, NULL, set_up,
+              test_writer_commit_no_change, tear_down);
+  g_test_add ("/writer/commit/redundant_change/1", Fixture, NULL, set_up,
+              test_writer_commit_empty_changes, tear_down);
+  g_test_add ("/writer/commit/redundant_change/2", Fixture, NULL, set_up,
+              test_writer_commit_real_changes, tear_down);
 
   retval = g_test_run ();