Re: [PATCH v3 10/21] pack-bitmap: add support for bitmap indexes

[Thomas Rast <tr@thomasrast.ch>]

TLDR: nitpicks.  Thanks for a very nice read.

I do think it's worth fixing the syntax pedantry at the end so that we
can keep supporting arcane compilers, but otherwise, meh.

> +static int open_pack_bitmap_1(struct packed_git *packfile)

This goes somewhat against the naming convention (if you can call it
that) used elsewhere in git.  Usually foo_1() is an implementation
detail of foo(), used because it is convenient to wrap the main part in
another function, e.g. so that it can consistently free resources or
some such.  But this one operates on one pack file, so in the terms of
the rest of git, it should probably be called open_pack_bitmap_one().

> +static void show_object(struct object *object, const struct name_path *path,
> +			const char *last, void *data)
> +{
> +	struct bitmap *base = data;
> +	int bitmap_pos;
> +
> +	bitmap_pos = bitmap_position(object->sha1);
> +
> +	if (bitmap_pos < 0) {
> +		char *name = path_name(path, last);
> +		bitmap_pos = ext_index_add_object(object, name);
> +		free(name);
> +	}
> +
> +	bitmap_set(base, bitmap_pos);
> +}
> +
> +static void show_commit(struct commit *commit, void *data)
> +{
> +}

A bit unfortunate that you inherit the strange show_* naming from
builtin/pack-objects.c, which seems to have stolen some code from
builtin/rev-list.c at some point without worrying about better naming...

> +static void show_objects_for_type(
> +	struct bitmap *objects,
> +	struct ewah_bitmap *type_filter,
> +	enum object_type object_type,
> +	show_reachable_fn show_reach)
> +{
[...]
> +	while (i < objects->word_alloc && ewah_iterator_next(&filter, &it)) {
> +		eword_t word = objects->words[i] & filter;
> +
> +		for (offset = 0; offset < BITS_IN_WORD; ++offset) {
> +			const unsigned char *sha1;
> +			struct revindex_entry *entry;
> +			uint32_t hash = 0;
> +
> +			if ((word >> offset) == 0)
> +				break;
> +
> +			offset += ewah_bit_ctz64(word >> offset);
> +
> +			if (pos + offset < bitmap_git.reuse_objects)
> +				continue;
> +
> +			entry = &bitmap_git.reverse_index->revindex[pos + offset];
> +			sha1 = nth_packed_object_sha1(bitmap_git.pack, entry->nr);
> +
> +			show_reach(sha1, object_type, 0, hash, bitmap_git.pack, entry->offset);
> +		}

You have a very nice bitmap_each_bit() function in ewah/bitmap.c, why
not use it here?

> +int reuse_partial_packfile_from_bitmap(struct packed_git **packfile,
> +				       uint32_t *entries,
> +				       off_t *up_to)
> +{
> +	/*
> +	 * Reuse the packfile content if we need more than
> +	 * 90% of its objects
> +	 */
> +	static const double REUSE_PERCENT = 0.9;

Curious: is this based on some measurements or just a guess?


> diff --git a/pack-bitmap.h b/pack-bitmap.h
[...]
> +static const char BITMAP_IDX_SIGNATURE[] = {'B', 'I', 'T', 'M'};;

There's a stray ; at the end of the line that is technically not
permitted:

pack-bitmap.h:22:65: warning: ISO C does not allow extra ‘;’ outside of a function [-Wpedantic]

> +enum pack_bitmap_opts {
> +	BITMAP_OPT_FULL_DAG = 1,

And I think this trailing comma on the last enum item is also strictly
speaking not allowed, even though it is very nice to have:

pack-bitmap.h:28:27: warning: comma at end of enumerator list [-Wpedantic]

-- 
Thomas Rast
tr@thomasrast.ch