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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2023, 19:58 UTC
Message-ID: <xmqqk01eqr3m.fsf@gitster.g>
In-Reply-To: <766b25e1-2d7a-7b5c-10a9-43e545a57dba@gmail.com>

Rubén Justo <rjusto@gmail.com> writes:

Show 6 quoted lines

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

Show 8 quoted lines

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

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

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.