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

Glen Choo [off-list ref] writes:

quoted

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.