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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 20, 2022, 10:10 UTC
Message-ID: <220420.86a6cg83b0.gmgdl@evledraar.gmail.com>
In-Reply-To: <a4320f2fcf35f180e1c585be4105194f1555a874.1650448612.git.gitgitgadget@gmail.com>

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

Show 11 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.
>
> 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...

Show 31 quoted lines

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