Re: [PATCH 2/2] remote: add remote_state to struct repository

From: Glen Choo <chooglen@google.com>
Date: Oct 8, 2021, 17:30 UTC
Message-ID: <kl6lo87zxw3x.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqq4k9so15i.fsf@gitster.g>

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

Show 9 quoted lines

>> -static void add_pushurl_alias(struct remote *remote, const char *url)
>> +static void add_pushurl_alias(struct remote_state *remote_state,
>> +			      struct remote *remote, const char *url)
>>  {
>
> I am not sure if this is a good interface.  It allows a caller to
> obtain "struct remote *" instance from somewhere, and feed it with
> an instance of "struct remote_state *" that has nothing to do with
> the "struct remote *", no?

Valid point, this interface should not be so easy to misuse.

> Instead, shouldn't "struct remote *" _know_ which remote-state it
> came from?

I am less certain about this. The pattern of
container->contained->container is convenient for callers, but requires
very deliberate maintenance and the reduced separation might promote
thoughtless use of the interfaces e.g. "Should I use struct remote_state
+ name or struct remote? Eh doesn't matter, they're equivalent."

Instead, we could converge on a pattern of:

* struct remote_state + name when the caller does something in the
  context of the remote's repository.
* struct remote when the caller doesn't need the remote's repository

I think we might do this in slightly different ways for branches vs remotes.

> I didn't look, but I suspect that there may be similar problems with
> other structures like "branch" in this change.

Looking into it, it appears that only static functions pass struct remote_state and struct remote at the same time. That gives us a lot of leeway to clean things up inside of remote.c.

The same cannot be said for struct branch, that will be much harder to clean up. In fact, rereading patch 2, I missed many implicit references to the_repository in the branch functions, so the pattern of struct remote_state + struct branch would actually more pervasive than it seems.

However, I suspect that we don't need to pass around struct branch very often. We may be able to maintain referential integrity by passing the branch name instead.