Re: [PATCH v4 0/8] rev-list: implement object type filter

[Jeff King <peff@peff.net>]

On Wed, Apr 14, 2021 at 02:07:07PM -0700, Junio C Hamano wrote:

>  * But if we look at the the hits from
> 
>     $ git grep -C2 -E -e '([.]|->)(tag|tree|blob)_objects' \*.[ch]
> 
>    it is clear that the code is (at least trying to be) prepared for
>    them to be set independently.  The .tree_objects member is often
>    checked without checking others to mark the tree objects on the
>    edge of the range uninteresting, for example.
> 
>    It of course is unknown how well the code is actually prepared
>    for these three bits to be set independently, as nobody can set
>    these bits independently.

Yeah, as somebody who has added or touched a lot of those paths, I've
often wondered this myself: what would break if you asked for blobs but
not trees? I think it does not work like you'd expect, because
list-objects.c:process_tree() will not recurse the trees at all (and
hence you'd never see any blob).

>  * Which makes a naïve reader to wonder if it would be sufficient
>    to have a silly option, like this:
> 
>  	} else if (!strcmp(arg, "--filter=object:type=tree")) {
>  		revs->tag_objects = 0;
>  		revs->tree_objects = 1;
>  		revs->blob_objects = 0;
> 
>    in the same if/else if/... cascade as the above (and other types
>    as well), in order to do the same thing as this series.
> 
> And, the above led to the question---the patches in your series
> apparently do a lot more (even if we discount the option parsing
> part), and I was wondering if that is because the independence
> between these three bits the existing code aspires to maintain is
> broken.

I don't think the code Patrick added is that much more complex. Most if
it was cleaning up rough edges in the filtering system, and making sure
that bitmaps supported this style of filtering. I _think_ the existing
bitmap code would Just Work with the example you showed above. It uses
list-objects.c to do any fill-in traversal, and then
traverse_bitmap_commit_list() uses the type-bitmaps to filter the
results.

But it would be likewise broken for the case of "no trees, just
blobs", because of the problem in list-objects.c that I mentioned (but
worse, it would only be _half_ broken; we might even produce the right
answer if we don't have to do any fill-in traversal!).

Anyway. I do think this all could be done using the existing bits in
rev_info. But there is value in making it all part of the modular
filtering system.

-Peff