On 22-ene-2023 11:58:05, Junio C Hamano wrote:
> Rubén Justo <rjusto@xxxxxxxxx> 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.
