Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree

From: Rubén Justo <rjusto@gmail.com>
Date: Jan 22, 2023, 11:51 UTC
Message-ID: <766b25e1-2d7a-7b5c-10a9-43e545a57dba@gmail.com>
In-Reply-To: <xmqqbkmruykg.fsf@gitster.g>

On 21-ene-2023 17:50:55, Junio C Hamano wrote:

Show 7 quoted lines

> 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.

Show 9 quoted lines

> > -	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.

Show 5 quoted lines

> 
> 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.

Show 15 quoted lines

> 
> > 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.

Show 13 quoted lines

> 
> > +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.

Show 16 quoted lines

> >  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.

Show 21 quoted lines

> 
> > +		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.