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

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

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))) {

Hmm, when moving the pack index file, should we unlink() the old,
downloaded one first?

-- 
Cheers,
Ray Chuan