* Re: [PATCH RFC v3 2/2] Move libgit.a sources into separate "lib/" directory
From: Junio C Hamano @ 2026-07-20 22:43 UTC (permalink / raw)
To: brian m. carlson
Cc: Patrick Steinhardt, git, Elijah Newren, Derrick Stolee,
SZEDER Gábor, Johannes Schindelin, Phillip Wood
In-Reply-To: <al6Yz_QMlyU1GETv@fruit.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> I would very much welcome better rename support and I'm sure the
> community would as well. If we can incentivize ourselves to step up and
> implement that, I'm all for it.
I would welcome such an effort. It is, however, a different story
to move things simply because we want to move them, without a
concrete need or strategy to do so.
In any case, the root level of the 'lib/' directory introduced by
the 'ps/libgit-in-subdir' topic is full of source files, with only a
small number of focused subdirectories like 'odb/', 'refs/', and
'ewah/' mixed in to house specific subsystems. This merely shifts
the clutter one level down without resolving it.
I would rather see a structure where each subsystem-like group
carves out its own directory.
I do not particularly care whether such a directory lives at the
root level or inside 'lib/'. But if we were to establish a sensible
grouping, I suspect we would not need a 'lib/' directory solely to
house the 'refs/' and 'odb/' subdirectories. Instead, it would be
sufficiently clean to have 'refs/', 'odb/', and other subsystem
directories directly under the root level.
I do not think we want to do this in a single large change. If we
were to move everything to 'lib/' only to then need to further group
them into subdirectories of 'lib/', it would subject us to multiple
rounds of disruption. I suspect it would be far less disruptive if
we migrated one subsystem at a time, directly to a new directory
immediately below the root level.
^ permalink raw reply
* [PATCH v2 2/2] fetch-pack: accept "pack" output for packfile URIs
From: Ted Nyman @ 2026-07-20 22:34 UTC (permalink / raw)
To: git; +Cc: gitster, me, peff, ps, karthik.188, sandals, avarab
In-Reply-To: <cover.1784582665.git.tnyman@openai.com>
When index-pack finds an existing keep file it reports pack rather than
keep. Accept either result from http-fetch, and only register a keep
lockfile when this fetch created it.
Read the pack/keep prefix and hash without consuming any following fsck
output, validate the reported pack hash against the advertised hash, and
exercise a packfile URI fetch with a pre-existing keep file.
Signed-off-by: Ted Nyman <tnyman@openai.com>
---
fetch-pack.c | 33 ++++++++++++++++++---------------
t/t5702-protocol-v2.sh | 31 +++++++++++++++++++++++++++++++
2 files changed, 49 insertions(+), 15 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 120e01f3cf..509b91527b 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1887,9 +1887,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;
@@ -1907,16 +1908,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);
@@ -1925,16 +1927,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);
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 9f6cf4142d..1861eb7d7c 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -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 &&
--
2.55.0.125.g9b41d4ddb3
^ permalink raw reply related
* [PATCH v2 1/2] http: avoid concurrent appends to partial packs
From: Ted Nyman @ 2026-07-20 22:33 UTC (permalink / raw)
To: git; +Cc: gitster, me, peff, ps, karthik.188, sandals, avarab
In-Reply-To: <cover.1784582665.git.tnyman@openai.com>
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.
Signed-off-by: Ted Nyman <tnyman@openai.com>
---
Documentation/git-http-fetch.adoc | 13 +-
http-fetch.c | 7 +-
http-push.c | 3 +-
http-walker.c | 3 +-
http.c | 53 ++++---
t/t5550-http-fetch-dumb.sh | 223 ++++++++++++++++++++++++++++++
6 files changed, 271 insertions(+), 31 deletions(-)
diff --git a/Documentation/git-http-fetch.adoc b/Documentation/git-http-fetch.adoc
index 2200f073c4..60ca91cf3a 100644
--- a/Documentation/git-http-fetch.adoc
+++ b/Documentation/git-http-fetch.adoc
@@ -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
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) {
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);
diff --git a/http-push.c b/http-push.c
index 3c23cbba27..03dc8102a1 100644
--- a/http-push.c
+++ b/http-push.c
@@ -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 {
diff --git a/http-walker.c b/http-walker.c
index b58a3b2a92..abafca84d6 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -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;
diff --git a/http.c b/http.c
index b4e7b8d00b..9b9f4efe28 100644
--- a/http.c
+++ b/http.c
@@ -2688,10 +2688,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;
@@ -2704,13 +2707,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;
}
@@ -2738,22 +2736,42 @@ 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);
+ /* Reopen without O_CREAT so MinGW permits another writer to 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();
@@ -2762,12 +2780,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",
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index b0080bf204..7acae96a96 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -293,6 +293,229 @@ 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 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 "HTTP/[0-9.]* 416" 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;
+ 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: $!";
+ EOF
+ {
+ (
+ if ! "$TRASH_DIRECTORY/slow-pack-server" "$pack" \
+ "$TRASH_DIRECTORY/server-ready" \
+ "$TRASH_DIRECTORY/first-ready"
+ then
+ echo failed >"$TRASH_DIRECTORY/server-ready" &&
+ echo failed >"$TRASH_DIRECTORY/first-ready" &&
+ exit 1
+ fi
+ ) >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 "$server_pid" &&
+ wait "$first_pid" &&
+ wait "$second_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 &&
--
2.55.0.125.g9b41d4ddb3
^ permalink raw reply related
* [PATCH v2 0/2] packfile URIs: support concurrent downloads
From: Ted Nyman @ 2026-07-20 22:33 UTC (permalink / raw)
To: git; +Cc: gitster, me, peff, ps, karthik.188, sandals, avarab
In-Reply-To: <cover.1783982021.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. Two Git processes fetching the same pack into one object
database can append to that file concurrently, which can corrupt the
temporary pack.
Following Peff's suggestion in the v1 discussion, the first patch keeps
the predictable temporary name but opens the file without append mode.
Each downloader keeps its own file offset, so overlapping responses for
the same pack write the same bytes at the same offsets. This preserves
resumable downloads and protects both packfile URI and ordinary dumb
HTTP requests. Concurrent downloaders may still transfer the same
suffix; this series avoids adding cross-process coordination and a
stale-owner policy.
A second downloader can also find that the partial pack has completed
and request a range starting at EOF. Servers may respond with HTTP 416
in that case. Treat that response as a completed download and let
index-pack validate the pack. Keep the open descriptor for indexing so
another downloader can safely remove the temporary path.
The second 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 200 and 206 responses, and a pre-existing .keep file.
Changes since v1:
* Preserve resumability by removing append mode instead of using a
unique temporary file for each download.
* Handle the EOF-range/416 and concurrent-unlink cases, including the
Windows sharing behavior.
* Add a deterministic overlapping-download regression test.
* Read the pack/keep prefix and hash without consuming later fsck
output, as suggested during review.
* Correct the stale --index-pack-args documentation and error text;
the repeatable --index-pack-arg option is already supported.
The v1 discussion is at:
https://lore.kernel.org/git/cover.1783982021.git.tnyman@openai.com/
Ted Nyman (2):
http: avoid concurrent appends to partial packs
fetch-pack: accept "pack" output for packfile URIs
Documentation/git-http-fetch.adoc | 13 +-
fetch-pack.c | 33 +++--
http-fetch.c | 7 +-
http-push.c | 3 +-
http-walker.c | 3 +-
http.c | 53 ++++---
t/t5550-http-fetch-dumb.sh | 223 ++++++++++++++++++++++++++++++
t/t5702-protocol-v2.sh | 31 +++++
8 files changed, 320 insertions(+), 46 deletions(-)
Range-diff against v1:
1: 32eb9b0831 ! 1: 160a9b9fd0 http: use unique tempfiles for packfile URI downloads
@@ Metadata
Author: Ted Nyman <tnyman@openai.com>
## Commit message ##
- http: use unique tempfiles for packfile URI downloads
+ http: avoid concurrent appends to partial packs
- 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.
+ 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.
- 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.
+ 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.
- 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.
+ 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.
Signed-off-by: Ted Nyman <tnyman@openai.com>
- Signed-off-by: Junio C Hamano <gitster@pobox.com>
## Documentation/git-http-fetch.adoc ##
@@ Documentation/git-http-fetch.adoc: commit-id::
@@ Documentation/git-http-fetch.adoc: commit-id::
- 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.
++ 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.
- --index-pack-args=<args>::
- For internal use only. The command to run on the contents of the
+ --recover::
+ Verify that everything reachable from target is fetched. Used after
- ## http.c ##
-@@ http.c: int http_get_info_packs(const char *base_url, struct packfile_list *packs)
+ ## http-fetch.c ##
+@@ http-fetch.c: static void fetch_single_packfile(struct object_id *packfile_hash,
- void release_http_pack_request(struct http_pack_request *preq)
- {
-- if (preq->packfile) {
-+ if (preq->tempfile) {
-+ delete_tempfile(&preq->tempfile);
-+ preq->packfile = NULL;
-+ } else if (preq->packfile) {
- fclose(preq->packfile);
- preq->packfile = NULL;
+ 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-fetch.c: 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);
+@@ http-fetch.c: 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);
+
+ ## 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;
int ret = 0;
-- fclose(preq->packfile);
-+ if (preq->tempfile)
-+ close_tempfile_gently(preq->tempfile);
-+ else
-+ fclose(preq->packfile);
++ /* 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);
- tmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);
+ 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;
- cleanup:
- close(tmpfile_fd);
-- unlink(preq->tmpfile.buf);
-+ if (preq->tempfile)
-+ delete_tempfile(&preq->tempfile);
-+ else
-+ unlink(preq->tmpfile.buf);
+- if (run_command(&ip)) {
++ if (run_command(&ip))
+ ret = -1;
+- goto cleanup;
+- }
+-
+-cleanup:
+- close(tmpfile_fd);
+ unlink(preq->tmpfile.buf);
return ret;
}
-
-@@ http.c: void http_install_packfile(struct packed_git *p,
- packfile_store_add_pack(files->packed, p);
- }
-
--struct http_pack_request *new_http_pack_request(
-- const unsigned char *packed_git_hash, const char *base_url) {
--
-- struct strbuf buf = STRBUF_INIT;
--
-- end_url_with_slash(&buf, base_url);
-- strbuf_addf(&buf, "objects/pack/pack-%s.pack",
-- hash_to_hex(packed_git_hash));
-- return new_direct_http_pack_request(packed_git_hash,
-- strbuf_detach(&buf, NULL));
--}
--
--struct http_pack_request *new_direct_http_pack_request(
-- const unsigned char *packed_git_hash, char *url)
-+static struct http_pack_request *new_http_pack_request_for_url(
-+ const unsigned char *packed_git_hash, char *url, int resumable)
+@@ 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)
{
- off_t prev_posn = 0;
+- off_t prev_posn = 0;
++ off_t prev_posn;
struct http_pack_request *preq;
-@@ http.c: struct http_pack_request *new_direct_http_pack_request(
++ 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");
+ odb_pack_name(the_repository, &preq->tmpfile, packed_git_hash, "pack");
+ strbuf_addstr(&preq->tmpfile, ".temp");
- preq->packfile = fopen(preq->tmpfile.buf, "a");
-+ if (resumable) {
-+ odb_pack_name(the_repository, &preq->tmpfile,
-+ packed_git_hash, "pack");
-+ strbuf_addstr(&preq->tmpfile, ".temp");
-+ preq->packfile = fopen(preq->tmpfile.buf, "a");
-+ } else {
-+ strbuf_addf(&preq->tmpfile, "%s/pack/tmp_pack_XXXXXX",
-+ repo_get_object_directory(the_repository));
-+ preq->tempfile = mks_tempfile_m(preq->tmpfile.buf, 0444);
-+ if (preq->tempfile) {
-+ strbuf_reset(&preq->tmpfile);
-+ strbuf_addstr(&preq->tmpfile,
-+ get_tempfile_path(preq->tempfile));
-+ preq->packfile = fdopen_tempfile(preq->tempfile, "w");
+- if (!preq->packfile) {
+- error("Unable to open local file %s for pack",
+- preq->tmpfile.buf);
++ /* Reopen without O_CREAT so MinGW permits another writer to 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;
+ }
- if (!preq->packfile) {
- error("Unable to open local file %s for pack",
- preq->tmpfile.buf);
++ 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();
@@ http.c: struct http_pack_request *new_direct_http_pack_request(
- * If there is data present from a previous transfer attempt,
- * resume where it left off
- */
+ 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 (resumable)
-+ prev_posn = ftello(preq->packfile);
+ if (prev_posn > 0) {
if (http_is_verbose)
fprintf(stderr,
"Resuming fetch of pack %s at byte %"PRIuMAX"\n",
-@@ http.c: struct http_pack_request *new_direct_http_pack_request(
- return preq;
-
- abort:
-- strbuf_release(&preq->tmpfile);
-- free(preq->url);
-- free(preq);
-+ release_http_pack_request(preq);
- return NULL;
- }
-
-+struct http_pack_request *new_http_pack_request(
-+ const unsigned char *packed_git_hash, const char *base_url)
-+{
-+ struct strbuf buf = STRBUF_INIT;
-+
-+ end_url_with_slash(&buf, base_url);
-+ strbuf_addf(&buf, "objects/pack/pack-%s.pack",
-+ hash_to_hex(packed_git_hash));
-+ return new_http_pack_request_for_url(packed_git_hash,
-+ strbuf_detach(&buf, NULL), 1);
-+}
-+
-+struct http_pack_request *new_direct_http_pack_request(
-+ const unsigned char *packed_git_hash, char *url)
-+{
-+ return new_http_pack_request_for_url(packed_git_hash, url, 0);
-+}
-+
- /* Helpers for fetching objects (loose) */
- static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
- void *data)
-
- ## http.h ##
-@@ http.h: struct http_pack_request {
-
- FILE *packfile;
- struct strbuf tmpfile;
-+ struct tempfile *tempfile;
- struct active_request_slot *slot;
- struct curl_slist *headers;
- };
## 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"
'
-+test_expect_success PIPE 'concurrent http-fetch --packfile' '
++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 PIPE 'concurrent http-fetch --packfile accepts a complete partial' '
+ git init packfileclient-concurrent &&
-+ HASH=$(git -C "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git rev-parse HEAD) &&
+ 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 &&
@@ t/t5550-http-fetch-dumb.sh: test_expect_success 'http-fetch --packfile' '
+ read continue <"$GIT_TEST_WAIT_CONTINUE" &&
+ exec git index-pack "$@"
+ EOF
-+
-+ # Hold the first download before it is indexed, so that the second
-+ # download installs the pack first.
+ {
+ (
+ if ! PATH="$TRASH_DIRECTORY:$PATH" \
+ GIT_TEST_WAIT_READY="$TRASH_DIRECTORY/first-ready" \
+ GIT_TEST_WAIT_CONTINUE="$TRASH_DIRECTORY/first-continue" \
-+ git -C packfileclient-concurrent http-fetch \
-+ --packfile="$packhash" \
++ 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 \
++ --index-pack-arg=--stdin --index-pack-arg=--keep \
+ "$HTTPD_URL/dumb/repo_pack.git/$p" >first.out
+ then
+ echo failed >"$TRASH_DIRECTORY/first-ready" &&
@@ t/t5550-http-fetch-dumb.sh: test_expect_success 'http-fetch --packfile' '
+ } &&
+ 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 -C packfileclient-concurrent http-fetch \
-+ --packfile="$packhash" \
++ 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 \
++ --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_path_is_missing \
-+ "packfileclient-concurrent/.git/objects/pack/pack-$packhash.pack.temp" &&
-+ find packfileclient-concurrent/.git/objects/pack \
-+ -name "tmp_pack_*" -print >tmpfiles &&
-+ test_must_be_empty tmpfiles &&
++ test_grep "Range: bytes=64-" first.trace &&
++ test_grep "Range: bytes=[0-9]*-" second.trace &&
++ test_grep "HTTP/[0-9.]* 416" 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;
++ 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: $!";
++ EOF
++ {
++ (
++ if ! "$TRASH_DIRECTORY/slow-pack-server" "$pack" \
++ "$TRASH_DIRECTORY/server-ready" \
++ "$TRASH_DIRECTORY/first-ready"
++ then
++ echo failed >"$TRASH_DIRECTORY/server-ready" &&
++ echo failed >"$TRASH_DIRECTORY/first-ready" &&
++ exit 1
++ fi
++ ) >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 "$server_pid" &&
++ wait "$first_pid" &&
++ wait "$second_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 &&
-@@ t/t5550-http-fetch-dumb.sh: test_expect_success 'http-fetch --packfile with corrupt pack' '
- git init packfileclient &&
- p=$(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad1.git && ls objects/pack/pack-*.pack) &&
- test_must_fail git -C packfileclient http-fetch --packfile \
-- "$HTTPD_URL"/dumb/repo_bad1.git/$p
-+ "$HTTPD_URL"/dumb/repo_bad1.git/$p &&
-+ find packfileclient/.git/objects/pack -name "tmp_pack_*" -print >tmpfiles &&
-+ test_must_be_empty tmpfiles
- '
-
- test_expect_success 'fetch notices corrupt idx' '
2: e73de423f0 ! 2: 9b41d4ddb3 fetch-pack: accept "pack" output for packfile URIs
@@ Metadata
## Commit message ##
fetch-pack: accept "pack" output for packfile URIs
- When "index-pack --keep" creates a .keep file, it reports
- "keep<TAB><hash>". If the file already exists, index-pack leaves it
- untouched and reports "pack<TAB><hash>" instead.
+ When index-pack finds an existing keep file it reports pack rather than
+ keep. Accept either result from http-fetch, and only register a keep
+ lockfile when this fetch created it.
- Since dd4b732df7 (upload-pack: send part of packfile response as uri,
- 2020-06-10), fetch-pack has accepted only the "keep" form for packs
- downloaded through packfile URIs. A concurrent fetch can install the
- same pack and create its .keep file before another process reaches
- index-pack. The latter process then fails even though index-pack
- completed successfully.
-
- Accept both successful forms. Add a path to pack_lockfiles only for the
- "keep" form, so cleanup removes only a keep file created by the current
- process and preserves a pre-existing one.
-
- Add a regression test which pre-creates a keep file and verifies that a
- fetch succeeds without changing it.
+ Read the pack/keep prefix and hash without consuming any following fsck
+ output, validate the reported pack hash against the advertised hash, and
+ exercise a packfile URI fetch with a pre-existing keep file.
Signed-off-by: Ted Nyman <tnyman@openai.com>
- Signed-off-by: Junio C Hamano <gitster@pobox.com>
## fetch-pack.c ##
@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
}
for (i = 0; i < packfile_uris.nr; i++) {
-+ int created_keep = 0;
++ bool created_keep;
int j;
struct child_process cmd = CHILD_PROCESS_INIT;
- char packname[GIT_MAX_HEXSZ + 1];
-+ char packname[GIT_MAX_HEXSZ + 6];
-+ const char *packhash;
-+ const int packname_len = the_hash_algo->hexsz + 6;
++ char packhash[GIT_MAX_HEXSZ + 1];
const char *uri = packfile_uris.items[i].string +
the_hash_algo->hexsz + 1;
@@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
- 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, packname, packname_len) != packname_len ||
-+ packname[packname_len - 1] != '\n')
-+ die("fetch-pack: expected pack or keep, TAB, hash, "
-+ "then LF in http-fetch output");
-+ packname[packname_len - 1] = '\0';
-+ if (skip_prefix(packname, "keep\t", &packhash))
-+ created_keep = 1;
-+ else if (!skip_prefix(packname, "pack\t", &packhash))
-+ die("fetch-pack: expected pack or keep, TAB, hash, "
-+ "then LF in http-fetch output");
++ 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);
base-commit: f60db8d575adb79761d363e026fb49bddf330c73
--
2.55.0.125.g9b41d4ddb3
^ permalink raw reply
* [PATCH 2/2] stash: avoid sparse-index expansion for in-cone paths
From: tnyman @ 2026-07-20 22:31 UTC (permalink / raw)
To: git; +Cc: Ted Nyman, Derrick Stolee, Taylor Blau, Jeff King, Victoria Dye
In-Reply-To: <20260720223118.62821-4-tnyman@openai.com>
From: Ted Nyman <tnyman@openai.com>
`git stash push -- <pathspec>` expands a sparse index before checking
whether the pathspec matches any tracked paths. This is unnecessary
when the pathspec is wholly inside the sparse-checkout cone and makes
a path-limited stash proportional to the size of the full index.
Use `pathspec_needs_expanded_index()` to expand only when a pathspec
can match part of a sparse-directory entry, as `git rm` and `git
reset` already do. Keep the full-index behavior for pathspecs that
need it.
Add compatibility coverage for literal, prefixed, wildcard, file,
multiple, staged, and missing pathspecs. Add the corresponding
path-limited stash case to p2000.
On a cone-mode repository with 349,525 tracked paths and 49 sparse
index entries, the best of three runs changed from 18.87s to 0.06s.
Trace2 reported four index expansions before this change and none
after it.
Signed-off-by: Ted Nyman <tnyman@openai.com>
---
builtin/stash.c | 4 +-
t/perf/p2000-sparse-operations.sh | 1 +
t/t1092-sparse-checkout-compatibility.sh | 55 ++++++++++++++++++++++++
3 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index c4809f299a313b..72c52571f8c06c 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1702,8 +1702,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
if (!include_untracked && ps->nr) {
char *ps_matched = xcalloc(ps->nr, 1);
- /* TODO: audit for interaction with sparse-index. */
- ensure_full_index(the_repository->index);
+ if (pathspec_needs_expanded_index(the_repository->index, ps))
+ ensure_full_index(the_repository->index);
for (size_t i = 0; i < the_repository->index->cache_nr; i++)
ce_path_match(the_repository->index, the_repository->index->cache[i], ps,
ps_matched);
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index aadf22bc2f0bb2..548a61cd9064bc 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -108,6 +108,7 @@ test_perf_on_all () {
test_perf_on_all git status
test_perf_on_all 'git stash && git stash pop'
+test_perf_on_all "git stash push -- $SPARSE_CONE/a && git stash pop"
test_perf_on_all 'echo >>new && git stash -u && git stash pop'
test_perf_on_all git add -A
test_perf_on_all git add .
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index d0b42371663f9d..4140c4d8ef2436 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -1598,6 +1598,61 @@ test_expect_success 'sparse-index is not expanded: stash' '
ensure_not_expanded stash pop
'
+test_expect_success 'sparse-index is not expanded: stash in-cone pathspec' '
+ init_repos &&
+
+ echo unrelated >>sparse-index/deep/e &&
+ echo literal >>sparse-index/deep/a &&
+ ensure_not_expanded stash push -- deep/a &&
+ test_grep ! literal sparse-index/deep/a &&
+ test_grep unrelated sparse-index/deep/e &&
+ ensure_not_expanded stash pop &&
+ test_grep literal sparse-index/deep/a &&
+
+ echo prefixed >>sparse-index/deep/a &&
+ ensure_not_expanded -C deep stash push -- a &&
+ test_grep ! prefixed sparse-index/deep/a &&
+ test_grep unrelated sparse-index/deep/e &&
+ ensure_not_expanded stash pop &&
+ test_grep prefixed sparse-index/deep/a &&
+
+ echo wildcard >>sparse-index/deep/a &&
+ ensure_not_expanded stash push -- "deep/a*" &&
+ test_grep ! wildcard sparse-index/deep/a &&
+ test_grep unrelated sparse-index/deep/e &&
+ ensure_not_expanded stash pop &&
+ test_grep wildcard sparse-index/deep/a &&
+
+ echo pathspec-file >>sparse-index/deep/a &&
+ echo deep/a >pathspec-file &&
+ ensure_not_expanded stash push --pathspec-from-file=../pathspec-file &&
+ test_grep ! pathspec-file sparse-index/deep/a &&
+ test_grep unrelated sparse-index/deep/e &&
+ ensure_not_expanded stash pop &&
+ test_grep pathspec-file sparse-index/deep/a &&
+
+ echo multiple-a >>sparse-index/deep/a &&
+ echo multiple-e >>sparse-index/deep/e &&
+ ensure_not_expanded stash push -- deep/a deep/e &&
+ test_grep ! multiple-a sparse-index/deep/a &&
+ test_grep ! multiple-e sparse-index/deep/e &&
+ ensure_not_expanded stash pop &&
+ test_grep multiple-a sparse-index/deep/a &&
+ test_grep multiple-e sparse-index/deep/e &&
+
+ echo staged >>sparse-index/deep/a &&
+ git -C sparse-index add deep/a &&
+ ensure_not_expanded stash push --staged -- deep/a &&
+ test_grep ! staged sparse-index/deep/a &&
+ test_grep unrelated sparse-index/deep/e &&
+ ensure_not_expanded stash pop --index &&
+ test_grep staged sparse-index/deep/a &&
+ test_must_fail git -C sparse-index diff --cached --quiet -- deep/a &&
+
+ ensure_not_expanded ! stash push -- deep/does-not-exist &&
+ test_grep "did not match any file" sparse-index-error
+'
+
test_expect_success 'describe tested on all' '
init_repos &&
^ permalink raw reply related
* [PATCH 1/2] pathspec: use match for sparse-index expansion checks
From: tnyman @ 2026-07-20 22:31 UTC (permalink / raw)
To: git; +Cc: Ted Nyman, Derrick Stolee, Taylor Blau, Jeff King, Victoria Dye
In-Reply-To: <20260720223118.62821-4-tnyman@openai.com>
From: Ted Nyman <tnyman@openai.com>
The pathspec parser computes `len` and `nowildcard_len` from
`item.match`, which includes any prefix added when a command is run
from a subdirectory. `item.original` can still contain the shorter,
unprefixed argument.
Using `item.original + item.nowildcard_len` in
`pathspec_needs_expanded_index()` can therefore read past the end of
the allocation. AddressSanitizer reports a heap-buffer-overflow for
prefixed wildcard pathspecs passed to `git rm` and `git reset` with a
sparse index.
The mismatch dates back to 4d1cfc1351 ("reset: make --mixed
sparse-aware", 2021-11-29), which introduced the helper using
`item.original`. b29ad38322 ("pathspec.h: move
pathspec_needs_expanded_index() from reset.c to here", 2022-08-07)
later moved it to `pathspec.c` and preserved the affected comparisons.
Use `item.match` consistently when checking whether a pathspec can
match a sparse-directory entry. Add coverage for prefixed wildcard
pathspecs so both commands keep the index sparse.
Signed-off-by: Ted Nyman <tnyman@openai.com>
---
pathspec.c | 12 ++++++------
t/t1092-sparse-checkout-compatibility.sh | 7 +++++++
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/pathspec.c b/pathspec.c
index f78b22709ccb67..281858f21f9c59 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -847,9 +847,9 @@ int pathspec_needs_expanded_index(struct index_state *istate,
* - not-in-cone/bar*: may need expanded index
* - **.c: may need expanded index
*/
- if (strspn(item.original + item.nowildcard_len, "*") ==
+ if (strspn(item.match + item.nowildcard_len, "*") ==
(unsigned int)(item.len - item.nowildcard_len) &&
- path_in_cone_mode_sparse_checkout(item.original, istate))
+ path_in_cone_mode_sparse_checkout(item.match, istate))
continue;
for (pos = 0; pos < istate->cache_nr; pos++) {
@@ -865,7 +865,7 @@ int pathspec_needs_expanded_index(struct index_state *istate,
*/
if ((unsigned int)item.nowildcard_len >
ce_namelen(ce) &&
- !strncmp(item.original, ce->name,
+ !strncmp(item.match, ce->name,
ce_namelen(ce))) {
res = 1;
break;
@@ -876,13 +876,13 @@ int pathspec_needs_expanded_index(struct index_state *istate,
* directory and the pathspec does not match the whole
* directory, need to expand the index.
*/
- if (!strncmp(item.original, ce->name, item.nowildcard_len) &&
- wildmatch(item.original, ce->name, 0)) {
+ if (!strncmp(item.match, ce->name, item.nowildcard_len) &&
+ wildmatch(item.match, ce->name, 0)) {
res = 1;
break;
}
}
- } else if (!path_in_cone_mode_sparse_checkout(item.original, istate) &&
+ } else if (!path_in_cone_mode_sparse_checkout(item.match, istate) &&
!matches_skip_worktree(pathspec, i, &skip_worktree_seen))
res = 1;
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 9814431cd74aff..d0b42371663f9d 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2119,6 +2119,13 @@ test_expect_success 'sparse index is not expanded: rm' '
ensure_not_expanded rm -r deep
'
+test_expect_success 'sparse index is not expanded: prefixed wildcard pathspec' '
+ init_repos &&
+
+ ensure_not_expanded -C deep rm --dry-run -- "a*" &&
+ ensure_not_expanded -C deep reset base -- "a*"
+'
+
test_expect_success 'grep with and --cached' '
init_repos &&
^ permalink raw reply related
* [PATCH 0/2] stash: avoid sparse-index expansion for in-cone paths
From: tnyman @ 2026-07-20 22:31 UTC (permalink / raw)
To: git; +Cc: Ted Nyman, Derrick Stolee, Taylor Blau, Jeff King, Victoria Dye
From: Ted Nyman <tnyman@openai.com>
`git stash push -- <pathspec>` expands a sparse index before checking
whether the pathspec matches a tracked path. A pathspec wholly inside
the sparse-checkout cone cannot match part of a sparse-directory entry,
so that expansion needlessly makes the command proportional to the full
index size.
The first patch fixes the pathspec helper to use the parsed, prefixed
path consistently. The existing code can read past the end of the
unprefixed path for a wildcard passed to `git rm` or `git reset` from a
subdirectory; AddressSanitizer reports a heap-buffer-overflow in that
case.
The second patch uses the helper in `git stash push`, following the same
approach as bcf96cfca6 ("rm: expand the index only when necessary",
2022-08-07). It adds compatibility coverage for the supported pathspec
forms and a path-limited stash case to p2000.
On a cone-mode repository with 349,525 tracked paths and 49 sparse-index
entries, the best of three runs was:
before: 18.87s (2.93s user + 15.62s system), 4 expansions
after: 0.06s (0.01s user + 0.02s system), 0 expansions
A full-index control was unchanged (1.62s before, 1.65s after).
The series is based on 48bbf81c29 ("The 5th batch", 2026-07-19), the
current master. Focused sparse-index, stash, pathspec, rm, reset,
SHA-256, and unit-test coverage passes. Clang, GCC, and sanitizer
builds also pass.
Ted Nyman (2):
pathspec: use match for sparse-index expansion checks
stash: avoid sparse-index expansion for in-cone paths
builtin/stash.c | 4 +-
pathspec.c | 12 ++---
t/perf/p2000-sparse-operations.sh | 1 +
t/t1092-sparse-checkout-compatibility.sh | 62 ++++++++++++++++++++++++
4 files changed, 71 insertions(+), 8 deletions(-)
base-commit: 48bbf81c29ca9a4479ec7850fe206518682cdb2f
^ permalink raw reply
* Re: [PATCH RFC v3 2/2] Move libgit.a sources into separate "lib/" directory
From: Junio C Hamano @ 2026-07-20 22:14 UTC (permalink / raw)
To: Johannes Schindelin
Cc: SZEDER Gábor, Patrick Steinhardt, git, brian m. carlson,
Elijah Newren, Derrick Stolee, Phillip Wood
In-Reply-To: <2d455ecf-972e-e3ce-54bc-683050c04282@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > My own (obviously subjective and biased) take is that the tradeoff is
>> > worth it, as these issues are a one-time cost while the benefits to
>> > discoverability will be permanent.
>>
>> It is not a one-time cost, but will be an ongoing burden.
>
> It is maybe drawn-out, but it is a one-time cost. It's not like we're
> going to mass-rename source files to move them to `lib/` every two weeks
> from now on.
Since the topic was posted, I have dealt with the fallout from it at
least twice a day (which, when we are lucky, is not a huge time
sink, as I have mostly automated it by now), and again every time a
new topic is posted that touches the moved files in substantial ways
or adds new files that ought to be moved. The latter is the most
time-consuming to handle. This will continue until all contemporary
topics, as well as the topic in question, graduate.
If that is not an ongoing burden, I do not know what is.
> And this statement neglects to acknowledge that the lack of clean
> organization of source code files is an ongoing burden _right now_, and
> would be at least partially addressed by the move.
At least, Gábor does not seem to think that the lack of clean
organization is so severe as to warrant a massive code churn like
this.
I value stability much more than prettiness. If we had started out
with almost nothing at the root level and almost everything in
either 'lib' or 'builtin', I would have strongly preferred to keep
that structure. But since we have been using a layout that has all
built-in commands in 'builtin', with subsystems like 'refs' and
'odb' in their own directories, and everything else at the root
level, I would prefer to keep that organization until a substantial
subsystem update wants to carve out a new location for itself, just
as past updates to create 'builtin', 'refs', and 'odb' did.
Compared to those past moves, the proposed change looks more like
churn for the sake of moving things around, without achieving any
real organizational improvement.
I must say that I, too, remain skeptical.
^ permalink raw reply
* Re: [PATCH RFC v3 2/2] Move libgit.a sources into separate "lib/" directory
From: brian m. carlson @ 2026-07-20 21:53 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Elijah Newren, Derrick Stolee,
SZEDER Gábor, Johannes Schindelin, Phillip Wood
In-Reply-To: <20260701-pks-libgit-in-subdir-v3-2-5e4860056094@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1666 bytes --]
On 2026-07-01 at 06:59:27, Patrick Steinhardt wrote:
> This move does not come for free though:
>
> - The mass rename introduces a cutoff point in the history of every
> moved file, as tools like git-log(1) do not follow renames by
> default.
>
> - Any in-flight or not-yet-submitted topic that touches the moved
> files will have to be rebased, and backporting fixes across the
> boundary becomes more cumbersome as a patch can no longer apply
> cleanly to both the old and the new layout.
>
> My own (obviously subjective and biased) take is that the tradeoff is
> worth it, as these issues are a one-time cost while the benefits to
> discoverability will be permanent.
I agree this is worth it. I found it odd even when I started working on
Git many years back that most of our code was placed directly in the
repository root when most other projects put it under a directory. I
think it would be valuable both for existing contributors and for new
ones to tidy this up.
> Furthermore, especially the first downside is a limitation in Git
> itself. We're not the first or last project to do such a mass rename. So
> if our provided tools are insufficient, then we should improve them to
> make the experience better for other projects, as well. Subjecting
> ourselves to the same pain may even give us more incentive to eventually
> improve rename following for everyone.
I would very much welcome better rename support and I'm sure the
community would as well. If we can incentivize ourselves to step up and
implement that, I'm all for it.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply
* gerrit code review once more (was: Re: [PATCH v3 0/9] sequencer: do not record dropped commits as) rewritten
From: Oswald Buddenhagen @ 2026-07-20 21:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqy0f5d25g.fsf@gitster.g>
On Mon, Jul 20, 2026 at 10:03:23AM -0700, Junio C Hamano wrote:
>Actually, reducing the signal to a single bit, 'did I or did I not
>see a +1 from them?', means Gerrit users see less 'spam' but must
>make decisions based on too little signal. I do not know whether
>that is an advantage.
>
>With your email, we can at least discern that your comment is much
>closer to an 'Acked-by' than a 'Reviewed-by', and we can respect
>that distinction when judging whether there is sufficient consensus
>on the list to move the topic forward.
>
gerrit discerns from -2 to +2 (*), so that angle is covered (**).
https://gerrit-review.googlesource.com/Documentation/config-labels.html#label_Code-Review
(*) actually however many levels the project chooses to configure,
though things aren't as smooth when deviating from the defaults
(**) mostly - https://issues.gerritcodereview.com/issues/40000793
^ permalink raw reply
* Re: [PATCH 4/4] doc: convert git-request-pull synopsis and options to new style
From: Junio C Hamano @ 2026-07-20 21:26 UTC (permalink / raw)
To: Jean-Noël AVILA; +Cc: Jean-Noël Avila via GitGitGadget, git
In-Reply-To: <23179740.EfDdHjke4D@piment-oiseau>
Jean-Noël AVILA <jn.avila@free.fr> writes:
> "Widely accepted", I do not know. I would better frame it as "because at least
> four is needed and I'm lazy, then it's four". I'm not expert enough the
> asciidoc specification to have a definitive answer of mine, but the
> asciidoctor specification says exactly four [1]
>
> We could indeed apply the rule of 4 dashes by default.
>
> Note that this only applies because it is a listing block which does not
> accept nesting.
>
> I will reroll.
Thanks. Sounds like a good plan to me.
^ permalink raw reply
* Re: import-zips
From: Chris Packham @ 2026-07-20 21:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: GIT
In-Reply-To: <xmqq8q75izpk.fsf@gitster.g>
On Tue, Jul 21, 2026 at 12:59 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Chris Packham <judge.packham@gmail.com> writes:
>
> > I wasn't sure if there would be any interest in taking the changes
> > back to git.git/contrib (or if the use of AI would rule that out).
> > Anyway it's there on my github page if anyone else needs it. If
> > someone wants me to turn the changes into a series for git.git I'm
> > happy to do that too.
>
> It is wonderful to hear that a piece of software that has long
> been abandonware now has someone who 'cares' much more than
> anyone who has touched the 'git.git' tree in years.
>
'Cares' is a bit strong. More like 'wanted to use it and had some
spare AI tokens'.
> I would rather remove unmaintained pieces of software from the
> 'contrib/' directory, and optionally replace them with a pointer
> to the location of a maintained fork. So, no, I would not want
> to be a gatekeeper for that part of the 'contrib/' directory
> when I do not have a particular need, interest, or the
> expertise to properly maintain it, if I can avoid it.
Yep fair enough. I guess you can consider this a somewhat maintained
fork (as in I've updated it and used the result once). I'll see if any
other users appear.
^ permalink raw reply
* Re: [PATCH 4/4] doc: convert git-request-pull synopsis and options to new style
From: Jean-Noël AVILA @ 2026-07-20 20:39 UTC (permalink / raw)
To: Jean-Noël Avila via GitGitGadget, Junio C Hamano; +Cc: git
In-Reply-To: <xmqqfr1eleyx.fsf@gitster.g>
On Monday, 20 July 2026 01:47:02 CEST Junio C Hamano wrote:
> "Jean-Noël Avila via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > @@ -54,11 +54,15 @@ the `v1.0` release, and want it to be integrated into
the
> > project.>
> > First you push that change to your public repository for others to
> >
> > see:
> > - git push https://git.ko.xz/project master
> > +-----
> > +git push https://git.ko.xz/project master
> > +-----
> >
> > Then, you run this command:
> > - git request-pull v1.0 https://git.ko.xz/project master
> > +------
> > +git request-pull v1.0 https://git.ko.xz/project master
> > +------
> >
> > which will produce a request to the upstream, summarizing the
> > changes between the `v1.0` release and your `master`, to pull it
> >
> > @@ -67,11 +71,15 @@ from your public repository.
> >
> > If you pushed your change to a branch whose name is different from
> > the one you have locally, e.g.
> >
> > - git push https://git.ko.xz/project master:for-linus
> > +-----
> > +git push https://git.ko.xz/project master:for-linus
> > +-----
> >
> > then you can ask that to be pulled with
> >
> > - git request-pull v1.0 https://git.ko.xz/project master:for-linus
> > +-----
> > +git request-pull v1.0 https://git.ko.xz/project master:for-linus
> > +-----
>
> Is there a widely accepted guideline among AsciiDoc users governing
> how many dashes should delimit these blocks, other than "at least
> four, with the opening and closing counts matching"? If so, what is
> it? We see five, six, five, and five dashes in the proposed changes
> above, and in '[PATCH 1/4]' we saw nine. Even if varying counts are
> functionally equivalent, the inconsistency is a bit distracting.
>
> Thanks.
>
> [Footnote]
>
> * an excerpt from [PATCH 1/4]
>
> diff --git a/Documentation/git-imap-send.adoc b/Documentation/git-imap-
send.adoc
> index 538b91afc0..dd1e0a3718 100644
> --- a/Documentation/git-imap-send.adoc
> +++ b/Documentation/git-imap-send.adoc
> @@ -192,7 +192,10 @@ supports only `XOAUTH2` as the mechanism.
>
> Once the commits are ready to be sent, run the following command:
>
> - $ git format-patch --cover-letter -M --stdout origin/master | git imap-
send
> +
> +---------
> +$ git format-patch --cover-letter -M --stdout origin/master | git imap-send
> +---------
"Widely accepted", I do not know. I would better frame it as "because at least
four is needed and I'm lazy, then it's four". I'm not expert enough the
asciidoc specification to have a definitive answer of mine, but the
asciidoctor specification says exactly four [1]
We could indeed apply the rule of 4 dashes by default.
Note that this only applies because it is a listing block which does not
accept nesting.
I will reroll.
[1]: https://docs.asciidoctor.org/asciidoc/latest/verbatim/listing-blocks/
#delimited-listing-block
^ permalink raw reply
* Re: [PATCH 2/4] doc: convert git-format-patch synopsis and options to new style
From: Jean-Noël AVILA @ 2026-07-20 20:09 UTC (permalink / raw)
To: Jean-Noël Avila via GitGitGadget, Junio C Hamano; +Cc: git
In-Reply-To: <xmqqldb6lfa6.fsf@gitster.g>
On Monday, 20 July 2026 01:40:17 CEST Junio C Hamano wrote:
> "Jean-Noël Avila via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > Backtick-quote all option terms in the OPTIONS section, convert
> > standalone placeholders to _<placeholder>_ form, and convert
> > single-quoted commands and tools in prose to backtick form.
>
> OK.
>
> > @@ -708,15 +708,15 @@ BASE TREE INFORMATION
> >
> > The base tree information block is used for maintainers or third party
> > testers to know the exact state the patch series applies to. It consists
> >
> > +of the "base commit", which is a well-known commit that is part of the
> >
> > stable part of the project history everybody else works off of, and zero
> >
> > +or more "prerequisite patches", which are well-known patches in flight
> > +that is not yet part of the "base commit" that need to be applied on top
> > +of "base commit" in topological order before the patches can be applied.
>
> GIven that the last part of this hunk below uses backtick-quoting
> for `prerequisite patch` and `patch id`, shouldn't the references to
> `base commit`, and `prerequisite patch(es)` in the above also be
> backtick quoted for consistency?
>
In fact, the formatting were swapped. For proper rendering and preservation of
spaces, it should be:
The "base commit" is shown as "`base-commit:` " followed by the 40-hex of
the commit object name. A "prerequisite patch" is shown as
"`prerequisite-patch-id:` " followed by the 40-hex "patch id", which can
be obtained by passing the patch through the `git patch-id --stable`
command.
Only the constant strings are back-ticked. The others are only quoted.
Will reroll.
> > +The "base commit" is shown as "base-commit: " followed by the 40-hex of
> > +the commit object name. A `prerequisite patch` is shown as
> > +"prerequisite-patch-id: " followed by the 40-hex `patch id`, which can
> >
> > be obtained by passing the patch through the `git patch-id --stable`
> > command.
^ permalink raw reply
* Re: [PATCH 2/2] remote: resolve URL-valued push tracking remotes
From: Harald Nordgren @ 2026-07-20 19:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Harald Nordgren via GitGitGadget, git
In-Reply-To: <xmqq4ihtcx8g.fsf@gitster.g>
Thanks for your continued support on all my topics!
Yes, I should clarify in the commit message what the actual motivation
is, which is for me to handle remote renames in a smoother way, since
'gh' renmames remotes when forking a repo which is messing with
@{push} and compareBranches for 'git status'.
Harald
^ permalink raw reply
* Re: [PATCH 2/2] remote: resolve URL-valued push tracking remotes
From: Junio C Hamano @ 2026-07-20 18:49 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget; +Cc: git, Harald Nordgren
In-Reply-To: <ff645b21591a4b365b30acaf67a295510889141c.1784538618.git.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> A branch may name its push destination with a URL instead of a
> configured remote. This is useful in fork workflows, where the original
> remote is renamed to "upstream", the fork is added as "origin", and an
> existing branch.<name>.pushRemote continues to contain the fork URL.
>
> Git can still push through the anonymous remote created for that URL.
> However, the anonymous remote has no fetch refspec. Git therefore cannot
> resolve @{push} to origin/<branch> or update that remote-tracking branch
> after a push. The push can succeed, or report that everything is up to
> date, while status continues to compare against a stale tracking ref or
> cannot show the push branch at all.
Let me try to think aloud, rephrasing the explanation with a
slightly more concrete illustration, to see whether I understand
what you are trying to achieve.
The current system allows you to set:
[branch "mytopic"]
pushRemote = https://hosting.site/users/me/mine.git/
[remote "notlinked"]
url = https://hosting.site/users/me/mine.git/
push = refs/heads/mytopic
fetch = refs/heads/*:refs/remotes/notlinked/*
but when on the 'mytopic' branch, @{push} cannot determine which
branch at the remote repository to update, so it cannot map it back
to our remote-tracking branch ('refs/remotes/notlinked/mytopic' in
the above illustration).
A question. Do we currently accept a string that is not a remote
name as the value for 'branch.<name>.pushRemote' by design?
The 'git config --help' output explains that:
- 'branch.<name>.pushRemote' overrides 'branch.<name>.remote' and
'remote.pushDefault'; and
- 'branch.<name>.remote' and 'remote.pushDefault' tell 'git fetch'
and 'git push' which remote to work with.
It therefore seems clear that setting a string that is not a remote
name (such as a URL) as the value for these three variables is a
misconfiguration in the current system.
I am not saying that it should stay that way forever. But please
re-read your first sentence and tell me whether it is clear that the
patch extends the current system with a new feature. It was far
from clear to me and caused significant confusion. Writing it like
this:
Under the current system, a branch cannot name its push
destination using a URL. If we were to extend the system
to allow this, such and such benefits would become
possible.
would have been far less confusing.
If that is what you are doing, that is.
> A uniquely matching configured remote already provides the missing
> mapping.
A very good consideration. It was the first thing that came to my
mind while I was thinking aloud, constructing an illustration with
'notlinked', wondering "what if there is another remote, with the
same URL, but different 'push' configuration?".
> Use its fetch refspec when resolving the push tracking branch
> and when updating tracking refs after a push.
Is this not needless, and is mentioning it not confusing? If I
understand correctly, what the change entails is:
* If the value of 'branch.<name>.pushRemote' (call it X) is 'not' a
remote name, try to see whether there is a unique remote that
has either (1) a 'pushurl' whose value matches X, or (2) no
'pushurl' but a 'url' whose value matches X. If no such remote
exists, simply abort and refuse to proceed.
* If there is such a remote, pretend that the value of
'branch.<name>.pushRemote' were the name of that remote, and do
everything else as usual.
And mapping the current branch name to its push destination via
'remote.<name>.push' to find the name of the destination branch at
the remote, and then mapping it back to our remote-tracking branch
using 'remote.<name>.fetch', is not something new that this topic
needs to update, no?
Thanks. Once I understand what you are trying to achieve, I will
offer further comments on the implementation, as I find this topic
potentially quite interesting.
^ permalink raw reply
* Re: [PATCH 1/2] remote: pass repository to push tracking helper
From: Junio C Hamano @ 2026-07-20 18:23 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget; +Cc: git, Harald Nordgren
In-Reply-To: <fc70895732f406ecdbaea7a5b9a3fda4fb03df67.1784538618.git.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> The push tracking helper currently only needs the push remote. However,
> resolving a URL-valued remote requires access to the repository's list
> of configured remotes.
It is unclear to me what 'resolving a URL-valued remote' means.
Could you describe what you are trying to achieve, without relying
on unexplained terms like 'to resolve' and 'URL-valued remote',
which seem to carry specialized meanings in this context?
Thanks.
> Pass the repository through the existing callers and mark the parameter
> as unused for now. This prepares the helper for that lookup without
> changing its behavior.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
> remote.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index e6c52c850c..89d0f9e2d8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1887,7 +1887,8 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
> return branch->merge[0]->dst;
> }
>
> -static char *tracking_for_push_dest(struct remote *remote,
> +static char *tracking_for_push_dest(struct repository *repo UNUSED,
> + struct remote *remote,
> const char *refname,
> struct strbuf *err)
> {
> @@ -1925,13 +1926,13 @@ static char *branch_get_push_1(struct repository *repo,
> _("push refspecs for '%s' do not include '%s'"),
> remote->name, branch->name);
>
> - ret = tracking_for_push_dest(remote, dst, err);
> + ret = tracking_for_push_dest(repo, remote, dst, err);
> free(dst);
> return ret;
> }
>
> if (remote->mirror)
> - return tracking_for_push_dest(remote, branch->refname, err);
> + return tracking_for_push_dest(repo, remote, branch->refname, err);
>
> switch (push_default) {
> case PUSH_DEFAULT_NOTHING:
> @@ -1939,7 +1940,7 @@ static char *branch_get_push_1(struct repository *repo,
>
> case PUSH_DEFAULT_MATCHING:
> case PUSH_DEFAULT_CURRENT:
> - return tracking_for_push_dest(remote, branch->refname, err);
> + return tracking_for_push_dest(repo, remote, branch->refname, err);
>
> case PUSH_DEFAULT_UPSTREAM:
> return xstrdup_or_null(branch_get_upstream(branch, err));
> @@ -1953,7 +1954,7 @@ static char *branch_get_push_1(struct repository *repo,
> up = branch_get_upstream(branch, err);
> if (!up)
> return NULL;
> - cur = tracking_for_push_dest(remote, branch->refname, err);
> + cur = tracking_for_push_dest(repo, remote, branch->refname, err);
> if (!cur)
> return NULL;
> if (strcmp(cur, up)) {
^ permalink raw reply
* Re: [PATCH v3 0/2] bisect: add --auto-reset to leave when done
From: Junio C Hamano @ 2026-07-20 17:20 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget; +Cc: git, Johannes Sixt, Harald Nordgren
In-Reply-To: <pull.2335.v3.git.git.1784538619.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Add a --reset-when-found option to git bisect that resets the bisect session
> when culprit is found.
>
> Changes in v3:
>
> * Rename --auto-reset to --reset-when-found, including internal names.
> * Defer git bisect run cleanup until captured output is printed and
> BISECT_RUN is closed. Drop the open-descriptor preparatory change,
> retaining the existing filename-based output handling.
The range-diff looks very busy, but it mostly looks like a fallout
caused by the renaming of the option and internal functions and enum
to match the updated name.
Looking good. Is everybody happy with this version? If so I'll
mark the topic for 'next' soonish.
Thanks.
^ permalink raw reply
* Re: git config: unintuitive behaviour with --global and --no-includes
From: Junio C Hamano @ 2026-07-20 17:09 UTC (permalink / raw)
To: Jeff King; +Cc: Hendrik Jaeger, git
In-Reply-To: <20260720125145.GA5100@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> IMHO lbmk is wrong to be using "--global" in the first place. Looking at
> the source, it is trying to check whether the user has set up their
> identity. But it is not lbmk's business whether you did it in the
> --global config file, or elsewhere!
Exactly.
> Though note there is one other hitch, which is that the user can set
> author.* and committer.* as specific variables, since 39ab4d0951
> (config: allow giving separate author and committer idents, 2019-02-04).
> I suspect not many people do that, but that would also be something that
> a config-specific check would have to handle (but "git var" would do
> automatically).
>
> So I think you might consider sending a bug report to lbmk. Feel free to
> point at this thread, and I'm happy to discuss further with them.
Thanks for your thoughtful and thorough explanation.
The environment variables 'GIT_{AUTHOR,COMMITTER}_{NAME,EMAIL}' also
play a part in determining the author andcommitter identities, so
'git var AUTHOR_IDENT' would be the correct choice here.
^ permalink raw reply
* Re: [PATCH v3 0/9] sequencer: do not record dropped commits as rewritten
From: Junio C Hamano @ 2026-07-20 17:03 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git
In-Reply-To: <al4RYuWKqAr-IlFC@ugly.lan>
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> On Sun, Jul 19, 2026 at 12:29:31PM -0700, Junio C Hamano wrote:
>>It looks like this is now ready to go? Any further comments?
>>
> you can add whatever footer is appropriate for "i read it, it seems to
> make sense, but i didn't double-check" for me.
>
> (same for phillip's new 2-patch series.)
>
> (it feels silly to "spam" the list with such low-value verdicts. i
> really miss gerrit code review here, where i'd leave a +1 in passing.)
Actually, reducing the signal to a single bit, 'did I or did I not
see a +1 from them?', means Gerrit users see less 'spam' but must
make decisions based on too little signal. I do not know whether
that is an advantage.
With your email, we can at least discern that your comment is much
closer to an 'Acked-by' than a 'Reviewed-by', and we can respect
that distinction when judging whether there is sufficient consensus
on the list to move the topic forward.
In any case, thank you for reading it over and letting us know that
you found nothing glaringly wrong. That is indeed valuable
information.
Thanks.
^ permalink raw reply
* Re: [PATCH] trace2: tolerate failed timestamp formatting
From: Taylor Blau @ 2026-07-20 14:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Derrick Stolee, Derrick Stolee via GitGitGadget, git
In-Reply-To: <xmqqzezlhgyo.fsf@gitster.g>
On Mon, Jul 20, 2026 at 07:29:51AM -0700, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
>
> >> Would it make more sense to fix the xsnprintf()/libintl boundary and
> >> treat Trace2 reentrancy separately? I still can't explain why the
> >> allocation failed, so there may be another GfW-specific piece I’m
> >> missing.
> >
> > I think that your suggested change has merits and should be pursued.
> > I'll explore it a bit to confirm.
>
> That band-aid may be a good idea, but I would prefer not to see the
> conditional in a common source file like 'wrapper.c'. Somewhere
> MinGW-specific would be more appropriate, would it not?
Yeah, to be clear, I do not think that putting the '#define' here in
'wrapper.c' is appropriate, and included it in my original email only to
demonstrate the shape of the proposed solution.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] trace2: tolerate failed timestamp formatting
From: Junio C Hamano @ 2026-07-20 14:29 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Taylor Blau, Derrick Stolee via GitGitGadget, git
In-Reply-To: <c8d443a5-3cfb-4752-8716-cf0d8fadd9d3@gmail.com>
Derrick Stolee <stolee@gmail.com> writes:
>> Would it make more sense to fix the xsnprintf()/libintl boundary and
>> treat Trace2 reentrancy separately? I still can't explain why the
>> allocation failed, so there may be another GfW-specific piece I’m
>> missing.
>
> I think that your suggested change has merits and should be pursued.
> I'll explore it a bit to confirm.
That band-aid may be a good idea, but I would prefer not to see the
conditional in a common source file like 'wrapper.c'. Somewhere
MinGW-specific would be more appropriate, would it not?
> The other justification I'd like to make in my patch is that the
> xsnprintf() calls die() and the trace2 machinery should be die()-free
> whenever possible. Solving both possible causes is likely the right
> long-term approach.
That is indeed worth considering.
You mention a few calls to xstrdup() that can potentially abort, and
I agree that anything that triggers malloc() and notices that we are
out of memory can probably do little better than to die. But are
there other operations that may cause us to exit, even though we are
not in an unrecoverable state (such as an out-of-memory condition)?
Thanks.
^ permalink raw reply
* Re: [PATCH RFC v3 2/2] Move libgit.a sources into separate "lib/" directory
From: Johannes Schindelin @ 2026-07-20 14:24 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Patrick Steinhardt, git, brian m. carlson, Junio C Hamano,
Elijah Newren, Derrick Stolee, Phillip Wood
In-Reply-To: <alR9GDNTbdjWB4dq@szeder.dev>
[-- Attachment #1: Type: text/plain, Size: 2728 bytes --]
Hi Gábor,
On Mon, 13 Jul 2026, SZEDER Gábor wrote:
> On Wed, Jul 01, 2026 at 08:59:27AM +0200, Patrick Steinhardt wrote:
> > This move does not come for free though:
> >
> > - The mass rename introduces a cutoff point in the history of every
> > moved file, as tools like git-log(1) do not follow renames by
> > default.
> >
> > - Any in-flight or not-yet-submitted topic that touches the moved
> > files will have to be rebased, and backporting fixes across the
> > boundary becomes more cumbersome as a patch can no longer apply
> > cleanly to both the old and the new layout.
> >
> > My own (obviously subjective and biased) take is that the tradeoff is
> > worth it, as these issues are a one-time cost while the benefits to
> > discoverability will be permanent.
>
> It is not a one-time cost, but will be an ongoing burden.
It is maybe drawn-out, but it is a one-time cost. It's not like we're
going to mass-rename source files to move them to `lib/` every two weeks
from now on.
And this statement neglects to acknowledge that the lack of clean
organization of source code files is an ongoing burden _right now_, and
would be at least partially addressed by the move.
> > Furthermore, especially the first downside is a limitation in Git
> > itself. We're not the first or last project to do such a mass rename. So
> > if our provided tools are insufficient, then we should improve them to
> > make the experience better for other projects, as well. Subjecting
> > ourselves to the same pain may even give us more incentive to eventually
> > improve rename following for everyone.
>
> I'm uncertain how that should work, and rather sceptical that it would
> work at all.
I am painfully reminded of all the arguments against migrating certain
repositories to Git. They sounded exactly like this.
> Some have expressed that it is a pain to deal with the fallout of this
> patch. Should we then come up with those envisioned improvements,
> whatever they might be? I'm fairly certain that I won't have the time
> for that. Or should you do those improvements, because, after all,
> you thrust upon us this churn? Then it would certainly be better to
> come up with those improvements first...
>
> Overall, I remain unconvinced, and maintain that this just trades one
> annoyance for the other, and it's not worth it.
In case anyone was waiting for differing opinion, for a vote in favor of a
better structure of Git's source code, and of equipping the Git project
itself with the all-too-common need for improved support for following
mass-renames/moves across criss-cross merges, I am happy to provide.
Ciao,
Johannes
^ permalink raw reply
* Re: import-zips
From: Junio C Hamano @ 2026-07-20 12:59 UTC (permalink / raw)
To: Chris Packham; +Cc: GIT
In-Reply-To: <CAFOYHZBTAGiugQVOJrc4kJQkuhcSDiT1ruim7A1+6EW1iKAUNQ@mail.gmail.com>
Chris Packham <judge.packham@gmail.com> writes:
> I wasn't sure if there would be any interest in taking the changes
> back to git.git/contrib (or if the use of AI would rule that out).
> Anyway it's there on my github page if anyone else needs it. If
> someone wants me to turn the changes into a series for git.git I'm
> happy to do that too.
It is wonderful to hear that a piece of software that has long
been abandonware now has someone who 'cares' much more than
anyone who has touched the 'git.git' tree in years.
I would rather remove unmaintained pieces of software from the
'contrib/' directory, and optionally replace them with a pointer
to the location of a maintained fork. So, no, I would not want
to be a gatekeeper for that part of the 'contrib/' directory
when I do not have a particular need, interest, or the
expertise to properly maintain it, if I can avoid it.
^ permalink raw reply
* Re: git config: unintuitive behaviour with --global and --no-includes
From: Jeff King @ 2026-07-20 12:51 UTC (permalink / raw)
To: Hendrik Jaeger; +Cc: git
In-Reply-To: <20260720113402.0dc16abe@frustcomp.hnjs.home.arpa>
On Mon, Jul 20, 2026 at 11:34:02AM +0200, Hendrik Jaeger wrote:
> The manpage says:
> > Respect include.* directives in config files when looking up
> > values. Defaults to off when a specific file is given (e.g., using
> > --file, --global, etc) and on when searching all config files.
>
> IMHO it makes sense the way it is phrased “when a specific file is
> given” but then seems to turn into non-sense when --global is given as
> an example. Giving --global is not “giving a specific file” but
> “restricting to a specific scope”, which may `include` other files.
> The results seem inconsistent and counterintuitive to me.
>
> Am I misunderstanding anything here?
> Is this behaviour intended?
> If it is intended, can someone please explain the rationale behind it? I don’t get it, it seems wrong to me.
The behavior you're seeing is intended. Regarding "a specific scope", I
don't think that's an unreasonable way to think about it. But it's not
how Git thinks about it, and in particular back when --include was added
and this behavior was set, "--global" was literally a synonym for
"--file=$HOME/.gitconfig".
As for the rationale, it is a mix of backwards compatibility and
least-surprise. The include functionality was tacked on to the existing
config parser, and we did not want to surprise anybody who asked for a
specific file by showing them results for another file. This is
especially important for reading untrusted input like .gitmodules, but
also for writing.
> Regarding the initial issue: I just added --includes to the call in
> lbmk and it works just fine, so there is no need to address this. I
> only mentioned it for context to how I got to looking into this
> behaviour.
IMHO lbmk is wrong to be using "--global" in the first place. Looking at
the source, it is trying to check whether the user has set up their
identity. But it is not lbmk's business whether you did it in the
--global config file, or elsewhere! So it should probably just use a
straight "git config user.name", which will do the same resolution that
Git will do internally.
The "--global" was added in their 4a280c62 (.gitcheck: re-write
entirely. force global config., 2023-08-27), but I don't see any
rationale given.
Depending on what they are trying to check, it might be even better
still for it to use "git var GIT_AUTHOR_IDENT". That will give the
actual ident Git will derive, including things like checking $EMAIL in
the environment and so on.
So if the intent is "will Git come up with some ident", then that is the
most accurate way to check it. But if the intent is "did the user
specifically configure Git (because we are worried that values derived
from GECOS and $EMAIL might not be accurate)", then checking user.*
specifically is closer to that.
Though note there is one other hitch, which is that the user can set
author.* and committer.* as specific variables, since 39ab4d0951
(config: allow giving separate author and committer idents, 2019-02-04).
I suspect not many people do that, but that would also be something that
a config-specific check would have to handle (but "git var" would do
automatically).
So I think you might consider sending a bug report to lbmk. Feel free to
point at this thread, and I'm happy to discuss further with them.
-Peff
^ permalink raw reply
page: next (older)
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox