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

[Glen Choo <chooglen@google.com>]

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.

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

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