[PATCH v6 0/6] packfile URIs: support concurrent downloads
- From
Ted Nyman <tnyman@openai.com>
- Date
- Jul 27, 2026, 00:28 UTC
- Message-ID
- <cover.1785111375.git.tnyman@openai.com>
- In-Reply-To
- <cover.1785047139.git.tnyman@openai.com>
Packfile URI and dumb HTTP downloads stage packs at objects/pack/pack-<hash>.pack.temp so an interrupted transfer can resume. Opening that file in append mode forces every write to its current end. Two Git processes fetching the same pack into one object database can therefore append duplicate data and corrupt the pack.
The first patch separates the unrelated --index-pack-arg documentation and error-message correction requested during review.
The second patch fixes an existing double-close when finish_http_pack_request() passes its staging-file descriptor to index-pack. start_command() already takes ownership of that descriptor, including when starting the child fails.
The third patch handles a completed partial pack independently of concurrent downloads. A previous attempt can finish the transfer but fail before indexing it; retrying then requests a range starting at EOF. Servers may respond with HTTP 416 in that case. Treat the response as a completed download and let index-pack validate the pack.
The fourth patch keeps the predictable staging name but removes append mode. Each downloader seeks once to the current end, requests the corresponding Range, and writes using its own descriptor offset. Since the staging key must identify immutable pack contents, overlapping responses write identical bytes at identical offsets. There is no need for pwrite(2) or cross-process coordination, and resumption continues to work for both packfile URI and ordinary dumb HTTP downloads.
The fifth patch handles the additional MinGW sharing requirement. Its non-append O_RDWR open grants FILE_SHARE_DELETE only for an existing file. Create a missing staging file exclusively, close it, and reopen it without O_CREAT so every retained descriptor permits another downloader to unlink the path.
The final patch handles the related .keep race. When another process has already created the keep file, index-pack reports "pack<TAB><hash>" instead of "keep<TAB><hash>". Accept both successful forms and remove only keep files created by the current process. Read only the prefix and hash so any following fsck output remains available to fetch-pack.
The tests cover resumption, a completed partial returning 416, overlapping downloads, unlinking the staging path while index-pack holds its descriptor, and a pre-existing .keep file. The completed-partial and unlink tests do not require FIFOs, so they can run on MinGW even though the concurrent-download test is skipped there.
Changes since v5:
* Split the existing double-close fix, HTTP 416 handling, generic concurrent-download fix, and Windows sharing fix into separate patches. * Replace the FIFO-based concurrent HTTP 416 test with a standalone completed-partial test. Besides simplifying the test, this covers the non-concurrent interrupted-download case directly. * Keep the final production code unchanged.
Each patch passes t5550-http-fetch-dumb.sh. The final series also passes t5702-protocol-v2.sh, and the overlapping-download test passes 240 runs with 12 parallel stress jobs.
The v5 discussion is at:
https://lore.kernel.org/git/cover.1785047139.git.tnyman@openai.com/
Ted Nyman (6): http-fetch: correct --index-pack-arg documentation http: avoid closing index-pack input twice http: accept HTTP 416 for complete partial packs http: avoid concurrent appends to partial packs http: permit unlinking partial packs on Windows fetch-pack: accept "pack" output for packfile URIs
Documentation/git-http-fetch.adoc | 14 +- fetch-pack.c | 33 ++--- http-fetch.c | 7 +- http-push.c | 3 +- http-walker.c | 3 +- http.c | 56 +++++--- t/t5550-http-fetch-dumb.sh | 204 ++++++++++++++++++++++++++++++ t/t5702-protocol-v2.sh | 31 +++++ 8 files changed, 305 insertions(+), 46 deletions(-)
Range-diff against v5:
1: a79af009ea = 1: b5050a88ca http-fetch: correct --index-pack-arg documentation
-: ---------- > 2: 28662b0fd8 http: avoid closing index-pack input twice
-: ---------- > 3: 677e5399eb http: accept HTTP 416 for complete partial packs
2: d9667c93b0 ! 4: 7a83eb7091 http: avoid concurrent appends to partial packs
@@ Commit message
pack contents, overlapping responses write the same bytes at the same
offsets instead of extending the file with duplicate data.
- MinGW's non-append O_RDWR open grants FILE_SHARE_DELETE only for an
- existing file. Create a missing partial pack exclusively, close it, and
- reopen it without O_CREAT so every retained descriptor permits another
- downloader to unlink the staging path. Duplicate that descriptor for
- index-pack instead of reopening the path after closing the stream;
- index-pack installs its own pack and the shared staging file is only
- unlinked, never renamed. Accept HTTP 416 when a partial pack is already
- complete and let index-pack validate its contents.
+ Duplicate the staging descriptor for index-pack instead of reopening the
+ path after closing the stream. Another downloader may unlink the staging
+ path before indexing begins, but index-pack can still read the retained
+ descriptor.
- Exercise resumed transfers, EOF ranges, overlapping 200 and 206
- responses, and unlinking the staging path while index-pack still holds
- its descriptor. Clarify the staging-key documentation.
+ Exercise resumed transfers and overlapping 200 and 206 responses, and
+ clarify the staging-key documentation.
Signed-off-by: Ted Nyman <tnyman@openai.com>
@@ Documentation/git-http-fetch.adoc: commit-id::
--index-pack-arg=<arg>::
- ## http-fetch.c ##
-@@ http-fetch.c: 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) {
- struct url_info url;
- char *nurl = url_normalize(preq->url, &url);
- if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {
-
- ## http-push.c ##
-@@ http-push.c: static void finish_request(struct transfer_request *request)
-
- } else if (request->state == RUN_FETCH_PACKED) {
- int fail = 1;
-- if (request->curl_result != CURLE_OK) {
-+ if (request->curl_result != CURLE_OK &&
-+ request->http_code != 416) {
- fprintf(stderr, "Unable to get pack file %s\n%s",
- request->url, curl_errorstr);
- } else {
-
- ## http-walker.c ##
-@@ http-walker.c: static int http_fetch_pack(struct walker *walker, struct alt_base *repo,
-
- 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) {
- error("Unable to get pack file %s\n%s", preq->url,
- curl_errorstr);
- goto abort;
-
## http.c ##
@@ http.c: int finish_http_pack_request(struct http_pack_request *preq)
int tmpfile_fd;
@@ http.c: int finish_http_pack_request(struct http_pack_request *preq)
ip.git_cmd = 1;
ip.in = tmpfile_fd;
-@@ http.c: int finish_http_pack_request(struct http_pack_request *preq)
- else
- ip.no_stdout = 1;
-
-- if (run_command(&ip)) {
-+ if (run_command(&ip))
- ret = -1;
-- goto cleanup;
-- }
--
--cleanup:
-- close(tmpfile_fd);
- unlink(preq->tmpfile.buf);
- return ret;
- }
@@ http.c: struct http_pack_request *new_http_pack_request(
struct http_pack_request *new_direct_http_pack_request(
const unsigned char *packed_git_hash, char *url)
@@ http.c: struct http_pack_request *new_http_pack_request(
- if (!preq->packfile) {
- error("Unable to open local file %s for pack",
- preq->tmpfile.buf);
-+ /*
-+ * MinGW's non-append O_RDWR open grants FILE_SHARE_DELETE only for an
-+ * existing file; reopen a newly created file so others may unlink it.
-+ */
-+ for (;;) {
-+ fd = open(preq->tmpfile.buf, O_RDWR);
-+ if (fd >= 0 || errno != ENOENT)
-+ break;
-+ fd = open(preq->tmpfile.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
-+ if (fd >= 0) {
-+ close(fd);
-+ continue;
-+ }
-+ if (errno != EEXIST)
-+ break;
-+ }
++ fd = open(preq->tmpfile.buf, O_RDWR | O_CREAT, 0666);
+ if (fd < 0) {
+ error_errno("unable to open local file %s for pack",
+ preq->tmpfile.buf);
- goto abort;
- }
++ goto abort;
++ }
+ prev_posn = lseek(fd, 0, SEEK_END);
+ if (prev_posn < 0) {
+ error_errno("unable to seek local file %s for pack",
+ preq->tmpfile.buf);
+ close(fd);
-+ goto abort;
-+ }
+ goto abort;
+ }
+ preq->packfile = xfdopen(fd, "w");
preq->slot = get_active_slot();
@@ http.c: struct http_pack_request *new_direct_http_pack_request(
"Resuming fetch of pack %s at byte %"PRIuMAX"\n",
## t/t5550-http-fetch-dumb.sh ##
-@@ t/t5550-http-fetch-dumb.sh: test_expect_success 'http-fetch --packfile' '
- git -C packfileclient cat-file -e "$HASH"
+@@ t/t5550-http-fetch-dumb.sh: test_expect_success 'http-fetch --packfile accepts an already complete partial'
+ git -C packfileclient-complete cat-file -e "$HASH"
'
+test_expect_success 'http-fetch --packfile resumes a partial download' '
@@ t/t5550-http-fetch-dumb.sh: test_expect_success 'http-fetch --packfile' '
+ git -C packfileclient-resume cat-file -e "$HASH"
+'
+
-+test_expect_success 'http-fetch --packfile permits unlink while indexing' '
-+ git init packfileclient-unlink &&
-+ p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
-+ ls objects/pack/pack-*.pack) &&
-+ tmpfile="packfileclient-unlink/.git/objects/pack/pack-$ARBITRARY.pack.temp" &&
-+ write_script git-unlink-index-pack <<-\EOF &&
-+ test -f "$GIT_TEST_PACK_TEMP" || exit 1
-+ rm "$GIT_TEST_PACK_TEMP" || exit 1
-+ exec git index-pack "$@"
-+ EOF
-+ test_when_finished "rm -f git-unlink-index-pack" &&
-+ PATH="$TRASH_DIRECTORY:$PATH" \
-+ GIT_TEST_PACK_TEMP="$TRASH_DIRECTORY/$tmpfile" \
-+ git -C packfileclient-unlink http-fetch --packfile="$ARBITRARY" \
-+ --index-pack-arg=unlink-index-pack \
-+ --index-pack-arg=--stdin --index-pack-arg=--keep \
-+ "$HTTPD_URL/dumb/repo_pack.git/$p" >out &&
-+ test_path_is_missing "$tmpfile" &&
-+ git -C packfileclient-unlink cat-file -e "$HASH"
-+'
-+
-+test_expect_success PIPE 'concurrent http-fetch --packfile accepts a complete partial' '
-+ git init packfileclient-concurrent &&
-+ p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
-+ ls objects/pack/pack-*.pack) &&
-+ packhash=$(basename "$p" .pack) &&
-+ packhash=${packhash#pack-} &&
-+ tmpfile="packfileclient-concurrent/.git/objects/pack/pack-$packhash.pack.temp" &&
-+ test_copy_bytes 64 <"$HTTPD_DOCUMENT_ROOT_PATH/repo_pack.git/$p" >"$tmpfile" &&
-+ mkfifo first-ready first-continue &&
-+ exec 8<>first-ready &&
-+ exec 9<>first-continue &&
-+ write_script git-wait-index-pack <<-\EOF &&
-+ echo ready >"$GIT_TEST_WAIT_READY" &&
-+ read continue <"$GIT_TEST_WAIT_CONTINUE" &&
-+ exec git index-pack "$@"
-+ EOF
-+ {
-+ (
-+ if ! PATH="$TRASH_DIRECTORY:$PATH" \
-+ GIT_TEST_WAIT_READY="$TRASH_DIRECTORY/first-ready" \
-+ GIT_TEST_WAIT_CONTINUE="$TRASH_DIRECTORY/first-continue" \
-+ GIT_TRACE_CURL="$TRASH_DIRECTORY/first.trace" \
-+ git -C packfileclient-concurrent http-fetch --packfile="$packhash" \
-+ --index-pack-arg=wait-index-pack \
-+ --index-pack-arg=--stdin --index-pack-arg=--keep \
-+ "$HTTPD_URL/dumb/repo_pack.git/$p" >first.out
-+ then
-+ echo failed >"$TRASH_DIRECTORY/first-ready" &&
-+ exit 1
-+ fi
-+ ) &
-+ first_pid=$!
-+ } &&
-+ test_when_finished "
-+ echo continue >&9
-+ kill $first_pid 2>/dev/null || :
-+ wait $first_pid 2>/dev/null || :
-+ exec 8>&-
-+ exec 9>&-
-+ rm -f first-ready first-continue git-wait-index-pack
-+ " &&
-+ read ready <&8 &&
-+ test "$ready" = ready &&
-+ GIT_TRACE_CURL="$TRASH_DIRECTORY/second.trace" \
-+ git -C packfileclient-concurrent http-fetch --packfile="$packhash" \
-+ --index-pack-arg=index-pack \
-+ --index-pack-arg=--stdin --index-pack-arg=--keep \
-+ "$HTTPD_URL/dumb/repo_pack.git/$p" >second.out &&
-+ echo continue >&9 &&
-+ wait "$first_pid" &&
-+ printf "pack\t%s\n" "$packhash" >expect &&
-+ test_cmp expect first.out &&
-+ printf "keep\t%s\n" "$packhash" >expect &&
-+ test_cmp expect second.out &&
-+ test_grep "Range: bytes=64-" first.trace &&
-+ test_grep "Range: bytes=[0-9]*-" second.trace &&
-+ test_grep "416 Requested Range Not Satisfiable" second.trace &&
-+ test_path_is_missing "$tmpfile" &&
-+ git -C packfileclient-concurrent cat-file -e "$HASH"
-+'
-+
+test_expect_success PERL,PIPE 'concurrent http-fetch --packfile cannot corrupt an overlapping download' '
+ git init packfileclient-overlap &&
+ blob=$(test-tool genrandom pack-overlap 2m |
-: ---------- > 5: 87a20ac80f http: permit unlinking partial packs on Windows
3: fee6f292cb = 6: be9e2fe273 fetch-pack: accept "pack" output for packfile URIsbase-commit: 5d2e7709234afea1b6ddb25cd4f60d3d5fb3c200
-- 2.55.0.openai.131.g83a728de1eb6