Glen Choo <chooglen@xxxxxxxxxx> writes:
>> If the answer is yes, then the function need to take both branch and
>> remote_state as two separate parameters. If the answer is no (i.e.
>> we never ask hypothetical questions, we just ask what we should do
>> in the current, real, state), then the function can just take the
>> remote_state and remote_state->branch being NULL would be used as a
>> signal that the HEAD is detached. I suspect it is the former, as
>> this information is used in string-name-to-object translation for
>> "topic@{push}" in object-name.c::interpret_branch_mark() function.
>
> I agree that the need for hypothetical "what happens if HEAD were
> detached?" questions may arise, though I'm not sure if there are any
> right now. When we call branch_get(NULL), the expected return value is
> the "current_branch". If there is no "current_branch" i.e. the return
> value of branch_get() is the NULL branch. A NULL branch is not usable -
> branch_get_push() and branch_get_upstream() return error messages
> indicating "HEAD does not point to a branch". (these are the functions
> used by object-name.c::interpret_branch_mark()).
>
> Given the convention of "NULL branch == detached HEAD", how do we
> proceed? Some options:
>
> a) Represent detached HEAD with a non-NULL "struct branch". This will
> let us continue using the remote_state backpointer, which makes many
> interfaces clean, but is somewhat invasive, error-prone and it uses
> "struct branch" for something that is not a branch, which is itself
> an error-prone interface.
I'd rather not to go there.
> b) Keep the backpointer, but add remote_state as a parameter to
> pushremote_for_branch(). The least possible effort to fix the problem
> and might be 'easy' but is inconsistent with the other functions and
> is prone to misuse because the backpointer and parameter can be
> different.
Make the function take a remote_state as a parameter (instead of,
not in addition to, the branch parameter), and use the remote_state
structure to find which branch's branch specific configuration we
want to use by checking its current_branch member.
I think that would be the best approach for "no, we won't ask
hypothetical question" case.
On the other hand,...
> 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).
... if we want to support the what-if callers, I
think the best approach would be a slight variant of c) above.
That is, pass branch and remote_state as two parameters, and when
branch is not NULL, barf if it is not among remote_state.branches[],
to protect against nonsense combinations.
Thanks.
