* [PATCH v3 01/11] http.c: Remove bad free of static block
2010-04-16 2:03 ` Tay Ray Chuan
@ 2010-04-17 20:07 ` Shawn O. Pearce
2010-04-17 20:07 ` [PATCH v3 02/11] t5550-http-fetch: Use subshell for repository operations Shawn O. Pearce
` (9 subsequent siblings)
10 siblings, 0 replies; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-17 20:07 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
The filename variable here is pointing to a block of memory that
was allocated by sha1_file.c and is also held in a static variable
scoped within the sha1_pack_name() function. Doing a free() here is
returning that memory to the allocator while we might still try to
reuse it on a subsequent sha1_pack_name() invocation. That's not
acceptable, so don't free it.
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v3 02/11] t5550-http-fetch: Use subshell for repository operations
2010-04-16 2:03 ` Tay Ray Chuan
2010-04-17 20:07 ` [PATCH v3 01/11] http.c: Remove bad free of static block Shawn O. Pearce
@ 2010-04-17 20:07 ` Shawn O. Pearce
2010-04-17 20:07 ` [PATCH v3 03/11] http.c: Tiny refactoring of finish_http_pack_request Shawn O. Pearce
` (8 subsequent siblings)
10 siblings, 0 replies; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-17 20:07 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
Change into the server repository's directory using a subshell,
so we can return back to the top of the trash directory before
doing anything more in the test script.
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v3 03/11] http.c: Tiny refactoring of finish_http_pack_request
2010-04-16 2:03 ` Tay Ray Chuan
2010-04-17 20:07 ` [PATCH v3 01/11] http.c: Remove bad free of static block Shawn O. Pearce
2010-04-17 20:07 ` [PATCH v3 02/11] t5550-http-fetch: Use subshell for repository operations Shawn O. Pearce
@ 2010-04-17 20:07 ` Shawn O. Pearce
2010-04-17 20:07 ` [PATCH v3 04/11] http.c: Drop useless != NULL test in finish_http_pack_request Shawn O. Pearce
` (7 subsequent siblings)
10 siblings, 0 replies; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-17 20:07 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
Always remove the struct packed_git from the active list, even
if the rename of the temporary file fails.
While we are here, simplify the code a bit by using a common
local variable name ("p") to hold the relevant packed_git.
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v3 04/11] http.c: Drop useless != NULL test in finish_http_pack_request
2010-04-16 2:03 ` Tay Ray Chuan
` (2 preceding siblings ...)
2010-04-17 20:07 ` [PATCH v3 03/11] http.c: Tiny refactoring of finish_http_pack_request Shawn O. Pearce
@ 2010-04-17 20:07 ` Shawn O. Pearce
2010-04-17 20:07 ` [PATCH v3 05/11] http.c: Don't store destination name in request structures Shawn O. Pearce
` (6 subsequent siblings)
10 siblings, 0 replies; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-17 20:07 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
The test preq->packfile != NULL is always true. If packfile was
actually NULL when entering this function the ftell() above would
crash out with a SIGSEGV, resulting in never reaching this point.
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v3 05/11] http.c: Don't store destination name in request structures
2010-04-16 2:03 ` Tay Ray Chuan
` (3 preceding siblings ...)
2010-04-17 20:07 ` [PATCH v3 04/11] http.c: Drop useless != NULL test in finish_http_pack_request Shawn O. Pearce
@ 2010-04-17 20:07 ` Shawn O. Pearce
2010-04-18 3:36 ` Tay Ray Chuan
2010-04-17 20:07 ` [PATCH v3 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result Shawn O. Pearce
` (5 subsequent siblings)
10 siblings, 1 reply; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-17 20:07 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
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>
---
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v3 05/11] http.c: Don't store destination name in request structures
2010-04-17 20:07 ` [PATCH v3 05/11] http.c: Don't store destination name in request structures Shawn O. Pearce
@ 2010-04-18 3:36 ` Tay Ray Chuan
0 siblings, 0 replies; 46+ messages in thread
From: Tay Ray Chuan @ 2010-04-18 3:36 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Junio C Hamano, git, Johannes Sixt, Ilari Liusvaara,
Michael J Gruber, Christian Halstrick, jan.sievers, Matthias Sohn
Hi,
On Sat, 17 Apr 2010 13:07:38 -0700
"Shawn O. Pearce" <spearce@spearce.org> wrote:
> 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>
now that there's a single user of char *filename, we might as well do
away with it.
PS. I think having the below as a separate patch is better than
squashing it in, as it might be detrimental to patch #05's readability
in the latter case.
-->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>
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
^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH v3 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result
2010-04-16 2:03 ` Tay Ray Chuan
` (4 preceding siblings ...)
2010-04-17 20:07 ` [PATCH v3 05/11] http.c: Don't store destination name in request structures Shawn O. Pearce
@ 2010-04-17 20:07 ` Shawn O. Pearce
2010-04-18 3:14 ` Tay Ray Chuan
2010-04-17 20:07 ` [PATCH v3 07/11] Introduce close_pack_index to permit replacement Shawn O. Pearce
` (4 subsequent siblings)
10 siblings, 1 reply; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-17 20:07 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
Most of the time the dumb HTTP transport is run without the verbose
flag set, so we only need the result of sha1_to_hex(sha1) once, to
construct the pack URL. Don't bother with an unnecessary malloc,
copy, free chain of this buffer.
If verbose is set, we'll format the SHA-1 twice now. But this
tiny extra CPU time spent is nothing compared to the slowdown that
is usually imposed by the verbose messages being sent to the tty,
and its entirely trivial compared to the latency involved with the
remote HTTP server sending something as big as a pack file.
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v3 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result
2010-04-17 20:07 ` [PATCH v3 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result Shawn O. Pearce
@ 2010-04-18 3:14 ` Tay Ray Chuan
0 siblings, 0 replies; 46+ messages in thread
From: Tay Ray Chuan @ 2010-04-18 3:14 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Junio C Hamano, git, Johannes Sixt, Ilari Liusvaara,
Michael J Gruber, Christian Halstrick, jan.sievers, Matthias Sohn
On Sat, 17 Apr 2010 13:07:39 -0700
"Shawn O. Pearce" <spearce@spearce.org> wrote:
> 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
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v3 07/11] Introduce close_pack_index to permit replacement
2010-04-16 2:03 ` Tay Ray Chuan
` (5 preceding siblings ...)
2010-04-17 20:07 ` [PATCH v3 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result Shawn O. Pearce
@ 2010-04-17 20:07 ` Shawn O. Pearce
2010-04-17 20:07 ` [PATCH v3 08/11] Extract verify_pack_index for reuse from verify_pack Shawn O. Pearce
` (3 subsequent siblings)
10 siblings, 0 replies; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-17 20:07 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
By closing the pack index, a caller can later overwrite the index
with an updated index file, possibly after converting from v1 to
the v2 format. Because p->index_data is NULL after close, on the
next access the index will be opened again and the other members
will be updated with new data.
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v3 08/11] Extract verify_pack_index for reuse from verify_pack
2010-04-16 2:03 ` Tay Ray Chuan
` (6 preceding siblings ...)
2010-04-17 20:07 ` [PATCH v3 07/11] Introduce close_pack_index to permit replacement Shawn O. Pearce
@ 2010-04-17 20:07 ` Shawn O. Pearce
2010-04-17 20:07 ` [PATCH v3 09/11] Allow parse_pack_index on temporary files Shawn O. Pearce
` (2 subsequent siblings)
10 siblings, 0 replies; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-17 20:07 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
The dumb HTTP transport should verify an index is completely valid
before trying to use it. That requires checking the header/footer
but also checking the complete content SHA-1. All of this logic is
already in the front half of verify_pack, so pull it out into a new
function that can be reused.
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v3 09/11] Allow parse_pack_index on temporary files
2010-04-16 2:03 ` Tay Ray Chuan
` (7 preceding siblings ...)
2010-04-17 20:07 ` [PATCH v3 08/11] Extract verify_pack_index for reuse from verify_pack Shawn O. Pearce
@ 2010-04-17 20:07 ` Shawn O. Pearce
2010-04-17 20:07 ` [PATCH v3 10/11] http-fetch: Use index-pack rather than verify-pack to check packs Shawn O. Pearce
2010-04-17 20:07 ` [PATCH v3 11/11] http-fetch: Use temporary files for pack-*.idx until verified Shawn O. Pearce
10 siblings, 0 replies; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-17 20:07 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
The easiest way to verify a pack index is to open it through the
standard parse_pack_index function, permitting the header check
to happen when the file is mapped. However, the dumb HTTP client
needs to verify a pack index before its moved into its proper file
name within the objects/pack directory, to prevent a corrupt index
from being made available. So permit the caller to specify the
exact path of the index file.
For now we're still using the final destination name within the
sole call site in http.c, but eventually we will start to parse
the temporary path instead.
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v3 10/11] http-fetch: Use index-pack rather than verify-pack to check packs
2010-04-16 2:03 ` Tay Ray Chuan
` (8 preceding siblings ...)
2010-04-17 20:07 ` [PATCH v3 09/11] Allow parse_pack_index on temporary files Shawn O. Pearce
@ 2010-04-17 20:07 ` Shawn O. Pearce
2010-04-18 3:07 ` Tay Ray Chuan
2010-04-17 20:07 ` [PATCH v3 11/11] http-fetch: Use temporary files for pack-*.idx until verified Shawn O. Pearce
10 siblings, 1 reply; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-17 20:07 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
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>
---
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v3 10/11] http-fetch: Use index-pack rather than verify-pack to check packs
2010-04-17 20:07 ` [PATCH v3 10/11] http-fetch: Use index-pack rather than verify-pack to check packs Shawn O. Pearce
@ 2010-04-18 3:07 ` Tay Ray Chuan
0 siblings, 0 replies; 46+ messages in thread
From: Tay Ray Chuan @ 2010-04-18 3:07 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Junio C Hamano, git, Johannes Sixt, Ilari Liusvaara,
Michael J Gruber, Christian Halstrick, jan.sievers, Matthias Sohn
Hi,
On Sat, 17 Apr 2010 13:07:43 -0700
"Shawn O. Pearce" <spearce@spearce.org> wrote:
> 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));
I think this should be done later, after we have run index-pack
successfully. A good place would be probably after the if() block here:
> + if (run_command(&ip)) {
> + unlink(preq->tmpfile);
> + unlink(tmp_idx);
> + free(tmp_idx);
> return -1;
> - install_packed_git(p);
> + }
--
Cheers,
Ray Chuan
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v3 11/11] http-fetch: Use temporary files for pack-*.idx until verified
2010-04-16 2:03 ` Tay Ray Chuan
` (9 preceding siblings ...)
2010-04-17 20:07 ` [PATCH v3 10/11] http-fetch: Use index-pack rather than verify-pack to check packs Shawn O. Pearce
@ 2010-04-17 20:07 ` Shawn O. Pearce
2010-04-18 3:57 ` Tay Ray Chuan
10 siblings, 1 reply; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-17 20:07 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
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.
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v3 11/11] http-fetch: Use temporary files for pack-*.idx until verified
2010-04-17 20:07 ` [PATCH v3 11/11] http-fetch: Use temporary files for pack-*.idx until verified Shawn O. Pearce
@ 2010-04-18 3:57 ` Tay Ray Chuan
2010-04-19 14:23 ` [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx Shawn O. Pearce
` (6 more replies)
0 siblings, 7 replies; 46+ messages in thread
From: Tay Ray Chuan @ 2010-04-18 3:57 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Junio C Hamano, git, Johannes Sixt, Ilari Liusvaara,
Michael J Gruber, Christian Halstrick, jan.sievers, Matthias Sohn
Hi,
On Sat, 17 Apr 2010 13:07:44 -0700
"Shawn O. Pearce" <spearce@spearce.org> wrote:
> 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
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx
2010-04-18 3:57 ` Tay Ray Chuan
@ 2010-04-19 14:23 ` Shawn O. Pearce
2010-04-19 14:46 ` Tay Ray Chuan
2010-04-19 14:23 ` [PATCH v4 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result Shawn O. Pearce
` (5 subsequent siblings)
6 siblings, 1 reply; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-19 14:23 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
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.
Total series diff since v3 is this shocking one line change, most
of the edits were to commit messages:
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);
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx
2010-04-19 14:23 ` [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx Shawn O. Pearce
@ 2010-04-19 14:46 ` Tay Ray Chuan
2010-04-19 14:49 ` Shawn O. Pearce
0 siblings, 1 reply; 46+ messages in thread
From: Tay Ray Chuan @ 2010-04-19 14:46 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Junio C Hamano, git, Johannes Sixt, Ilari Liusvaara,
Michael J Gruber, Christian Halstrick, jan.sievers, Matthias Sohn
Hi,
On Mon, 19 Apr 2010 07:23:04 -0700
"Shawn O. Pearce" <spearce@spearce.org> wrote:
> 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
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>
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx
2010-04-19 14:46 ` Tay Ray Chuan
@ 2010-04-19 14:49 ` Shawn O. Pearce
2010-04-20 4:33 ` Tay Ray Chuan
0 siblings, 1 reply; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-19 14:49 UTC (permalink / raw)
To: Tay Ray Chuan
Cc: Junio C Hamano, git, Johannes Sixt, Ilari Liusvaara,
Michael J Gruber, Christian Halstrick, jan.sievers, Matthias Sohn
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.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx
2010-04-19 14:49 ` Shawn O. Pearce
@ 2010-04-20 4:33 ` Tay Ray Chuan
0 siblings, 0 replies; 46+ messages in thread
From: Tay Ray Chuan @ 2010-04-20 4:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
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>
I noticed that the inlined patch (in message
<20100419224643.00001ff1@unknown>) being acked here wasn't applied to
the topic branch 'sp/maint-dumb-http-pack-reidx' in pu; just a
heads-up in case you've missed something.
--
Cheers,
Ray Chuan
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result
2010-04-18 3:57 ` Tay Ray Chuan
2010-04-19 14:23 ` [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx Shawn O. Pearce
@ 2010-04-19 14:23 ` Shawn O. Pearce
2010-04-19 14:23 ` [PATCH v4 07/11] Introduce close_pack_index to permit replacement Shawn O. Pearce
` (4 subsequent siblings)
6 siblings, 0 replies; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-19 14:23 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn, Junio C Hamano
Most of the time the dumb HTTP transport is run without the verbose
flag set, so we only need the result of sha1_to_hex(sha1) once, to
construct the pack URL. Don't bother with an unnecessary malloc,
copy, free chain of this buffer.
If verbose is set, we'll format the SHA-1 twice now. But this
tiny extra CPU time spent is nothing compared to the slowdown that
is usually imposed by the verbose messages being sent to the tty,
and is entirely trivial compared to the latency involved with the
remote HTTP server sending something as big as a pack file.
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v4 07/11] Introduce close_pack_index to permit replacement
2010-04-18 3:57 ` Tay Ray Chuan
2010-04-19 14:23 ` [PATCH v4 00/11] Resend sp/maint-dumb-http-pack-reidx Shawn O. Pearce
2010-04-19 14:23 ` [PATCH v4 06/11] http.c: Remove unnecessary strdup of sha1_to_hex result Shawn O. Pearce
@ 2010-04-19 14:23 ` Shawn O. Pearce
2010-04-19 14:23 ` [PATCH v4 08/11] Extract verify_pack_index for reuse from verify_pack Shawn O. Pearce
` (3 subsequent siblings)
6 siblings, 0 replies; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-19 14:23 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn, Junio C Hamano
By closing the pack index, a caller can later overwrite the index
with an updated index file, possibly after converting from v1 to
the v2 format. Because p->index_data is NULL after close, on the
next access the index will be opened again and the other members
will be updated with new data.
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v4 08/11] Extract verify_pack_index for reuse from verify_pack
2010-04-18 3:57 ` Tay Ray Chuan
` (2 preceding siblings ...)
2010-04-19 14:23 ` [PATCH v4 07/11] Introduce close_pack_index to permit replacement Shawn O. Pearce
@ 2010-04-19 14:23 ` Shawn O. Pearce
2010-04-19 14:23 ` [PATCH v4 09/11] Allow parse_pack_index on temporary files Shawn O. Pearce
` (2 subsequent siblings)
6 siblings, 0 replies; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-19 14:23 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn, Junio C Hamano
The dumb HTTP transport should verify an index is completely valid
before trying to use it. That requires checking the header/footer
but also checking the complete content SHA-1. All of this logic is
already in the front half of verify_pack, so pull it out into a new
function that can be reused.
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v4 09/11] Allow parse_pack_index on temporary files
2010-04-18 3:57 ` Tay Ray Chuan
` (3 preceding siblings ...)
2010-04-19 14:23 ` [PATCH v4 08/11] Extract verify_pack_index for reuse from verify_pack Shawn O. Pearce
@ 2010-04-19 14:23 ` Shawn O. Pearce
2010-04-19 14:23 ` [PATCH v4 10/11] http-fetch: Use index-pack rather than verify-pack to check packs Shawn O. Pearce
2010-04-19 14:23 ` [PATCH v4 11/11] http-fetch: Use temporary files for pack-*.idx until verified Shawn O. Pearce
6 siblings, 0 replies; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-19 14:23 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn, Junio C Hamano
The easiest way to verify a pack index is to open it through the
standard parse_pack_index function, permitting the header check
to happen when the file is mapped. However, the dumb HTTP client
needs to verify a pack index before its moved into its proper file
name within the objects/pack directory, to prevent a corrupt index
from being made available. So permit the caller to specify the
exact path of the index file.
For now we're still using the final destination name within the
sole call site in http.c, but eventually we will start to parse
the temporary path instead.
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* [PATCH v4 10/11] http-fetch: Use index-pack rather than verify-pack to check packs
2010-04-18 3:57 ` Tay Ray Chuan
` (4 preceding siblings ...)
2010-04-19 14:23 ` [PATCH v4 09/11] Allow parse_pack_index on temporary files Shawn O. Pearce
@ 2010-04-19 14:23 ` Shawn O. Pearce
2010-04-19 14:35 ` Tay Ray Chuan
2010-04-19 14:23 ` [PATCH v4 11/11] http-fetch: Use temporary files for pack-*.idx until verified Shawn O. Pearce
6 siblings, 1 reply; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-19 14:23 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 10/11] http-fetch: Use index-pack rather than verify-pack to check packs
2010-04-19 14:23 ` [PATCH v4 10/11] http-fetch: Use index-pack rather than verify-pack to check packs Shawn O. Pearce
@ 2010-04-19 14:35 ` Tay Ray Chuan
0 siblings, 0 replies; 46+ messages in thread
From: Tay Ray Chuan @ 2010-04-19 14:35 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Junio C Hamano, git, Johannes Sixt, Ilari Liusvaara,
Michael J Gruber, Christian Halstrick, jan.sievers, Matthias Sohn
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 11/11] http-fetch: Use temporary files for pack-*.idx until verified
2010-04-18 3:57 ` Tay Ray Chuan
` (5 preceding siblings ...)
2010-04-19 14:23 ` [PATCH v4 10/11] http-fetch: Use index-pack rather than verify-pack to check packs Shawn O. Pearce
@ 2010-04-19 14:23 ` Shawn O. Pearce
6 siblings, 0 replies; 46+ messages in thread
From: Shawn O. Pearce @ 2010-04-19 14:23 UTC (permalink / raw)
To: Junio C Hamano, Tay Ray Chuan
Cc: git, Johannes Sixt, Ilari Liusvaara, Michael J Gruber,
Christian Halstrick, jan.sievers, Matthias Sohn, Junio C Hamano
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.
Check that we do not have the pack index file before invoking
fetch_pack_index(); that way, we can do without the has_pack_index()
check in fetch_pack_index().
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
^ permalink raw reply related [flat|nested] 46+ messages in thread