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

Junio C Hamano [off-list ref] writes:

quoted

   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/ (local)