Re: [PATCH v2 05/10] bundle-uri: download in creationToken order
- From
Victoria Dye <vdye@github.com>
- Date
- Jan 27, 2023, 19:17 UTC
- Message-ID
- <a2896d5b-f395-68df-1f23-356d0128cb9c@github.com>
- In-Reply-To
- <7cfaa3c518cbedb65c585cc02015bb21ae24e9fa.1674487310.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> +struct bundles_for_sorting {...
> +static int append_bundle(struct remote_bundle_info *bundle, void *data)
...
Show 5 quoted lines
> +/** > + * For use in QSORT() to get a list sorted by creationToken > + * in decreasing order. > + */ > +static int compare_creation_token_decreasing(const void *va, const void *vb)
These new function/struct names are all unambiguous. Looks good!
Show 13 quoted lines
> + cur = 0;
> + while (cur >= 0 && cur < bundles.nr) {
> + struct remote_bundle_info *bundle = bundles.items[cur];
> + if (!bundle->file) {
> + /*
> + * Not downloaded yet. Try downloading.
> + *
> + * Note that bundle->file is non-NULL if a download
> + * was attempted, even if it failed to download.
> + */
> + if (fetch_bundle_uri_internal(ctx.r, bundle, ctx.depth + 1, ctx.list)) {
> + /* Mark as unbundled so we do not retry. */
> + bundle->unbundled = 1;This implicitly shows that, unlike a failed unbundling, a failed download is always erroneous behavior, with the added benefit of avoiding (potentially expensive) download re-attempts.
Show 39 quoted lines
> +
> + /* Try looking deeper in the list. */
> + move_direction = 1;
> + goto stack_operation;
> + }
> +
> + /* We expect bundles when using creationTokens. */
> + if (!is_bundle(bundle->file, 1)) {
> + warning(_("file downloaded from '%s' is not a bundle"),
> + bundle->uri);
> + break;
> + }
> + }
> +
> + if (bundle->file && !bundle->unbundled) {
> + /*
> + * This was downloaded, but not successfully
> + * unbundled. Try unbundling again.
> + */
> + if (unbundle_from_file(ctx.r, bundle->file)) {
> + /* Try looking deeper in the list. */
> + move_direction = 1;
> + } else {
> + /*
> + * Succeeded in unbundle. Retry bundles
> + * that previously failed to unbundle.
> + */
> + move_direction = -1;
> + bundle->unbundled = 1;
> + }
> + }
> +
> + /*
> + * Else case: downloaded and unbundled successfully.
> + * Skip this by moving in the same direction as the
> + * previous step.
> + */
> +
> +stack_operation:Other than this label, it looks like you've replaced all of the "stack-based" language. Should this be replaced as well? No problem if not, I just wasn't sure whether it was left that way intentionally.
Show 17 quoted lines
> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh > index 474432c8ace..6f9417a0afb 100755 > --- a/t/t5558-clone-bundle-uri.sh > +++ b/t/t5558-clone-bundle-uri.sh > @@ -401,17 +401,43 @@ test_expect_success 'clone bundle list (http, creationToken)' ' > git -C clone-list-http-2 cat-file --batch-check <oids && > > cat >expect <<-EOF && > - $HTTPD_URL/bundle-1.bundle > - $HTTPD_URL/bundle-2.bundle > - $HTTPD_URL/bundle-3.bundle > + $HTTPD_URL/bundle-list > $HTTPD_URL/bundle-4.bundle > + $HTTPD_URL/bundle-3.bundle > + $HTTPD_URL/bundle-2.bundle > + $HTTPD_URL/bundle-1.bundle > + EOF
Ooh, interesting - using the new "test_remote_https_urls", these tests now also verify that the bundles were downloaded in decreasing order when using the 'creationToken' heuristic. That's a nice extra confirmation that the heuristic is working as intended.
> +test_expect_success 'clone incomplete bundle list (http, creationToken)' '
...
> +test_expect_success 'auto-discover multiple bundles from HTTP clone: creationToken heuristic' '
These tests look good as well, especially 'clone incomplete bundle list's now-more descriptive name.