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

From: Glen Choo <chooglen@google.com>
Date: Oct 13, 2021, 21:56 UTC
Message-ID: <kl6ltuhk1tdg.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqtuhkfzw8.fsf@gitster.g>

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

Show 29 quoted lines

>>    However, I believe that some questions *shouldn't* be answered by
>>    just struct branch. The prime example in this series is
>>    branch_get_push() - it conceptually answers 'What is the pushremote
>>    of this branch', but the behavior we want is closer to 'If
>>    configured, give me the pushremote for the branch. Otherwise, give me
>>    the default pushremote of the repo.'. This is arguably a relevant
>>    detail that should be exposed to callers.
>
> It is a good example why such a function can just take an instance
> of branch, and the caller,
>
>  (1) who does care about the fallback, can be assured that the logic
>      falls back to the correct repository; and
>
>  (2) who does not care about the fallback and sees it a mere
>      implementation detail of "I am on this branch; give me the
>      remote to push to", do not have to know what, other than the
>      branch object, needs to be passed.
>
> if we explicitly record a branch object which repository it was
> taken from.
>
> There may be some other (real) reason where the resistance comes
> from, that you may not be telling us, though.  But in what was
> described in the message I am responding to, I didn't see much
> convincing reason to argue _for_ keeping the contained objects
> ignorant of the container and forcing callers to pass both to
> functions that use both the container and contained to compute
> something.

My primary line of thinking is that adding the backpointer from struct branch to struct repository adds "unnecessary" coupling between the two objects, which causes the more concrete problems of:

* Inconsistency between other functions that use struct repository as a
  "context object". We now find ourselves having to justify why branch
  functions can bypass the context object using a special pointer,
  whereas other functions and structs (say, struct object) cannot.
* Interface misuse, where callers can now dereference branch->repository
  even though it is meant to be internal. This is also a possible source
  of inconsistency.
* Extra memory consumption.

A counterpoint to what I said is [1], where Jonathan eventually added the backpointer from struct ref_store to struct repository, which does give the nice benefits of referential integrity and abstraction that you cited. However in most of that series, struct ref_store is meant to be the context object, but due to poor internal abstraction, ref_store ends up depending on repository in some form or another and thus the backpointer is more defensible (as opposed to passing two context objects). I do not think we are in the same position with struct branch; struct branch seems sufficiently separated to me.

I'm not opposed to adding a backpointer if it helps us get to a good final state, but I currently suspect that the final state still involves passing around struct repository as a context object.

[1] https://lore.kernel.org/git/xmqqlf3gib0p.fsf@gitster.g/