"Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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?
