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

From: Thomas Rast <tr@thomasrast.ch>
Date: Nov 29, 2013, 21:21 UTC
Message-ID: <87siuedhvj.fsf@thomasrast.ch>
In-Reply-To: <20131114124432.GJ10757@sigill.intra.peff.net>

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().

Show 20 quoted lines

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

Show 6 quoted lines

> +static void show_objects_for_type(
> +	struct bitmap *objects,
> +	struct ewah_bitmap *type_filter,
> +	enum object_type object_type,
> +	show_reachable_fn show_reach)
> +{

[...]

Show 21 quoted lines

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

Show 9 quoted lines

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