Hi
On Fri, Apr 16, 2010 at 5:25 AM, Shawn O. Pearce <spearce@xxxxxxxxxxx> 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;
> - }
> -
It probably should be mentioned in the commit message or elsewhere that
as fetch_and_setup_pack_index() now checks for the pack index locally
before fetching, we no longer need this check.
> 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;
The conflation of "ret" as the result of both verify_pack_index() and
move_temp_to_file() is pretty confusing.
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html