From: "Rubén Justo" <rjusto@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git List <git@vger.kernel.org>, Phillip Wood <phillip.wood123@gmail.com>
Subject: Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree
Date: Sun, 22 Jan 2023 12:51:02 +0100 [thread overview]
Message-ID: <766b25e1-2d7a-7b5c-10a9-43e545a57dba@gmail.com> (raw)
In-Reply-To: <xmqqbkmruykg.fsf@gitster.g>
On 21-ene-2023 17:50:55, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>
> > Let's stop using find_shared_symref() in die_if_checked_out(), to handle
> > correctly ignore_current_worktree.
>
> This says what the code stops using, but does not say what it uses
> instead.
I thought the code for that was a better and clearer description. I'll add
some details to the message.
> Factoring is_shared_symref() out of find_shared_symref() is probably
> a good idea that can stand alone without any other change in this
> patch as a single step, and then a second step can update
> die_if_checked_out() using the new function.
OK. I'll split that in two.
> As the proposed log message explained, updating die_if_checked_out()
> with this patch would fix a bug---can we demonstrate the existing
> breakage and protect the fix from future breakages by adding a test
> or two?
2/3 and 3/3, I think makes more sense on its own commit.
> > - const struct worktree *wt;
> > + int i;
> > +
> > + for (i = 0; worktrees[i]; i++)
> > + {
>
> Style. WRite the above on a single line, i.e.
>
> for (i = 0; worktrees[i]; i++) {
Sorry. I'll fix that.
>
> Optionally, we can lose the separate declaration of "i" by using C99
> variable declaration, i.e.
>
> for (int i = 0; worktrees[i]; i++) {
OK. Yes, I was playing with this, changed my mind and ended up with this and
the other style below, mess.
>
> > diff --git a/worktree.c b/worktree.c
> > index aa43c64119..d500d69e4c 100644
> > --- a/worktree.c
> > +++ b/worktree.c
> > @@ -403,6 +403,33 @@ int is_worktree_being_bisected(const struct worktree *wt,
> > * bisect). New commands that do similar things should update this
> > * function as well.
> > */
>
> The above comment is about find_shared_symref() which iterates over
> worktrees and find the one that uses the named symref. Now the
> comment appears to apply to is_shared_symref() which does not
> iterate but takes one specific worktree instance. Do their
> differences necessitate some updates to the comment?
I think the comment still makes sense as is for the new function, both the
description and the recommendation. I will review it again.
>
> > +int is_shared_symref(const struct worktree *wt, const char *symref,
> > + const char *target)
> > +{
>
> What this function does sound more like "is target in use in this
> particular worktree by being pointed at by the symref?" IOW, I do
> not see where "shared" comes into its name from.
>
> "HEAD" that is tentatively detached while bisecting or rebasing the
> "target" branch is still considered to point at the "target", so
> perhaps symref_points_at_target() or something?
>
I tried to maintain the terms as much as possible. I'll think about the name
you suggest.
> > const struct worktree *find_shared_symref(struct worktree **worktrees,
> > const char *symref,
> > const char *target)
> > @@ -411,31 +438,8 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
> > int i = 0;
> >
> > for (i = 0; worktrees[i]; i++) {
>
> Not a new problem, but the initialization on the declaration of "i"
> is redundant and unnecessary. Again, we can use the C99 style, i.e.
>
> const struct worktree *existing = NULL;
> - int i = 0;
> -
> - for (i = 0; worktrees[i]; i++) {
> + for (int i = 0; worktrees[i]; i++) {
I'll fix this.
>
> > + if (is_shared_symref(worktrees[i], symref, target)) {
> > + existing = worktrees[i];
> > break;
> > }
> > }
> > diff --git a/worktree.h b/worktree.h
> > index 9dcea6fc8c..7889c4761d 100644
> > --- a/worktree.h
> > +++ b/worktree.h
> > @@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
> > const char *symref,
> > const char *target);
> >
> > +/*
> > + * Returns true if a symref points to a ref in a worktree.
> > + */
>
> Make it clear that what you called "a ref" in the above is what is
> called "target" below.
>
Again, that was an attempt to maintain the terms from find_shared_symref().
Thank you for your review.
next prev parent reply other threads:[~2023-01-22 11:51 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-17 0:36 [PATCH] worktree: teach find_shared_symref to ignore current worktree Rubén Justo
2023-01-17 23:27 ` Junio C Hamano
2023-01-18 23:50 ` Rubén Justo
2023-01-19 10:48 ` Phillip Wood
2023-01-19 23:18 ` Rubén Justo
2023-01-22 1:20 ` [PATCH v2 0/3] fix die_if_checked_out() when ignore_current_worktree Rubén Justo
2023-01-22 1:23 ` [PATCH v2 1/3] branch: " Rubén Justo
2023-01-22 1:50 ` Junio C Hamano
2023-01-22 11:51 ` Rubén Justo [this message]
2023-01-22 19:58 ` Junio C Hamano
2023-01-22 23:21 ` Rubén Justo
2023-01-24 10:35 ` Phillip Wood
2023-01-26 3:07 ` Rubén Justo
2023-01-22 1:28 ` [PATCH v2 2/3] rebase: refuse to switch to a branch already checked out elsewhere (test) Rubén Justo
2023-01-22 1:28 ` [PATCH v2 3/3] switch: reject if the branch is " Rubén Justo
2023-02-04 23:19 ` [PATCH v3 0/4] fix die_if_checked_out() when ignore_current_worktree Rubén Justo
2023-02-04 23:25 ` [PATCH v3 1/4] worktree: introduce is_shared_symref() Rubén Justo
2023-02-07 10:44 ` Phillip Wood
2023-02-04 23:25 ` [PATCH v3 2/4] branch: fix die_if_checked_out() when ignore_current_worktree Rubén Justo
2023-02-06 16:56 ` Ævar Arnfjörð Bjarmason
2023-02-06 23:09 ` Rubén Justo
2023-02-07 10:50 ` Phillip Wood
2023-02-07 12:58 ` Ævar Arnfjörð Bjarmason
2023-02-04 23:26 ` [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test) Rubén Justo
2023-02-06 16:59 ` Ævar Arnfjörð Bjarmason
2023-02-06 23:16 ` Rubén Justo
2023-02-07 10:52 ` Phillip Wood
2023-02-08 0:43 ` Rubén Justo
2023-02-08 5:19 ` Junio C Hamano
2023-02-08 22:09 ` Rubén Justo
2023-02-04 23:26 ` [PATCH v3 4/4] switch: reject if the branch is " Rubén Justo
2023-02-15 4:17 ` Eric Sunshine
2023-02-15 22:17 ` Rubén Justo
2023-02-25 14:14 ` [PATCH v4 0/4] fix die_if_checked_out() when ignore_current_worktree Rubén Justo
2023-02-25 14:21 ` [PATCH v4 1/4] worktree: introduce is_shared_symref() Rubén Justo
2023-02-25 14:22 ` [PATCH v4 2/4] branch: fix die_if_checked_out() when ignore_current_worktree Rubén Justo
2023-02-25 14:22 ` [PATCH v4 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test) Rubén Justo
2023-02-25 14:22 ` [PATCH v4 4/4] switch: reject if the branch is " Rubén Justo
2023-02-25 22:50 ` [PATCH v4 0/4] fix die_if_checked_out() when ignore_current_worktree Junio C Hamano
2023-02-27 0:00 ` Rubén Justo
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=766b25e1-2d7a-7b5c-10a9-43e545a57dba@gmail.com \
--to=rjusto@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=phillip.wood123@gmail.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).