Re: [PATCH 17/23] midx: read objects from multi-pack-index

From: Duy Nguyen <pclouds@gmail.com>
Date: Jun 9, 2018, 17:56 UTC
Message-ID: <CACsJy8CqoKWgd=ekxozaKzRjrU3NN9KUxrspk_iP+zxBVcV5pA@mail.gmail.com>
In-Reply-To: <20180607140338.32440-18-dstolee@microsoft.com>

On Thu, Jun 7, 2018 at 6:55 PM Derrick Stolee <stolee@gmail.com> wrote:

Show 27 quoted lines

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  midx.c         | 96 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  midx.h         |  2 ++
>  object-store.h |  1 +
>  packfile.c     |  8 ++++-
>  4 files changed, 104 insertions(+), 3 deletions(-)
>
> diff --git a/midx.c b/midx.c
> index 5e9290ca8f..6eca8f1b12 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -3,6 +3,7 @@
>  #include "dir.h"
>  #include "csum-file.h"
>  #include "lockfile.h"
> +#include "sha1-lookup.h"
>  #include "object-store.h"
>  #include "packfile.h"
>  #include "midx.h"
> @@ -64,7 +65,7 @@ struct midxed_git *load_midxed_git(const char *object_dir)
>
>         m = xcalloc(1, sizeof(*m) + strlen(object_dir) + 1);
>         strcpy(m->object_dir, object_dir);
> -       m->data = midx_map;
> +       m->data = (const unsigned char*)midx_map;

Hmm? Why is this typecast only needed now? Or is it not really needed at all?

Show 11 quoted lines

>
>         m->signature = get_be32(m->data);
>         if (m->signature != MIDX_SIGNATURE) {
> @@ -145,7 +146,9 @@ struct midxed_git *load_midxed_git(const char *object_dir)
>
>         m->num_objects = ntohl(m->chunk_oid_fanout[255]);
>
> -       m->pack_names = xcalloc(m->num_packs, sizeof(const char *));
> +       m->packs = xcalloc(m->num_packs, sizeof(*m->packs));
> +
> +       ALLOC_ARRAY(m->pack_names, m->num_packs);

Please make this ALLOC_ARRAY change in the patch that adds xcalloc(m->num_packs).

Show 20 quoted lines

>         for (i = 0; i < m->num_packs; i++) {
>                 if (i) {
>                         if (ntohl(m->chunk_pack_lookup[i]) <= ntohl(m->chunk_pack_lookup[i - 1])) {
> @@ -175,6 +178,95 @@ struct midxed_git *load_midxed_git(const char *object_dir)
>         exit(1);
>  }
>
> +static int prepare_midx_pack(struct midxed_git *m, uint32_t pack_int_id)
> +{
> +       struct strbuf pack_name = STRBUF_INIT;
> +
> +       if (pack_int_id >= m->num_packs)
> +               BUG("bad pack-int-id");
> +
> +       if (m->packs[pack_int_id])
> +               return 0;
> +
> +       strbuf_addstr(&pack_name, m->object_dir);
> +       strbuf_addstr(&pack_name, "/pack/");
> +       strbuf_addstr(&pack_name, m->pack_names[pack_int_id]);

Just use strbuf_addf()

> +
> +       m->packs[pack_int_id] = add_packed_git(pack_name.buf, pack_name.len, 1);
> +       strbuf_release(&pack_name);
> +       return !m->packs[pack_int_id];

This is a weird return value convention. Normally we go zero/negative or non-zero/zero for success/failure.

> +}

-- 
Duy