[PATCH 1/2] http: use unique tempfiles for packfile URI downloads

[Ted Nyman <tnyman@openai.com>]

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.

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.

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.

Signed-off-by: Ted Nyman <tnyman@openai.com>
---
 Documentation/git-http-fetch.adoc |  5 +-
 http.c                            | 77 +++++++++++++++++++++----------
 http.h                            |  1 +
 t/t5550-http-fetch-dumb.sh        | 72 ++++++++++++++++++++++++++++-
 4 files changed, 126 insertions(+), 29 deletions(-)

diff --git a/Documentation/git-http-fetch.adoc b/Documentation/git-http-fetch.adoc
index 2200f073c4..533bf381c4 100644
--- a/Documentation/git-http-fetch.adoc
+++ b/Documentation/git-http-fetch.adoc
@@ -48,9 +48,8 @@ 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 arbitrary. The output of index-pack is printed to stdout.
+	Requires --index-pack-args.
 
 --index-pack-args=<args>::
 	For internal use only. The command to run on the contents of the
diff --git a/http.c b/http.c
index b4e7b8d00b..5a46e7c65c 100644
--- a/http.c
+++ b/http.c
@@ -2668,7 +2668,10 @@ int http_get_info_packs(const char *base_url, struct packfile_list *packs)
 
 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;
 	}
@@ -2688,7 +2691,10 @@ 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);
 	preq->packfile = NULL;
 
 	tmpfile_fd = xopen(preq->tmpfile.buf, O_RDONLY);
@@ -2711,7 +2717,10 @@ int finish_http_pack_request(struct http_pack_request *preq)
 
 cleanup:
 	close(tmpfile_fd);
-	unlink(preq->tmpfile.buf);
+	if (preq->tempfile)
+		delete_tempfile(&preq->tempfile);
+	else
+		unlink(preq->tmpfile.buf);
 	return ret;
 }
 
@@ -2723,20 +2732,8 @@ 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)
 {
 	off_t prev_posn = 0;
 	struct http_pack_request *preq;
@@ -2746,9 +2743,22 @@ struct http_pack_request *new_direct_http_pack_request(
 
 	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 (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);
@@ -2766,8 +2776,9 @@ struct http_pack_request *new_direct_http_pack_request(
 	 * 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",
@@ -2779,12 +2790,28 @@ 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)
diff --git a/http.h b/http.h
index 729c51904d..2c900779f5 100644
--- a/http.h
+++ b/http.h
@@ -224,6 +224,7 @@ struct http_pack_request {
 
 	FILE *packfile;
 	struct strbuf tmpfile;
+	struct tempfile *tempfile;
 	struct active_request_slot *slot;
 	struct curl_slist *headers;
 };
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index b0080bf204..314a74c433 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -293,6 +293,74 @@ test_expect_success 'http-fetch --packfile' '
 	git -C packfileclient cat-file -e "$HASH"
 '
 
+test_expect_success PIPE 'concurrent http-fetch --packfile' '
+	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-} &&
+
+	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
+
+	# 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" \
+				--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
+		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" \
+		--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_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 &&
+	git -C packfileclient-concurrent cat-file -e "$HASH"
+'
+
 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 &&
@@ -313,7 +381,9 @@ 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.55.0