From: Phillip Wood Date: Thu, 20 Oct 2022 09:50:41 GMT Subject: Re: [PATCH v3 1/8] rebase --apply: remove duplicated code Message-ID: In-Reply-To: Hi Junio On 13/10/2022 19:13, Junio C Hamano wrote: > Junio C Hamano writes: > >> "Phillip Wood via GitGitGadget" writes: >> >>> From: Phillip Wood >>> >>> 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.