Merge pull request #2267 from dbnicholson/pull-depth-fixes

Pull depth fixes

src/libostree/ostree-repo-pull-private.h

@@ -88,7 +88,7 @@ typedef struct {
   GHashTable       *ref_keyring_map; /* Maps OstreeCollectionRef to keyring remote name */
   GPtrArray        *static_delta_superblocks;
   GHashTable       *expected_commit_sizes; /* Maps commit checksum to known size */
-  GHashTable       *commit_to_depth; /* Maps commit checksum maximum depth */
+  GHashTable       *commit_to_depth; /* Maps parent commit checksum maximum depth */
   GHashTable       *scanned_metadata; /* Maps object name to itself */
   GHashTable       *fetched_detached_metadata; /* Map<checksum,GVariant> */
   GHashTable       *requested_metadata; /* Maps object name to itself */

src/libostree/ostree-repo-pull.c

@@ -1113,6 +1113,18 @@ on_metadata_written (GObject           *object,
   check_outstanding_requests_handle_error (pull_data, &local_error);
 }
 
+static gboolean
+is_parent_commit (OtPullData *pull_data,
+                  const char *checksum)
+{
+  /* FIXME: Only parent commits are added to the commit_to_depth table,
+   * so if the checksum isn't in the table then a new commit chain is
+   * being started. However, if the desired commit was a parent in a
+   * previously followed chain, then this will be wrong.
+   */
+  return g_hash_table_contains (pull_data->commit_to_depth, checksum);
+}
+
 static void
 meta_fetch_on_complete (GObject           *object,
                         GAsyncResult      *result,
@@ -1155,10 +1167,13 @@ meta_fetch_on_complete (GObject           *object,
             }
 
           /* When traversing parents, do not fail on a missing commit.
-           * We may be pulling from a partial repository that ends in
+           * We may be pulling from a partial repository that ends in a
-           * a dangling parent reference. */
+           * dangling parent reference. This logic should match the
+           * local case in scan_one_metadata_object.
+           */
           else if (objtype == OSTREE_OBJECT_TYPE_COMMIT &&
-                   pull_data->maxdepth != 0)
+                   pull_data->maxdepth != 0 &&
+                   is_parent_commit (pull_data, checksum))
             {
               g_clear_error (&local_error);
               /* If the remote repo supports tombstone commits, check if the commit was intentionally
@@ -1542,8 +1557,6 @@ scan_commit_object (OtPullData                 *pull_data,
   else
     {
       depth = pull_data->maxdepth;
-      g_hash_table_insert (pull_data->commit_to_depth, g_strdup (checksum),
-                           GINT_TO_POINTER (depth));
     }
 
 #ifndef OSTREE_DISABLE_GPGME
@@ -1684,40 +1697,19 @@ scan_commit_object (OtPullData                 *pull_data,
         return FALSE;
     }
 
-  if (parent_csum_bytes != NULL && pull_data->maxdepth == -1)
+  if (parent_csum_bytes != NULL && (pull_data->maxdepth == -1 || depth > 0))
-    {
-      queue_scan_one_metadata_object_c (pull_data, parent_csum_bytes,
-                                        OSTREE_OBJECT_TYPE_COMMIT, NULL,
-                                        recursion_depth + 1, NULL);
-    }
-  else if (parent_csum_bytes != NULL && depth > 0)
     {
       char parent_checksum[OSTREE_SHA256_STRING_LEN+1];
-      gpointer parent_depthp;
-      int parent_depth;
-
       ostree_checksum_inplace_from_bytes (parent_csum_bytes, parent_checksum);
 
-      if (g_hash_table_lookup_extended (pull_data->commit_to_depth, parent_checksum,
+      int parent_depth = (depth > 0) ? depth - 1 : -1;
-                                        NULL, &parent_depthp))
+      g_hash_table_insert (pull_data->commit_to_depth, g_strdup (parent_checksum),
-        {
+                           GINT_TO_POINTER (parent_depth));
-          parent_depth = GPOINTER_TO_INT (parent_depthp);
+      queue_scan_one_metadata_object_c (pull_data, parent_csum_bytes,
-        }
+                                        OSTREE_OBJECT_TYPE_COMMIT,
-      else
+                                        NULL,
-        {
+                                        recursion_depth + 1,
-          parent_depth = depth - 1;
+                                        NULL);
-        }
-
-      if (parent_depth >= 0)
-        {
-          g_hash_table_insert (pull_data->commit_to_depth, g_strdup (parent_checksum),
-                               GINT_TO_POINTER (parent_depth));
-          queue_scan_one_metadata_object_c (pull_data, parent_csum_bytes,
-                                            OSTREE_OBJECT_TYPE_COMMIT,
-                                            NULL,
-                                            recursion_depth + 1,
-                                            NULL);
-        }
     }
 
   /* We only recurse to looking whether we need dirtree/dirmeta
@@ -1830,10 +1822,46 @@ scan_one_metadata_object (OtPullData                 *pull_data,
             return FALSE;
         }
 
+      g_autoptr(GError) local_error = NULL;
       if (!_ostree_repo_import_object (pull_data->repo, pull_data->remote_repo_local,
                                        objtype, checksum, pull_data->importflags,
-                                       cancellable, error))
+                                       cancellable, &local_error))
-        return FALSE;
+        {
+          /* When traversing parents, do not fail on a missing commit.
+           * We may be pulling from a partial repository that ends in a
+           * dangling parent reference. This logic should match the
+           * remote case in meta_fetch_on_complete.
+           *
+           * Note early return.
+           */
+          if (g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND) &&
+              objtype == OSTREE_OBJECT_TYPE_COMMIT &&
+              pull_data->maxdepth != 0 &&
+              is_parent_commit (pull_data, checksum))
+            {
+              g_clear_error (&local_error);
+
+              /* If the remote repo supports tombstone commits, check if
+               * the commit was intentionally deleted.
+               */
+              if (pull_data->has_tombstone_commits)
+                {
+                  if (!_ostree_repo_import_object (pull_data->repo, pull_data->remote_repo_local,
+                                                   OSTREE_OBJECT_TYPE_TOMBSTONE_COMMIT,
+                                                   checksum, pull_data->importflags,
+                                                   cancellable, error))
+                    return FALSE;
+                }
+
+              return TRUE;
+            }
+          else
+            {
+              g_propagate_error (error, g_steal_pointer (&local_error));
+              return FALSE;
+            }
+        }
+
       /* The import API will fetch both the commit and detached metadata, so
        * add it to the hash to avoid re-fetching it below.
        */

tests/test-local-pull-depth.sh

@@ -25,7 +25,7 @@ set -euo pipefail
 
 setup_test_repository "archive"
 
-echo "1..1"
+echo "1..3"
 
 cd ${test_tmpdir}
 mkdir repo2
@@ -62,3 +62,35 @@ find repo2/state -name '*.commitpartial' | wc -l > commitpartialcount
 assert_file_has_content commitpartialcount "^0$"
 
 echo "ok local pull depth"
+
+# Check that pulling with depth != 0 succeeds with a missing parent
+# commit. Prune the remote to truncate the history.
+cd ${test_tmpdir}
+${CMD_PREFIX} ostree --repo=repo prune --refs-only --depth=0
+
+rm -rf repo2/refs/heads/* repo2/refs/remotes/* repo2/objects/*/*.commit
+${CMD_PREFIX} ostree --repo=repo2 pull-local --depth=1 repo
+find repo/objects -name '*.commit' | wc -l > commitcount
+assert_file_has_content commitcount "^1$"
+find repo/state -name '*.commitpartial' | wc -l > commitpartialcount
+assert_file_has_content commitpartialcount "^0$"
+
+rm -rf repo2/refs/heads/* repo2/refs/remotes/* repo2/objects/*/*.commit
+${CMD_PREFIX} ostree --repo=repo2 pull-local --depth=-1 repo
+find repo/objects -name '*.commit' | wc -l > commitcount
+assert_file_has_content commitcount "^1$"
+find repo/state -name '*.commitpartial' | wc -l > commitpartialcount
+assert_file_has_content commitpartialcount "^0$"
+
+echo "ok local pull depth missing parent"
+
+# Check that it errors if the ref head commit is missing.
+cd ${test_tmpdir}
+rm -f repo/objects/*/*.commit
+
+rm -rf repo2/refs/heads/* repo2/refs/remotes/* repo2/objects/*/*.commit
+if ${CMD_PREFIX} ostree --repo=repo2 pull-local --depth=-1 repo; then
+    fatal "Local pull with depth -1 succeeded with missing HEAD commit"
+fi
+
+echo "ok local pull depth missing HEAD commit"

tests/test-pull-depth.sh

@@ -25,7 +25,7 @@ set -euo pipefail
 
 setup_fake_remote_repo1 "archive"
 
-echo '1..1'
+echo '1..3'
 
 cd ${test_tmpdir}
 mkdir repo
@@ -35,21 +35,63 @@ ${CMD_PREFIX} ostree --repo=repo remote add --set=gpg-verify=false origin $(cat
 ${CMD_PREFIX} ostree --repo=repo pull --depth=0 origin main
 find repo/objects -name '*.commit' | wc -l > commitcount
 assert_file_has_content commitcount "^1$"
+find repo/state -name '*.commitpartial' | wc -l > commitpartialcount
+assert_file_has_content commitpartialcount "^0$"
 
 ${CMD_PREFIX} ostree --repo=repo pull --depth=0 origin main
 find repo/objects -name '*.commit' | wc -l > commitcount
 assert_file_has_content commitcount "^1$"
+find repo/state -name '*.commitpartial' | wc -l > commitpartialcount
+assert_file_has_content commitpartialcount "^0$"
 
 ${CMD_PREFIX} ostree --repo=repo pull --depth=1 origin main
 find repo/objects -name '*.commit' | wc -l > commitcount
 assert_file_has_content commitcount "^2$"
+find repo/state -name '*.commitpartial' | wc -l > commitpartialcount
+assert_file_has_content commitpartialcount "^0$"
 
 ${CMD_PREFIX} ostree --repo=repo pull --depth=1 origin main
 find repo/objects -name '*.commit' | wc -l > commitcount
 assert_file_has_content commitcount "^2$"
+find repo/state -name '*.commitpartial' | wc -l > commitpartialcount
+assert_file_has_content commitpartialcount "^0$"
 
 ${CMD_PREFIX} ostree --repo=repo pull --depth=-1 origin main
 find repo/objects -name '*.commit' | wc -l > commitcount
 assert_file_has_content commitcount "^3$"
+find repo/state -name '*.commitpartial' | wc -l > commitpartialcount
+assert_file_has_content commitpartialcount "^0$"
 
 echo "ok pull depth"
+
+# Check that pulling with depth != 0 succeeds with a missing parent
+# commit. Prune the remote to truncate the history.
+cd ${test_tmpdir}
+${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo prune --refs-only --depth=0
+
+rm -rf repo/refs/heads/* repo/refs/remotes/* repo/objects/*/*.commit
+${CMD_PREFIX} ostree --repo=repo pull --depth=1 origin main
+find repo/objects -name '*.commit' | wc -l > commitcount
+assert_file_has_content commitcount "^1$"
+find repo/state -name '*.commitpartial' | wc -l > commitpartialcount
+assert_file_has_content commitpartialcount "^0$"
+
+rm -rf repo/refs/heads/* repo/refs/remotes/* repo/objects/*/*.commit
+${CMD_PREFIX} ostree --repo=repo pull --depth=-1 origin main
+find repo/objects -name '*.commit' | wc -l > commitcount
+assert_file_has_content commitcount "^1$"
+find repo/state -name '*.commitpartial' | wc -l > commitpartialcount
+assert_file_has_content commitpartialcount "^0$"
+
+echo "ok pull depth missing parent"
+
+# Check that it errors if the ref head commit is missing.
+cd ${test_tmpdir}
+rm -f ostree-srv/gnomerepo/objects/*/*.commit
+
+rm -rf repo/refs/heads/* repo/refs/remotes/* repo/objects/*/*.commit
+if ${CMD_PREFIX} ostree --repo=repo pull --depth=-1 origin main; then
+    fatal "Pull with depth -1 succeeded with missing HEAD commit"
+fi
+
+echo "ok pull depth missing HEAD commit"