Re: [PATCH v2 6/8] pack-bitmap: implement object type filter

From: Jeff King <peff@peff.net>
Date: Apr 6, 2021, 17:48 UTC
Message-ID: <YGyfBc9I6YtyDWSr@coredump.intra.peff.net>
In-Reply-To: <8073ab665b07cf653478482f801a06e072233230.1615813673.git.ps@pks.im>

On Mon, Mar 15, 2021 at 02:14:55PM +0100, Patrick Steinhardt wrote:

> The preceding commit has added a new object filter for git-rev-list(1)
> which allows to filter objects by type. Implement the equivalent filter
> for packfile bitmaps so that we can answer these queries fast.

Makes sense. The implementation looks pretty sensible. One observation:

Show 16 quoted lines

> +static void filter_bitmap_object_type(struct bitmap_index *bitmap_git,
> +				      struct object_list *tip_objects,
> +				      struct bitmap *to_filter,
> +				      enum object_type object_type)
> +{
> +	enum object_type t;
> +
> +	if (object_type < OBJ_COMMIT || object_type > OBJ_TAG)
> +		BUG("filter_bitmap_object_type given invalid object");
> +
> +	for (t = OBJ_COMMIT; t <= OBJ_TAG; t++) {
> +		if (t == object_type)
> +			continue;
> +		filter_bitmap_exclude_type(bitmap_git, tip_objects, to_filter, t);
> +	}
> +}

The OBJ_* constants are a contiguous set between COMMIT and TAG, and it has to remain this way (because we use them to decipher the type fields in pack files). But I don't think we've generally baked that assumption into the code in this way.

Writing it out long-hand would be something like:

  if (t != OBJ_COMMIT)
	filter_bitmap_exclude_type(bitmap_git, tip_objects, to_filter, OBJ_COMMIT);
  if (t != OBJ_TREE)
	filter_bitmap_exclude_type(bitmap_git, tip_objects, to_filter, OBJ_TREE);

and so on, which isn't too bad. I dunno. That may be overly picky.

-Peff