From 015513b8f979ab0769ad36da868544378e553041 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 3 Nov 2017 18:43:00 +0000 Subject: [PATCH] lib/pull: Avoid error if current with --require-static-deltas A tricky thing here that caused this to go past a lot of our tests is that the code was mostly OK if there was an available delta from an older commit. But this case broke if we e.g. had a new OS deployment and did a `--require-static-deltas` pull, i.e. the initial state. I cleaned up our "find static delta state" function to return an enumeration, and extended it with an "already have the commit" state. A problem I then hit is that we've historically fetched detached metadata for non-delta pulls, even if the commit hasn't changed. I decided not to do that for `--require-static-deltas` pulls for now; otherwise the code gets notably more complex. Closes: https://github.com/ostreedev/ostree/issues/1321 Closes: #1323 Approved by: jlebon --- src/libostree/ostree-repo-pull.c | 119 +++++++++++++++++++++++++------ tests/pull-test.sh | 6 ++ 2 files changed, 102 insertions(+), 23 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 20fa8277..b4310027 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -2325,19 +2325,39 @@ process_one_static_delta (OtPullData *pull_data, return ret; } -/* Loop over the static delta data we got from the summary, - * and find the newest commit for @out_from_revision that - * goes to @to_revision. +/* + * DELTA_SEARCH_RESULT_UNCHANGED: + * We already have the commit. * - * Additionally, @out_have_scratch_delta will be set to %TRUE - * if there is a %NULL → @to_revision delta, also known as + * DELTA_SEARCH_RESULT_NO_MATCH: + * No deltas were found. + * + * DELTA_SEARCH_RESULT_FROM: + * A regular delta was found, and the "from" revision will be + * set in `from_revision`. + * + * DELTA_SEARCH_RESULT_SCRATCH: + * There is a %NULL → @to_revision delta, also known as * a "from scratch" delta. */ +typedef struct { + enum { + DELTA_SEARCH_RESULT_UNCHANGED, + DELTA_SEARCH_RESULT_NO_MATCH, + DELTA_SEARCH_RESULT_FROM, + DELTA_SEARCH_RESULT_SCRATCH, + } result; + char from_revision[OSTREE_SHA256_STRING_LEN+1]; +} DeltaSearchResult; + +/* Loop over the static delta data we got from the summary, + * and find the a delta path (if available) that goes to @to_revision. + * See the enum in `DeltaSearchResult` for available result types. + */ static gboolean get_best_static_delta_start_for (OtPullData *pull_data, const char *to_revision, - gboolean *out_have_scratch_delta, - char **out_from_revision, + DeltaSearchResult *out_result, GCancellable *cancellable, GError **error) { @@ -2348,7 +2368,28 @@ get_best_static_delta_start_for (OtPullData *pull_data, g_assert (pull_data->summary_deltas_checksums != NULL); - *out_have_scratch_delta = FALSE; + out_result->result = DELTA_SEARCH_RESULT_NO_MATCH; + out_result->from_revision[0] = '\0'; + + /* First, do we already have this commit completely downloaded? */ + gboolean have_to_rev; + if (!ostree_repo_has_object (pull_data->repo, OSTREE_OBJECT_TYPE_COMMIT, + to_revision, &have_to_rev, + cancellable, error)) + return FALSE; + if (have_to_rev) + { + OstreeRepoCommitState to_rev_state; + if (!ostree_repo_load_commit (pull_data->repo, to_revision, + NULL, &to_rev_state, error)) + return FALSE; + if (!(to_rev_state & OSTREE_REPO_COMMIT_STATE_PARTIAL)) + { + /* We already have this commit, we're done! */ + out_result->result = DELTA_SEARCH_RESULT_UNCHANGED; + return TRUE; /* Early return */ + } + } /* Loop over all deltas known from the summary file, * finding ones which go to to_revision */ @@ -2366,9 +2407,17 @@ get_best_static_delta_start_for (OtPullData *pull_data, continue; if (cur_from_rev) - g_ptr_array_add (candidates, g_steal_pointer (&cur_from_rev)); + { + g_ptr_array_add (candidates, g_steal_pointer (&cur_from_rev)); + } else - *out_have_scratch_delta = TRUE; + { + /* We note that we have a _SCRATCH delta here, but we'll prefer using + * "from" deltas (obviously, they'll be smaller) where possible if we + * find one below. + */ + out_result->result = DELTA_SEARCH_RESULT_SCRATCH; + } } /* Loop over our candidates, find the newest one */ @@ -2407,7 +2456,11 @@ get_best_static_delta_start_for (OtPullData *pull_data, } } - *out_from_revision = g_strdup (newest_candidate); + if (newest_candidate) + { + out_result->result = DELTA_SEARCH_RESULT_FROM; + memcpy (out_result->from_revision, newest_candidate, OSTREE_SHA256_STRING_LEN+1); + } return TRUE; } @@ -3082,25 +3135,45 @@ initiate_request (OtPullData *pull_data, /* If we have a summary, we can use the newer logic */ if (pull_data->summary) { - gboolean have_scratch_delta = FALSE; + DeltaSearchResult deltares; /* Look for a delta to @to_revision in the summary data */ - if (!get_best_static_delta_start_for (pull_data, to_revision, - &have_scratch_delta, &delta_from_revision, + if (!get_best_static_delta_start_for (pull_data, to_revision, &deltares, pull_data->cancellable, error)) return FALSE; - if (delta_from_revision) /* Did we find a delta FROM commit? */ - initiate_delta_request (pull_data, delta_from_revision, to_revision, ref); - else if (have_scratch_delta) /* No delta FROM, do we have a scratch? */ - initiate_delta_request (pull_data, NULL, to_revision, ref); - else if (pull_data->require_static_deltas) /* No deltas found; are they required? */ + switch (deltares.result) { - set_required_deltas_error (error, (ref != NULL) ? ref->ref_name : "", to_revision); - return FALSE; + case DELTA_SEARCH_RESULT_NO_MATCH: + { + if (pull_data->require_static_deltas) /* No deltas found; are they required? */ + { + set_required_deltas_error (error, (ref != NULL) ? ref->ref_name : "", to_revision); + return FALSE; + } + else /* No deltas, fall back to object fetches. */ + queue_scan_one_metadata_object (pull_data, to_revision, OSTREE_OBJECT_TYPE_COMMIT, NULL, 0, ref); + } + break; + case DELTA_SEARCH_RESULT_FROM: + initiate_delta_request (pull_data, deltares.from_revision, to_revision, ref); + break; + case DELTA_SEARCH_RESULT_SCRATCH: + initiate_delta_request (pull_data, NULL, to_revision, ref); + break; + case DELTA_SEARCH_RESULT_UNCHANGED: + { + /* If we already have the commit, here things get a little special; we've historically + * fetched detached metadata, so let's keep doing that. But in the --require-static-deltas + * path, we don't, under the assumption the user wants as little network traffic as + * possible. + */ + if (pull_data->require_static_deltas) + break; + else + queue_scan_one_metadata_object (pull_data, to_revision, OSTREE_OBJECT_TYPE_COMMIT, NULL, 0, ref); + } } - else /* No deltas, fall back to object fetches. */ - queue_scan_one_metadata_object (pull_data, to_revision, OSTREE_OBJECT_TYPE_COMMIT, NULL, 0, ref); } else if (ref != NULL) { diff --git a/tests/pull-test.sh b/tests/pull-test.sh index 3f8030e0..c09feb30 100644 --- a/tests/pull-test.sh +++ b/tests/pull-test.sh @@ -381,6 +381,12 @@ fi ${CMD_PREFIX} ostree --repo=repo fsck done +# Test no-op with deltas: https://github.com/ostreedev/ostree/issues/1321 +cd ${test_tmpdir} +repo_init --no-gpg-verify +${CMD_PREFIX} ostree --repo=repo pull origin main +${CMD_PREFIX} ostree --repo=repo pull --require-static-deltas origin main + cd ${test_tmpdir} repo_init --no-gpg-verify ${CMD_PREFIX} ostree --repo=repo pull origin main@${prev_rev}