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

From: Jeff King <peff@peff.net>
Date: Apr 15, 2021, 09:57 UTC
Message-ID: <YHgOCUI++AY/7zwX@coredump.intra.peff.net>
In-Reply-To: <xmqqmtu0y3r8.fsf@gitster.g>

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

Show 12 quoted lines

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

Show 16 quoted lines

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