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

Junio C Hamano [off-list ref] writes:

quoted

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.

Actually, I take half of that back, as I think this would be the
best direction in the longer term, and it conceptually is sound.
After all, detached HEAD is not all that special--- when we ask for
the "current branch" and the HEAD happens to be detached, we treat
it as a perfectly valid state and behave as if we are on that
unnamed branch.  The state probably deserves to be represented as a
"struct branch" instance.

I do agree with you that this approach would involve significant
short-term pain, as a lot of existing code may say "NULL is how we
represent detached HEAD" and they have to be identified and upgraded
before the above plan calls for.

So, I tend to think that in the shorter term, perhaps a safer
variant of (c) that allows us to ask "not-current@{push}" would be a
good way to go, but when things have stabilized, we should revisit
and see if it is feasible to represent a detached HEAD state with an
instance of "struct branch" and how simpler the interface would become
when we did so.

Thanks.

Junio C Hamano [off-list ref] writes:

quoted

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.

Sounds reasonable to me. The resulting interface would look like the v2
one, but internally, this additional safety check will prevent misuse.

quoted

quoted

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.

Actually, I take half of that back, as I think this would be the
best direction in the longer term, and it conceptually is sound.
After all, detached HEAD is not all that special--- when we ask for
the "current branch" and the HEAD happens to be detached, we treat
it as a perfectly valid state and behave as if we are on that
unnamed branch.  The state probably deserves to be represented as a
"struct branch" instance.
[...] when things have stabilized, we should revisit
and see if it is feasible to represent a detached HEAD state with an
instance of "struct branch" and how simpler the interface would become
when we did so.

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 ;)

Glen Choo [off-list ref] writes:

Junio C Hamano [off-list ref] writes:

quoted

quoted

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.

Sounds reasonable to me. The resulting interface would look like the v2
one, but internally, this additional safety check will prevent misuse.

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.

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 ;-)

Junio C Hamano [off-list ref] writes:

Glen Choo [off-list ref] writes:

quoted

Junio C Hamano [off-list ref] writes:

quoted

quoted

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.

quoted

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.