On 9/29/2022 5:58 PM, Jonathan Tan wrote:
> "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?
You are right! Thanks.
>> +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?
By "file is missing" I think you mean "we never successfully downloaded
that file" and I agree that we should skip those bundles. I'll add more
tests for ANY mode to hopefully catch these issues.
>> +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.
It's a little bit backwards to have the "terminate early with nonzero
value" signal "success", but it would work. With careful commenting, I
think it's doable.
> I see that there are ALL tests, but could we have an ANY test as well?
Yes, excellent point. They are absolutely necessary.
Thanks,
-Stolee
