On Tue, Apr 13, 2021 at 08:05:52PM +0200, SZEDER Gábor wrote:
> > [but now ask it to exclude promisor objects, and it's much slower;
> > this is because is_promisor_object() opens up each tree in the pack in
> > order to see which "promised" objects it mentions]
>
> I don't understand this: 'git rev-list --count --all' only counts
> commit objects, so why should it open any trees at all?
Because the promisor code is a bit over-eager. There's actually one
small error in what I wrote above. In that particular rev-list, we're
not calling is_promisor_object() at all, because we'll already have
excluded all the promisor objects by marking them UNINTERESTING and
SEEN.
So:
- in is_promisor_object(), we load the _whole_ list of promisor
objects, which requires opening trees to find out about referenced
blobs. In theory it could know that we only care about commits, but
it's not connected to a particular traversal, so it gets the whole
list.
- mark_uninteresting() is the code where we pre-mark the objects from
the promisor pack as UNINTERESTING, and that was loading all of the
trees in this case. And it could know that we are not looking at
--objects, so there's no need to touch trees. But after my patches,
we do not load the contents of _any_ objects at all in that
function. We could avoid even creating "struct object" for
non-commits there, too, but that would imply looking up the type of
each object (so more CPU, though it would save us some memory when
we only care about commits). I suspect in practice that most callers
would generally pass --objects anyway, though (e.g., your original
pack-objects that started this thread certainly cares about
non-commits).
> > + /*
> > + * yikes, do we really need to parse here? maybe
>
> Heh, a "yikes" here and a "yuck" in your previous patch... This issue
> was worth reporting :)
Yeah. I think the client side of a lot of this partial-clone / promisor
stuff is not very mature. It's waiting on people to start using it and
finding all of these rough edges.
-Peff
