Re: [PATCH v2 1/2] http: avoid concurrent appends to partial packs

Ted Nyman <[email protected]> writes:

> Pack requests stage downloads in a predictable partial-pack file so an
> interrupted transfer can be resumed. Both packfile URI and ordinary dumb
> HTTP requests use this staging path. Opening it in append mode lets
> concurrent fetches interleave their writes, corrupting the pack or
> causing a later fetch to request a range at EOF.
>
> Open the partial pack read-write, seek to its current end, and retain a
> per-descriptor offset for incoming data. Reopen newly created partial
> packs without O_CREAT so Windows permits concurrent unlink, and keep the
> descriptor for index-pack when another downloader removes the staging
> path. Accept HTTP 416 when a partial pack is already complete.
>
> Exercise resumed transfers, EOF ranges, and overlapping 200 and 206
> responses. Clarify the staging-key documentation and correct the stale
> --index-pack-args spelling in the documentation and error messages; the
> repeatable --index-pack-arg option is already accepted.

Hmph.  So the idea is to allow multiple processes to open the same
file and, because they all know where their respective chunks of
data fit in the final file, have them use pwrite(2) to deposit those
pieces at the exact target locations, and this prevents them from
stepping on each other's toes?

I cannot exactly explain why but it somehow makes me feel dirty.

It is also surprising that the workaround on MinGW works when
one of these multiple processes finishes writing and attempts to
finalize the temporary file while others still have open file
descriptors to the same file.

> -	The hash is used to determine the name of the temporary file and is
> -	arbitrary. The output of index-pack is printed to stdout. Requires
> -	--index-pack-args.
> +	The hash is used to determine the name of the temporary file. It need
> +	not be the pack hash, but it must uniquely identify the pack contents
> +	for resumption. The output of index-pack is printed to stdout. Requires
> +	one or more --index-pack-arg options.

OK.

> ---index-pack-args=<args>::
> -	For internal use only. The command to run on the contents of the
> -	downloaded pack. Arguments are URL-encoded separated by spaces.
> +--index-pack-arg=<arg>::
> +	For internal use only. An argument to the command run on the contents
> +	of the downloaded pack. This option can be specified multiple times.

Was the 'internal use only' thing renamed in order to prevent the
new code from accidentally working with an older caller?

    ... goes and notices that the code uses singular form throughout ...

Ah, no, this is an unrelated typo fix that remains valid even if the
rest of this patch is dropped.  Good catch.

It would be easier to review the actual changes if this cleanup were
isolated in a preliminary patch.  Are there other cleanup changes in
this series that fall into the same category?

> diff --git a/http-fetch.c b/http-fetch.c
> index f9b6ecb061..05f68f306a 100644
> --- a/http-fetch.c
> +++ b/http-fetch.c
> @@ -70,7 +70,8 @@ static void fetch_single_packfile(struct object_id *packfile_hash,
>  
>  	if (start_active_slot(preq->slot)) {
>  		run_active_slot(preq->slot);
> -		if (results.curl_result != CURLE_OK) {
> +		if (results.curl_result != CURLE_OK &&
> +		    results.http_code != 416) {

We do not seem to use symbolic constants for these '4xx' codes (or
'2xx', for that matter), so I will let that pass.  Eventually, we
may want to give symbolic constants to them to improve readability,
but doing so is certainly outside the scope of this topic.

> @@ -155,7 +156,7 @@ int cmd_main(int argc, const char **argv)
>  
>  	if (packfile) {
>  		if (!index_pack_args.nr)
> -			die(_("the option '%s' requires '%s'"), "--packfile", "--index-pack-args");
> +			die(_("the option '%s' requires '%s'"), "--packfile", "--index-pack-arg");

This and ...

> @@ -164,7 +165,7 @@ int cmd_main(int argc, const char **argv)
>  	}
>  
>  	if (index_pack_args.nr)
> -		die(_("the option '%s' requires '%s'"), "--index-pack-args", "--packfile");
> +		die(_("the option '%s' requires '%s'"), "--index-pack-arg", "--packfile");
>  
>  	if (commits_on_stdin) {
>  		commits = walker_targets_stdin(&commit_id, &write_ref);

... this is the same "index-pack-arg" fix and can be moved to a
separate preliminary clean-up patch.

Thanks.