From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_PASS, SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 3DA1E1F4B4 for ; Thu, 15 Apr 2021 09:57:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230260AbhDOJ5x (ORCPT ); Thu, 15 Apr 2021 05:57:53 -0400 Received: from cloud.peff.net ([104.130.231.41]:53212 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229481AbhDOJ5x (ORCPT ); Thu, 15 Apr 2021 05:57:53 -0400 Received: (qmail 10260 invoked by uid 109); 15 Apr 2021 09:57:30 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Thu, 15 Apr 2021 09:57:30 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 14817 invoked by uid 111); 15 Apr 2021 09:57:31 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Thu, 15 Apr 2021 05:57:31 -0400 Authentication-Results: peff.net; auth=none Date: Thu, 15 Apr 2021 05:57:29 -0400 From: Jeff King To: Junio C Hamano Cc: Patrick Steinhardt , git@vger.kernel.org, Christian Couder , Taylor Blau , Philip Oakley Subject: Re: [PATCH v4 0/8] rev-list: implement object type filter Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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