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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 20, 2022, 18:25 UTC
Message-ID: <xmqq35i7r4rj.fsf@gitster.g>
In-Reply-To: <a4320f2fcf35f180e1c585be4105194f1555a874.1650448612.git.gitgitgadget@gmail.com>

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> -	strbuf_reset(&msg);

This is unnecessary, because we have released immediately before.

Show 7 quoted lines

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When we are reattaching HEAD after a fast-forward we can use
> move_to_original_branch() rather than open coding a copy of that
> code. The old code appears to handle the case where the rebase is
> started from a detached HEAD but in fact in that case there is nothing
> to do as we have already updated HEAD.

At the beginning of move_to_original_branch(), there is an early return to do nothing when the head is detached. But the code before the pre-context of the hunk already detached the head at the "onto", and the fast-forward case wants to leave the detached head pointing at that "onto" commit, so the early return without doing anything is exactly what we want to see.

Does that mean that the original code had left two reflog entries for HEAD, one to detach at the "onto" commit, and then in this block to say "rebase finished" here? With the new code, because we return early from move_to_original_branch(), we no longer leave the "rebase finished" record in the reflog of HEAD?

Is it done deliberately as an improvement, or an oversight that led to a possible regression?

Or do we still add the reflog entry for HEAD that says "rebase finished" somewhere else when we trigger the early return and I am misreading the code?

Show 16 quoted lines

> >  	if (oideq(&merge_base, &options.orig_head)) {
>  		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;
>  	}