Re: [PATCH 3/4] pack-bitmap.c: support 'tree:0' filtering

From: Taylor Blau <me@ttaylorr.com>
Date: May 5, 2020, 15:59 UTC
Message-ID: <20200505155951.GA69300@syl.local>
In-Reply-To: <xmqq7dxrc5r9.fsf@gitster.c.googlers.com>

On Mon, May 04, 2020 at 10:25:46PM -0700, Junio C Hamano wrote:

Show 20 quoted lines

> Taylor Blau <me@ttaylorr.com> writes:
>
> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index 3693c9e62f..195ee8cad0 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -749,7 +749,7 @@ static void filter_bitmap_exclude_type(struct bitmap_index *bitmap_git,
> >  	eword_t mask;
> >  	uint32_t i;
> >
> > -	if (type != OBJ_BLOB)
> > +	if (type != OBJ_BLOB && type != OBJ_TREE)
> >  		BUG("filter_bitmap_exclude_type: unsupported type '%d'", type);
>
> OK.  This is the same as the previous step, but why would we even
> need this guard?  find_tip_objects() is equipped to find tips of any
> object type, iterating on the bitmap for "type", or flipping the
> bits in the to_filter bitmap, does not have any limitation to the
> blob type in the previous step, and there is no limitation to the
> blob or tree types after this step, either, no?

I think we need some sort of guard here, since we could receive any value of object_type, but you're right that this isn't the right one. It should probably be something like:

  if (type < OBJ_COMMIT || type > OBJ_TAG)

to pick out the sentinel values like OBJ_BAD and OBJ_NONE, as well as the pack-specific types, like OBJ_OFS_DELTA and so on.

I fixed this locally, and will resend it along with the rest of v2 in a day or so. Thanks for a review :).

Show 45 quoted lines

> > @@ -867,6 +867,20 @@ static void filter_bitmap_blob_limit(struct bitmap_index *bitmap_git,
> >  	bitmap_free(tips);
> >  }
> >
> > +static void filter_bitmap_tree_depth(struct bitmap_index *bitmap_git,
> > +				     struct object_list *tip_objects,
> > +				     struct bitmap *to_filter,
> > +				     unsigned long limit)
> > +{
> > +	if (limit)
> > +		BUG("filter_bitmap_tree_depth given non-zero limit");
>
> This one does make sense, because the code to exclude all trees and
> all blobs we have below won't be able to cull only trees at a given
> depth or deeper.
>
> > +	filter_bitmap_exclude_type(bitmap_git, tip_objects, to_filter,
> > +				   OBJ_TREE);
> > +	filter_bitmap_exclude_type(bitmap_git, tip_objects, to_filter,
> > +				   OBJ_BLOB);
>
> And these two are quite straight-forward.
>
> > +}
> > +
> >  static int filter_bitmap(struct bitmap_index *bitmap_git,
> >  			 struct object_list *tip_objects,
> >  			 struct bitmap *to_filter,
> > @@ -890,6 +904,15 @@ static int filter_bitmap(struct bitmap_index *bitmap_git,
> >  		return 0;
> >  	}
> >
> > +	if (filter->choice == LOFC_TREE_DEPTH &&
> > +	    filter->tree_exclude_depth == 0) {
> > +		if (bitmap_git)
> > +			filter_bitmap_tree_depth(bitmap_git, tip_objects,
> > +						 to_filter,
> > +						 filter->tree_exclude_depth);
>
> I briefly wondered if it is cleaner to read if we hardcode 0 as the
> last argument.  But if the helper function ever learns how to filter
> by tree with non-zero depth, we can only tweak the if() condition
> without changing the call, so the way you wrote it is the right way.
>
> Thanks.

Thanks, Taylor