git fetch over http:// left my repo broken

Christian Halstrick· Apr 15, 2010, 09:51 UTC · lore

Hi,

Git should detect this error and let the fetch fail, right?

Here is what I did:

> git --version

git version 1.6.6.1

> uname --all

> git remote -v

> git fetch origin

...

> git fsck --full

# get the size of the corrupted pack file

> ls -l ./objects/pack/pack-b5dceb0bae390d6540f881be686839f7e691fa0b.pack

# dump the start of the corrupted file: looks ok

> xxd -l 64 ./objects/pack/pack-b5dceb0bae390d6540f881be686839f7e691fa0b.pack

>  xxd -s 766911990 -l 128 ./objects/pack/pack-b5dceb0bae390d6540f881be686839f7e691fa0b.pack

# search for the string "maintenance" in the binary pack file

> grep -C5 -a "maintenance" ./objects/pack/pack-b5dceb0bae390d6540f881be686839f7e691fa0b.pack

<div id="error" class="status404">
  <img alt="Repo unavailable due to maintenanace" height="219"
src="http://assets3.github.com/images/error/octocat_construction.gif?5cb2cfa6f35ac1b0c322a78a129d7531177b36d7"
width="243" />
  <h1>Repository temporarily unavailable.</h1>
  <p>The backend storage is temporarily offline. Usually this means the<br />
     storage server is undergoing maintenance. Your repository should <br />
     be available again very soon.</p>
</div>

        </div>

>

Ciao
 Chris

Michael J Gruber· Apr 15, 2010, 09:58 UTC · re: Christian Halstrick · lore

Re: git fetch over http:// left my repo broken

Christian Halstrick venit, vidit, dixit 15.04.2010 11:51:

> Hi,
> 
> some days back I fetched from a github repo with http protocol and
> afterwards my local repo was broken. Since the fetch was done by a
> cronjob I don't know whether the fetch reported an error. Problem is
> that one pack file was corrupted because the github servers put the
> repo I wanted to clone into some maintenance mode while I was
> fetching. The pack file includes at the end the html source code -
> which makes these files clearly corrupted.
> 
> Git should detect this error and let the fetch fail, right?

Right. And Github should not pull your repo away from under your feet.

If you move away the broken pack, do you get any dangling refs?

Michael

Re: git fetch over http:// left my repo broken

On Thu, Apr 15, 2010 at 11:58:27AM +0200, Michael J Gruber wrote:

> Christian Halstrick venit, vidit, dixit 15.04.2010 11:51:
> 
> But still, Git should be able to deal with broken servers. The problem
> is: If the server does not report any problem but simply serves a broken
> pack (with correct header), how should Git notice? It would require a
> fsck before accepting any new pack.

-Ilari

Shawn O. Pearce· Apr 15, 2010, 14:15 UTC · re: Ilari Liusvaara · lore

Re: git fetch over http:// left my repo broken

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:

> On Thu, Apr 15, 2010 at 11:58:27AM +0200, Michael J Gruber wrote:
> > Christian Halstrick venit, vidit, dixit 15.04.2010 11:51:
> > 
> > But still, Git should be able to deal with broken servers. The problem
> > is: If the server does not report any problem but simply serves a broken
> > pack (with correct header), how should Git notice? It would require a
> > fsck before accepting any new pack.
> 
> Pack trailer hash. Apparently dumb HTTP fetch needs to bypass pack to index
> conversion somehow since index-pack aborts if trailer hash check fails (not
> to mention other failures corrupt pack may cause).

I'll put together a patch shortly.

-- 
Shawn.

Shawn O. Pearce· Apr 15, 2010, 19:09 UTC · re: Shawn O. Pearce · lore

[PATCH 0/6] detect dumb HTTP pack file corruption

Shawn O. Pearce (6):
  http.c: Remove bad free of static block
  t5550-http-fetch: Use subshell for repository operations
  http.c: Tiny refactoring of finish_http_pack_request
  http.c: Drop useless != NULL test in finish_http_pack_request
  http-fetch: Use index-pack rather than verify-pack to check packs
  http-fetch: Use temporary files for pack-*.idx until verified

 cache.h               |    3 +-
 http.c                |  118 +++++++++++++++++++++++++++++++++----------------
 http.h                |    1 -
 pack-check.c          |   15 +++++--
 pack.h                |    1 +
 sha1_file.c           |   17 +++++--
 t/t5550-http-fetch.sh |   37 ++++++++++++++-
 7 files changed, 140 insertions(+), 52 deletions(-)

Junio C Hamano· Apr 17, 2010, 17:56 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 0/6] detect dumb HTTP pack file corruption

Shawn O. Pearce· Apr 17, 2010, 19:11 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/6] detect dumb HTTP pack file corruption

Junio C Hamano <gitster@pobox.com> wrote:

> Hmph, I am getting failures from "[index v2] 6 verify-pack detects CRC
> mismatch" in t5302 when this is applied to 'maint' (or when the result is
> merged to 'master').

-- 
Shawn.

Shawn O. Pearce· Apr 15, 2010, 19:09 UTC · re: Shawn O. Pearce · lore

[PATCH 1/6] http.c: Remove bad free of static block

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 http.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index 4814217..f26625e 100644
--- a/http.c
+++ b/http.c
@@ -1082,7 +1082,6 @@ struct http_pack_request *new_http_pack_request(
 	return preq;
 
 abort:
-	free(filename);
 	free(preq->url);
 	free(preq);
 	return NULL;

-- 
1.7.1.rc1.269.ga27c7

Shawn O. Pearce· Apr 15, 2010, 19:09 UTC · re: Shawn O. Pearce · lore

[PATCH 2/6] t5550-http-fetch: Use subshell for repository operations

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 t/t5550-http-fetch.sh |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 8cfce96..78c31c9 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -55,9 +55,10 @@ test_expect_success 'http remote detects correct HEAD' '
 
 test_expect_success 'fetch packed objects' '
 	cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
-	cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
-	git --bare repack &&
-	git --bare prune-packed &&
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+	 git --bare repack &&
+	 git --bare prune-packed
+	) &&
 	git clone $HTTPD_URL/dumb/repo_pack.git
 '
 

-- 
1.7.1.rc1.269.ga27c7

Shawn O. Pearce· Apr 15, 2010, 19:09 UTC · re: Shawn O. Pearce · lore

[PATCH 3/6] http.c: Tiny refactoring of finish_http_pack_request

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 http.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/http.c b/http.c
index f26625e..4558f11 100644
--- a/http.c
+++ b/http.c
@@ -1002,8 +1002,9 @@ int finish_http_pack_request(struct http_pack_request *preq)
 {
 	int ret;
 	struct packed_git **lst;
+	struct packed_git *p = preq->target;
 
-	preq->target->pack_size = ftell(preq->packfile);
+	p->pack_size = ftell(preq->packfile);
 
 	if (preq->packfile != NULL) {
 		fclose(preq->packfile);
@@ -1011,18 +1012,17 @@ int finish_http_pack_request(struct http_pack_request *preq)
 		preq->slot->local = NULL;
 	}
 
-	ret = move_temp_to_file(preq->tmpfile, preq->filename);
-	if (ret)
-		return ret;
-
 	lst = preq->lst;
-	while (*lst != preq->target)
+	while (*lst != p)
 		lst = &((*lst)->next);
 	*lst = (*lst)->next;
 
-	if (verify_pack(preq->target))
+	ret = move_temp_to_file(preq->tmpfile, preq->filename);
+	if (ret)
+		return ret;
+	if (verify_pack(p))
 		return -1;
-	install_packed_git(preq->target);
+	install_packed_git(p);
 
 	return 0;
 }

-- 
1.7.1.rc1.269.ga27c7

Shawn O. Pearce· Apr 15, 2010, 19:09 UTC · re: Shawn O. Pearce · lore

[PATCH 4/6] http.c: Drop useless != NULL test in finish_http_pack_request

Simplify the code by just removing the conditional.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 http.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/http.c b/http.c
index 4558f11..64e0c18 100644
--- a/http.c
+++ b/http.c
@@ -1005,12 +1005,9 @@ int finish_http_pack_request(struct http_pack_request *preq)
 	struct packed_git *p = preq->target;
 
 	p->pack_size = ftell(preq->packfile);
-
-	if (preq->packfile != NULL) {
-		fclose(preq->packfile);
-		preq->packfile = NULL;
-		preq->slot->local = NULL;
-	}
+	fclose(preq->packfile);
+	preq->packfile = NULL;
+	preq->slot->local = NULL;
 
 	lst = preq->lst;
 	while (*lst != p)

-- 
1.7.1.rc1.269.ga27c7

Shawn O. Pearce· Apr 15, 2010, 19:09 UTC · re: Shawn O. Pearce · lore

[PATCH 5/6] http-fetch: Use index-pack rather than verify-pack to check packs

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 cache.h               |    1 +
 http.c                |   42 ++++++++++++++++++++++++++++++++++--------
 http.h                |    1 -
 sha1_file.c           |   11 +++++++++--
 t/t5550-http-fetch.sh |   15 +++++++++++++++
 5 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/cache.h b/cache.h
index 5eb0573..4150603 100644
--- a/cache.h
+++ b/cache.h
@@ -916,6 +916,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
 
 extern void pack_report(void);
 extern int open_pack_index(struct packed_git *);
+extern void close_pack_index(struct packed_git *);
 extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);
 extern void close_pack_windows(struct packed_git *);
 extern void unuse_pack(struct pack_window **);
diff --git a/http.c b/http.c
index 64e0c18..aa3e380 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;
@@ -1000,11 +1001,13 @@ 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];
 
-	p->pack_size = ftell(preq->packfile);
+	close_pack_index(p);
 	fclose(preq->packfile);
 	preq->packfile = NULL;
 	preq->slot->local = NULL;
@@ -1014,13 +1017,37 @@ int finish_http_pack_request(struct http_pack_request *preq)
 		lst = &((*lst)->next);
 	*lst = (*lst)->next;
 
-	ret = move_temp_to_file(preq->tmpfile, preq->filename);
-	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);
+	}
 
+	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;
 }
 
@@ -1043,7 +1070,6 @@ struct http_pack_request *new_http_pack_request(
 	preq->url = strbuf_detach(&buf, NULL);
 
 	filename = sha1_pack_name(target->sha1);
-	snprintf(preq->filename, sizeof(preq->filename), "%s", filename);
 	snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp", filename);
 	preq->packfile = fopen(preq->tmpfile, "a");
 	if (!preq->packfile) {
diff --git a/http.h b/http.h
index 5c9441c..e4a8126 100644
--- a/http.h
+++ b/http.h
@@ -152,7 +152,6 @@ struct http_pack_request
 	struct packed_git *target;
 	struct packed_git **lst;
 	FILE *packfile;
-	char filename[PATH_MAX];
 	char tmpfile[PATH_MAX];
 	struct curl_slist *range_header;
 	struct active_request_slot *slot;
diff --git a/sha1_file.c b/sha1_file.c
index ff65328..820063e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -599,6 +599,14 @@ void unuse_pack(struct pack_window **w_cursor)
 	}
 }
 
+void close_pack_index(struct packed_git *p)
+{
+	if (p->index_data) {
+		munmap((void *)p->index_data, p->index_size);
+		p->index_data = NULL;
+	}
+}
+
 /*
  * This is used by git-repack in case a newly created pack happens to
  * contain the same set of objects as an existing one.  In that case
@@ -620,8 +628,7 @@ void free_pack_by_name(const char *pack_name)
 			close_pack_windows(p);
 			if (p->pack_fd != -1)
 				close(p->pack_fd);
-			if (p->index_data)
-				munmap((void *)p->index_data, p->index_size);
+			close_pack_index(p);
 			free(p->bad_object_sha1);
 			*pp = p->next;
 			free(p);
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 78c31c9..bdac8d7 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 &&
+	 dd if=/dev/zero of=$p bs=256 count=1 seek=1
+	) &&
+	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.269.ga27c7

Johannes Sixt· Apr 15, 2010, 19:34 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 5/6] http-fetch: Use index-pack rather than verify-pack to check packs

On Donnerstag, 15. April 2010, Shawn O. Pearce wrote:

> +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 &&
> +	 dd if=/dev/zero of=$p bs=256 count=1 seek=1

	printf %0256d 0 | dd of=$p bs=256 count=1 seek=1

for the benefit of us poor Windowsers who do not have /dev/zero.

Perhaps you want to add conv=notrunc.

Ditto in patch 6/6.

Shawn O. Pearce· Apr 15, 2010, 21:25 UTC · re: Johannes Sixt · lore

[PATCH v2 5/6] http-fetch: Use index-pack rather than verify-pack to check packs

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 cache.h               |    1 +
 http.c                |   42 ++++++++++++++++++++++++++++++++++--------
 http.h                |    1 -
 sha1_file.c           |   11 +++++++++--
 t/t5550-http-fetch.sh |   15 +++++++++++++++
 5 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/cache.h b/cache.h
index 5eb0573..4150603 100644
--- a/cache.h
+++ b/cache.h
@@ -916,6 +916,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
 
 extern void pack_report(void);
 extern int open_pack_index(struct packed_git *);
+extern void close_pack_index(struct packed_git *);
 extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);
 extern void close_pack_windows(struct packed_git *);
 extern void unuse_pack(struct pack_window **);
diff --git a/http.c b/http.c
index 64e0c18..aa3e380 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;
@@ -1000,11 +1001,13 @@ 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];
 
-	p->pack_size = ftell(preq->packfile);
+	close_pack_index(p);
 	fclose(preq->packfile);
 	preq->packfile = NULL;
 	preq->slot->local = NULL;
@@ -1014,13 +1017,37 @@ int finish_http_pack_request(struct http_pack_request *preq)
 		lst = &((*lst)->next);
 	*lst = (*lst)->next;
 
-	ret = move_temp_to_file(preq->tmpfile, preq->filename);
-	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);
+	}
 
+	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;
 }
 
@@ -1043,7 +1070,6 @@ struct http_pack_request *new_http_pack_request(
 	preq->url = strbuf_detach(&buf, NULL);
 
 	filename = sha1_pack_name(target->sha1);
-	snprintf(preq->filename, sizeof(preq->filename), "%s", filename);
 	snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp", filename);
 	preq->packfile = fopen(preq->tmpfile, "a");
 	if (!preq->packfile) {
diff --git a/http.h b/http.h
index 5c9441c..e4a8126 100644
--- a/http.h
+++ b/http.h
@@ -152,7 +152,6 @@ struct http_pack_request
 	struct packed_git *target;
 	struct packed_git **lst;
 	FILE *packfile;
-	char filename[PATH_MAX];
 	char tmpfile[PATH_MAX];
 	struct curl_slist *range_header;
 	struct active_request_slot *slot;
diff --git a/sha1_file.c b/sha1_file.c
index ff65328..820063e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -599,6 +599,14 @@ void unuse_pack(struct pack_window **w_cursor)
 	}
 }
 
+void close_pack_index(struct packed_git *p)
+{
+	if (p->index_data) {
+		munmap((void *)p->index_data, p->index_size);
+		p->index_data = NULL;
+	}
+}
+
 /*
  * This is used by git-repack in case a newly created pack happens to
  * contain the same set of objects as an existing one.  In that case
@@ -620,8 +628,7 @@ void free_pack_by_name(const char *pack_name)
 			close_pack_windows(p);
 			if (p->pack_fd != -1)
 				close(p->pack_fd);
-			if (p->index_data)
-				munmap((void *)p->index_data, p->index_size);
+			close_pack_index(p);
 			free(p->bad_object_sha1);
 			*pp = p->next;
 			free(p);
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.269.ga27c7

Tay Ray Chuan· Apr 16, 2010, 02:55 UTC · re: Shawn O. Pearce · lore

Re: [PATCH v2 5/6] http-fetch: Use index-pack rather than verify-pack to check packs

Hi,

On Fri, Apr 16, 2010 at 5:25 AM, Shawn O. Pearce <spearce@spearce.org> wrote:

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

This is exceedingly interesting.

> diff --git a/cache.h b/cache.h
> index 5eb0573..4150603 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -916,6 +916,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
>
>  extern void pack_report(void);
>  extern int open_pack_index(struct packed_git *);
> +extern void close_pack_index(struct packed_git *);
>  extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);
>  extern void close_pack_windows(struct packed_git *);
>  extern void unuse_pack(struct pack_window **);
> [snip]
> diff --git a/sha1_file.c b/sha1_file.c
> index ff65328..820063e 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -599,6 +599,14 @@ void unuse_pack(struct pack_window **w_cursor)
>        }
>  }
>
> +void close_pack_index(struct packed_git *p)
> +{
> +       if (p->index_data) {
> +               munmap((void *)p->index_data, p->index_size);
> +               p->index_data = NULL;
> +       }
> +}
> +
>  /*
>  * This is used by git-repack in case a newly created pack happens to
>  * contain the same set of objects as an existing one.  In that case
> @@ -620,8 +628,7 @@ void free_pack_by_name(const char *pack_name)
>                        close_pack_windows(p);
>                        if (p->pack_fd != -1)
>                                close(p->pack_fd);
> -                       if (p->index_data)
> -                               munmap((void *)p->index_data, p->index_size);
> +                       close_pack_index(p);
>                        free(p->bad_object_sha1);
>                        *pp = p->next;
>                        free(p);

Perhaps these could go into a separate patch.

> diff --git a/http.c b/http.c
> index 64e0c18..aa3e380 100644
> --- a/http.c
> +++ b/http.c
> [snip]
> @@ -1014,13 +1017,37 @@ int finish_http_pack_request(struct http_pack_request *preq)
>                lst = &((*lst)->next);
>        *lst = (*lst)->next;
>
> -       ret = move_temp_to_file(preq->tmpfile, preq->filename);
> [snip]
> +       if (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1))
> [snip]
> @@ -1043,7 +1070,6 @@ struct http_pack_request *new_http_pack_request(
>        preq->url = strbuf_detach(&buf, NULL);
>
>        filename = sha1_pack_name(target->sha1);
> -       snprintf(preq->filename, sizeof(preq->filename), "%s", filename);
>        snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp", filename);
>        preq->packfile = fopen(preq->tmpfile, "a");
>        if (!preq->packfile) {
> [snip]
> diff --git a/http.h b/http.h
> index 5c9441c..e4a8126 100644
> --- a/http.h
> +++ b/http.h
> @@ -152,7 +152,6 @@ struct http_pack_request
>        struct packed_git *target;
>        struct packed_git **lst;
>        FILE *packfile;
> -       char filename[PATH_MAX];
>        char tmpfile[PATH_MAX];
>        struct curl_slist *range_header;
>        struct active_request_slot *slot;

Why this change? Just curious, nothing strong against it.

> +       tmp_idx = xstrdup(preq->tmpfile);
> +       strcpy(tmp_idx + strlen(tmp_idx) - strlen(".pack.temp"),
> +              ".idx.temp");

Could we use a strbuf here?

> [snip]
> +       if (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1))
> +        || move_temp_to_file(tmp_idx, sha1_pack_index_name(p->sha1))) {

-- 
Cheers,
Ray Chuan

Shawn O. Pearce· Apr 17, 2010, 19:30 UTC · re: Tay Ray Chuan · lore

Re: [PATCH v2 5/6] http-fetch: Use index-pack rather than verify-pack to check packs

Tay Ray Chuan <rctay89@gmail.com> wrote:

> > @@ -152,7 +152,6 @@ struct http_pack_request
> >        struct packed_git *target;
> >        struct packed_git **lst;
> >        FILE *packfile;
> > -       char filename[PATH_MAX];
> 
> Why this change? Just curious, nothing strong against it.

Split into a new patch.  I'll share my reasons in the commit message.
:-)
 

> > +       tmp_idx = xstrdup(preq->tmpfile);
> > +       strcpy(tmp_idx + strlen(tmp_idx) - strlen(".pack.temp"),
> > +              ".idx.temp");
> 
> Could we use a strbuf here?

Doesn't seem worth it.  I just started trying to rework this with a
strbuf and I just don't see any benefit here.  We know the tmpfile
ends with ".pack.temp" when we created this request structure.  So
a strdup and overwrite of the tail just works.
 

> > +       if (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1))
> > +        || move_temp_to_file(tmp_idx, sha1_pack_index_name(p->sha1))) {
> 
> Hmm, when moving the pack index file, should we unlink() the old,
> downloaded one first?

Yup, good point, thanks.

-- 
Shawn.

Shawn O. Pearce· Apr 15, 2010, 21:25 UTC · re: Johannes Sixt · lore

[PATCH v2 6/6] http-fetch: Use temporary files for pack-*.idx until verified

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 cache.h               |    2 +-
 http.c                |   62 +++++++++++++++++++++++++++++++-----------------
 pack-check.c          |   15 ++++++++---
 pack.h                |    1 +
 sha1_file.c           |    6 +++-
 t/t5550-http-fetch.sh |   15 ++++++++++++
 6 files changed, 72 insertions(+), 29 deletions(-)

diff --git a/cache.h b/cache.h
index 4150603..0d101e4 100644
--- a/cache.h
+++ b/cache.h
@@ -905,7 +905,7 @@ struct extra_have_objects {
 extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);
 extern int server_supports(const char *feature);
 
-extern struct packed_git *parse_pack_index(unsigned char *sha1);
+extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
 extern void prepare_packed_git(void);
 extern void reprepare_packed_git(void);
diff --git a/http.c b/http.c
index aa3e380..2d88034 100644
--- a/http.c
+++ b/http.c
@@ -897,47 +897,65 @@ int http_fetch_ref(const char *base, struct ref *ref)
 }
 
 /* Helpers for fetching packs */
-static int fetch_pack_index(unsigned char *sha1, const char *base_url)
+static char *fetch_pack_index(unsigned char *sha1, const char *base_url)
 {
-	int ret = 0;
-	char *hex = xstrdup(sha1_to_hex(sha1));
-	char *filename;
-	char *url = NULL;
+	char *url, *tmp;
 	struct strbuf buf = STRBUF_INIT;
 
-	if (has_pack_index(sha1)) {
-		ret = 0;
-		goto cleanup;
-	}
-
 	if (http_is_verbose)
-		fprintf(stderr, "Getting index for pack %s\n", hex);
+		fprintf(stderr, "Getting index for pack %s\n", sha1_to_hex(sha1));
 
 	end_url_with_slash(&buf, base_url);
-	strbuf_addf(&buf, "objects/pack/pack-%s.idx", hex);
+	strbuf_addf(&buf, "objects/pack/pack-%s.idx", sha1_to_hex(sha1));
 	url = strbuf_detach(&buf, NULL);
 
-	filename = sha1_pack_index_name(sha1);
-	if (http_get_file(url, filename, 0) != HTTP_OK)
-		ret = error("Unable to get pack index %s\n", url);
+	strbuf_addf(&buf, "%s.temp", sha1_pack_index_name(sha1));
+	tmp = strbuf_detach(&buf, NULL);
+
+	if (http_get_file(url, tmp, 0) != HTTP_OK) {
+		error("Unable to get pack index %s\n", url);
+		free(tmp);
+		tmp = NULL;
+	}
 
-cleanup:
-	free(hex);
 	free(url);
-	return ret;
+	return tmp;
 }
 
 static int fetch_and_setup_pack_index(struct packed_git **packs_head,
 	unsigned char *sha1, const char *base_url)
 {
 	struct packed_git *new_pack;
+	char *tmp_idx = NULL;
 
-	if (fetch_pack_index(sha1, base_url))
-		return -1;
+	if (!has_pack_index(sha1)) {
+		tmp_idx = fetch_pack_index(sha1, base_url);
+		if (!tmp_idx)
+			return -1;
+	}
 
-	new_pack = parse_pack_index(sha1);
-	if (!new_pack)
+	new_pack = parse_pack_index(sha1, tmp_idx);
+	if (!new_pack) {
+		if (tmp_idx) {
+			unlink(tmp_idx);
+			free(tmp_idx);
+		}
 		return -1; /* parse_pack_index() already issued error message */
+	}
+
+	if (tmp_idx) {
+		int ret;
+
+		ret = verify_pack_index(new_pack);
+		if (!ret) {
+			close_pack_index(new_pack);
+			ret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));
+		}
+		free(tmp_idx);
+		if (ret)
+			return -1;
+	}
+
 	new_pack->next = *packs_head;
 	*packs_head = new_pack;
 	return 0;
diff --git a/pack-check.c b/pack-check.c
index 166ca70..9baba12 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -133,14 +133,13 @@ static int verify_packfile(struct packed_git *p,
 	return err;
 }
 
-int verify_pack(struct packed_git *p)
+int verify_pack_index(struct packed_git *p)
 {
 	off_t index_size;
 	const unsigned char *index_base;
 	git_SHA_CTX ctx;
 	unsigned char sha1[20];
 	int err = 0;
-	struct pack_window *w_curs = NULL;
 
 	if (open_pack_index(p))
 		return error("packfile %s index not opened", p->pack_name);
@@ -154,9 +153,17 @@ int verify_pack(struct packed_git *p)
 	if (hashcmp(sha1, index_base + index_size - 20))
 		err = error("Packfile index for %s SHA1 mismatch",
 			    p->pack_name);
+	return err;
+}
+
+int verify_pack(struct packed_git *p)
+{
+	int err = 0;
+	struct pack_window *w_curs = NULL;
 
-	/* Verify pack file */
-	err |= verify_packfile(p, &w_curs);
+	err |= verify_pack_index(p);
+	if (!err)
+		err |= verify_packfile(p, &w_curs);
 	unuse_pack(&w_curs);
 
 	return err;
diff --git a/pack.h b/pack.h
index d268c01..bb27576 100644
--- a/pack.h
+++ b/pack.h
@@ -57,6 +57,7 @@ struct pack_idx_entry {
 
 extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);
 extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);
+extern int verify_pack_index(struct packed_git *);
 extern int verify_pack(struct packed_git *);
 extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);
 extern char *index_pack_lockfile(int fd);
diff --git a/sha1_file.c b/sha1_file.c
index 820063e..232e14d 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -838,12 +838,14 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)
 	return p;
 }
 
-struct packed_git *parse_pack_index(unsigned char *sha1)
+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)
 {
-	const char *idx_path = sha1_pack_index_name(sha1);
 	const char *path = sha1_pack_name(sha1);
 	struct packed_git *p = alloc_packed_git(strlen(path) + 1);
 
+	if (!idx_path)
+		idx_path = sha1_pack_index_name(sha1);
+
 	strcpy(p->pack_name, path);
 	hashcpy(p->sha1, sha1);
 	if (check_packed_git_idx(idx_path, p)) {
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 1a4dfc9..fc675b5 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -77,6 +77,21 @@ test_expect_success 'fetch notices corrupt pack' '
 	)
 '
 
+test_expect_success 'fetch notices corrupt idx' '
+	cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	 p=`ls objects/pack/pack-*.idx` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad2.git &&
+	(cd repo_bad2.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad2.git &&
+	 test 0 = `ls objects/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.269.ga27c7

Tay Ray Chuan· Apr 16, 2010, 02:03 UTC · re: Shawn O. Pearce · lore

Re: [PATCH v2 6/6] http-fetch: Use temporary files for pack-*.idx until verified

Hi

On Fri, Apr 16, 2010 at 5:25 AM, Shawn O. Pearce <spearce@spearce.org> wrote:

> [snip]
> diff --git a/pack-check.c b/pack-check.c
> index 166ca70..9baba12 100644
> --- a/pack-check.c
> +++ b/pack-check.c
> @@ -133,14 +133,13 @@ static int verify_packfile(struct packed_git *p,
>        return err;
>  }
>
> -int verify_pack(struct packed_git *p)
> +int verify_pack_index(struct packed_git *p)
>  {
>        off_t index_size;
>        const unsigned char *index_base;
>        git_SHA_CTX ctx;
>        unsigned char sha1[20];
>        int err = 0;
> -       struct pack_window *w_curs = NULL;
>
>        if (open_pack_index(p))
>                return error("packfile %s index not opened", p->pack_name);
> @@ -154,9 +153,17 @@ int verify_pack(struct packed_git *p)
>        if (hashcmp(sha1, index_base + index_size - 20))
>                err = error("Packfile index for %s SHA1 mismatch",
>                            p->pack_name);
> +       return err;
> +}
> +
> +int verify_pack(struct packed_git *p)
> +{
> +       int err = 0;
> +       struct pack_window *w_curs = NULL;
>
> -       /* Verify pack file */
> -       err |= verify_packfile(p, &w_curs);
> +       err |= verify_pack_index(p);
> +       if (!err)
> +               err |= verify_packfile(p, &w_curs);
>        unuse_pack(&w_curs);
>
>        return err;
> diff --git a/pack.h b/pack.h
> index d268c01..bb27576 100644
> --- a/pack.h
> +++ b/pack.h
> @@ -57,6 +57,7 @@ struct pack_idx_entry {
>
>  extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);
>  extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);
> +extern int verify_pack_index(struct packed_git *);
>  extern int verify_pack(struct packed_git *);
>  extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);
>  extern char *index_pack_lockfile(int fd);

These should probably go into a separate patch.

> diff --git a/http.c b/http.c
> index aa3e380..2d88034 100644
> --- a/http.c
> +++ b/http.c
> @@ -897,47 +897,65 @@ int http_fetch_ref(const char *base, struct ref *ref)
>  }
>
>  /* Helpers for fetching packs */
> -static int fetch_pack_index(unsigned char *sha1, const char *base_url)
> +static char *fetch_pack_index(unsigned char *sha1, const char *base_url)
>  {
> -       int ret = 0;
> -       char *hex = xstrdup(sha1_to_hex(sha1));
> [snip]
>        if (http_is_verbose)
> -               fprintf(stderr, "Getting index for pack %s\n", hex);
> +               fprintf(stderr, "Getting index for pack %s\n", sha1_to_hex(sha1));
>
>        end_url_with_slash(&buf, base_url);
> -       strbuf_addf(&buf, "objects/pack/pack-%s.idx", hex);
> +       strbuf_addf(&buf, "objects/pack/pack-%s.idx", sha1_to_hex(sha1));
>        url = strbuf_detach(&buf, NULL);

I think the replacing of "hex" with "sha1_to_hex(sha1)" is unrelated.

> -       if (has_pack_index(sha1)) {
> -               ret = 0;
> -               goto cleanup;
> -       }
> -

>  static int fetch_and_setup_pack_index(struct packed_git **packs_head,
>        unsigned char *sha1, const char *base_url)
>  {
> [snip]
> +               ret = verify_pack_index(new_pack);
> +               if (!ret) {
> +                       close_pack_index(new_pack);
> +                       ret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));
> +               }
> +               free(tmp_idx);
> +               if (ret)
> +                       return -1;

Also, perhaps the below could be squashed in to reduce if()'s on tmp_idx.

-- >8 --

diff --git a/http.c b/http.c
index 6b7b899..e5bb54a 100644
--- a/http.c
+++ b/http.c
@@ -945,35 +945,37 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,
 {
 	struct packed_git *new_pack;
 	char *tmp_idx = NULL;
+	int ret;
 
-	if (!has_pack_index(sha1)) {
-		tmp_idx = fetch_pack_index(sha1, base_url);
-		if (!tmp_idx)
-			return -1;
+	if (has_pack_index(sha1)) {
+		new_pack = parse_pack_index(sha1, NULL);
+		if (!new_pack)
+			return -1; /* parse_pack_index() already issued error message */
+		goto add_pack;
 	}
 
+	tmp_idx = fetch_pack_index(sha1, base_url);
+	if (!tmp_idx)
+		return -1;
+
 	new_pack = parse_pack_index(sha1, tmp_idx);
 	if (!new_pack) {
-		if (tmp_idx) {
-			unlink(tmp_idx);
-			free(tmp_idx);
-		}
+		unlink(tmp_idx);
+		free(tmp_idx);
+
 		return -1; /* parse_pack_index() already issued error message */
 	}
 
-	if (tmp_idx) {
-		int ret;
-
-		ret = verify_pack_index(new_pack);
-		if (!ret) {
-			close_pack_index(new_pack);
-			ret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));
-		}
-		free(tmp_idx);
-		if (ret)
-			return -1;
+	ret = verify_pack_index(new_pack);
+	if (!ret) {
+		close_pack_index(new_pack);
+		ret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));
 	}
+	free(tmp_idx);
+	if (ret)
+		return -1;
 
+add_pack:
 	new_pack->next = *packs_head;
 	*packs_head = new_pack;
 	return 0;

-- 
Cheers,
Ray Chuan

Shawn O. Pearce· Apr 17, 2010, 20:07 UTC · re: Tay Ray Chuan · lore

[PATCH v3 01/11] http.c: Remove bad free of static block

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 http.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index 4814217..f26625e 100644
--- a/http.c
+++ b/http.c
@@ -1082,7 +1082,6 @@ struct http_pack_request *new_http_pack_request(
 	return preq;
 
 abort:
-	free(filename);
 	free(preq->url);
 	free(preq);
 	return NULL;

-- 
1.7.1.rc1.269.ga27c7

Shawn O. Pearce· Apr 17, 2010, 20:07 UTC · re: Tay Ray Chuan · lore

[PATCH v3 02/11] t5550-http-fetch: Use subshell for repository operations

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 t/t5550-http-fetch.sh |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 8cfce96..78c31c9 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -55,9 +55,10 @@ test_expect_success 'http remote detects correct HEAD' '
 
 test_expect_success 'fetch packed objects' '
 	cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
-	cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
-	git --bare repack &&
-	git --bare prune-packed &&
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git &&
+	 git --bare repack &&
+	 git --bare prune-packed
+	) &&
 	git clone $HTTPD_URL/dumb/repo_pack.git
 '
 

-- 
1.7.1.rc1.269.ga27c7

Shawn O. Pearce· Apr 17, 2010, 20:07 UTC · re: Tay Ray Chuan · lore

[PATCH v3 03/11] http.c: Tiny refactoring of finish_http_pack_request

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 http.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/http.c b/http.c
index f26625e..4558f11 100644
--- a/http.c
+++ b/http.c
@@ -1002,8 +1002,9 @@ int finish_http_pack_request(struct http_pack_request *preq)
 {
 	int ret;
 	struct packed_git **lst;
+	struct packed_git *p = preq->target;
 
-	preq->target->pack_size = ftell(preq->packfile);
+	p->pack_size = ftell(preq->packfile);
 
 	if (preq->packfile != NULL) {
 		fclose(preq->packfile);
@@ -1011,18 +1012,17 @@ int finish_http_pack_request(struct http_pack_request *preq)
 		preq->slot->local = NULL;
 	}
 
-	ret = move_temp_to_file(preq->tmpfile, preq->filename);
-	if (ret)
-		return ret;
-
 	lst = preq->lst;
-	while (*lst != preq->target)
+	while (*lst != p)
 		lst = &((*lst)->next);
 	*lst = (*lst)->next;
 
-	if (verify_pack(preq->target))
+	ret = move_temp_to_file(preq->tmpfile, preq->filename);
+	if (ret)
+		return ret;
+	if (verify_pack(p))
 		return -1;
-	install_packed_git(preq->target);
+	install_packed_git(p);
 
 	return 0;
 }

-- 
1.7.1.rc1.269.ga27c7

Shawn O. Pearce· Apr 17, 2010, 20:07 UTC · re: Tay Ray Chuan · lore

[PATCH v3 04/11] http.c: Drop useless != NULL test in finish_http_pack_request

Simplify the code by just removing the conditional.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 http.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/http.c b/http.c
index 4558f11..64e0c18 100644
--- a/http.c
+++ b/http.c
@@ -1005,12 +1005,9 @@ int finish_http_pack_request(struct http_pack_request *preq)
 	struct packed_git *p = preq->target;
 
 	p->pack_size = ftell(preq->packfile);
-
-	if (preq->packfile != NULL) {
-		fclose(preq->packfile);
-		preq->packfile = NULL;
-		preq->slot->local = NULL;
-	}
+	fclose(preq->packfile);
+	preq->packfile = NULL;
+	preq->slot->local = NULL;
 
 	lst = preq->lst;
 	while (*lst != p)

-- 
1.7.1.rc1.269.ga27c7

Shawn O. Pearce· Apr 17, 2010, 20:07 UTC · re: Tay Ray Chuan · lore

[PATCH v3 05/11] http.c: Don't store destination name in request structures

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 http-walker.c |    2 +-
 http.c        |   14 ++++++--------
 http.h        |    2 --
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/http-walker.c b/http-walker.c
index ef99ae6..8ca76d0 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -510,7 +510,7 @@ static int fetch_object(struct walker *walker, struct alt_base *repo, unsigned c
 		ret = error("File %s has bad hash", hex);
 	} else if (req->rename < 0) {
 		ret = error("unable to write sha1 filename %s",
-			    req->filename);
+			    sha1_file_name(req->sha1));
 	}
 
 	release_http_object_request(req);
diff --git a/http.c b/http.c
index 64e0c18..c75eb95 100644
--- a/http.c
+++ b/http.c
@@ -1014,7 +1014,7 @@ int finish_http_pack_request(struct http_pack_request *preq)
 		lst = &((*lst)->next);
 	*lst = (*lst)->next;
 
-	ret = move_temp_to_file(preq->tmpfile, preq->filename);
+	ret = move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1));
 	if (ret)
 		return ret;
 	if (verify_pack(p))
@@ -1043,7 +1043,6 @@ struct http_pack_request *new_http_pack_request(
 	preq->url = strbuf_detach(&buf, NULL);
 
 	filename = sha1_pack_name(target->sha1);
-	snprintf(preq->filename, sizeof(preq->filename), "%s", filename);
 	snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp", filename);
 	preq->packfile = fopen(preq->tmpfile, "a");
 	if (!preq->packfile) {
@@ -1133,7 +1132,6 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	freq->localfile = -1;
 
 	filename = sha1_file_name(sha1);
-	snprintf(freq->filename, sizeof(freq->filename), "%s", filename);
 	snprintf(freq->tmpfile, sizeof(freq->tmpfile),
 		 "%s.temp", filename);
 
@@ -1162,8 +1160,8 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	}
 
 	if (freq->localfile < 0) {
-		error("Couldn't create temporary file %s for %s: %s",
-		      freq->tmpfile, freq->filename, strerror(errno));
+		error("Couldn't create temporary file %s: %s",
+		      freq->tmpfile, strerror(errno));
 		goto abort;
 	}
 
@@ -1210,8 +1208,8 @@ struct http_object_request *new_http_object_request(const char *base_url,
 			prev_posn = 0;
 			lseek(freq->localfile, 0, SEEK_SET);
 			if (ftruncate(freq->localfile, 0) < 0) {
-				error("Couldn't truncate temporary file %s for %s: %s",
-					  freq->tmpfile, freq->filename, strerror(errno));
+				error("Couldn't truncate temporary file %s: %s",
+					  freq->tmpfile, strerror(errno));
 				goto abort;
 			}
 		}
@@ -1287,7 +1285,7 @@ int finish_http_object_request(struct http_object_request *freq)
 		return -1;
 	}
 	freq->rename =
-		move_temp_to_file(freq->tmpfile, freq->filename);
+		move_temp_to_file(freq->tmpfile, sha1_file_name(freq->sha1));
 
 	return freq->rename;
 }
diff --git a/http.h b/http.h
index 5c9441c..84bdbd0 100644
--- a/http.h
+++ b/http.h
@@ -152,7 +152,6 @@ struct http_pack_request
 	struct packed_git *target;
 	struct packed_git **lst;
 	FILE *packfile;
-	char filename[PATH_MAX];
 	char tmpfile[PATH_MAX];
 	struct curl_slist *range_header;
 	struct active_request_slot *slot;
@@ -167,7 +166,6 @@ extern void release_http_pack_request(struct http_pack_request *preq);
 struct http_object_request
 {
 	char *url;
-	char filename[PATH_MAX];
 	char tmpfile[PATH_MAX];
 	int localfile;
 	CURLcode curl_result;

-- 
1.7.1.rc1.269.ga27c7

Tay Ray Chuan· Apr 18, 2010, 03:36 UTC · re: Shawn O. Pearce · lore

Re: [PATCH v3 05/11] http.c: Don't store destination name in request structures

Hi,

> The destination name within the object store is easily computed
> on demand, reusing a static buffer held by sha1_file.c.  We don't
> need to copy the entire path into the request structure for safe
> keeping, when it can be easily reformatted after the download has
> been completed.
> 
> This reduces the size of the per-request structure, and removes
> yet another PATH_MAX based limit.
> 
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

-->8--
From: Tay Ray Chuan <rctay89@gmail.com>
Subject: [PATCH] http.c::new_http_pack_request: do away with the temp variable filename

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>

diff --git a/http.c b/http.c
index c75eb95..110cff9 100644
--- a/http.c
+++ b/http.c
@@ -1027,7 +1027,6 @@ int finish_http_pack_request(struct http_pack_request *preq)
 struct http_pack_request *new_http_pack_request(
        struct packed_git *target, const char *base_url)
 {
-       char *filename;
        long prev_posn = 0;
        char range[RANGE_HEADER_SIZE];
        struct strbuf buf = STRBUF_INIT;
@@ -1042,8 +1041,8 @@ struct http_pack_request *new_http_pack_request(
                sha1_to_hex(target->sha1));
        preq->url = strbuf_detach(&buf, NULL);
 
-       filename = sha1_pack_name(target->sha1);
-       snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp", filename);
+       snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp",
+               sha1_pack_name(target->sha1));
        preq->packfile = fopen(preq->tmpfile, "a");
        if (!preq->packfile) {
                error("Unable to open local file %s for pack",
--

-- 
Cheers,
Ray Chuan

Shawn O. Pearce· Apr 17, 2010, 20:07 UTC · re: Tay Ray Chuan · lore

[PATCH v3 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 http.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index c75eb95..1a52740 100644
--- a/http.c
+++ b/http.c
@@ -899,7 +899,6 @@ int http_fetch_ref(const char *base, struct ref *ref)
 static int fetch_pack_index(unsigned char *sha1, const char *base_url)
 {
 	int ret = 0;
-	char *hex = xstrdup(sha1_to_hex(sha1));
 	char *filename;
 	char *url = NULL;
 	struct strbuf buf = STRBUF_INIT;
@@ -910,10 +909,10 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)
 	}
 
 	if (http_is_verbose)
-		fprintf(stderr, "Getting index for pack %s\n", hex);
+		fprintf(stderr, "Getting index for pack %s\n", sha1_to_hex(sha1));
 
 	end_url_with_slash(&buf, base_url);
-	strbuf_addf(&buf, "objects/pack/pack-%s.idx", hex);
+	strbuf_addf(&buf, "objects/pack/pack-%s.idx", sha1_to_hex(sha1));
 	url = strbuf_detach(&buf, NULL);
 
 	filename = sha1_pack_index_name(sha1);
@@ -921,7 +920,6 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)
 		ret = error("Unable to get pack index %s\n", url);
 
 cleanup:
-	free(hex);
 	free(url);
 	return ret;
 }

-- 
1.7.1.rc1.269.ga27c7

Tay Ray Chuan· Apr 18, 2010, 03:14 UTC · re: Shawn O. Pearce · lore

Re: [PATCH v3 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result

> and its entirely trivial compared to the latency involved with the

Minor nit: s/its/is/, ie. "is entirely trivial".

Junio, perhaps you could squash that in?

> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

Acked-by: Tay Ray Chuan <rctay89@gmail.com>

-- 
Cheers,
Ray Chuan

Shawn O. Pearce· Apr 17, 2010, 20:07 UTC · re: Tay Ray Chuan · lore

[PATCH v3 07/11] Introduce close_pack_index to permit replacement

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 cache.h     |    1 +
 sha1_file.c |   11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 5eb0573..4150603 100644
--- a/cache.h
+++ b/cache.h
@@ -916,6 +916,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
 
 extern void pack_report(void);
 extern int open_pack_index(struct packed_git *);
+extern void close_pack_index(struct packed_git *);
 extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);
 extern void close_pack_windows(struct packed_git *);
 extern void unuse_pack(struct pack_window **);
diff --git a/sha1_file.c b/sha1_file.c
index ff65328..820063e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -599,6 +599,14 @@ void unuse_pack(struct pack_window **w_cursor)
 	}
 }
 
+void close_pack_index(struct packed_git *p)
+{
+	if (p->index_data) {
+		munmap((void *)p->index_data, p->index_size);
+		p->index_data = NULL;
+	}
+}
+
 /*
  * This is used by git-repack in case a newly created pack happens to
  * contain the same set of objects as an existing one.  In that case
@@ -620,8 +628,7 @@ void free_pack_by_name(const char *pack_name)
 			close_pack_windows(p);
 			if (p->pack_fd != -1)
 				close(p->pack_fd);
-			if (p->index_data)
-				munmap((void *)p->index_data, p->index_size);
+			close_pack_index(p);
 			free(p->bad_object_sha1);
 			*pp = p->next;
 			free(p);

-- 
1.7.1.rc1.269.ga27c7

Shawn O. Pearce· Apr 17, 2010, 20:07 UTC · re: Tay Ray Chuan · lore

[PATCH v3 08/11] Extract verify_pack_index for reuse from verify_pack

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 pack-check.c |   15 ++++++++++++---
 pack.h       |    1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/pack-check.c b/pack-check.c
index 166ca70..395fb95 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -133,14 +133,13 @@ static int verify_packfile(struct packed_git *p,
 	return err;
 }
 
-int verify_pack(struct packed_git *p)
+int verify_pack_index(struct packed_git *p)
 {
 	off_t index_size;
 	const unsigned char *index_base;
 	git_SHA_CTX ctx;
 	unsigned char sha1[20];
 	int err = 0;
-	struct pack_window *w_curs = NULL;
 
 	if (open_pack_index(p))
 		return error("packfile %s index not opened", p->pack_name);
@@ -154,8 +153,18 @@ int verify_pack(struct packed_git *p)
 	if (hashcmp(sha1, index_base + index_size - 20))
 		err = error("Packfile index for %s SHA1 mismatch",
 			    p->pack_name);
+	return err;
+}
+
+int verify_pack(struct packed_git *p)
+{
+	int err = 0;
+	struct pack_window *w_curs = NULL;
+
+	err |= verify_pack_index(p);
+	if (!p->index_data)
+		return -1;
 
-	/* Verify pack file */
 	err |= verify_packfile(p, &w_curs);
 	unuse_pack(&w_curs);
 
diff --git a/pack.h b/pack.h
index d268c01..bb27576 100644
--- a/pack.h
+++ b/pack.h
@@ -57,6 +57,7 @@ struct pack_idx_entry {
 
 extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);
 extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);
+extern int verify_pack_index(struct packed_git *);
 extern int verify_pack(struct packed_git *);
 extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);
 extern char *index_pack_lockfile(int fd);

-- 
1.7.1.rc1.269.ga27c7

Shawn O. Pearce· Apr 17, 2010, 20:07 UTC · re: Tay Ray Chuan · lore

[PATCH v3 09/11] Allow parse_pack_index on temporary files

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 cache.h     |    2 +-
 http.c      |    2 +-
 sha1_file.c |    3 +--
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 4150603..0d101e4 100644
--- a/cache.h
+++ b/cache.h
@@ -905,7 +905,7 @@ struct extra_have_objects {
 extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);
 extern int server_supports(const char *feature);
 
-extern struct packed_git *parse_pack_index(unsigned char *sha1);
+extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
 extern void prepare_packed_git(void);
 extern void reprepare_packed_git(void);
diff --git a/http.c b/http.c
index 1a52740..9f3dfc1 100644
--- a/http.c
+++ b/http.c
@@ -932,7 +932,7 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,
 	if (fetch_pack_index(sha1, base_url))
 		return -1;
 
-	new_pack = parse_pack_index(sha1);
+	new_pack = parse_pack_index(sha1, sha1_pack_index_name(sha1));
 	if (!new_pack)
 		return -1; /* parse_pack_index() already issued error message */
 	new_pack->next = *packs_head;
diff --git a/sha1_file.c b/sha1_file.c
index 820063e..74bba79 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -838,9 +838,8 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)
 	return p;
 }
 
-struct packed_git *parse_pack_index(unsigned char *sha1)
+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)
 {
-	const char *idx_path = sha1_pack_index_name(sha1);
 	const char *path = sha1_pack_name(sha1);
 	struct packed_git *p = alloc_packed_git(strlen(path) + 1);
 

-- 
1.7.1.rc1.269.ga27c7

Shawn O. Pearce· Apr 17, 2010, 20:07 UTC · re: Tay Ray Chuan · lore

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

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 http.c                |   43 ++++++++++++++++++++++++++++++++++++-------
 t/t5550-http-fetch.sh |   15 +++++++++++++++
 2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/http.c b/http.c
index 9f3dfc1..a7352b7 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,15 @@ 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);
+	unlink(sha1_pack_index_name(p->sha1));
 
-	p->pack_size = ftell(preq->packfile);
 	fclose(preq->packfile);
 	preq->packfile = NULL;
 	preq->slot->local = NULL;
@@ -1012,13 +1017,37 @@ 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);
+	}
 
+	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.269.ga27c7

Tay Ray Chuan· Apr 18, 2010, 03:07 UTC · re: Shawn O. Pearce · lore

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

Hi,

> diff --git a/http.c b/http.c
> index 9f3dfc1..a7352b7 100644
> --- a/http.c
> +++ b/http.c

[snip]

> @@ -998,11 +999,15 @@ void release_http_pack_request(struct http_pack_request *preq)
>  
>  int finish_http_pack_request(struct http_pack_request *preq)
>  {

[snip]

> +	unlink(sha1_pack_index_name(p->sha1));

> +	if (run_command(&ip)) {
> +		unlink(preq->tmpfile);
> +		unlink(tmp_idx);
> +		free(tmp_idx);
>  		return -1;
> -	install_packed_git(p);
> +	}

-- 
Cheers,
Ray Chuan

Shawn O. Pearce· Apr 17, 2010, 20:07 UTC · re: Tay Ray Chuan · lore

[PATCH v3 11/11] http-fetch: Use temporary files for pack-*.idx until verified

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 http.c                |   56 ++++++++++++++++++++++++++++++++++--------------
 t/t5550-http-fetch.sh |   15 +++++++++++++
 2 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/http.c b/http.c
index a7352b7..9d68d02 100644
--- a/http.c
+++ b/http.c
@@ -897,18 +897,11 @@ int http_fetch_ref(const char *base, struct ref *ref)
 }
 
 /* Helpers for fetching packs */
-static int fetch_pack_index(unsigned char *sha1, const char *base_url)
+static char *fetch_pack_index(unsigned char *sha1, const char *base_url)
 {
-	int ret = 0;
-	char *filename;
-	char *url = NULL;
+	char *url, *tmp;
 	struct strbuf buf = STRBUF_INIT;
 
-	if (has_pack_index(sha1)) {
-		ret = 0;
-		goto cleanup;
-	}
-
 	if (http_is_verbose)
 		fprintf(stderr, "Getting index for pack %s\n", sha1_to_hex(sha1));
 
@@ -916,26 +909,55 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)
 	strbuf_addf(&buf, "objects/pack/pack-%s.idx", sha1_to_hex(sha1));
 	url = strbuf_detach(&buf, NULL);
 
-	filename = sha1_pack_index_name(sha1);
-	if (http_get_file(url, filename, 0) != HTTP_OK)
-		ret = error("Unable to get pack index %s\n", url);
+	strbuf_addf(&buf, "%s.temp", sha1_pack_index_name(sha1));
+	tmp = strbuf_detach(&buf, NULL);
+
+	if (http_get_file(url, tmp, 0) != HTTP_OK) {
+		error("Unable to get pack index %s\n", url);
+		free(tmp);
+		tmp = NULL;
+	}
 
-cleanup:
 	free(url);
-	return ret;
+	return tmp;
 }
 
 static int fetch_and_setup_pack_index(struct packed_git **packs_head,
 	unsigned char *sha1, const char *base_url)
 {
 	struct packed_git *new_pack;
+	char *tmp_idx = NULL;
+	int ret;
+
+	if (has_pack_index(sha1)) {
+		new_pack = parse_pack_index(sha1, NULL);
+		if (!new_pack)
+			return -1; /* parse_pack_index() already issued error message */
+		goto add_pack;
+	}
 
-	if (fetch_pack_index(sha1, base_url))
+	tmp_idx = fetch_pack_index(sha1, base_url);
+	if (!tmp_idx)
 		return -1;
 
-	new_pack = parse_pack_index(sha1, sha1_pack_index_name(sha1));
-	if (!new_pack)
+	new_pack = parse_pack_index(sha1, tmp_idx);
+	if (!new_pack) {
+		unlink(tmp_idx);
+		free(tmp_idx);
+
 		return -1; /* parse_pack_index() already issued error message */
+	}
+
+	ret = verify_pack_index(new_pack);
+	if (!ret) {
+		close_pack_index(new_pack);
+		ret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));
+	}
+	free(tmp_idx);
+	if (ret)
+		return -1;
+
+add_pack:
 	new_pack->next = *packs_head;
 	*packs_head = new_pack;
 	return 0;
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 1a4dfc9..fc675b5 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -77,6 +77,21 @@ test_expect_success 'fetch notices corrupt pack' '
 	)
 '
 
+test_expect_success 'fetch notices corrupt idx' '
+	cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	 p=`ls objects/pack/pack-*.idx` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad2.git &&
+	(cd repo_bad2.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad2.git &&
+	 test 0 = `ls objects/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.269.ga27c7

Tay Ray Chuan· Apr 18, 2010, 03:57 UTC · re: Shawn O. Pearce · lore

Re: [PATCH v3 11/11] http-fetch: Use temporary files for pack-*.idx until verified

Hi,

> Verify that a downloaded pack-*.idx file is consistent and valid
> as an index file before we rename it into its final destination.
> This prevents a corrupt index file from later being treated as a
> usable file, confusing readers.

Perhaps this should be added in:

  Check that we do not have the pack index file before invoking
  fetch_and_setup_pack_index(); that way, we can do without the
  has_pack_index() check in fetch_and_setup_pack_index().

The above was referring to this hunk:

> diff --git a/http.c b/http.c

[snip]

> -	if (has_pack_index(sha1)) {
> -		ret = 0;
> -		goto cleanup;
> -	}
> -

-- 
Cheers,
Ray Chuan

Shawn O. Pearce· Apr 19, 2010, 14:23 UTC · re: Tay Ray Chuan · lore

[PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx

diff --git a/http.c b/http.c
index 83f6047..0813c9e 100644
--- a/http.c
+++ b/http.c
@@ -1028,7 +1028,6 @@ int finish_http_pack_request(struct http_pack_request *preq)
 	const char *ip_argv[8];
 
 	close_pack_index(p);
-	unlink(sha1_pack_index_name(p->sha1));
 
 	fclose(preq->packfile);
 	preq->packfile = NULL;
@@ -1062,6 +1061,8 @@ int finish_http_pack_request(struct http_pack_request *preq)
 		return -1;
 	}
 
+	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);

Tay Ray Chuan· Apr 19, 2010, 14:46 UTC · re: Shawn O. Pearce · lore

Re: [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx

Hi,

> This is a resend of the last half of the series, from patch 6/11
> to the end, to address some minor review comments.
> 
> Junio, I think you need to reset my branch to 0da8b2e7c80a6d
> ("http.c: Don't store destination name in structures"), and
> then apply this group.

the small patch below could also be applied to the rebased topic branch.

-->8--
From: Tay Ray Chuan <rctay89@gmail.com>
Subject: [PATCH] http.c::new_http_pack_request: do away with the temp variable filename

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>

diff --git a/http.c b/http.c
index c75eb95..110cff9 100644
--- a/http.c
+++ b/http.c
@@ -1027,7 +1027,6 @@ int finish_http_pack_request(struct http_pack_request *preq)
 struct http_pack_request *new_http_pack_request(
       struct packed_git *target, const char *base_url)
 {
-       char *filename;
       long prev_posn = 0;
       char range[RANGE_HEADER_SIZE];
       struct strbuf buf = STRBUF_INIT;
@@ -1042,8 +1041,8 @@ struct http_pack_request *new_http_pack_request(
               sha1_to_hex(target->sha1));
       preq->url = strbuf_detach(&buf, NULL);

-       filename = sha1_pack_name(target->sha1);
-       snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp", filename);
+       snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp",
+               sha1_pack_name(target->sha1));
       preq->packfile = fopen(preq->tmpfile, "a");
       if (!preq->packfile) {
               error("Unable to open local file %s for pack",
--

-- 
Cheers,
Ray Chuan

Shawn O. Pearce· Apr 19, 2010, 14:49 UTC · re: Tay Ray Chuan · lore

Re: [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx

Tay Ray Chuan <rctay89@gmail.com> wrote:

> the small patch below could also be applied to the rebased topic branch.
> 
> -->8--
> From: Tay Ray Chuan <rctay89@gmail.com>
> Subject: [PATCH] http.c::new_http_pack_request: do away with the temp variable filename
> 
> Now that the temporary variable char *filename is only used in one
> place, do away with it and just call sha1_pack_name() directly.
> 
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>

FWIW, Acked-by: Shawn O. Pearce <spearce@spearce.org>

-- 
Shawn.

Tay Ray Chuan· Apr 20, 2010, 04:33 UTC · re: Shawn O. Pearce · lore

Re: [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx

Hi Junio,

On Mon, Apr 19, 2010 at 10:49 PM, Shawn O. Pearce <spearce@spearce.org> wrote:

> Tay Ray Chuan <rctay89@gmail.com> wrote:
>> the small patch below could also be applied to the rebased topic branch.
>>
>> -->8--
>> From: Tay Ray Chuan <rctay89@gmail.com>
>> Subject: [PATCH] http.c::new_http_pack_request: do away with the temp variable filename
>>
>> Now that the temporary variable char *filename is only used in one
>> place, do away with it and just call sha1_pack_name() directly.
>>
>> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
>
> FWIW, Acked-by: Shawn O. Pearce <spearce@spearce.org>

-- 
Cheers,
Ray Chuan

Shawn O. Pearce· Apr 19, 2010, 14:23 UTC · re: Tay Ray Chuan · lore

[PATCH v4 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
Acked-by: Tay Ray Chuan <rctay89@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Message fixed, Acked-by added.
 
 No code change from v3.

 http.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index 7ee1ba5..95e3b8b 100644
--- a/http.c
+++ b/http.c
@@ -899,7 +899,6 @@ int http_fetch_ref(const char *base, struct ref *ref)
 static int fetch_pack_index(unsigned char *sha1, const char *base_url)
 {
 	int ret = 0;
-	char *hex = xstrdup(sha1_to_hex(sha1));
 	char *filename;
 	char *url = NULL;
 	struct strbuf buf = STRBUF_INIT;
@@ -910,10 +909,10 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)
 	}
 
 	if (http_is_verbose)
-		fprintf(stderr, "Getting index for pack %s\n", hex);
+		fprintf(stderr, "Getting index for pack %s\n", sha1_to_hex(sha1));
 
 	end_url_with_slash(&buf, base_url);
-	strbuf_addf(&buf, "objects/pack/pack-%s.idx", hex);
+	strbuf_addf(&buf, "objects/pack/pack-%s.idx", sha1_to_hex(sha1));
 	url = strbuf_detach(&buf, NULL);
 
 	filename = sha1_pack_index_name(sha1);
@@ -921,7 +920,6 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)
 		ret = error("Unable to get pack index %s\n", url);
 
 cleanup:
-	free(hex);
 	free(url);
 	return ret;
 }

-- 
1.7.1.rc1.279.g22727

Shawn O. Pearce· Apr 19, 2010, 14:23 UTC · re: Tay Ray Chuan · lore

[PATCH v4 07/11] Introduce close_pack_index to permit replacement

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 No change from v3.

 cache.h     |    1 +
 sha1_file.c |   11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 6e54993..0eba039 100644
--- a/cache.h
+++ b/cache.h
@@ -911,6 +911,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
 
 extern void pack_report(void);
 extern int open_pack_index(struct packed_git *);
+extern void close_pack_index(struct packed_git *);
 extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);
 extern void close_pack_windows(struct packed_git *);
 extern void unuse_pack(struct pack_window **);
diff --git a/sha1_file.c b/sha1_file.c
index c23cc5e..4e82654 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -606,6 +606,14 @@ void unuse_pack(struct pack_window **w_cursor)
 	}
 }
 
+void close_pack_index(struct packed_git *p)
+{
+	if (p->index_data) {
+		munmap((void *)p->index_data, p->index_size);
+		p->index_data = NULL;
+	}
+}
+
 /*
  * This is used by git-repack in case a newly created pack happens to
  * contain the same set of objects as an existing one.  In that case
@@ -627,8 +635,7 @@ void free_pack_by_name(const char *pack_name)
 			close_pack_windows(p);
 			if (p->pack_fd != -1)
 				close(p->pack_fd);
-			if (p->index_data)
-				munmap((void *)p->index_data, p->index_size);
+			close_pack_index(p);
 			free(p->bad_object_sha1);
 			*pp = p->next;
 			free(p);

-- 
1.7.1.rc1.279.g22727

Shawn O. Pearce· Apr 19, 2010, 14:23 UTC · re: Tay Ray Chuan · lore

[PATCH v4 08/11] Extract verify_pack_index for reuse from verify_pack

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 No change from v3.

 pack-check.c |   15 ++++++++++++---
 pack.h       |    1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/pack-check.c b/pack-check.c
index 166ca70..395fb95 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -133,14 +133,13 @@ static int verify_packfile(struct packed_git *p,
 	return err;
 }
 
-int verify_pack(struct packed_git *p)
+int verify_pack_index(struct packed_git *p)
 {
 	off_t index_size;
 	const unsigned char *index_base;
 	git_SHA_CTX ctx;
 	unsigned char sha1[20];
 	int err = 0;
-	struct pack_window *w_curs = NULL;
 
 	if (open_pack_index(p))
 		return error("packfile %s index not opened", p->pack_name);
@@ -154,8 +153,18 @@ int verify_pack(struct packed_git *p)
 	if (hashcmp(sha1, index_base + index_size - 20))
 		err = error("Packfile index for %s SHA1 mismatch",
 			    p->pack_name);
+	return err;
+}
+
+int verify_pack(struct packed_git *p)
+{
+	int err = 0;
+	struct pack_window *w_curs = NULL;
+
+	err |= verify_pack_index(p);
+	if (!p->index_data)
+		return -1;
 
-	/* Verify pack file */
 	err |= verify_packfile(p, &w_curs);
 	unuse_pack(&w_curs);
 
diff --git a/pack.h b/pack.h
index b759a23..880f9c2 100644
--- a/pack.h
+++ b/pack.h
@@ -57,6 +57,7 @@ struct pack_idx_entry {
 
 extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);
 extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);
+extern int verify_pack_index(struct packed_git *);
 extern int verify_pack(struct packed_git *);
 extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);
 extern char *index_pack_lockfile(int fd);

-- 
1.7.1.rc1.279.g22727

Shawn O. Pearce· Apr 19, 2010, 14:23 UTC · re: Tay Ray Chuan · lore

[PATCH v4 09/11] Allow parse_pack_index on temporary files

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 No change from v3.

 cache.h     |    2 +-
 http.c      |    2 +-
 sha1_file.c |    3 +--
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 0eba039..7db23ef 100644
--- a/cache.h
+++ b/cache.h
@@ -900,7 +900,7 @@ struct extra_have_objects {
 extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);
 extern int server_supports(const char *feature);
 
-extern struct packed_git *parse_pack_index(unsigned char *sha1);
+extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
 extern void prepare_packed_git(void);
 extern void reprepare_packed_git(void);
diff --git a/http.c b/http.c
index 95e3b8b..9c62632 100644
--- a/http.c
+++ b/http.c
@@ -932,7 +932,7 @@ static int fetch_and_setup_pack_index(struct packed_git **packs_head,
 	if (fetch_pack_index(sha1, base_url))
 		return -1;
 
-	new_pack = parse_pack_index(sha1);
+	new_pack = parse_pack_index(sha1, sha1_pack_index_name(sha1));
 	if (!new_pack)
 		return -1; /* parse_pack_index() already issued error message */
 	new_pack->next = *packs_head;
diff --git a/sha1_file.c b/sha1_file.c
index 4e82654..9f3f514 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -845,9 +845,8 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)
 	return p;
 }
 
-struct packed_git *parse_pack_index(unsigned char *sha1)
+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)
 {
-	const char *idx_path = sha1_pack_index_name(sha1);
 	const char *path = sha1_pack_name(sha1);
 	struct packed_git *p = alloc_packed_git(strlen(path) + 1);
 

-- 
1.7.1.rc1.279.g22727

Shawn O. Pearce· Apr 19, 2010, 14:23 UTC · re: Tay Ray Chuan · lore

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

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

Tay Ray Chuan· Apr 19, 2010, 14:35 UTC · re: Shawn O. Pearce · lore

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

Hi,

On Mon, Apr 19, 2010 at 10:23 PM, Shawn O. Pearce <spearce@spearce.org> wrote:

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

Looks good.

Acked-by: Tay Ray Chuan <rctay89@gmail.com>

-- 
Cheers,
Ray Chuan

Shawn O. Pearce· Apr 19, 2010, 14:23 UTC · re: Tay Ray Chuan · lore

[PATCH v4 11/11] http-fetch: Use temporary files for pack-*.idx until verified

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Added paragraph describing the move of has_pack_index() to the
 commit message.
 
 No code change from v3.

 http.c                |   56 ++++++++++++++++++++++++++++++++++--------------
 t/t5550-http-fetch.sh |   15 +++++++++++++
 2 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/http.c b/http.c
index 2ebd679..0813c9e 100644
--- a/http.c
+++ b/http.c
@@ -897,18 +897,11 @@ int http_fetch_ref(const char *base, struct ref *ref)
 }
 
 /* Helpers for fetching packs */
-static int fetch_pack_index(unsigned char *sha1, const char *base_url)
+static char *fetch_pack_index(unsigned char *sha1, const char *base_url)
 {
-	int ret = 0;
-	char *filename;
-	char *url = NULL;
+	char *url, *tmp;
 	struct strbuf buf = STRBUF_INIT;
 
-	if (has_pack_index(sha1)) {
-		ret = 0;
-		goto cleanup;
-	}
-
 	if (http_is_verbose)
 		fprintf(stderr, "Getting index for pack %s\n", sha1_to_hex(sha1));
 
@@ -916,26 +909,55 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)
 	strbuf_addf(&buf, "objects/pack/pack-%s.idx", sha1_to_hex(sha1));
 	url = strbuf_detach(&buf, NULL);
 
-	filename = sha1_pack_index_name(sha1);
-	if (http_get_file(url, filename, 0) != HTTP_OK)
-		ret = error("Unable to get pack index %s\n", url);
+	strbuf_addf(&buf, "%s.temp", sha1_pack_index_name(sha1));
+	tmp = strbuf_detach(&buf, NULL);
+
+	if (http_get_file(url, tmp, 0) != HTTP_OK) {
+		error("Unable to get pack index %s\n", url);
+		free(tmp);
+		tmp = NULL;
+	}
 
-cleanup:
 	free(url);
-	return ret;
+	return tmp;
 }
 
 static int fetch_and_setup_pack_index(struct packed_git **packs_head,
 	unsigned char *sha1, const char *base_url)
 {
 	struct packed_git *new_pack;
+	char *tmp_idx = NULL;
+	int ret;
+
+	if (has_pack_index(sha1)) {
+		new_pack = parse_pack_index(sha1, NULL);
+		if (!new_pack)
+			return -1; /* parse_pack_index() already issued error message */
+		goto add_pack;
+	}
 
-	if (fetch_pack_index(sha1, base_url))
+	tmp_idx = fetch_pack_index(sha1, base_url);
+	if (!tmp_idx)
 		return -1;
 
-	new_pack = parse_pack_index(sha1, sha1_pack_index_name(sha1));
-	if (!new_pack)
+	new_pack = parse_pack_index(sha1, tmp_idx);
+	if (!new_pack) {
+		unlink(tmp_idx);
+		free(tmp_idx);
+
 		return -1; /* parse_pack_index() already issued error message */
+	}
+
+	ret = verify_pack_index(new_pack);
+	if (!ret) {
+		close_pack_index(new_pack);
+		ret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));
+	}
+	free(tmp_idx);
+	if (ret)
+		return -1;
+
+add_pack:
 	new_pack->next = *packs_head;
 	*packs_head = new_pack;
 	return 0;
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 1a4dfc9..fc675b5 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -77,6 +77,21 @@ test_expect_success 'fetch notices corrupt pack' '
 	)
 '
 
+test_expect_success 'fetch notices corrupt idx' '
+	cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	 p=`ls objects/pack/pack-*.idx` &&
+	 chmod u+w $p &&
+	 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
+	) &&
+	mkdir repo_bad2.git &&
+	(cd repo_bad2.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad2.git &&
+	 test 0 = `ls objects/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

Shawn O. Pearce· Apr 15, 2010, 19:09 UTC · re: Shawn O. Pearce · lore

[PATCH 6/6] http-fetch: Use temporary files for pack-*.idx until verified

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 cache.h               |    2 +-
 http.c                |   62 +++++++++++++++++++++++++++++++-----------------
 pack-check.c          |   15 ++++++++---
 pack.h                |    1 +
 sha1_file.c           |    6 +++-
 t/t5550-http-fetch.sh |   15 ++++++++++++
 6 files changed, 72 insertions(+), 29 deletions(-)

diff --git a/cache.h b/cache.h
index 4150603..0d101e4 100644
--- a/cache.h
+++ b/cache.h
@@ -905,7 +905,7 @@ struct extra_have_objects {
 extern struct ref **get_remote_heads(int in, struct ref **list, int nr_match, char **match, unsigned int flags, struct extra_have_objects *);
 extern int server_supports(const char *feature);
 
-extern struct packed_git *parse_pack_index(unsigned char *sha1);
+extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
 extern void prepare_packed_git(void);
 extern void reprepare_packed_git(void);
diff --git a/http.c b/http.c
index aa3e380..2d88034 100644
--- a/http.c
+++ b/http.c
@@ -897,47 +897,65 @@ int http_fetch_ref(const char *base, struct ref *ref)
 }
 
 /* Helpers for fetching packs */
-static int fetch_pack_index(unsigned char *sha1, const char *base_url)
+static char *fetch_pack_index(unsigned char *sha1, const char *base_url)
 {
-	int ret = 0;
-	char *hex = xstrdup(sha1_to_hex(sha1));
-	char *filename;
-	char *url = NULL;
+	char *url, *tmp;
 	struct strbuf buf = STRBUF_INIT;
 
-	if (has_pack_index(sha1)) {
-		ret = 0;
-		goto cleanup;
-	}
-
 	if (http_is_verbose)
-		fprintf(stderr, "Getting index for pack %s\n", hex);
+		fprintf(stderr, "Getting index for pack %s\n", sha1_to_hex(sha1));
 
 	end_url_with_slash(&buf, base_url);
-	strbuf_addf(&buf, "objects/pack/pack-%s.idx", hex);
+	strbuf_addf(&buf, "objects/pack/pack-%s.idx", sha1_to_hex(sha1));
 	url = strbuf_detach(&buf, NULL);
 
-	filename = sha1_pack_index_name(sha1);
-	if (http_get_file(url, filename, 0) != HTTP_OK)
-		ret = error("Unable to get pack index %s\n", url);
+	strbuf_addf(&buf, "%s.temp", sha1_pack_index_name(sha1));
+	tmp = strbuf_detach(&buf, NULL);
+
+	if (http_get_file(url, tmp, 0) != HTTP_OK) {
+		error("Unable to get pack index %s\n", url);
+		free(tmp);
+		tmp = NULL;
+	}
 
-cleanup:
-	free(hex);
 	free(url);
-	return ret;
+	return tmp;
 }
 
 static int fetch_and_setup_pack_index(struct packed_git **packs_head,
 	unsigned char *sha1, const char *base_url)
 {
 	struct packed_git *new_pack;
+	char *tmp_idx = NULL;
 
-	if (fetch_pack_index(sha1, base_url))
-		return -1;
+	if (!has_pack_index(sha1)) {
+		tmp_idx = fetch_pack_index(sha1, base_url);
+		if (!tmp_idx)
+			return -1;
+	}
 
-	new_pack = parse_pack_index(sha1);
-	if (!new_pack)
+	new_pack = parse_pack_index(sha1, tmp_idx);
+	if (!new_pack) {
+		if (tmp_idx) {
+			unlink(tmp_idx);
+			free(tmp_idx);
+		}
 		return -1; /* parse_pack_index() already issued error message */
+	}
+
+	if (tmp_idx) {
+		int ret;
+
+		ret = verify_pack_index(new_pack);
+		if (!ret) {
+			close_pack_index(new_pack);
+			ret = move_temp_to_file(tmp_idx, sha1_pack_index_name(sha1));
+		}
+		free(tmp_idx);
+		if (ret)
+			return -1;
+	}
+
 	new_pack->next = *packs_head;
 	*packs_head = new_pack;
 	return 0;
diff --git a/pack-check.c b/pack-check.c
index 166ca70..9baba12 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -133,14 +133,13 @@ static int verify_packfile(struct packed_git *p,
 	return err;
 }
 
-int verify_pack(struct packed_git *p)
+int verify_pack_index(struct packed_git *p)
 {
 	off_t index_size;
 	const unsigned char *index_base;
 	git_SHA_CTX ctx;
 	unsigned char sha1[20];
 	int err = 0;
-	struct pack_window *w_curs = NULL;
 
 	if (open_pack_index(p))
 		return error("packfile %s index not opened", p->pack_name);
@@ -154,9 +153,17 @@ int verify_pack(struct packed_git *p)
 	if (hashcmp(sha1, index_base + index_size - 20))
 		err = error("Packfile index for %s SHA1 mismatch",
 			    p->pack_name);
+	return err;
+}
+
+int verify_pack(struct packed_git *p)
+{
+	int err = 0;
+	struct pack_window *w_curs = NULL;
 
-	/* Verify pack file */
-	err |= verify_packfile(p, &w_curs);
+	err |= verify_pack_index(p);
+	if (!err)
+		err |= verify_packfile(p, &w_curs);
 	unuse_pack(&w_curs);
 
 	return err;
diff --git a/pack.h b/pack.h
index d268c01..bb27576 100644
--- a/pack.h
+++ b/pack.h
@@ -57,6 +57,7 @@ struct pack_idx_entry {
 
 extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);
 extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);
+extern int verify_pack_index(struct packed_git *);
 extern int verify_pack(struct packed_git *);
 extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);
 extern char *index_pack_lockfile(int fd);
diff --git a/sha1_file.c b/sha1_file.c
index 820063e..232e14d 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -838,12 +838,14 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)
 	return p;
 }
 
-struct packed_git *parse_pack_index(unsigned char *sha1)
+struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)
 {
-	const char *idx_path = sha1_pack_index_name(sha1);
 	const char *path = sha1_pack_name(sha1);
 	struct packed_git *p = alloc_packed_git(strlen(path) + 1);
 
+	if (!idx_path)
+		idx_path = sha1_pack_index_name(sha1);
+
 	strcpy(p->pack_name, path);
 	hashcpy(p->sha1, sha1);
 	if (check_packed_git_idx(idx_path, p)) {
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index bdac8d7..ee170d3 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -77,6 +77,21 @@ test_expect_success 'fetch notices corrupt pack' '
 	)
 '
 
+test_expect_success 'fetch notices corrupt idx' '
+	cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/repo_pack.git "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+	 p=`ls objects/pack/pack-*.idx` &&
+	 chmod u+w $p &&
+	 dd if=/dev/zero of=$p bs=256 count=1 seek=1
+	) &&
+	mkdir repo_bad2.git &&
+	(cd repo_bad2.git &&
+	 git --bare init &&
+	 test_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad2.git &&
+	 test 0 = `ls objects/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.269.ga27c7

Re: git fetch over http:// left my repo broken

On Thu, Apr 15, 2010 at 11:51:25AM +0200, Christian Halstrick wrote:

> Hi,
> 
> some days back I fetched from a github repo with http protocol and
> afterwards my local repo was broken. Since the fetch was done by a
> cronjob I don't know whether the fetch reported an error. Problem is
> that one pack file was corrupted because the github servers put the
> repo I wanted to clone into some maintenance mode while I was
> fetching. The pack file includes at the end the html source code -
> which makes these files clearly corrupted.
> 
> Git should detect this error and let the fetch fail, right?

-Ilari