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

[Junio C Hamano <gitster@pobox.com>]

Glen Choo <chooglen@google.com> 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.