From: Glen Choo <chooglen@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v3 2/4] remote: use remote_state parameter internally
Date: Mon, 25 Oct 2021 16:00:05 -0700 [thread overview]
Message-ID: <kl6lbl3c7lsa.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <xmqqzgqwzvwx.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> I just checked. The repository the push is run in is bare and its
> HEAD is detached, pointing at a commit directly.
Thanks, I was able to reproduce the segfault using your config.
>> "current_branch" is allowed to be NULL when HEAD does not point to a
>> branch.
>
> Good point. It is a good justification to make the remote_state
> available to the function, as branch==NULL that signals "there is no
> current branch in the repository" cannot be dereferenced to get to
> either the repository or the remote_state, yet the function needs
> access to remote_state even when branch==NULL.
Yes, I wish we had noticed this sooner in our discussion and the fault
is mine. It seems that pushremote_for_branch() is a prime example of
"get the settings from the branch if possible, but default to the
correct repository settings otherwise.", which is difficult to express
if remote_state is not available to the function.
> What "branch" is pushremote_for_branch() meant to take? Is there a
> caller that asks a hypothetical "I know this is not a branch that is
> the current branch in the repository, but to which remote would we
> push if this branch _were_ the current one?" (and passes NULL to
> mean "there is a checked out branch, but what happens if our HEAD
> were detached?") question? Even if there isn't currently, do we
> want to add such callers in the future?
>
> 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.
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.
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).
d) Default to the_repository in pushremote_for_branch(). Easy, but
incorrect in general.
e) Some kind of reimagining of the remotes interfaces that doesn't
have this problem. One possible approach is to remove branches from
the remotes system altogether, since remotes are primarily concerned
with _branch tracking information_ and not really _branches_ per se;
perhaps we are being led astray by our terminology. If possible, this
is probably the most elegant long term solution, but it's
time-consuming and it's not clear how we will get there.
Currently, my preference is to go with (c). We can create a clear
expectation to callers that branch tracking information is not complete
without a repository, thus a repository is always supplied, explicitly
or not. If so, the remote_state parameter looks less like an
implementation detail, especially since a NULL branch is allowed.
I know we have already considered and abandoned (c) after v2, but has
your opinion changed after considering the new information?
next prev parent reply other threads:[~2021-10-25 23:00 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-07 19:07 [PATCH 0/2] remote: replace static variables with struct remote_state Glen Choo via GitGitGadget
2021-10-07 19:07 ` [PATCH 1/2] remote: move static variables into struct Glen Choo via GitGitGadget
2021-10-07 23:36 ` Junio C Hamano
2021-10-07 19:07 ` [PATCH 2/2] remote: add remote_state to struct repository Glen Choo via GitGitGadget
2021-10-07 23:39 ` Junio C Hamano
2021-10-08 17:30 ` Glen Choo
2021-10-13 19:31 ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Glen Choo
2021-10-13 19:31 ` [PATCH v2 1/3] remote: move static variables into per-repository struct Glen Choo
2021-10-13 20:21 ` Junio C Hamano
2021-10-14 17:25 ` Glen Choo
2021-10-14 18:33 ` Junio C Hamano
2021-10-13 19:31 ` [PATCH v2 2/3] remote: use remote_state parameter internally Glen Choo
2021-10-13 20:23 ` Junio C Hamano
2021-10-13 19:31 ` [PATCH v2 3/3] remote: add struct repository parameter to external functions Glen Choo
2021-10-13 20:24 ` Junio C Hamano
2021-10-13 20:11 ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Junio C Hamano
2021-10-13 20:27 ` Junio C Hamano
2021-10-13 22:00 ` Glen Choo
2021-10-13 21:56 ` Glen Choo
2021-10-13 23:37 ` Junio C Hamano
2021-10-14 1:25 ` Glen Choo
2021-10-19 22:43 ` [PATCH v3 0/4] " Glen Choo
2021-10-19 22:43 ` [PATCH v3 1/4] remote: move static variables into per-repository struct Glen Choo
2021-10-19 22:43 ` [PATCH v3 2/4] remote: use remote_state parameter internally Glen Choo
2021-10-20 19:45 ` Junio C Hamano
2021-10-20 20:31 ` Junio C Hamano
2021-10-20 22:08 ` Junio C Hamano
2021-10-25 18:09 ` Glen Choo
2021-10-25 19:36 ` Glen Choo
2021-10-25 20:33 ` Junio C Hamano
2021-10-25 23:00 ` Glen Choo [this message]
2021-10-26 0:45 ` Junio C Hamano
2021-10-26 1:22 ` Junio C Hamano
2021-10-26 17:04 ` Glen Choo
2021-10-27 2:28 ` Junio C Hamano
2021-10-27 17:59 ` Glen Choo
2021-10-27 20:03 ` Junio C Hamano
2021-10-19 22:43 ` [PATCH v3 3/4] remote: remove the_repository->remote_state from static methods Glen Choo
2021-10-19 22:43 ` [PATCH v3 4/4] remote: add struct repository parameter to external functions Glen Choo
2021-10-28 18:30 ` [PATCH v4 0/6] remote: replace static variables with struct remote_state Glen Choo
2021-10-28 18:30 ` [PATCH v4 1/6] t5516: add test case for pushing remote refspecs Glen Choo
2021-10-28 20:17 ` Junio C Hamano
2021-11-15 18:42 ` Jonathan Tan
2021-11-15 20:09 ` Glen Choo
2021-10-28 18:30 ` [PATCH v4 2/6] remote: move static variables into per-repository struct Glen Choo
2021-10-28 18:30 ` [PATCH v4 3/6] remote: use remote_state parameter internally Glen Choo
2021-10-28 18:30 ` [PATCH v4 4/6] remote: remove the_repository->remote_state from static methods Glen Choo
2021-11-15 18:48 ` Jonathan Tan
2021-10-28 18:31 ` [PATCH v4 5/6] remote: die if branch is not found in repository Glen Choo
2021-11-15 18:50 ` Jonathan Tan
2021-11-15 20:06 ` Glen Choo
2021-11-16 17:45 ` Jonathan Tan
2021-10-28 18:31 ` [PATCH v4 6/6] remote: add struct repository parameter to external functions Glen Choo
2021-11-15 18:55 ` Jonathan Tan
2021-11-15 21:44 ` Glen Choo
2021-11-12 0:01 ` [PATCH v4 0/6] remote: replace static variables with struct remote_state Glen Choo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=kl6lbl3c7lsa.fsf@chooglen-macbookpro.roam.corp.google.com \
--to=chooglen@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).