OstreeMutableTree: Add parent pointer and fix bug

src/libostree/ostree-mutable-tree.c

@@ -47,10 +47,24 @@ struct OstreeMutableTree
 {
   GObject parent_instance;
 
+  /* The parent directory to this one.  We don't hold a ref because this mtree
+   * is owned by the parent.  We can be certain that any mtree only has one
+   * parent because external users can't set this, it's only set when we create
+   * a child from within this file (see insert_child_mtree). We ensure that the
+   * parent pointer is either valid or NULL because when the parent is destroyed
+   * it sets parent = NULL on all its children (see remove_child_mtree) */
+  OstreeMutableTree *parent;
+
   /* This is the checksum of the Dirtree object that corresponds to the current
    * contents of this directory.  contents_checksum can be NULL if the SHA was
-   * never calculated or contents of the mtree has been modified.  Even if
-   * contents_checksum is not NULL it may be out of date. */
+   * never calculated or contents of this mtree or any subdirectory has been
+   * modified.  If a contents_checksum is NULL then all the parent's checksums
+   * will be NULL (see `invalidate_contents_checksum`).
+   *
+   * Note: This invariant is partially maintained externally - we
+   * rely on the callers of `ostree_mutable_tree_set_contents_checksum` to have
+   * first ensured that the mtree contents really does correspond to this
+   * checksum */
   char *contents_checksum;
 
   /* This is the checksum of the DirMeta object that holds the uid, gid, mode
@@ -66,6 +80,8 @@ struct OstreeMutableTree
 
 G_DEFINE_TYPE (OstreeMutableTree, ostree_mutable_tree, G_TYPE_OBJECT)
 
+static void invalidate_contents_checksum (OstreeMutableTree *self);
+
 static void
 ostree_mutable_tree_finalize (GObject *object)
 {
@@ -90,19 +106,61 @@ ostree_mutable_tree_class_init (OstreeMutableTreeClass *klass)
   gobject_class->finalize = ostree_mutable_tree_finalize;
 }
 
+/* This must not be made public or we can't maintain the invariant that any
+ * OstreeMutableTree has only one parent.
+ *
+ * Ownership of @child is transferred from the caller to @self */
+static void
+insert_child_mtree (OstreeMutableTree *self, const gchar* name,
+                    OstreeMutableTree *child)
+{
+  g_assert_null (child->parent);
+  g_hash_table_insert (self->subdirs, g_strdup (name), child);
+  child->parent = self;
+  invalidate_contents_checksum (self);
+}
+
+static void
+remove_child_mtree (gpointer data)
+{
+  /* Each mtree has shared ownership of its children and each child has a
+   * non-owning reference back to parent.  If the parent goes out of scope the
+   * children may still be alive because they're reference counted. This
+   * removes the reference to the parent before it goes stale. */
+  OstreeMutableTree *child = (OstreeMutableTree*) data;
+  child->parent = NULL;
+  g_object_unref (child);
+}
+
 static void
 ostree_mutable_tree_init (OstreeMutableTree *self)
 {
   self->files = g_hash_table_new_full (g_str_hash, g_str_equal,
                                        g_free, g_free);
   self->subdirs = g_hash_table_new_full (g_str_hash, g_str_equal,
-                                         g_free, (GDestroyNotify)g_object_unref);
+                                         g_free, remove_child_mtree);
+}
+
+static void
+invalidate_contents_checksum (OstreeMutableTree *self)
+{
+  while (self) {
+    if (!self->contents_checksum)
+      break;
+
+    g_clear_pointer (&self->contents_checksum, g_free);
+    self = self->parent;
+  }
 }
 
 void
 ostree_mutable_tree_set_metadata_checksum (OstreeMutableTree *self,
                                            const char        *checksum)
 {
+  if (g_strcmp0 (checksum, self->metadata_checksum) == 0)
+    return;
+
+  invalidate_contents_checksum (self->parent);
   g_free (self->metadata_checksum);
   self->metadata_checksum = g_strdup (checksum);
 }
@@ -117,33 +175,21 @@ void
 ostree_mutable_tree_set_contents_checksum (OstreeMutableTree *self,
                                            const char        *checksum)
 {
+  if (g_strcmp0 (checksum, self->contents_checksum) == 0)
+    return;
+
+  if (checksum && self->contents_checksum)
+    g_warning ("Setting a contents checksum on an OstreeMutableTree that "
+        "already has a checksum set.  Old checksum %s, new checksum %s",
+        self->contents_checksum, checksum);
+
   g_free (self->contents_checksum);
   self->contents_checksum = g_strdup (checksum);
 }
 
 const char *
 ostree_mutable_tree_get_contents_checksum (OstreeMutableTree *self)
 {
-  if (!self->contents_checksum)
-    return NULL;
-
-  /* Ensure the cache is valid; this implementation is a bit
-   * lame in that we walk the whole tree every time this
-   * getter is called; a better approach would be to invalidate
-   * all of the parents whenever a child is modified.
-   *
-   * However, we only call this function once right now.
-   */
-  GLNX_HASH_TABLE_FOREACH_V (self->subdirs, OstreeMutableTree*, subdir)
-    {
-      if (!ostree_mutable_tree_get_contents_checksum (subdir))
-        {
-          g_free (self->contents_checksum);
-          self->contents_checksum = NULL;
-          return NULL;
-        }
-    }
-
   return self->contents_checksum;
 }
 
@@ -176,7 +222,7 @@ ostree_mutable_tree_replace_file (OstreeMutableTree *self,
       goto out;
     }
 
-  ostree_mutable_tree_set_contents_checksum (self, NULL);
+  invalidate_contents_checksum (self);
   g_hash_table_replace (self->files,
                         g_strdup (name),
                         g_strdup (checksum));
@@ -221,8 +267,7 @@ ostree_mutable_tree_ensure_dir (OstreeMutableTree *self,
   if (!ret_dir)
     {
       ret_dir = ostree_mutable_tree_new ();
-      ostree_mutable_tree_set_contents_checksum (self, NULL);
-      g_hash_table_insert (self->subdirs, g_strdup (name), g_object_ref (ret_dir));
+      insert_child_mtree (self, name, g_object_ref (ret_dir));
     }
 
   ret = TRUE;
@@ -305,7 +350,7 @@ ostree_mutable_tree_ensure_parent_dirs (OstreeMutableTree  *self,
         {
           next = ostree_mutable_tree_new ();
           ostree_mutable_tree_set_metadata_checksum (next, metadata_checksum);
-          g_hash_table_insert (subdir->subdirs, g_strdup (name), next);
+          insert_child_mtree (subdir, g_strdup (name), next);
         }
 
       subdir = next;

tests/test-mutable-tree.c

@@ -31,6 +31,7 @@
 static void
 test_metadata_checksum (void)
 {
+  g_autoptr(GError) error = NULL;
   const char *checksum = "12345678901234567890123456789012";
   glnx_unref_object OstreeMutableTree *tree = ostree_mutable_tree_new ();
 
@@ -39,6 +40,27 @@ test_metadata_checksum (void)
   ostree_mutable_tree_set_metadata_checksum (tree, checksum);
 
   g_assert_cmpstr (checksum, ==, ostree_mutable_tree_get_metadata_checksum (tree));
+
+  /* If a child tree's metadata changes the parent tree's contents needs to be
+   * recalculated */
+  glnx_unref_object OstreeMutableTree *subdir = NULL;
+  g_assert (ostree_mutable_tree_ensure_dir (tree, "subdir", &subdir, &error));
+  g_assert_nonnull (subdir);
+
+  ostree_mutable_tree_set_contents_checksum (
+      subdir, "11111111111111111111111111111111");
+  ostree_mutable_tree_set_metadata_checksum (
+      subdir, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
+  ostree_mutable_tree_set_contents_checksum (
+      tree, "abcdefabcdefabcdefabcdefabcdefab");
+
+  g_assert_cmpstr (ostree_mutable_tree_get_contents_checksum (tree), ==,
+      "abcdefabcdefabcdefabcdefabcdefab");
+  ostree_mutable_tree_set_metadata_checksum (
+      subdir, "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb");
+  g_assert_null (ostree_mutable_tree_get_contents_checksum (tree));
+  g_assert_cmpstr (ostree_mutable_tree_get_contents_checksum (subdir), ==,
+      "11111111111111111111111111111111");
 }
 
 static void
@@ -162,13 +184,12 @@ test_contents_checksum (void)
   const char *subdir_checksum = "ABCD0123456789012345678901234567";
   glnx_unref_object OstreeMutableTree *tree = ostree_mutable_tree_new ();
   glnx_unref_object OstreeMutableTree *subdir = NULL;
-  g_autoptr(GError) error = NULL;
   g_assert_null (ostree_mutable_tree_get_contents_checksum (tree));
 
   ostree_mutable_tree_set_contents_checksum (tree, checksum);
   g_assert_cmpstr (checksum, ==, ostree_mutable_tree_get_contents_checksum (tree));
 
-  g_assert (ostree_mutable_tree_ensure_dir (tree, "subdir", &subdir, &error));
+  g_assert (ostree_mutable_tree_ensure_dir (tree, "subdir", &subdir, NULL));
   g_assert_nonnull (subdir);
 
   ostree_mutable_tree_set_contents_checksum (subdir, subdir_checksum);