Re: [PATCH v3 1/8] rebase --apply: remove duplicated code
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 12, 2022, 20:36 UTC
- Message-ID
- <xmqqedvclqxm.fsf@gitster.g>
- In-Reply-To
- <a84cf971a753e294555ca8f2b7eaa4c75a8fa491.1665567312.git.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 7 quoted lines
> 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?
Show 5 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.
Show 33 quoted lines
> 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;
> }