Re: [PATCH v2 05/10] bundle-uri: download in creationToken order

[Victoria Dye <vdye@github.com>]

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.