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

From: Glen Choo <chooglen@google.com>
Date: Oct 27, 2021, 17:59 UTC
Message-ID: <kl6l35om9wn9.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqq8ryfxkuj.fsf@gitster.g>

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

Show 11 quoted lines

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

Show 8 quoted lines

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