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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 13, 2021, 21:03 UTC
Message-ID: <xmqqfsztlwxb.fsf@gitster.g>
In-Reply-To: <YHVMEdeDv9ZeXRAU@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

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

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.