git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v4 10/11] http-fetch: Use index-pack rather than verify-pack to check packs

From
Shawn O. Pearce <spearce@spearce.org>
Date
Apr 19, 2010, 14:23 UTC
Message-ID
<1271686990-16363-6-git-send-email-spearce@spearce.org>
In-Reply-To
<20100418115744.0000238b@unknown>

To ensure we don't leave a corrupt pack file positioned as though it were a valid pack file, run index-pack on the temporary pack before we rename it to its final name. If index-pack crashes out when it discovers file corruption (e.g. GitHub's error HTML at the end of the file), simply delete the temporary files to cleanup.

By waiting until the pack has been validated before we move it to its final name, we eliminate a race condition where another concurrent reader might try to access the pack at the same time that we are still trying to verify its not corrupt.

Switching from verify-pack to index-pack is a change in behavior, but it should turn out better for users. The index-pack algorithm tries to minimize disk seeks, as well as the number of times any given object is inflated, by organizing its work along delta chains. The verify-pack logic does not attempt to do this, thrashing the delta base cache and the filesystem cache.

By recreating the index file locally, we also can automatically upgrade from a v1 pack table of contents to v2. This makes the CRC32 data available for use during later repacks, even if the server didn't have them on hand.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 Moved unlink of index to after the index-pack is successful,
 per Tay Ray Chuan's request.
 Removed Junio SOB line since the logic changed.
 http.c                |   44 +++++++++++++++++++++++++++++++++++++-------
 t/t5550-http-fetch.sh |   15 +++++++++++++++
 2 files changed, 52 insertions(+), 7 deletions(-)
diff --git a/http.c b/http.c
index 9c62632..2ebd679 100644
--- a/http.c
+++ b/http.c
@@ -1,6 +1,7 @@
 #include "http.h"
 #include "pack.h"
 #include "sideband.h"
+#include "run-command.h"
 
 int data_received;
 int active_requests;
@@ -998,11 +999,14 @@ void release_http_pack_request(struct http_pack_request *preq)
 
 int finish_http_pack_request(struct http_pack_request *preq)
 {
-	int ret;
 	struct packed_git **lst;
 	struct packed_git *p = preq->target;
+	char *tmp_idx;
+	struct child_process ip;
+	const char *ip_argv[8];
+
+	close_pack_index(p);
 
-	p->pack_size = ftell(preq->packfile);
 	fclose(preq->packfile);
 	preq->packfile = NULL;
 	preq->slot->local = NULL;
@@ -1012,13 +1016,39 @@ int finish_http_pack_request(struct http_pack_request *preq)
 		lst = &((*lst)->next);
 	*lst = (*lst)->next;
 
-	ret = move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1));
-	if (ret)
-		return ret;
-	if (verify_pack(p))
+	tmp_idx = xstrdup(preq->tmpfile);
+	strcpy(tmp_idx + strlen(tmp_idx) - strlen(".pack.temp"),
+	       ".idx.temp");
+
+	ip_argv[0] = "index-pack";
+	ip_argv[1] = "-o";
+	ip_argv[2] = tmp_idx;
+	ip_argv[3] = preq->tmpfile;
+	ip_argv[4] = NULL;
+
+	memset(&ip, 0, sizeof(ip));
+	ip.argv = ip_argv;
+	ip.git_cmd = 1;
+	ip.no_stdin = 1;
+	ip.no_stdout = 1;
+
+	if (run_command(&ip)) {
+		unlink(preq->tmpfile);
+		unlink(tmp_idx);
+		free(tmp_idx);
 		return -1;
-	install_packed_git(p);
+	}
+
+	unlink(sha1_pack_index_name(p->sha1));
 
+	if (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1))
+	 || move_temp_to_file(tmp_idx, sha1_pack_index_name(p->sha1))) {
+		free(tmp_idx);
+		return -1;
+	}
+
+	install_packed_git(p);
+	free(tmp_idx);
 	return 0;
 }
 
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 78c31c9..1a4dfc9 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -62,6 +62,21 @@ test_expect_success 'fetch packed objects' '
 	git clone $HTTPD_URL/dumb/repo_pack.git
 '
 
+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 &&
+	 p=`ls objects/pack/pack-*.pack` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad1.git &&
+	(cd repo_bad1.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad1.git &&
+	 test 0 = `ls objects/pack/pack-*.pack | wc -l`
+	)
+'
+
 test_expect_success 'did not use upload-pack service' '
 	grep '/git-upload-pack' <"$HTTPD_ROOT_PATH"/access.log >act
 	: >exp

-- 
1.7.1.rc1.279.g22727
Previous: Shawn O. PearceNext: Tay Ray Chuan
Message 42 of 46 in “git fetch over http:// left my repo broken”
  1. Christian HalstrickApr 15, 2010
  2. Michael J GruberApr 15, 2010
  3. Ilari LiusvaaraApr 15, 2010
  4. Shawn O. PearceApr 15, 2010
  5. 0/6 detect dumb HTTP pack file corruptionShawn O. Pearce, Apr 15, 2010
  6. Junio C HamanoApr 17, 2010
  7. Shawn O. PearceApr 17, 2010
  8. 1/6 http.c: Remove bad free of static blockShawn O. Pearce, Apr 15, 2010
  9. 2/6 t5550-http-fetch: Use subshell for repository operationsShawn O. Pearce, Apr 15, 2010
  10. 3/6 http.c: Tiny refactoring of finish_http_pack_requestShawn O. Pearce, Apr 15, 2010
  11. 4/6 http.c: Drop useless != NULL test in finish_http_pack_requestShawn O. Pearce, Apr 15, 2010
  12. 5/6 http-fetch: Use index-pack rather than verify-pack to check packsShawn O. Pearce, Apr 15, 2010
  13. Johannes SixtApr 15, 2010
  14. 5/6 http-fetch: Use index-pack rather than verify-pack to check packsShawn O. Pearce, Apr 15, 2010
  15. Tay Ray ChuanApr 16, 2010
  16. Shawn O. PearceApr 17, 2010
  17. 6/6 http-fetch: Use temporary files for pack-*.idx until verifiedShawn O. Pearce, Apr 15, 2010
  18. Tay Ray ChuanApr 16, 2010
  19. 01/11 http.c: Remove bad free of static blockShawn O. Pearce, Apr 17, 2010
  20. 02/11 t5550-http-fetch: Use subshell for repository operationsShawn O. Pearce, Apr 17, 2010
  21. 03/11 http.c: Tiny refactoring of finish_http_pack_requestShawn O. Pearce, Apr 17, 2010
  22. 04/11 http.c: Drop useless != NULL test in finish_http_pack_requestShawn O. Pearce, Apr 17, 2010
  23. 05/11 http.c: Don't store destination name in request structuresShawn O. Pearce, Apr 17, 2010
  24. Tay Ray ChuanApr 18, 2010
  25. 06/11 http.c: Remove unnecessary strdup of sha1_to_hex resultShawn O. Pearce, Apr 17, 2010
  26. Tay Ray ChuanApr 18, 2010
  27. 07/11 Introduce close_pack_index to permit replacementShawn O. Pearce, Apr 17, 2010
  28. 08/11 Extract verify_pack_index for reuse from verify_packShawn O. Pearce, Apr 17, 2010
  29. 09/11 Allow parse_pack_index on temporary filesShawn O. Pearce, Apr 17, 2010
  30. 10/11 http-fetch: Use index-pack rather than verify-pack to check packsShawn O. Pearce, Apr 17, 2010
  31. Tay Ray ChuanApr 18, 2010
  32. 11/11 http-fetch: Use temporary files for pack-*.idx until verifiedShawn O. Pearce, Apr 17, 2010
  33. Tay Ray ChuanApr 18, 2010
  34. 00/11 Resend sp/maint-dumb-http-pack-reidxShawn O. Pearce, Apr 19, 2010
  35. Tay Ray ChuanApr 19, 2010
  36. Shawn O. PearceApr 19, 2010
  37. Tay Ray ChuanApr 20, 2010
  38. 06/11 http.c: Remove unnecessary strdup of sha1_to_hex resultShawn O. Pearce, Apr 19, 2010
  39. 07/11 Introduce close_pack_index to permit replacementShawn O. Pearce, Apr 19, 2010
  40. 08/11 Extract verify_pack_index for reuse from verify_packShawn O. Pearce, Apr 19, 2010
  41. 09/11 Allow parse_pack_index on temporary filesShawn O. Pearce, Apr 19, 2010
  42. 10/11 http-fetch: Use index-pack rather than verify-pack to check packsShawn O. Pearce, Apr 19, 2010
  43. Tay Ray ChuanApr 19, 2010
  44. 11/11 http-fetch: Use temporary files for pack-*.idx until verifiedShawn O. Pearce, Apr 19, 2010
  45. 6/6 http-fetch: Use temporary files for pack-*.idx until verifiedShawn O. Pearce, Apr 15, 2010
  46. Ilari LiusvaaraApr 15, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.