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

["Rubén Justo" <rjusto@gmail.com>]

On 22-ene-2023 11:58:05, Junio C Hamano wrote:

> Rubén Justo <rjusto@gmail.com> writes:
> 
> >> 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.
> 
> Hmph, how so?  Especially once you split 1/3 into the preliminary
> refactoring and the real fix, the fix becomes fairly small and
> clear.  And the tests to protect the fix would go best in the same
> commit.

My intention is to protect rebase (2/3) and switch (3/3).  If any of
those tests break, even if die_if_checked_out() is no longer used by
them, I try to make the original intent clear with that in there.

die_if_checked_out() was initially fine, the ignore_current_worktree was
unfortunately introduced.  I haven't checked, but other callers not
affected by the change, i.e. ignore_current_worktree = 0, his tests
should have protected them by the change.

You are right, in a future reroll, split 1/3 could leave a fairly small
commit, maybe not a bad thing.  Definitely this need a reroll, because
of the style issues, but I will wait some time for other reviewers. 

> >> 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.
> 
> OK.  Thanks.
> 
> >> > +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.
> 
> When you did not change a thing in such a way that it does not
> change the relationship between that thing and other things, it
> makes perfect sense to keep the same term to refer to the thing.
> Otherwise, once the thing starts playing different roles in the
> world, there may be a better word to refer to the updated and
> improved thing.

I tried to maintain the relationship and the role, too.  Just introduce
the helper, as Phillip suggested and I think it is a good idea. 

Thank you.