Hi Junio
On 13/10/2022 19:13, Junio C Hamano wrote:
Junio C Hamano <gitster@xxxxxxxxx> writes:
"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
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?
Yes, you're right
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.
I've just checked and the tests added in the next patch do test this
path (they fail if I revert this commit). I should be able to swap the
two patches round to demonstrate the change in behavior and rework the
commit commit message for this patch.
Best Wishes
Phillip
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.
