On Tue, Apr 13, 2021 at 02:03:12PM -0700, Junio C Hamano wrote:
> 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.

> 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