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

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 14, 2021, 11:59 UTC
Message-ID: <YHbZNL0gzw0+1k5h@ncase>
In-Reply-To: <xmqqfsztlwxb.fsf@gitster.g>

On Tue, Apr 13, 2021 at 02:03:12PM -0700, Junio C Hamano wrote:

Show 34 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > Thanks. I have no more nits to pick. The only question left for me is
> > the big one of "do we like this with --filter, or should it be some kind
> > of rev-list option", as discussed in:
> >
> >   https://lore.kernel.org/git/YHB7R8hVRt+V+i2W@coredump.intra.peff.net/
> 
> I do agree that adding "--blobs", "--trees", and "--tags" options to
> the "--objects" (and implicit --commits) to rev-list parameters is a
> lot more in line with the original design by Linus when we added
> "--objects" (vs not giving it).  We even internally have had the "do
> we show trees?" "do we show blobs?" separate bits from the very
> beginning of the "--objects" feature at 9de48752 (git-rev-list: add
> option to list all objects (not just commits), 2005-06-24), which
> was extended to cover tags at 3c90f03d (Prepare git-rev-list for
> tracking tag objects too, 2005-06-29).  The basic design principle
> hasn't changed when the code was reorganized to be closer to the
> current shape at ae563542 (First cut at libifying revlist
> generation, 2006-02-25).
> 
> But back then, we didn't have mechanisms to filter rev-list output
> using other criteria, which brought us the umbrella notation to use
> "--filter=...", so as a notation, it might be OK, provided if
> 
> 	git rev-list \
> 	    --filter=object:type=tag \
> 	    --filter=object:type=commit \
> 	    --filter=object:type=tree \
> 	    --filter=object:type=blob "$@ther args"
> 
> is an equivalent to existing
> 
> 	git rev-list --objects "$@ther args"

Filters are currently AND filters, so specifying them multiple times would cause us to print nothing. And I don't think we should treat object:type filters any different compared to the other filters, because that could limit our ability to iterate in the future where we may want to add OR filters.

I initially said that I didn't want to add `object:type=tag,commit` filters as a way to say that all types which are either a tag or commit should be printed because I thought that having the ability to combine filters with an OR instead of an AND would be the better design here. But on the other hand, there is no reason we cannot have both, and it would implement your above usecase, even though syntax is different.

Show 10 quoted lines

> I however have to wonder why this need so much code (in other words,
> why do we need more than just adding something similar to this block
> in the revision.c machinery:
> 
> 	} else if (!strcmp(arg, "--objects")) {
> 		revs->tag_objects = 1;
> 		revs->tree_objects = 1;
> 		revs->blob_objects = 1;
> 
> that flips <type>_objects member bits?) though.

So if we make this part of the rev machinery, I guess your idea is that we can just put the logic into `show_object()`? That would indeed be a lot simpler to implement with a lot less code. But on the other hand, it's also less efficient: we cannot stop walking down the graph like we can with the current design when e.g. saying "Only give me {tags,commits,trees}". And with bitmap indices, we can even skip all objects of a certain type at once, without having to load the object, right inside the filtering logic. If this was part of `show_object()`, we wouldn't be able to do so (easily).

Anyway, this is assuming that I'm not misinterpreting what you mean by your above comment. So please let me know if I misunderstood.

Patrick