Re: [PATCH v2 9/9] bundle-uri: fetch a list of bundles

[Jonathan Tan <jonathantanmy@google.com>]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +static int download_bundle_to_file(struct remote_bundle_info *bundle, void *data)
> +{
> +	int res;
> +	struct bundle_list_context *ctx = data;
> +
> +	if (ctx->mode == BUNDLE_MODE_ANY && ctx->count)
> +		return 0;
> +
> +	res = fetch_bundle_uri_internal(ctx->r, bundle, ctx->depth + 1, ctx->list);
> +
> +	/*
> +	 * Only increment count if the download succeeded. If our mode is
> +	 * BUNDLE_MODE_ANY, then we will want to try other URIs in the
> +	 * list in case they work instead.
> +	 */
> +	if (!res)
> +		ctx->count++;
> +	return res;
> +}

So this returns nonzero if a download fails...

> +static int download_bundle_list(struct repository *r,
> +				struct bundle_list *local_list,
> +				struct bundle_list *global_list,
> +				int depth)
> +{
> +	struct bundle_list_context ctx = {
> +		.r = r,
> +		.list = global_list,
> +		.depth = depth + 1,
> +		.mode = local_list->mode,
> +	};
> +
> +	return for_all_bundles_in_list(local_list, download_bundle_to_file, &ctx);
> +}

...and for_all_bundles_in_list does not proceed with the rest of the
loop if any callback invocation returns nonzero. Don't we need to
continue retrying the others if the mode is ANY?

> +static int attempt_unbundle(struct remote_bundle_info *info, void *data)
> +{
> +	struct attempt_unbundle_context *ctx = data;
> +
> +	if (info->unbundled || !unbundle_from_file(ctx->r, info->file)) {
> +		ctx->success_count++;
> +		info->unbundled = 1;
> +	} else {
> +		ctx->failure_count++;
> +	}
> +
> +	return 0;
> +}

Do we need to handle the case in which a file is missing but it's
expected because the mode is ANY and another file was successfully
downloaded?

> +static int unbundle_all_bundles(struct repository *r,
> +				struct bundle_list *list)
> +{
> +	int last_success_count = -1;
> +	struct attempt_unbundle_context ctx = {
> +		.r = r,
> +	};
> +
> +	/*
> +	 * Iterate through all bundles looking for ones that can
> +	 * successfully unbundle. If any succeed, then perhaps another
> +	 * will succeed in the next attempt.
> +	 */
> +	while (last_success_count < ctx.success_count) {
> +		last_success_count = ctx.success_count;
> +
> +		ctx.success_count = 0;
> +		ctx.failure_count = 0;
> +		for_all_bundles_in_list(list, attempt_unbundle, &ctx);

I think it would have been clearer if the invocation to
for_all_bundles_in_list were to stop early if a bundle has been
successfully unbundled, and then you can just run this loop n times,
instead of needing to reset the success count each time in order to
check that the latest count is more than the prior one. But this works
too.

[snip tests]

I see that there are ALL tests, but could we have an ANY test as well?