Re: [PATCH v2 10/19] pack-bitmap: add support for bitmap indexes

From: Duy Nguyen <pclouds@gmail.com>
Date: Oct 26, 2013, 10:14 UTC
Message-ID: <CACsJy8Cv5WMB=L+fQCj-ZURZC3ZdEqXaqqW_O2ZD-HNcC_V3yg@mail.gmail.com>
In-Reply-To: <20131025060345.GH23098@sigill.intra.peff.net>

If it's not mentioned yet, maybe you should note that this code currently supports only one pack with .bitmap file.

On Fri, Oct 25, 2013 at 1:03 PM, Jeff King <peff@peff.net> wrote:

Show 6 quoted lines

> diff --git a/khash.h b/khash.h
> new file mode 100644
> index 0000000..0fdf39d
> --- /dev/null
> +++ b/khash.h
> @@ -0,0 +1,342 @@

I notice the line continuations '\' in this file look more aligned if tab length is set to 4. No idea how many emacs users out there but it probably does not harm to put

/* -*- mode: c; tab-width: 4; -*- */

at the beginning of this file? Another option is realign the file, which I doubt is good because this file is imported.

Show 16 quoted lines

> +static int load_pack_bitmap(void)
> +{
> +       assert(bitmap_git.map && !bitmap_git.loaded);
> +
> +       bitmap_git.bitmaps = kh_init_sha1();
> +       bitmap_git.ext_index.positions = kh_init_sha1_pos();
> +       bitmap_git.reverse_index = revindex_for_pack(bitmap_git.pack);
> +
> +       if (!(bitmap_git.commits = read_bitmap_1(&bitmap_git)) ||
> +               !(bitmap_git.trees = read_bitmap_1(&bitmap_git)) ||
> +               !(bitmap_git.blobs = read_bitmap_1(&bitmap_git)) ||
> +               !(bitmap_git.tags = read_bitmap_1(&bitmap_git)))
> +               goto failed;
> +
> +       if (load_bitmap_entries_v1(&bitmap_git) < 0)
> +               goto failed;

I don't see any mechanism to protect us from corrupt .bitmap files. If .bitmap is not very large, maybe just check the trailing checksum in the file when we open it? Else maybe add a crc32 or something after each commit bitmap in .bitmap v2 and only verify the ones we actually use?

Show 10 quoted lines

> +static int open_pack_bitmap(void)
> +{
> +       struct packed_git *p;
> +
> +       assert(!bitmap_git.map && !bitmap_git.loaded);
> +
> +       prepare_packed_git();
> +       for (p = packed_git; p; p = p->next) {
> +               if (open_pack_bitmap_1(p) == 0)
> +                       return 0;

It maybe a good idea to go on anyway, checking for another .bitmap. Just warn the user about that if found.

Show 5 quoted lines

> +       }
> +
> +       return -1;
> +}
> +

-- 
Duy