Re: [PATCH v2 0/8] rev-parse: implement object type filter

From: Patrick Steinhardt <ps@pks.im>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org,
	Christian Couder <christian.couder@gmail.com>,
	Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH v2 0/8] rev-parse: implement object type filter
Date: Fri, 9 Apr 2021 13:14:26 +0200	[thread overview]
Message-ID: <YHA3EkH3HDe45EKA@ncase> (raw)
In-Reply-To: <YGyjtCWqdeCj3S3U@coredump.intra.peff.net>

[-- Attachment #1: Type: text/plain, Size: 2835 bytes --]

On Tue, Apr 06, 2021 at 02:08:52PM -0400, Jeff King wrote:
> On Sat, Mar 20, 2021 at 02:10:41PM -0700, Junio C Hamano wrote:
> 
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> > > this is the second version of my patch series which implements a new
> > > `object:type` filter for git-rev-parse(1) and git-upload-pack(1) and
> > > extends support for bitmap indices to work with combined filters.
> > > ...
> > > Please see the attached range-diff for more details.
> > 
> > Any comment from stakeholders?
> 
> Sorry, this languished on my to-review list for a while.
> 
> I took a careful look. I found a few small nits, but the code overall
> looks pretty good.
> 
> I do still find the use of the filter code here a _little_ bit
> off-putting. It makes perfect sense in some ways: we are asking rev-list
> to filter the output, and it keeps our implementation nice and simple.
> It took me a while to figure out what I think makes it weird, but I
> think it's:
> 
>   - the partial-clone feature exposes the filter mechanism in a very
>     transparent way. So while it's not _wrong_ to be able to ask for a
>     partial clone of only trees, it's an odd thing that nobody would
>     really use in practice. And so it's a bit funny that it gets
>     documented alongside blob:limit, etc.
> 
>   - for the same reason, it's very rigid. We have no way to say "this
>     filter OR that filter", and are unlikely to grow them (because this
>     is all part of the network protocol). Whereas it's perfectly
>     reasonable for somebody to ask for "trees and blobs" via rev-list.
> 
> I dunno. Those aren't objections exactly. Just trying to put my finger
> on why my initial reaction was "huh, why --filter?".

Yeah, I do kind of share these concerns. Ideally, we'd provide a nicer
only-user-facing interface to query the repository for various objects.
git-cat-file(1) would be the obvious thing that first gets into my mind,
where it would be nice to have it filter stuff. But then on the other
hand, it's really rather a simple "Give me what I tell you to" binary,
which is probably a good thing. Other than that I don't think there's
any executable that'd be a good fit -- we could do this via a new
git-list-objects(1), but then again git-rev-list(1) already does most of
what git-list-objects(1) would do, so why bother.

It kind of feels like git-checkout(1) to me: it does many things, and if
you know how to wield it it works perfectly fine. But the user interface
is lacking, which is why it was split up into git-switch(1) and
git-restore(1). It's telling already that the summary of git-rev-list(1)
is "Lists commit objects in reverse chronological order". I mean yes,
that's what it does in many cases. But there's just as many cases where
it doesn't.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]