diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 586b34fe3b..822bdd2952 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -2552,6 +2552,120 @@ process_one_static_delta (OtPullData *pull_data, return TRUE; } +static gboolean +normal_commit_reachable (OstreeRepo *self, + const char *cur_revision) +{ + g_autofree char *rev = g_strdup (cur_revision); + while (rev != NULL) + { + g_autoptr(GVariant) commit = NULL; + OstreeRepoCommitState commitstate; + g_autoptr(GError) local_error = NULL; + + if (!ostree_repo_load_commit (self, rev, &commit, &commitstate, &local_error)) + { + /* Ignore issues with the commit since we're going to try to pull a + * scratch delta, anyways. Just dump out the error unless it's the + * likely issue that the parent commit didn't exist. + */ + if (!g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND)) + g_debug ("Couldn't load commit %s: %s", rev, local_error->message); + + g_clear_error (&local_error); + break; + } + + /* Is this a normal commit? */ + if ((commitstate & OSTREE_REPO_COMMIT_STATE_PARTIAL) == 0) + return TRUE; + + g_free (rev); + rev = ostree_commit_get_parent (commit); + } + + /* No normal commit found in history */ + return FALSE; +} + +static gboolean +normal_bound_commit_exists (OtPullData *pull_data, + const OstreeCollectionRef *ref, + GCancellable *cancellable) +{ + g_return_val_if_fail (ref != NULL, FALSE); + + g_autofree char *collection_id = NULL; + if (ref->collection_id != NULL) + collection_id = g_strdup (ref->collection_id); + else + { + collection_id = get_remote_repo_collection_id (pull_data); + if (collection_id == NULL) + return FALSE; + } + + g_autoptr(GHashTable) objects = NULL; + g_autoptr(GError) list_error = NULL; + if (!ostree_repo_list_objects (pull_data->repo, OSTREE_REPO_LIST_OBJECTS_ALL, + &objects, cancellable, &list_error)) + { + g_debug ("Couldn't list objects: %s", list_error->message); + return FALSE; + } + + GLNX_HASH_TABLE_FOREACH (objects, GVariant*, serialized_key) + { + const char *checksum; + OstreeObjectType objtype; + ostree_object_name_deserialize (serialized_key, &checksum, &objtype); + if (objtype != OSTREE_OBJECT_TYPE_COMMIT) + continue; + + g_autoptr(GVariant) commit = NULL; + OstreeRepoCommitState commitstate; + g_autoptr(GError) local_error = NULL; + if (!ostree_repo_load_commit (pull_data->repo, checksum, &commit, + &commitstate, &local_error)) + { + /* Ignore issues with the commit since we're going to try to pull a + * scratch delta, anyways. + */ + g_debug ("Couldn't load commit %s: %s", checksum, local_error->message); + continue; + } + + /* Is this a partial commit? */ + if ((commitstate & OSTREE_REPO_COMMIT_STATE_PARTIAL) > 0) + continue; + + /* If the commit has the same collection binding and the ref is included + * in the ref bindings, we have a match. + */ + g_autoptr(GVariant) metadata = g_variant_get_child_value (commit, 0); + const char *collection_id_binding; + if (!g_variant_lookup (metadata, + OSTREE_COMMIT_META_KEY_COLLECTION_BINDING, + "&s", + &collection_id_binding)) + continue; + if (!g_str_equal (collection_id_binding, collection_id)) + continue; + + g_autofree const char **ref_bindings = NULL; + if (!g_variant_lookup (metadata, + OSTREE_COMMIT_META_KEY_REF_BINDING, + "^a&s", + &ref_bindings)) + continue; + if (g_strv_contains ((const char *const *) ref_bindings, ref->ref_name)) + return TRUE; + } + + /* No normal commit found with same bindings */ + return FALSE; +} + /* * DELTA_SEARCH_RESULT_UNCHANGED: * We already have the commit. @@ -2582,11 +2696,13 @@ typedef struct { * See the enum in `DeltaSearchResult` for available result types. */ static gboolean -get_best_static_delta_start_for (OtPullData *pull_data, - const char *to_revision, - DeltaSearchResult *out_result, - GCancellable *cancellable, - GError **error) +get_best_static_delta_start_for (OtPullData *pull_data, + const OstreeCollectionRef *ref, + const char *to_revision, + const char *cur_revision, + DeltaSearchResult *out_result, + GCancellable *cancellable, + GError **error) { /* Array of possible from checksums */ g_autoptr(GPtrArray) candidates = g_ptr_array_new_with_free_func (g_free); @@ -2688,6 +2804,30 @@ get_best_static_delta_start_for (OtPullData *pull_data, out_result->result = DELTA_SEARCH_RESULT_FROM; memcpy (out_result->from_revision, newest_candidate, OSTREE_SHA256_STRING_LEN+1); } + + /* If a from-scratch delta is available and deltas aren't required, we don’t + * want to use it if the ref or any of its ancestors already exists locally + * and is not partial. In that case only some of the objects in the new + * commit may be needed, so doing an object pull is likely more bandwidth + * efficient. + */ + if (out_result->result == DELTA_SEARCH_RESULT_SCRATCH && + !pull_data->require_static_deltas) + { + if (cur_revision != NULL && + normal_commit_reachable (pull_data->repo, cur_revision)) + { + g_debug ("Found normal commit in history of %s", cur_revision); + out_result->result = DELTA_SEARCH_RESULT_NO_MATCH; + } + else if (ref != NULL && + normal_bound_commit_exists (pull_data, ref, cancellable)) + { + g_debug ("Found normal commit with same bindings as %s", ref->ref_name); + out_result->result = DELTA_SEARCH_RESULT_NO_MATCH; + } + } + return TRUE; } @@ -3404,7 +3544,8 @@ initiate_request (OtPullData *pull_data, DeltaSearchResult deltares; /* Look for a delta to @to_revision in the summary data */ - if (!get_best_static_delta_start_for (pull_data, to_revision, &deltares, + if (!get_best_static_delta_start_for (pull_data, ref, to_revision, + delta_from_revision, &deltares, pull_data->cancellable, error)) return FALSE; @@ -3425,16 +3566,7 @@ initiate_request (OtPullData *pull_data, enqueue_one_static_delta_superblock_request (pull_data, deltares.from_revision, to_revision, ref); break; case DELTA_SEARCH_RESULT_SCRATCH: - { - /* If a from-scratch delta is available, we don’t want to use it if - * the ref already exists locally, since we are likely only a few - * commits out of date; so doing an object pull is likely more - * bandwidth efficient. */ - if (delta_from_revision != NULL) - queue_scan_one_metadata_object (pull_data, to_revision, OSTREE_OBJECT_TYPE_COMMIT, NULL, 0, ref); - else - enqueue_one_static_delta_superblock_request (pull_data, NULL, to_revision, ref); - } + enqueue_one_static_delta_superblock_request (pull_data, NULL, to_revision, ref); break; case DELTA_SEARCH_RESULT_UNCHANGED: { diff --git a/tests/pull-test.sh b/tests/pull-test.sh index 2cfd8e02f7..8d3a345bae 100644 --- a/tests/pull-test.sh +++ b/tests/pull-test.sh @@ -54,12 +54,13 @@ function verify_initial_contents() { assert_file_has_content baz/cow '^moo$' } +num_tests=44 +# 3 tests needs GPG support +num_gpg_tests=3 if has_gpgme; then - echo "1..34" -else - # 3 tests needs GPG support - echo "1..31" + num_tests=$((num_tests + num_gpg_tests)) fi +echo "1..${num_tests}" # Try both syntaxes repo_init --no-gpg-verify @@ -604,3 +605,198 @@ if ${CMD_PREFIX} ostree --repo=repo pull origin main 2>err.txt; then fi assert_file_has_content_literal err.txt 'error: Fetching checksum for ref ((empty), main): Invalid rev lots of html here lots of html here lots of html here lots of' echo "ok pull got HTML for a ref" + +# Restore ref for subsequent tests +mv ostree-srv/gnomerepo/refs/heads/main{.orig,} + +# Test scratch deltas with partial current ref +# https://github.com/ostreedev/ostree/issues/2004 +rm ostree-srv/gnomerepo/deltas -rf +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo refs --delete scratch +rm scratch-files -rf +mkdir scratch-files +echo "stuff for rev1" > scratch-files/rev1 +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo commit ${COMMIT_ARGS} -b scratch -s rev1 scratch-files +rev1=$(${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo rev-parse scratch) +echo "stuff for rev2" > scratch-files/rev2 +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo commit ${COMMIT_ARGS} -b scratch -s rev2 scratch-files +rev2=$(${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo rev-parse scratch) +echo "stuff for rev3" > scratch-files/rev3 +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo commit ${COMMIT_ARGS} -b scratch -s rev3 scratch-files +rev3=$(${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo rev-parse scratch) +rm scratch-files -rf + +# Make a scratch delta and then corrupt it so we can tell when the pull is +# fetching the delta and not falling back to an object pull. +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo static-delta --empty generate scratch +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo summary -u +superblock=$(find ostree-srv/gnomerepo/deltas -name superblock) +echo FAIL > ${superblock} + +# First pull with no existing ref. This should fail because it tries to +# pull the scratch delta. +repo_init --no-gpg-verify +if ${CMD_PREFIX} ostree --repo=repo pull origin scratch 2>err.txt; then + assert_not_reached "pull of corrupt scratch delta unexpectedly succeeded" +fi +assert_file_has_content err.txt "Invalid checksum for static delta" +echo "ok scratch delta (no existing ref)" + +# Pull with a partial HEAD ref. This should fail because it tries to pull the +# scratch delta. +repo_init --no-gpg-verify +${CMD_PREFIX} ostree --repo=repo pull origin --commit-metadata-only scratch +if ${CMD_PREFIX} ostree --repo=repo pull origin scratch 2>err.txt; then + assert_not_reached "pull of corrupt scratch delta unexpectedly succeeded" +fi +assert_file_has_content err.txt "Invalid checksum for static delta" +echo "ok scratch delta (partial HEAD only)" + +# Pull with a partial HEAD ref and partial parent. This should fail because it +# tries to pull the scratch delta. +repo_init --no-gpg-verify +${CMD_PREFIX} ostree --repo=repo pull origin --commit-metadata-only scratch@${rev2} +${CMD_PREFIX} ostree --repo=repo pull origin --commit-metadata-only scratch +if ${CMD_PREFIX} ostree --repo=repo pull origin scratch 2>err.txt; then + assert_not_reached "pull of corrupt scratch delta unexpectedly succeeded" +fi +assert_file_has_content err.txt "Invalid checksum for static delta" +echo "ok scratch delta (partial HEAD and partial parent)" + +# Pull with a partial grandparent. This should fail +# because it tries to pull the scratch delta. This is basically testing that +# there aren't issues with a commit that has not parent. +repo_init --no-gpg-verify +${CMD_PREFIX} ostree --repo=repo pull origin --commit-metadata-only scratch@${rev1} +if ${CMD_PREFIX} ostree --repo=repo pull origin scratch 2>err.txt; then + assert_not_reached "pull of corrupt scratch delta unexpectedly succeeded" +fi +assert_file_has_content err.txt "Invalid checksum for static delta" +echo "ok scratch delta (partial grandparent)" + +# Pull with a partial HEAD ref and normal parent. This should succeed with an +# object pull. +repo_init --no-gpg-verify +${CMD_PREFIX} ostree --repo=repo pull origin scratch@${rev2} +${CMD_PREFIX} ostree --repo=repo pull origin --commit-metadata-only scratch +${CMD_PREFIX} ostree --repo=repo pull origin scratch +echo "ok scratch delta (partial HEAD and normal parent)" + +# Pull with a partial HEAD ref, partial parent and normal grandparent. This +# should succeed with an object pull. +repo_init --no-gpg-verify +${CMD_PREFIX} ostree --repo=repo pull origin scratch@${rev1} +${CMD_PREFIX} ostree --repo=repo pull origin --commit-metadata-only scratch@${rev2} +${CMD_PREFIX} ostree --repo=repo pull origin --commit-metadata-only scratch +${CMD_PREFIX} ostree --repo=repo pull origin scratch +echo "ok scratch delta (partial HEAD, partial parent and normal grandparent)" + +# Pull with a partial HEAD ref and normal grandparent. Ideally this +# would use an object pull, but since the grandparent can't be reached +# by the HEAD commit it will result in a scratch delta. +repo_init --no-gpg-verify +${CMD_PREFIX} ostree --repo=repo pull origin scratch@${rev1} +${CMD_PREFIX} ostree --repo=repo pull origin --commit-metadata-only scratch +if ${CMD_PREFIX} ostree --repo=repo pull origin scratch 2>err.txt; then + assert_not_reached "pull of corrupt scratch delta unexpectedly succeeded" +fi +assert_file_has_content err.txt "Invalid checksum for static delta" +echo "ok scratch delta (partial HEAD and normal grandparent)" + +# Pull with a normal parent. This should succeed with an object pull. +repo_init --no-gpg-verify +${CMD_PREFIX} ostree --repo=repo pull origin scratch@${rev2} +${CMD_PREFIX} ostree --repo=repo pull origin scratch +echo "ok scratch delta (normal parent)" + +# Pull with a normal parent but require static deltas. This should fail +# because it tries to pull the scratch delta. +repo_init --no-gpg-verify +${CMD_PREFIX} ostree --repo=repo pull origin scratch@${rev2} +if ${CMD_PREFIX} ostree --repo=repo pull --require-static-deltas origin scratch 2>err.txt; then + assert_not_reached "pull of corrupt scratch delta unexpectedly succeeded" +fi +assert_file_has_content err.txt "Invalid checksum for static delta" +echo "ok scratch delta (require static deltas with normal parent)" + +# Clean up the deltas so we don't leave a corrupt one around +rm ostree-srv/gnomerepo/deltas -rf + +# Test using scratch deltas with unreachable commits but matching ref bindings. +# Add a collection ID to the remote to make some commits with appropriate +# bindings. +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo config set core.collection-id org.example.Repo +rm ostree-srv/gnomerepo/deltas -rf +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo refs --delete scratch +rm scratch-files -rf +mkdir scratch-files +echo "stuff for rev1" > scratch-files/rev1 +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo commit ${COMMIT_ARGS} -b scratch -s rev1 scratch-files +rev1=$(${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo rev-parse scratch) +echo "stuff for rev2" > scratch-files/rev2 +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo commit ${COMMIT_ARGS} -b scratch -s rev2 scratch-files +rev2=$(${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo rev-parse scratch) +echo "stuff for rev3" > scratch-files/rev3 +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo commit ${COMMIT_ARGS} -b scratch -s rev3 scratch-files +rev3=$(${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo rev-parse scratch) +rm scratch-files -rf + +# Make a scratch delta and then corrupt it so we can tell when the pull is +# fetching the delta and not falling back to an object pull. +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo static-delta --empty generate scratch +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo summary -u +superblock=$(find ostree-srv/gnomerepo/deltas -name superblock) +echo FAIL > ${superblock} + +# Pull with a partial HEAD ref and partial unreachable commit with matching ref +# bindings. This will search all commits for one bound to the requested ref and +# find the partial grandparent. This should fail because it tries to pull the +# scratch delta. +repo_init --no-gpg-verify +${CMD_PREFIX} ostree --repo=repo pull origin --commit-metadata-only scratch@${rev1} +${CMD_PREFIX} ostree --repo=repo pull origin --commit-metadata-only scratch +if ${CMD_PREFIX} ostree --repo=repo pull origin scratch 2>err.txt; then + assert_not_reached "pull of corrupt scratch delta unexpectedly succeeded" +fi +assert_file_has_content err.txt "Invalid checksum for static delta" +echo "ok scratch delta (partial HEAD and partial bound grandparent)" + +# Pull with a partial HEAD ref and normal unreachable commit with matching ref +# bindings. This will search all commits for one bound to the requested ref and +# find the normal grandparent. This will succeed with an object pull. +repo_init --no-gpg-verify +${CMD_PREFIX} ostree --repo=repo pull origin scratch@${rev1} +${CMD_PREFIX} ostree --repo=repo pull origin --commit-metadata-only scratch +${CMD_PREFIX} ostree --repo=repo pull origin scratch +echo "ok scratch delta (partial HEAD and normal bound grandparent)" + +# Pull the HEAD by revision with a normal unreachable commit with matching ref +# bindings but collection-id not set for the remote. Since the pull is by +# revision, it will not have the collection ID set in the requested ref and the +# search for the bound grandparent will fail. This should fail because it tries +# to pull the scratch delta. +repo_init --no-gpg-verify +${CMD_PREFIX} ostree --repo=repo pull origin scratch@${rev1} +${CMD_PREFIX} ostree --repo=repo pull origin --commit-metadata-only scratch@${rev3} +if ${CMD_PREFIX} ostree --repo=repo pull origin scratch@${rev3} 2>err.txt; then + assert_not_reached "pull of corrupt scratch delta unexpectedly succeeded" +fi +assert_file_has_content err.txt "Invalid checksum for static delta" +echo "ok scratch delta (partial HEAD by revision, normal bound grandparent and no remote collection-id)" + +# Set the remote's collection ID and pull the HEAD by revision with a normal +# unreachable commit with matching ref bindings but collection-id not set for +# the remote. Since the pull is by revision, it will not have the collection ID +# set in the requested ref, but the collection ref configured for the remote +# will be used. This will succeed with an object pull. +repo_init --no-gpg-verify --collection-id=org.example.Repo +${CMD_PREFIX} ostree --repo=repo pull origin scratch@${rev1} +${CMD_PREFIX} ostree --repo=repo pull origin --commit-metadata-only scratch@${rev3} +${CMD_PREFIX} ostree --repo=repo pull origin scratch@${rev3} +echo "ok scratch delta (partial HEAD by revision, normal bound grandparent and remote collection-id)" + +# Remove the remote repo's collection ID +${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo config unset core.collection-id + +# Clean up the deltas so we don't leave a corrupt one around +rm ostree-srv/gnomerepo/deltas -rf