Derrick Stolee via GitGitGadget wrote:
> +struct bundles_for_sorting {
...
> +static int append_bundle(struct remote_bundle_info *bundle, void *data)
...
> +/**
> + * 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!
> + 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.
> +
> + /* 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.
> 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.
