Re: [PATCH 1/2] repository: make repo_clear() idempotent

[Patrick Steinhardt <ps@pks.im>]

On Wed, Sep 02, 2026 at 02:49:07AM -0400, Jeff King wrote:
> On Wed, Sep 02, 2026 at 02:29:40AM -0400, Jeff King wrote:
> 
> > I'm not sure how much it's worth untangling all of this, but probably
> > not enough just to remove pointer indirection from repo->parsed_objects.
> 
> BTW, another curiosity: parsed_objects contains a pointer back to the
> repo that contains it! What could it possibly depend on in the repo
> itself?
> 
> As far as I can tell, the answer is nothing. We only ever access p->repo
> in order to get to p->repo->parsed_objects, which will always be the
> same as our original "p". There are some internal functions within
> object.c which could be simplified by passing around the
> parsed_object_pool directly.  But we also call lookup_commit() and a few
> other public functions, all of which take a repository struct. Even
> though they only use it to look at the parsed_objects field!
> 
> Structurally speaking these should be operating on a parsed_object_pool,
> since that's all they need. But from the caller's point of view that is
> just an implementation detail, and it is easier to pass in the whole
> repository.

I was at one point wondering whether the parsed object pool should
really be an implementation detail of the object database -- parsing
objects should not have to depend on the repository, but it really
should only interact with the object database. I hacked together a
series, but it grew _huge_ because I was of course also trying to bubble
up the changes into all subsystems that do parse objects rigth now. So I
discarded that idea eventually.

But I think making the parsed object pool become more self-contained is
a step into the right direction.

> So we'd probably need to provide functions that operate directly on the
> pool like:
> 
>   struct commit *lookup_commit_via_pool(struct parsed_object_pool *p,
>                                         const struct object_id *oid);
> 
> and then maintain wrappers like:
> 
>   struct commit *lookup_commit(struct repository *r,
>                                const struct object_id *oid)
>   {
> 	return lookup_commit_via_pool(r->parsed_objects, oid);
>   }
> 
> to avoid rewriting every caller with r->parsed_objects themselves.

I dunno. If we want to make this switch I'd say that we should go all or
nothing. Otherwise, if we retain both interfaces, I don't really feel
like it gains us anything at all.

> The patch below illustrates the minimal change to drop the repo pointer
> from parsed_object_pool. I think it more accurately represents the
> actual dependencies of the data structures, but it's a fair bit of churn
> for a minor amount of clarity. Probably not worth it.

I think there is value in it. While deglobalizing our state we tend to
just pass the repository explicitly into the subsystems, which is a good
step. But I think it really should only be the first step, where the
next step would be to reduce the state we pass around. So ideally,
subsystems should really only receive as input what they actually need.
This would eventually ensure that our subsystems are more self-contained 
and that they can be used more flexibly.

Thanks!

Patrick