Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state

From: Glen Choo <chooglen@google.com>
Date: Oct 14, 2021, 01:25 UTC
Message-ID: <kl6lmtnc1jpt.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqmtncebtb.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> IOW, we need to start somewhere to clean things up, so either we
> teach multi-repository support to remote.c from day one, or we pass
> a repository as a separate and redundant parameter, with the
> intention to later clean up.

I authored this topic with the assumption that passing a 'redundant' context object was the path forward, not something to clean up. I think this explains most of the divergence in opinion.

> ...I suspect that this piece is isolated
> enough that it is simpler to use as a starting point, and then the
> callers into remote.c API can then be cleaned up, gradually extending
> the circle.

Agree that this piece is a good place to start such work.

Show 6 quoted lines

> I do not think it is meant to be internal to begin with.  If we
> declare, after careful consideration, that an in-core branch object
> given by remote.c API always belongs to one and only one repository,
> then following the branch->repository pointer should be the
> documented and kosher way to find out the repository by anybody who
> is given the branch object.

In the specific case of an in-core object that can refer only to one repository, I think this is a fairly straightforward way of maintaining referential integrity. However I don't think that the approach of "contained object pointing to container" generalizes well - if I were passed a struct remote_state and I needed to access its repository later, is the ideal interface a backpointer, or is there a deeper problem with how things are structured?

IOW, whether or not we should use the backpointer seems to depend on the specifics of each use case. It might be productive to decide on guidelines, especially with regards to repository.

> ...A function that takes repo and branch
> to pretend as if it can work with an inconsistent pair is an
> invidation for the interface misuse.

Fair.

Show 6 quoted lines

>> objects). I do not think we are in the same position with struct branch;
>> struct branch seems sufficiently separated to me.
>
> Sufficiently separated in the sense that you take a branch from the
> parent repository and throwing it at a function with a pointer to an
> in-core repository for a submodule would make some sort of sense?

I meant "sufficiently separated" in the sense that a struct branch is meaningful to callers, even in the absence of a repository. Even in the case where we *might* care about the repository e.g. getting a default for remote_for_branch(), there are callers that make do with just branch->remote_name.

This was meant to contrast to ref_store, which relies on its specific repository for much of its internals and is difficult to separate. I am worried about the knock-on effects of taking a _relatively_ clean abstraction layer in struct branch and tying it back to the repository layer.

That said, you have rightly noted that we should not pretend that a branch from repo A can be thrown together with a pointer to repo B in any kind of meaningful way (or at least, not yet). A backpointer can prevent this.

I'm not fully convinced either way so I'll take time to mull over it; I would also like to start on the right foot with this subsystem.