Re: [PATCH v2 9/9] bundle-uri: fetch a list of bundles
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Sep 29, 2022, 21:58 UTC
- Message-ID
- <20220929215808.1438418-1-jonathantanmy@google.com>
- In-Reply-To
- <a6ab8f7c69925b5dfd95f3e4c83c6ccd4d2dfee3.1662734015.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 19 quoted lines
> +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...
Show 14 quoted lines
> +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?
Show 13 quoted lines
> +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?
Show 19 quoted lines
> +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?