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

[Ted Nyman <tnyman@openai.com>]

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.

Add a regression test which pre-creates a keep file and verifies that a
fetch succeeds without changing it.

Signed-off-by: Ted Nyman <tnyman@openai.com>
---
 fetch-pack.c           | 36 ++++++++++++++++++++----------------
 t/t5702-protocol-v2.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 120e01f3cf..a16b80177a 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1887,9 +1887,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	}
 
 	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;
 		const char *uri = packfile_uris.items[i].string +
 			the_hash_algo->hexsz + 1;
 
@@ -1907,16 +1910,16 @@ 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, 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");
 
 		parse_gitmodules_oids(cmd.out, &fsck_options.gitmodules_found);
 
@@ -1925,16 +1928,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