| Message ID | a84cf971a753e294555ca8f2b7eaa4c75a8fa491.1665567312.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: make reflog messages independent of the backend | expand |
"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? > 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. > identical call just above this hunk which can be seen by viewing the > diff with -U6. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > builtin/rebase.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index a2ca66b54be..51accb4fd61 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1808,19 +1808,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > * If the onto is a proper descendant of the tip of the branch, then > * we just fast-forwarded. > */ > - strbuf_reset(&msg); > if (oideq(&branch_base, &options.orig_head->object.oid)) { > printf(_("Fast-forwarded %s to %s.\n"), > branch_name, options.onto_name); > - strbuf_addf(&msg, "rebase finished: %s onto %s", > - options.head_name ? options.head_name : "detached HEAD", > - oid_to_hex(&options.onto->object.oid)); > - memset(&ropts, 0, sizeof(ropts)); > - ropts.branch = options.head_name; > - ropts.flags = RESET_HEAD_REFS_ONLY; > - ropts.head_msg = msg.buf; > - reset_head(the_repository, &ropts); > - strbuf_release(&msg); > + move_to_original_branch(&options); > ret = finish_rebase(&options); > goto cleanup; > }
Junio C Hamano <gitster@pobox.com> writes: > "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. >> 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.
Hi Junio On 13/10/2022 19:13, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> "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? 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.
diff with -U6. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- builtin/rebase.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index a2ca66b54be..51accb4fd61 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1808,19 +1808,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) * If the onto is a proper descendant of the tip of the branch, then * we just fast-forwarded. */ - strbuf_reset(&msg); if (oideq(&branch_base, &options.orig_head->object.oid)) { printf(_("Fast-forwarded %s to %s.\n"), branch_name, options.onto_name); - strbuf_addf(&msg, "rebase finished: %s onto %s", - options.head_name ? options.head_name : "detached HEAD", - oid_to_hex(&options.onto->object.oid)); - memset(&ropts, 0, sizeof(ropts)); - ropts.branch = options.head_name; - ropts.flags = RESET_HEAD_REFS_ONLY; - ropts.head_msg = msg.buf; - reset_head(the_repository, &ropts); - strbuf_release(&msg); + move_to_original_branch(&options); ret = finish_rebase(&options); goto cleanup; }
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. 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 identical call just above this hunk which can be seen by viewing the