Re: [PATCH 2/2] fetch-pack: accept "pack" output for packfile URIs

From: Jeff King <peff@peff.net>
Date: Jul 14, 2026, 07:12 UTC
Message-ID: <20260714071231.GD2516582@coredump.intra.peff.net>
In-Reply-To: <alVoA5-fDDPwKPZZ@com-76773>

On Mon, Jul 13, 2026 at 03:34:43PM -0700, Ted Nyman wrote:

Show 14 quoted lines

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

OK, that all makes sense.

Show 8 quoted lines

>  	for (i = 0; i < packfile_uris.nr; i++) {
> +		int created_keep = 0;
>  		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;

The "+ 6" here is gross, but not really any more than the bare "5" in the original code.

Calling it "packhash" made me wonder about this line of code:

> -		if (memcmp(packfile_uris.items[i].string, packname,
> +		if (memcmp(packfile_uris.items[i].string, packhash,

Surely we need to change more than this if we now have the hash rather than the whole packname? But no, the original code was really just storing the hash in packname.

Which was rather misleading, but it is not much better after your patch. Now packname still just has the packhash, along with the extra keep/pack marker.

Would a more generic name like "cmd_output" or something make sense? I also think this would all be much nicer with a strbuf (which would let us get rid of the magic numbers), but that is a slightly larger refactor:

diff --git a/fetch-pack.c b/fetch-pack.c
index 1e8461d07e..5f94f35c30 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1890,9 +1890,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		int created_keep = 0;
 		int j;
 		struct child_process cmd = CHILD_PROCESS_INIT;
-		char packname[GIT_MAX_HEXSZ + 6];
+		struct strbuf cmd_output = STRBUF_INIT;
 		const char *packhash;
-		const int packname_len = the_hash_algo->hexsz + 6;
 		const char *uri = packfile_uris.items[i].string +
 			the_hash_algo->hexsz + 1;
 
@@ -1910,14 +1909,11 @@ 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, 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))
+		if (strbuf_read(&cmd_output, cmd.out, 0) < 0)
+			die("failed to read http-fetch output");
+		if (skip_prefix(cmd_output.buf, "keep\t", &packhash))
 			created_keep = 1;
-		else if (!skip_prefix(packname, "pack\t", &packhash))
+		else if (!skip_prefix(cmd_output.buf, "pack\t", &packhash))
 			die("fetch-pack: expected pack or keep, TAB, hash, "
 			    "then LF in http-fetch output");
 


BTW, two things that puzzled me while poking at your patch (but neither
I think are new or the fault of your patch):

  1. git-http-fetch documents --index-pack-args, but the actual option
     is the singular --index-pack-arg. The caller in fetch-pack
     obviously uses the one that works.

  2. The code you're touching insists on reading "keep" in the output,
     but wouldn't that depend on feeding "--keep" via index_pack_args? I
     didn't see immediately where we set it, but I certainly don't think
     your patch could be making anything worse here (since the existing
     code would have just died upon seeing a "pack" line). I think it
     happens as a side effect in get_pack(), which is...subtle. But
     again, not anything new.

-Peff