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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 21, 2026, 19:56 UTC
Message-ID: <xmqqo6g0unfb.fsf@gitster.g>
In-Reply-To: <160a9b9fd0982dadfbf6f8fbb378d1a3e9173698.1784582665.git.tnyman@openai.com>

Ted Nyman <tnyman@openai.com> writes:

Show 16 quoted lines

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

Show 7 quoted lines

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

Show 6 quoted lines

> ---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?

Show 11 quoted lines

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

Show 6 quoted lines

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

Show 9 quoted lines

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