Re: [PATCH v2 6/6] http-fetch: Use temporary files for pack-*.idx until verified
- From
Tay Ray Chuan <rctay89@gmail.com>
- Date
- Apr 16, 2010, 02:03 UTC
- Message-ID
- <20100416100307.0000423f@unknown>
- In-Reply-To
- <1271366704-25262-2-git-send-email-spearce@spearce.org>
Hi
On Fri, Apr 16, 2010 at 5:25 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
Show 53 quoted lines
> [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.
Show 22 quoted lines
> 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.
Show 5 quoted lines
> - 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.
Show 12 quoted lines
> 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