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

On Fri, Jul 24, 2026 at 01:14:25AM -0700, Ted Nyman wrote:
> 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 <[email protected]>
> ---
>  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.

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

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