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

[Duy Nguyen <pclouds@gmail.com>]

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:
> 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.

> +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?

> +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.

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