Re: [PATCH v4 3/3] fetch-pack: accept "pack" output for packfile URIs
- From
Taylor Blau <ttaylorr@openai.com>
- Date
- Jul 24, 2026, 21:46 UTC
- Message-ID
- <amPdMQH3QRLnDpl0@com-79390>
- In-Reply-To
- <d9063deb60354eb731e34c453cd6730e1098f905.1784874850.git.tnyman@openai.com>
On Fri, Jul 24, 2026 at 01:14:25AM -0700, Ted Nyman wrote:
Show 27 quoted lines
> 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 29c41132ee..e9f24fbd63 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];OK, so we keep track of whether or not we got "keep" as part of the output.
While here, "packname" is renamed to "packhash", which I think is reasonable, especially to indicate that the buffer is sized accordingly. We happen to read the preceding "pack" or "keep" into that same buffer, which I think is fine. If we wanted to be pedantic we could read that into a separate buffer, but I don't think such separation is necessary.
Show 15 quoted lines
> 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);Makes sense.
Show 12 quoted lines
>
> - 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';Likewise. The rest of this file and the test also look good.
Thanks, Taylor