[dconf/patch/service-check: 3/6] Changeset: make dconf_changeset_filter_changes filter out key/path resets when appropriate

[Daniel Playfair Cal <danielplayfaircal src gnome org>]

commit e9c578eae70e706da72feff3d795371cb29bfe53
Author: Daniel Playfair Cal <daniel playfair cal gmail com>
Date:   Mon Aug 13 14:25:47 2018 +1000

    Changeset: make dconf_changeset_filter_changes filter out key/path resets when appropriate

 common/dconf-changeset.c | 38 +++++++++++++++++++++++---
 tests/changeset.c        | 69 +++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 90 insertions(+), 17 deletions(-)
---
diff --git a/common/dconf-changeset.c b/common/dconf-changeset.c
index 952118a..49293ca 100644
--- a/common/dconf-changeset.c
+++ b/common/dconf-changeset.c
@@ -793,7 +793,7 @@ dconf_changeset_filter_changes (DConfChangeset *base,
                                 DConfChangeset *changes)
 {
   DConfChangeset *result = NULL;
-  GHashTableIter iter;
+  GHashTableIter iter_changes;
   gpointer key, val;
 
   g_return_val_if_fail (base->is_database, NULL);
@@ -805,13 +805,43 @@ dconf_changeset_filter_changes (DConfChangeset *base,
    * 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->table);
-  while (g_hash_table_iter_next (&iter, &key, &val))
+  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 (base_val == NULL || !g_variant_equal (val, base_val))
+      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 ();
 
diff --git a/tests/changeset.c b/tests/changeset.c
index d22dcb8..d75e8d0 100644
--- a/tests/changeset.c
+++ b/tests/changeset.c
@@ -639,12 +639,14 @@ test_filter_changes (void)
   // Define test changesets as serialised g_variant strings
   const gchar *empty = NULL;
   const gchar *a1 = "{'/a': @mv <'value1'>}";
-  const gchar *a_null = "{'/a': @mv nothing}";
   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 *reset = "{'/': @mv nothing}";
+  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}";
   gchar *filtered;
 
   /* an empty changeset would not change an empty database */
@@ -662,7 +664,8 @@ test_filter_changes (void)
   g_assert_cmpstr (filtered, ==, a1);
   g_free (filtered);
 
-  /* a changeset would change a database with the same keys but different values */
+  /* a changeset would change a database with the same keys but
+   * different values */
   filtered = call_filter_changes (a1, a2);
   g_assert_cmpstr (filtered, ==, a2);
   g_free (filtered);
@@ -675,25 +678,65 @@ test_filter_changes (void)
   g_assert_cmpstr (filtered, ==, b2);
   g_free (filtered);
 
-  /* A changeset would change a database with some equal and some new values */
+  /* A changeset would change a database with some equal and some new
+   * values */
   filtered = call_filter_changes (a1, a1b2);
   g_assert_cmpstr (filtered, ==, b2);
   g_free (filtered);
 
-  /* A changeset would not change a database with some equal and some new values */
+  /* A changeset would not change a database with some equal and some
+   * new values */
   g_assert_null (call_filter_changes (a1b2, a1));
 
-  /* Resets should count when there are values to be reset */
-  filtered = call_filter_changes (a_null, reset);
-  g_assert_cmpstr (filtered, ==, reset);
+  /* A root reset has an effect on a database with values */
+  filtered = call_filter_changes (a1, root_reset);
+  g_assert_cmpstr (filtered, ==, root_reset);
   g_free (filtered);
-  filtered = call_filter_changes (a1b2, reset);
-  g_assert_cmpstr (filtered, ==, reset);
+
+  filtered = call_filter_changes (a1b2, root_reset);
+  g_assert_cmpstr (filtered, ==, root_reset);
+  g_free (filtered);
+
+  /* A root reset would have no effect on an empty database */
+  filtered = call_filter_changes (empty, root_reset);
+  g_assert_cmpstr (filtered, ==, NULL);
+  g_free (filtered);
+
+  /* A key reset would have no effect on an empty database */
+  filtered = call_filter_changes (empty, key_reset);
+  g_assert_cmpstr (filtered, ==, NULL);
+  g_free (filtered);
+
+  /* A key reset would have no effect on a database with other keys */
+  filtered = call_filter_changes (b2, key_reset);
+  g_assert_cmpstr (filtered, ==, NULL);
+  g_free (filtered);
+
+  /* A key reset would have an effect on a database containing that
+   * key */
+  filtered = call_filter_changes (a1, key_reset);
+  g_assert_cmpstr (filtered, ==, key_reset);
+  g_free (filtered);
+
+  filtered = call_filter_changes (a1b1, key_reset);
+  g_assert_cmpstr (filtered, ==, key_reset);
+  g_free (filtered);
+
+  /* A partial reset would have no effect on an empty database */
+  filtered = call_filter_changes (empty, partial_reset);
+  g_assert_cmpstr (filtered, ==, NULL);
+  g_free (filtered);
+
+  /* A partial reset would have no effect on a database with other
+   * values */
+  filtered = call_filter_changes (a1, partial_reset);
+  g_assert_cmpstr (filtered, ==, NULL);
   g_free (filtered);
 
-  /* FIXME: ideally, a reset would have no effect on an empty database */
-  filtered = call_filter_changes (empty, reset);
-  g_assert_cmpstr (filtered, ==, reset);
+  /* A partial reset would have an effect on a database with some values
+   * under that path */
+  filtered = call_filter_changes (a1r1, partial_reset);
+  g_assert_cmpstr (filtered, ==, partial_reset);
   g_free (filtered);
 }