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

> "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.