Merge branch 'tn/packfile-uri-concurrency' into seen

Concurrent downloads of packfiles via packfile URIs and dumb HTTP have
been made safer by avoiding concurrent appends to the staging file.
Opening the file in read-write mode and maintaining separate file
offsets prevents corruption while preserving resumability.  The
'fetch-pack' command has also been updated to tolerate pre-existing
'.keep' files.

Ejected tentatively out of 'seen' for CI failures.

* tn/packfile-uri-concurrency:
  fetch-pack: accept "pack" output for packfile URIs
  http: avoid concurrent appends to partial packs
  http-fetch: correct --index-pack-arg documentation
seen
Junio C Hamano 2026-07-25 14:33:24 -07:00
commit adb4fa0ccb
8 changed files with 350 additions and 46 deletions

View File

@ -48,13 +48,14 @@ 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 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.

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

--recover::
Verify that everything reachable from target is fetched. Used after

View File

@ -1854,9 +1854,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
}

for (i = 0; i < packfile_uris.nr; i++) {
bool created_keep;
int j;
struct child_process cmd = CHILD_PROCESS_INIT;
char packname[GIT_MAX_HEXSZ + 1];
char packhash[GIT_MAX_HEXSZ + 1];
const char *uri = packfile_uris.items[i].string +
the_hash_algo->hexsz + 1;

@ -1874,16 +1875,17 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
if (start_command(&cmd))
die("fetch-pack: unable to spawn http-fetch");

if (read_in_full(cmd.out, packname, 5) < 0 ||
memcmp(packname, "keep\t", 5))
die("fetch-pack: expected keep then TAB at start of http-fetch output");
if (read_in_full(cmd.out, packhash, 5) != 5 ||
(memcmp(packhash, "keep\t", 5) &&
memcmp(packhash, "pack\t", 5)))
die("fetch-pack: expected pack or keep then TAB at start of http-fetch output");
created_keep = !memcmp(packhash, "keep\t", 5);

if (read_in_full(cmd.out, packname,
the_hash_algo->hexsz + 1) < 0 ||
packname[the_hash_algo->hexsz] != '\n')
die("fetch-pack: expected hash then LF at end of http-fetch output");

packname[the_hash_algo->hexsz] = '\0';
if (read_in_full(cmd.out, packhash,
the_hash_algo->hexsz + 1) != the_hash_algo->hexsz + 1 ||
packhash[the_hash_algo->hexsz] != '\n')
die("fetch-pack: expected hash then LF in http-fetch output");
packhash[the_hash_algo->hexsz] = '\0';

parse_gitmodules_oids(cmd.out, &fsck_options.gitmodules_found);

@ -1892,16 +1894,17 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
if (finish_command(&cmd))
die("fetch-pack: unable to finish http-fetch");

if (memcmp(packfile_uris.items[i].string, packname,
if (memcmp(packfile_uris.items[i].string, packhash,
the_hash_algo->hexsz))
die("fetch-pack: pack downloaded from %s does not match expected hash %.*s",
uri, (int) the_hash_algo->hexsz,
packfile_uris.items[i].string);

string_list_append_nodup(pack_lockfiles,
xstrfmt("%s/pack/pack-%s.keep",
repo_get_object_directory(the_repository),
packname));
if (created_keep)
string_list_append_nodup(pack_lockfiles,
xstrfmt("%s/pack/pack-%s.keep",
repo_get_object_directory(the_repository),
packhash));
}
string_list_clear(&packfile_uris, 0);
strvec_clear(&index_pack_args);

View File

@ -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) {
struct url_info url;
char *nurl = url_normalize(preq->url, &url);
if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {
@ -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");

fetch_single_packfile(&packfile_hash, argv[arg],
index_pack_args.v);
@ -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);

View File

@ -595,7 +595,8 @@ 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 {

View File

@ -451,7 +451,8 @@ 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;

56
http.c
View File

@ -2690,10 +2690,13 @@ int finish_http_pack_request(struct http_pack_request *preq)
int tmpfile_fd;
int ret = 0;

/* Another downloader may unlink the staging path while we index it. */
tmpfile_fd = xdup(fileno(preq->packfile));
fclose(preq->packfile);
preq->packfile = NULL;

tmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);
if (lseek(tmpfile_fd, 0, SEEK_SET) < 0)
die_errno("unable to seek local file %s for pack",
preq->tmpfile.buf);

ip.git_cmd = 1;
ip.in = tmpfile_fd;
@ -2706,13 +2709,8 @@ 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;
}
@ -2740,22 +2738,45 @@ 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)
{
off_t prev_posn = 0;
off_t prev_posn;
struct http_pack_request *preq;
int fd;

CALLOC_ARRAY(preq, 1);
strbuf_init(&preq->tmpfile, 0);

preq->url = url;

odb_pack_name(the_repository, &preq->tmpfile, packed_git_hash, "pack");
strbuf_addstr(&preq->tmpfile, ".temp");
preq->packfile = fopen(preq->tmpfile.buf, "a");
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;
}
if (fd < 0) {
error_errno("unable to open local file %s for pack",
preq->tmpfile.buf);
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;
}
preq->packfile = xfdopen(fd, "w");

preq->slot = get_active_slot();
preq->headers = object_request_headers();
@ -2764,12 +2785,7 @@ struct http_pack_request *new_direct_http_pack_request(
curl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);
curl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER, preq->headers);

/*
* If there is data present from a previous transfer attempt,
* resume where it left off
*/
prev_posn = ftello(preq->packfile);
if (prev_posn>0) {
if (prev_posn > 0) {
if (http_is_verbose)
fprintf(stderr,
"Resuming fetch of pack %s at byte %"PRIuMAX"\n",

View File

@ -293,6 +293,256 @@ test_expect_success 'http-fetch --packfile' '
git -C packfileclient cat-file -e "$HASH"
'

test_expect_success 'http-fetch --packfile resumes a partial download' '
git init packfileclient-resume &&
p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
ls objects/pack/pack-*.pack) &&
tmpfile="packfileclient-resume/.git/objects/pack/pack-$ARBITRARY.pack.temp" &&
test_copy_bytes 64 <"$HTTPD_DOCUMENT_ROOT_PATH/repo_pack.git/$p" >"$tmpfile" &&
GIT_TRACE_CURL="$TRASH_DIRECTORY/resume.trace" \
git -C packfileclient-resume http-fetch --packfile="$ARBITRARY" \
--index-pack-arg=index-pack --index-pack-arg=--stdin \
--index-pack-arg=--keep \
"$HTTPD_URL/dumb/repo_pack.git/$p" >out &&
test_grep "Range: bytes=64-" resume.trace &&
test_path_is_missing "$tmpfile" &&
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 |
git -C "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git \
hash-object -w --stdin) &&
packhash=$(printf "%s\n" "$blob" |
git -C "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git \
pack-objects "$TRASH_DIRECTORY/overlap-pack") &&
pack="$TRASH_DIRECTORY/overlap-pack-$packhash.pack" &&
tmpfile="packfileclient-overlap/.git/objects/pack/pack-$packhash.pack.temp" &&
mkfifo server-ready first-ready &&
exec 7<>server-ready &&
exec 8<>first-ready &&
write_script slow-pack-server "$PERL_PATH" <<-\EOF &&
use strict;
use warnings;
use IO::Socket::INET;

my ($packfile, $server_ready, $first_ready) = @ARGV;
my $completed = 0;
END {
if (!$completed) {
signal_ready($server_ready, "failed");
signal_ready($first_ready, "failed");
}
}

$SIG{ALRM} = sub { die "timed out serving concurrent pack requests\n" };
alarm 60;

open(my $in, "<:raw", $packfile) or die "open $packfile: $!";
my $pack = do { local $/; <$in> };
close($in) or die "close $packfile: $!";
my $server = IO::Socket::INET->new(LocalAddr => "127.0.0.1",
LocalPort => 0, Proto => "tcp", Listen => 2, ReuseAddr => 1)
or die "listen: $!";

sub signal_ready {
my ($file, $value) = @_;
open(my $out, ">", $file) or die "open $file: $!";
print $out "$value\n" or die "write $file: $!";
close($out) or die "close $file: $!";
}

sub write_all {
my ($out, $data) = @_;
my $offset = 0;
while ($offset < length($data)) {
my $written = syswrite($out, $data,
length($data) - $offset, $offset);
defined($written) && $written or die "write response: $!";
$offset += $written;
}
}

sub start_response {
my $out = $server->accept() or die "accept: $!";
<$out> or die "read request: $!";
my $start = 0;
while (<$out>) {
last if /^\r?\n$/;
$start = $1 if /^Range: bytes=(\d+)-/i;
}
$start < length($pack) or die "invalid range $start";
my $length = length($pack) - $start;
my $middle = int($length / 2);
my $status = $start ? "206 Partial Content" : "200 OK";
my $headers = "HTTP/1.1 $status\r\n" .
"Content-Length: $length\r\n" .
($start ? "Content-Range: bytes $start-" .
(length($pack) - 1) . "/" . length($pack) . "\r\n" : "") .
"Connection: close\r\n\r\n";
write_all($out, $headers);
write_all($out, substr($pack, $start, $middle));
return ($out, $start + $middle);
}

signal_ready($server_ready, $server->sockport());
my ($first, $first_pos) = start_response();
signal_ready($first_ready, "ready");
my ($second, $second_pos) = start_response();
write_all($first, substr($pack, $first_pos));
write_all($second, substr($pack, $second_pos));
close($first) or die "close first response: $!";
close($second) or die "close second response: $!";
$completed = 1;
alarm 0;
EOF
{
"$TRASH_DIRECTORY/slow-pack-server" "$pack" \
"$TRASH_DIRECTORY/server-ready" \
"$TRASH_DIRECTORY/first-ready" >server.log 2>&1 &
server_pid=$!
} &&
test_when_finished "
kill $server_pid 2>/dev/null || :
wait $server_pid 2>/dev/null || :
exec 7>&-
exec 8>&-
rm -f server-ready first-ready slow-pack-server
" &&
read port <&7 &&
url="http://127.0.0.1:$port/pack" &&
{
(
if ! GIT_TRACE_CURL="$TRASH_DIRECTORY/overlap-first.trace" \
GIT_TRACE_CURL_NO_DATA=1 \
git -C packfileclient-overlap http-fetch --packfile="$packhash" \
--index-pack-arg=index-pack \
--index-pack-arg=--stdin --index-pack-arg=--keep \
"$url" >first.out
then
echo failed >"$TRASH_DIRECTORY/first-ready" &&
exit 1
fi
) &
first_pid=$!
} &&
test_when_finished "
kill $first_pid 2>/dev/null || :
wait $first_pid 2>/dev/null || :
" &&
read ready <&8 &&
test "$ready" = ready &&
test_path_is_file "$tmpfile" &&
test -s "$tmpfile" &&
{
GIT_TRACE_CURL="$TRASH_DIRECTORY/overlap-second.trace" \
GIT_TRACE_CURL_NO_DATA=1 \
git -C packfileclient-overlap http-fetch --packfile="$packhash" \
--index-pack-arg=index-pack \
--index-pack-arg=--stdin --index-pack-arg=--keep \
"$url" >second.out &
second_pid=$!
} &&
test_when_finished "
kill $second_pid 2>/dev/null || :
wait $second_pid 2>/dev/null || :
" &&
wait "$second_pid" &&
wait "$first_pid" &&
wait "$server_pid" &&
test_grep "HTTP/[0-9.]* 200" overlap-first.trace &&
test_grep "Range: bytes=[1-9][0-9]*-" overlap-second.trace &&
test_grep "HTTP/[0-9.]* 206" overlap-second.trace &&
printf "keep\t%s\npack\t%s\n" "$packhash" "$packhash" | sort >expect &&
sort first.out second.out >actual &&
test_cmp expect actual &&
test_path_is_missing "$tmpfile" &&
git -C packfileclient-overlap cat-file -e "$blob"
'

test_expect_success 'fetch notices corrupt pack' '
cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git &&
(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git &&

View File

@ -1291,6 +1291,37 @@ test_expect_success 'packfile URIs with fetch instead of clone' '
fetch "$HTTPD_URL/smart/http_parent"
'

test_expect_success 'packfile URI preserves an existing keep file' '
P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
rm -rf "$P" http_child keep.expect &&

git init "$P" &&
git -C "$P" config uploadpack.allowsidebandall true &&

echo my-blob >"$P/my-blob" &&
git -C "$P" add my-blob &&
git -C "$P" commit -m x &&
configure_exclusion "$P" my-blob >h &&

git init http_child &&
packhash=$(cat packh) &&
keep="http_child/.git/objects/pack/pack-$packhash.keep" &&
echo pre-existing >"$keep" &&
cp "$keep" keep.expect &&

GIT_TEST_SIDEBAND_ALL=1 \
git -C http_child -c protocol.version=2 \
-c fetch.uriprotocols=http,https \
fetch "$HTTPD_URL/smart/http_parent" &&

test_path_is_file \
"http_child/.git/objects/pack/pack-$packhash.pack" &&
test_path_is_file \
"http_child/.git/objects/pack/pack-$packhash.idx" &&
test_cmp keep.expect "$keep" &&
git -C http_child cat-file -e "$(cat h)"
'

test_expect_success 'fetching with valid packfile URI but invalid hash fails' '
P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
rm -rf "$P" http_child log &&