lib/commit: don't query devino cache for modified files

We can't use the cache if the file we want to commit has been modified
by the client through the file info or xattr modifiers. We would
prematurely look into the cache in `write_dfd_iter_to_mtree_internal`,
regardless of whether any filtering applied.

We remove that path there, and make sure that we only use the cache if
there were no modifications. We rename the `get_modified_xattrs` to
`get_final_xattrs` to reflect the fact that the xattrs may not be
modified.

One tricky bit that took me some time was that we now need to store the
st_dev & st_ino values in the GFileInfo because the cache lookup relies
on it. I'm guessing we regressed on this at some point.

This patch does slightly change the semantics of the xattr callback.
Previously, returning NULL from the cb meant no xattrs at all. Now, it
means to default to the on-disk state. We might want to consider putting
that behind a flag instead. Though it seems like a more useful behaviour
so that callers can only override the files they want to without losing
original on-disk state (and if they don't want that, just return an
empty GVariant).

Closes: #1165

Closes: #1170
Approved by: cgwalters

tests/basic-test.sh

@@ -19,7 +19,7 @@
 
 set -euo pipefail
 
-echo "1..$((72 + ${extra_basic_tests:-0}))"
+echo "1..$((73 + ${extra_basic_tests:-0}))"
 
 CHECKOUT_U_ARG=""
 CHECKOUT_H_ARGS="-H"
@@ -602,6 +602,24 @@ $OSTREE checkout test2 test2-checkout
 (cd test2-checkout && $OSTREE commit ${COMMIT_ARGS} --link-checkout-speedup -b test2 -s "tmp")
 echo "ok commit with link speedup"
 
+cd ${test_tmpdir}
+rm -rf test2-checkout
+$OSTREE checkout test2 test2-checkout
+# set cow to different perms, but re-set cowro to the same perms
+cat > statoverride.txt <<EOF
+=$((0600)) /baz/cow
+=$((0600)) /baz/cowro
+EOF
+$OSTREE commit ${COMMIT_ARGS} --statoverride=statoverride.txt \
+  --table-output --link-checkout-speedup -b test2-tmp test2-checkout > stats.txt
+$OSTREE diff test2 test2-tmp > diff-test2
+assert_file_has_content diff-test2 'M */baz/cow$'
+assert_not_file_has_content diff-test2 'M */baz/cowro$'
+assert_not_file_has_content diff-test2 'baz/saucer'
+# only /baz/cow is a cache miss
+assert_file_has_content stats.txt '^Content Written: 1$'
+echo "ok commit with link speedup and modifier"
+
 cd ${test_tmpdir}
 $OSTREE ls test2
 echo "ok ls with no argument"

tests/test-basic-c.c

@@ -236,6 +236,125 @@ test_object_writes (gconstpointer data)
   }
 }
 
+static GVariant*
+xattr_cb (OstreeRepo  *repo,
+          const char  *path,
+          GFileInfo   *file_info,
+          gpointer     user_data)
+{
+  GVariant *xattr = user_data;
+  if (g_str_equal (path, "/baz/cow"))
+    return g_variant_ref (xattr);
+  return NULL;
+}
+
+/* check that using a devino cache doesn't cause us to ignore xattr callbacks */
+static void
+test_devino_cache_xattrs (void)
+{
+  g_autoptr(GError) error = NULL;
+  gboolean ret = FALSE;
+
+  g_autoptr(GFile) repo_path = g_file_new_for_path ("repo");
+
+  /* re-initialize as bare */
+  ret = ot_test_run_libtest ("setup_test_repository bare", &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  gboolean on_overlay;
+  ret = ot_check_for_overlay (&on_overlay, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  if (on_overlay)
+    {
+      g_test_skip ("overlayfs detected");
+      return;
+    }
+
+  g_autoptr(OstreeRepo) repo = ostree_repo_new (repo_path);
+  ret = ostree_repo_open (repo, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  g_autofree char *csum = NULL;
+  ret = ostree_repo_resolve_rev (repo, "test2", FALSE, &csum, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  g_autoptr(OstreeRepoDevInoCache) cache = ostree_repo_devino_cache_new ();
+
+  OstreeRepoCheckoutAtOptions options = {0,};
+  options.no_copy_fallback = TRUE;
+  options.devino_to_csum_cache = cache;
+  ret = ostree_repo_checkout_at (repo, &options, AT_FDCWD, "checkout", csum, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  g_autoptr(OstreeMutableTree) mtree = ostree_mutable_tree_new ();
+  g_autoptr(OstreeRepoCommitModifier) modifier =
+    ostree_repo_commit_modifier_new (0, NULL, NULL, NULL);
+  ostree_repo_commit_modifier_set_devino_cache (modifier, cache);
+
+  g_auto(GVariantBuilder) builder;
+  g_variant_builder_init (&builder, (GVariantType*)"a(ayay)");
+  g_variant_builder_add (&builder, "(@ay@ay)",
+                         g_variant_new_bytestring ("user.myattr"),
+                         g_variant_new_bytestring ("data"));
+  g_autoptr(GVariant) orig_xattrs = g_variant_ref_sink (g_variant_builder_end (&builder));
+
+  ret = ostree_repo_prepare_transaction (repo, NULL, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  ostree_repo_commit_modifier_set_xattr_callback (modifier, xattr_cb, NULL, orig_xattrs);
+  ret = ostree_repo_write_dfd_to_mtree (repo, AT_FDCWD, "checkout",
+                                        mtree, modifier, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  g_autoptr(GFile) root = NULL;
+  ret = ostree_repo_write_mtree (repo, mtree, &root, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  /* now check that the final xattr matches */
+  g_autoptr(GFile) baz_child = g_file_get_child (root, "baz");
+  g_autoptr(GFile) cow_child = g_file_get_child (baz_child, "cow");
+
+  g_autoptr(GVariant) xattrs = NULL;
+  ret = ostree_repo_file_get_xattrs (OSTREE_REPO_FILE (cow_child), &xattrs, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  gboolean found_xattr = FALSE;
+  gsize n = g_variant_n_children (xattrs);
+  for (gsize i = 0; i < n; i++)
+    {
+      const guint8* name;
+      const guint8* value;
+      g_variant_get_child (xattrs, i, "(^&ay^&ay)", &name, &value);
+
+      if (g_str_equal ((const char*)name, "user.myattr"))
+        {
+          g_assert_cmpstr ((const char*)value, ==, "data");
+          found_xattr = TRUE;
+          break;
+        }
+    }
+
+  g_assert (found_xattr);
+
+  OstreeRepoTransactionStats stats;
+  ret = ostree_repo_commit_transaction (repo, &stats, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  /* we should only have had to checksum /baz/cow */
+  g_assert_cmpint (stats.content_objects_written, ==, 1);
+}
+
 int main (int argc, char **argv)
 {
   g_autoptr(GError) error = NULL;
@@ -243,13 +362,14 @@ int main (int argc, char **argv)
 
   g_test_init (&argc, &argv, NULL);
 
-  repo = ot_test_setup_repo (NULL, &error); 
+  repo = ot_test_setup_repo (NULL, &error);
   if (!repo)
     goto out;
 
   g_test_add_data_func ("/repo-not-system", repo, test_repo_is_not_system);
   g_test_add_data_func ("/raw-file-to-archive-stream", repo, test_raw_file_to_archive_stream);
   g_test_add_data_func ("/objectwrites", repo, test_object_writes);
+  g_test_add_func ("/xattrs-devino-cache", test_devino_cache_xattrs);
   g_test_add_func ("/remotename", test_validate_remotename);
 
   return g_test_run();