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

From: Jeff King <peff@peff.net>
To: Patrick Steinhardt <ps@pks.im>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org,
	Christian Couder <christian.couder@gmail.com>
Subject: Re: [PATCH 0/7] rev-parse: implement object type filter
Date: Thu, 11 Mar 2021 12:56:47 -0500	[thread overview]
Message-ID: <YEpZ30YOZ/akZ7pm@coredump.intra.peff.net> (raw)
In-Reply-To: <YEosm+RjD/YRIcLo@ncase>

On Thu, Mar 11, 2021 at 03:43:39PM +0100, Patrick Steinhardt wrote:

> > I do wonder if it's that bad for clients to be able to specify something
> > like this, though. Even though there's not that much use for it with a
> > regular partial clone, it could conceivably used for some special cases.
> > I do think it would be more useful if you could OR together multiple
> > types. Asking for "commits|tags|trees" is really the same as the already
> > useful "blob:none". And "commits|tags" is the same as tree:depth=0.
> 
> I did waste a few thoughts on how this should be handled. I see two ways
> of doing it:
> 
>     - We could just implement the new `object:type` filter such that it
>       directly supports OR'ing. That's the easy way to do it, but it's
>       inflexible.
> 
>     - We could extend combined filters to support OR-semantics in
>       addition to the current AND-semantics. In the end, that'd be a
>       much more flexible approach and potentially allow additional
>       usecases.
> 
> I lean more towards the latter as it feels like the better design. But
> it's more involved, and I'm not sure I want to do it as part of this
> patch series.

Yeah, I don't think that needs to be part of this series. The only thing
to consider for this series is whether it's a problem for clients to be
able to ask for type=blob from a server which has blindly turned on
uploadpack.allowFilter without restricting the types.

My gut is to say yes. Even if we don't have a particular use, I don't
think it hurts (and in general, I think people running public servers
with bitmaps really ought to set uploadpackfilter.allow=false anyway,
because stuff like non-zero tree-depth filters are expensive).

-Peff