On 1/27/2023 2:32 PM, Junio C Hamano wrote:
> Victoria Dye <vdye@xxxxxxxxxx> writes:
>
>>> + /*
>>> + * Not downloaded yet. Try downloading.
>>> + *
>>> + * Note that bundle->file is non-NULL if a download
>>> + * was attempted, even if it failed to download.
>>> + */
>>> + if (fetch_bundle_uri_internal(ctx.r, bundle, ctx.depth + 1, ctx.list)) {
>>> + /* Mark as unbundled so we do not retry. */
>>> + bundle->unbundled = 1;
>>
>> This implicitly shows that, unlike a failed unbundling, a failed download is
>> always erroneous behavior, with the added benefit of avoiding (potentially
>> expensive) download re-attempts.
>
> Hmph, I somehow was hoping that we'd allow an option to use range
> requests to resume an interrupted download in the future, so
> outright "always avoid attempts to download again" may not be what
> we want in the longer run. But being able to tell if download
> failed (and there will probably be more than "success/failure" bit,
> but something like "we got an explicit 401 not found" vs "we were
> disconnected after downloading a few megabytes"), and unbundling
> failed (where there is no point attempting) is a good idea.
I think there are two possible directions we can have when talking
about interrupted downloads:
1. The network connection was disconnected, and the client may want
to respond to that with a retry and a ranged request.
2. The client process itself terminates for some reason, and a
second process recognizes that some of the data already exists
and could be used for a range request of the remainder.
I think both of these would not be handled at this layer, but
instead further down, inside fetch_bundle_uri_internal()
(specifically further down in download_https_uri_to_file()).
Any retry logic should happen there, closer to the connection,
and at the layer of the current patch, we should assume that any
retry logic that was attempted ended up failing in the end.
Does that satisfy your concerns here?
Thanks,
-Stolee
