Re: [PATCH 1/2] http: use unique tempfiles for packfile URI downloads
- From
Taylor Blau <ttaylorr@openai.com>
- Date
- Jul 14, 2026, 04:06 UTC
- Message-ID
- <alW1tAnMtOznxrhK@com-79390>
- In-Reply-To
- <alVn-QmK3K91_tkH@com-76773>
On Mon, Jul 13, 2026 at 03:34:33PM -0700, Ted Nyman wrote:
Show 57 quoted lines
> Since 8d5d2a34df (http-fetch: support fetching packfiles by URL,
> 2020-06-10), packfile URI downloads have been staged at
> objects/pack/pack-<hash>.pack.temp.
>
> The path is derived from the advertised pack hash. Two processes
> fetching the same pack into a shared object database therefore open the
> same file for append. Their writes can corrupt the temporary pack. If
> one process arrives after the other has completed the download, it may
> instead try to resume at EOF, which some HTTP servers reject with 416.
>
> Use the tempfile API to give direct packfile URI downloads unique
> temporary files. Keep the deterministic path for ordinary dumb HTTP
> pack requests, which use it to resume a partial download left by an
> earlier invocation.
>
> This means that a packfile URI download cannot be resumed by a later
> invocation. A retry starts with an empty temporary file instead.
>
> Add a test which pauses one process after downloading the pack and
> starts another process using the same object database.
>
> Signed-off-by: Ted Nyman <tnyman@openai.com>
> ---
> Documentation/git-http-fetch.adoc | 5 +-
> http.c | 77 +++++++++++++++++++++----------
> http.h | 1 +
> t/t5550-http-fetch-dumb.sh | 72 ++++++++++++++++++++++++++++-
> 4 files changed, 126 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/git-http-fetch.adoc b/Documentation/git-http-fetch.adoc
> index 2200f073c4..533bf381c4 100644
> --- a/Documentation/git-http-fetch.adoc
> +++ b/Documentation/git-http-fetch.adoc
> @@ -48,9 +48,8 @@ commit-id::
> line (which is not expected in
> this case), 'git http-fetch' fetches the packfile directly at the given
> URL and uses index-pack to generate corresponding .idx and .keep files.
> - 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 arbitrary. The output of index-pack is printed to stdout.
> + Requires --index-pack-args.
>
> --index-pack-args=<args>::
> For internal use only. The command to run on the contents of the
> diff --git a/http.c b/http.c
> index b4e7b8d00b..5a46e7c65c 100644
> --- a/http.c
> +++ b/http.c
> @@ -2668,7 +2668,10 @@ int http_get_info_packs(const char *base_url, struct packfile_list *packs)
>
> void release_http_pack_request(struct http_pack_request *preq)
> {
> - if (preq->packfile) {
> + if (preq->tempfile) {
> + delete_tempfile(&preq->tempfile);
> + preq->packfile = NULL;We should be able to drop the assignment to NULL on the second line, since `delete_tempfile()` takes a double pointer to the 'struct packfile' and NULL's it out for us.
(The other callers appear to avoid explicitly setting `preq->tempfile` to NULL.)
The rest of the patch looks good to me.
Show 21 quoted lines
> diff --git a/http.h b/http.h
> index 729c51904d..2c900779f5 100644
> --- a/http.h
> +++ b/http.h
> @@ -224,6 +224,7 @@ struct http_pack_request {
>
> FILE *packfile;
> struct strbuf tmpfile;
> + struct tempfile *tempfile;
> struct active_request_slot *slot;
> struct curl_slist *headers;
> };
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index b0080bf204..314a74c433 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -293,6 +293,74 @@ test_expect_success 'http-fetch --packfile' '
> git -C packfileclient cat-file -e "$HASH"
> '
>
> +test_expect_success PIPE 'concurrent http-fetch --packfile' 'Phew ;-).
This is definitely tricky to test, but what you wrote here looks plausibly correct to me.
Thanks, Taylor