On Mon, May 04, 2020 at 10:25:46PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> 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 :).
> > @@ -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
