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

Patrick Steinhardt [off-list ref] 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?

Thanks.

On Sat, Mar 20, 2021 at 02:10:41PM -0700, Junio C Hamano wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

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

-Peff

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:

quoted

Patrick Steinhardt [off-list ref] writes:

quoted

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

On Fri, Apr 09, 2021 at 01:14:26PM +0200, Patrick Steinhardt wrote:

quoted

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.

I don't think cat-file does quite the same thing. An important part of
rev-list is that it is traversing. So it is determining both
reachability, but also eliminating excluded objects. For example, there
is no cat-file equivalent (and can never be) of:

  git rev-list --objects --filter=object:type=blob $old..$new

Likewise for list-objects (which cat-file really _does_ cover, with
--batch-all-objects). Obviously you can pair rev-list with cat-file to
traverse and then filter, but the whole point of this series is to do so
more efficiently.

So I think putting this into rev-list is the only sensible option. The
question is just whether to use --filter, or if it should be:

  git rev-list --show-blobs --show-trees $old..$new

with rules like:

  - if no --show-X is given, show only commits

  - if one or more --show-X is given, show all of them (but nothing else)

  - --objects is equivalent to providing each of --show-commits
    --show-blobs --show-trees --show-tags

-Peff