On 21-ene-2023 17:50:55, Junio C Hamano wrote:
> Rubén Justo <rjusto@xxxxxxxxx> 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.
