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

From: Junio C Hamano <gitster@pobox.com>
To: "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Phillip Wood" <phillip.wood123@gmail.com>,
	"Christian Couder" <christian.couder@gmail.com>,
	"Elijah Newren" <newren@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Calvin Wan" <calvinwan@google.com>,
	"Emily Shaffer" <emilyshaffer@google.com>,
	"Glen Choo" <chooglen@google.com>,
	"Victoria Dye" <vdye@github.com>,
	"Phillip Wood" <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v3 1/8] rebase --apply: remove duplicated code
Date: Wed, 12 Oct 2022 13:36:53 -0700	[thread overview]
Message-ID: <xmqqedvclqxm.fsf@gitster.g> (raw)
In-Reply-To: <a84cf971a753e294555ca8f2b7eaa4c75a8fa491.1665567312.git.gitgitgadget@gmail.com> (Phillip Wood via GitGitGadget's message of "Wed, 12 Oct 2022 09:35:05 +0000")

"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;
>  	}