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

[Tay Ray Chuan <rctay89@gmail.com>]

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;
> -       }
> -

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