Re: [PATCH v3 1/8] rebase --apply: remove duplicated code

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 13, 2022, 18:13 UTC
Message-ID: <xmqq35brh9re.fsf@gitster.g>
In-Reply-To: <xmqqedvclqxm.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 18 quoted lines

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Use move_to_original_branch() when reattaching HEAD after a fast-forward
>> rather than open coding a copy of that code. move_to_original_branch()
>> does not call reset_head() if head_name is NULL but there should be no
>> user visible changes even though we currently call reset_head() in that
>> case.
>
> move_to_original_branch() uses both .head_msg and .branch_msg and
> uses different messages for them, but the original code below only
> feeds .head_msg while .branch_msg leaves NULL, which leads
> reset.c::update_refs() to use the same message as .head_msg when it
> wants to use .branch_msg (i.e. the message recorded in the reflog of
> the branch).  
>
> Doesn't this difference result in a different behaviour?

I think "git rebase --apply A B" when B is already an descendant of A with a single strand of pearls would trigger the new logic, and instead of the old "rebase finished: %s onto %s" message used for both reflogs, calling move_to_original_branch() will give us "rebase finished: %s onto %s" in the branch reflog, while "rebase finished: returning to %s" in the HEAD reflog.

Note that I am not saying we should not change the behaviour. Saying "returning to X" in the reflog of HEAD may arguably be better than using the same "rebased X onto Y" for reflogs for both HEAD and the underlying branch.

But if that is what is going on, we should record it as improving the reflog message, not removing duplicated code.

Also, it would be good to have a test that demonstrates how the contents of the reflog changes with this change. It took me some time to figure out how to reach that codepath, even though it was relatively easy to see how the reflog message(s) used before and after the patch are different.

Show 12 quoted lines

>> The reason for this is that the reset_head() call does not add a
>> message to the reflog because we're not changing the commit that HEAD
>> points to and so lock_ref_for_update() elides the update. When head_name
>> is not NULL then reset_head() behaves like "git symbolic-ref" and so the
>> reflog is updated.
>
>> Note that the removal of "strbuf_release(&msg)" is safe as there is an
>
> The patch is removing strbuf_reset(), not _release(), here, though.
>
> We have released already so there is no point to reset it again, so
> the removal is still safe.

Thanks.