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

Commit Message

Phillip Wood April 20, 2022, 9:56 a.m. UTC

From: Phillip Wood <[e-mail]>

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.

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

Comments

Ævar Arnfjörð Bjarmason April 20, 2022, 10:10 a.m. UTC | #1

On Wed, Apr 20 2022, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <[e-mail]>
>
> 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.
>
> 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
> diff with -U6.

Instead of assuring the reader that this is OK, perhaps just squash this
in:
	
	diff --git a/builtin/rebase.c b/builtin/rebase.c
	index 4a664964c30..5108679e816 100644
	--- a/builtin/rebase.c
	+++ b/builtin/rebase.c
	@@ -1029,7 +1029,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
	 	int ret, flags, total_argc, in_progress = 0;
	 	int keep_base = 0;
	 	int ok_to_skip_pre_rebase = 0;
	-	struct strbuf msg = STRBUF_INIT;
	 	struct strbuf revisions = STRBUF_INIT;
	 	struct strbuf buf = STRBUF_INIT;
	 	struct object_id merge_base;
	@@ -1769,17 +1768,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
	 		printf(_("First, rewinding head to replay your work on top of "
	 			 "it...\n"));
	 
	-	strbuf_addf(&msg, "%s: checkout %s",
	-		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
	-	ropts.oid = &options.onto->object.oid;
	-	ropts.orig_head = &options.orig_head,
	-	ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
	+	{
	+		struct strbuf msg = STRBUF_INIT;
	+
	+		strbuf_addf(&msg, "%s: checkout %s",
	+			    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
	+			    options.onto_name);
	+		ropts.oid = &options.onto->object.oid;
	+		ropts.orig_head = &options.orig_head,
	+			ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
	 			RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
	-	ropts.head_msg = msg.buf;
	-	ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
	-	if (reset_head(the_repository, &ropts))
	-		die(_("Could not detach HEAD"));
	-	strbuf_release(&msg);
	+		ropts.head_msg = msg.buf;
	+		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
	+		if (reset_head(the_repository, &ropts))
	+			die(_("Could not detach HEAD"));
	+		strbuf_release(&msg);
	+	}
	 
	 	/*
	 	 * If the onto is a proper descendant of the tip of the branch, then

That's a 2-line change (aside from the scope addition) if viewed with
-w. I.e. before your change below we needed &msg in two places, now that
we only need it in one it seems better just to move the variable to be
scoped with its only user.

And maybe it's getting too much into unrelated cleanup, but this change
also leaves the loose end that the "ropts" variable no longer needs to
be shared. You added it in 6ae8086161d (reset_head(): take struct
rebase_head_opts, 2022-01-26) along whith the memset() removed here, but
(on top of the proposed squash) we could also do this:

	
	diff --git a/builtin/rebase.c b/builtin/rebase.c
	index 5108679e816..8e2dc74c834 100644
	--- a/builtin/rebase.c
	+++ b/builtin/rebase.c
	@@ -1043,7 +1043,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
	 	int reschedule_failed_exec = -1;
	 	int allow_preemptive_ff = 1;
	 	int preserve_merges_selected = 0;
	-	struct reset_head_opts ropts = { 0 };
	 	struct option builtin_rebase_options[] = {
	 		OPT_STRING(0, "onto", &options.onto_name,
	 			   N_("revision"),
	@@ -1275,7 +1274,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
	 	}
	 	case ACTION_SKIP: {
	 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
	-
	+		struct reset_head_opts ropts = { 0 };
	+	
	 		options.action = "skip";
	 		set_reflog_action(&options);
	 
	@@ -1291,6 +1291,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
	 	}
	 	case ACTION_ABORT: {
	 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
	+		struct reset_head_opts ropts = { 0 };
	+
	 		options.action = "abort";
	 		set_reflog_action(&options);
	 
	@@ -1770,6 +1772,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
	 
	 	{
	 		struct strbuf msg = STRBUF_INIT;
	+		struct reset_head_opts ropts = { 0 };
	 
	 		strbuf_addf(&msg, "%s: checkout %s",
	 			    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),

I.e. the "ropts" earlier in the function wasn't shared with the code
below, we'd "goto" past it...
>
> Signed-off-by: Phillip Wood <[e-mail]>
> ---
>  builtin/rebase.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index e942c300f8c..4832f16e675 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1782,19 +1782,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(&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;
>  	}

Junio C Hamano April 20, 2022, 6:25 p.m. UTC | #2

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

> -	strbuf_reset(&msg);

This is unnecessary, because we have released immediately before.

> From: Phillip Wood <[e-mail]>
>
> 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?

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

Junio C Hamano April 20, 2022, 6:39 p.m. UTC | #3

Junio C Hamano <[e-mail]> writes:

> "Phillip Wood via GitGitGadget" <[e-mail]> writes:
>
>> -	strbuf_reset(&msg);
>
> This is unnecessary, because we have released immediately before.

Sorry. I meant "reset is unnecessary and it is the right thing to
remove it here", but I just realized it can be misread to say "this
change is unnecessary".

Patch

diff with -U6.

Signed-off-by: Phillip Wood <[e-mail]>
---
 builtin/rebase.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e942c300f8c..4832f16e675 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1782,19 +1782,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(&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;
 	}