Re: [PATCH v3 2/4] remote: use remote_state parameter internally

[Glen Choo <chooglen@google.com>]

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

> Glen Choo <chooglen@google.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>> c) Replace the backpointer with a remote_state parameter. Expressive and
>>>>    fits the paradigm of "defaulting to the repo when needed", but
>>>>    interfaces are repetitive and shifts the responsibility of
>>>>    correctness to the caller (see v2).
> Hopefully.  Of course I think the implementation of the safety would
> actually be done, not by iterationg over branches[] array, but just
> checking branch->remote_state == remote_state pointer equality.

With (c), I plan to eliminate the backpointer altogether, so such a
check is not possible. However, I plan to add a branches_hash to
remote_state in the same way that we have remotes_hash for remote_state.

>> This "longer term direction" sounds like what I envisioned with (e). I
>> agree that detached HEAD is a state that should be expressed with more
>> than just NULL, though I'm not sure that "struct branch" is the correct
>> abstraction. No point bikeshedding now of course, we'll cross that
>> bridge when we get there ;)
>
> I actually was hoping that the time to cross the bridge was now,
> though ;-)

Hm, well I don't want to get too lost in the weeds here and lose sight
of the short-term objective. I have a few strands of thought, but
nothing concrete enough to propose a full alternative:

- It seems odd that the branches and the "current_branch" are handled by
  the remotes system, perhaps it would be clearer to move it elsewhere.
  Separating branches from "branch remote-tracking configuration" might
  clarify our thinking.
- branch_get(NULL) and branch_get("HEAD") are not entirely coherent with
  the idea of "getting a branch by name", perhaps we should just move
  this functionality into a different function.
- Similarly, variants of remote_for_branch() are misleading because they
  behave inconsistently when branch is NULL. 

I might not want to take action on these ideas though, e.g.
branch_get("HEAD") or remote_for_branch(NULL) are very convenient for
callers even though they behave slightly inconsisently. I'll propose a
longer term direction when I have a better grasp of the system and the
tradeoffs.