threads / patch / 57450

patch, 7 partsrebase --merge: fix reflog message after skipping

Subject: [PATCH 3/7] rebase --merge: fix reflog message after skipping

## tl;dr

73 messages between Feb 21, 2022 and Oct 26, 2022. Diffs are folded; open one to read it.

replies: 72people: 5as markdown or json

Phillip Wood via GitGitGadget· Feb 21, 2022, 11:10 UTC · lore

[PATCH 0/7] rebase: make reflog messages independent of the backend

This is a series of rebase reflog related patches with the aim of unifying the reflog messages from the two rebase backends.

 * improve rebase reflog test coverage
 * rebase --merge: fix reflog messages for --continue and --skip
 * rebase --apply: respect GIT_REFLOG_ACTION
 * rebase --abort: improve reflog message
 * unify reflog messages between the two rebase backends
This series is based on pw/use-inprocess-checkout-in-rebase
Phillip Wood (7):
  rebase --apply: remove duplicated code
  rebase --merge: fix reflog when continuing
  rebase --merge: fix reflog message after skipping
  rebase --apply: respect GIT_REFLOG_ACTION
  rebase --apply: make reflog messages match rebase --merge
  rebase --abort: improve reflog message
  rebase: cleanup action handling
 builtin/rebase.c          | 144 ++++++++++++-----------------
 sequencer.c               |   5 ++
 t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++-------
 3 files changed, 214 insertions(+), 120 deletions(-)
base-commit: 38c541ce94048cf72aa4f465be9314423a57f445
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1150
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Feb 21, 2022, 11:10 UTC · re: Phillip Wood via GitGitGadget · lore
From: Phillip Wood <phillip.wood@dunelm.org.uk>

The reflog message for every pick after running "rebase --skip" looks like

	rebase (skip) (pick): commit subject line
Fix this by not appending " (skip)" to the reflog action.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          |  2 --
 t/t3406-rebase-message.sh | 24 ++++++++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)
Show changes to 2 files +24 −2

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index cd9a4f3e2f1..36863117fba 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1273,8 +1273,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
 		options.action = "skip";
-		set_reflog_action(&options);
-
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 		ropts.flags = RESET_HEAD_HARD;
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 3ca2fbb0d59..8aa6a79acc1 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -163,6 +163,30 @@ test_reflog () {
 	# check there is only one new entry in the branch reflog
 	test_cmp_rev fast-forward@{1} X
 	'
+
+	test_expect_success "rebase $mode --skip reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout conflicts &&
+	test_when_finished "git reset --hard Q" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		test_must_fail git rebase $mode main &&
+		git rebase --skip
+	) &&
+
+	git log -g --format=%gs -4 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
+	${reflog_action:-rebase} (pick): Q
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
+	EOF
+	test_cmp expect actual
+	'
 }
 
 test_reflog --merge
-- 
gitgitgadget
Elijah Newren· Apr 17, 2022, 01:58 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH 3/7] rebase --merge: fix reflog message after skipping

On Mon, Feb 21, 2022 at 3:19 PM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 9 quoted lines
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The reflog message for every pick after running "rebase --skip" looks
> like
>
>         rebase (skip) (pick): commit subject line
>
> Fix this by not appending " (skip)" to the reflog action.
Nice catch, and cool that the fix was so simple.
Show 57 quoted lines
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c          |  2 --
>  t/t3406-rebase-message.sh | 24 ++++++++++++++++++++++++
>  2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index cd9a4f3e2f1..36863117fba 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1273,8 +1273,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                 struct string_list merge_rr = STRING_LIST_INIT_DUP;
>
>                 options.action = "skip";
> -               set_reflog_action(&options);
> -
>                 rerere_clear(the_repository, &merge_rr);
>                 string_list_clear(&merge_rr, 1);
>                 ropts.flags = RESET_HEAD_HARD;
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 3ca2fbb0d59..8aa6a79acc1 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -163,6 +163,30 @@ test_reflog () {
>         # check there is only one new entry in the branch reflog
>         test_cmp_rev fast-forward@{1} X
>         '
> +
> +       test_expect_success "rebase $mode --skip reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
> +       git checkout conflicts &&
> +       test_when_finished "git reset --hard Q" &&
> +
> +       (
> +               if test -n "$reflog_action"
> +               then
> +                       GIT_REFLOG_ACTION="$reflog_action" &&
> +                       export GIT_REFLOG_ACTION
> +               fi &&
> +               test_must_fail git rebase $mode main &&
> +               git rebase --skip
> +       ) &&
> +
> +       git log -g --format=%gs -4 >actual &&
> +       write_reflog_expect <<-EOF &&
> +       ${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
> +       ${reflog_action:-rebase} (pick): Q
> +       ${reflog_action:-rebase} (pick): P
> +       ${reflog_action:-rebase} (start): checkout main
> +       EOF
> +       test_cmp expect actual
> +       '
>  }
>
>  test_reflog --merge
> --
> gitgitgadget
>
Phillip Wood via GitGitGadget· Feb 21, 2022, 11:10 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH 4/7] rebase --apply: respect GIT_REFLOG_ACTION

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The reflog messages when finishing a rebase hard code "rebase" rather than using GIT_REFLOG_ACTION.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 7 ++++---
 t/t3406-rebase-message.sh | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)
Show changes to 2 files +5 −4

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 36863117fba..e50361fc2a9 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -580,10 +580,11 @@ static int move_to_original_branch(struct rebase_options *opts)
 	if (!opts->onto)
 		BUG("move_to_original_branch without onto");
 
-	strbuf_addf(&branch_reflog, "rebase finished: %s onto %s",
+	strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
+		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
-	strbuf_addf(&head_reflog, "rebase finished: returning to %s",
-		    opts->head_name);
+	strbuf_addf(&head_reflog, "%s finished: returning to %s",
+		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
 	ropts.branch = opts->head_name;
 	ropts.flags = RESET_HEAD_REFS_ONLY;
 	ropts.branch_msg = branch_reflog.buf;
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 8aa6a79acc1..bb2a4949abc 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
 write_reflog_expect () {
 	if test $mode = --apply
 	then
-		sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//'
+		sed 's/(finish)/finished/; s/ ([^)]*)//'
 	else
 		cat
 	fi >expect
-- 
gitgitgadget
Christian Couder· Apr 7, 2022, 13:59 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH 4/7] rebase --apply: respect GIT_REFLOG_ACTION

On Tue, Feb 22, 2022 at 6:41 AM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 5 quoted lines
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The reflog messages when finishing a rebase hard code "rebase" rather
> than using GIT_REFLOG_ACTION.
Yeah, but GIT_REFLOG_ACTION is something like "pick" or "continue" ...
Show 18 quoted lines
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c          | 7 ++++---
>  t/t3406-rebase-message.sh | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 36863117fba..e50361fc2a9 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -580,10 +580,11 @@ static int move_to_original_branch(struct rebase_options *opts)
>         if (!opts->onto)
>                 BUG("move_to_original_branch without onto");
>
> -       strbuf_addf(&branch_reflog, "rebase finished: %s onto %s",
> +       strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
> +                   getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
>                     opts->head_name, oid_to_hex(&opts->onto->object.oid));

... so this will say "pick finished..." instead of "rebase finished..." while it may be better to have something like "rebase (pick) finished...", or am I missing something?

> -       strbuf_addf(&head_reflog, "rebase finished: returning to %s",
> -                   opts->head_name);
> +       strbuf_addf(&head_reflog, "%s finished: returning to %s",
> +                   getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);

In patch 2/7 GIT_REFLOG_ACTION, instead of GIT_REFLOG_ACTION_ENVIRONMENT, is used.

It looks like GIT_REFLOG_ACTION_ENVIRONMENT is defined in reset.h while GIT_REFLOG_ACTION is defined in sequencer.c. Maybe we could get rid of one of these two, and use the same everywhere?

>         ropts.branch = opts->head_name;
>         ropts.flags = RESET_HEAD_REFS_ONLY;
>         ropts.branch_msg = branch_reflog.buf;
Phillip Wood· Apr 15, 2022, 14:03 UTC · re: Christian Couder · lore

Re: [PATCH 4/7] rebase --apply: respect GIT_REFLOG_ACTION

Hi Christian
On 07/04/2022 14:59, Christian Couder wrote:
Show 9 quoted lines
> On Tue, Feb 22, 2022 at 6:41 AM Phillip Wood via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> The reflog messages when finishing a rebase hard code "rebase" rather
>> than using GIT_REFLOG_ACTION.
> 
> Yeah, but GIT_REFLOG_ACTION is something like "pick" or "continue" ...

GIT_REFLOG_ACTION can be set in the environment by the user (or more likely a script) to change the command name in reflog messages created by rebase. At the moment we respect that for all the messages apart from the ones being changed here.

Best Wishes
Phillip
Show 38 quoted lines
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   builtin/rebase.c          | 7 ++++---
>>   t/t3406-rebase-message.sh | 2 +-
>>   2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 36863117fba..e50361fc2a9 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -580,10 +580,11 @@ static int move_to_original_branch(struct rebase_options *opts)
>>          if (!opts->onto)
>>                  BUG("move_to_original_branch without onto");
>>
>> -       strbuf_addf(&branch_reflog, "rebase finished: %s onto %s",
>> +       strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
>> +                   getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
>>                      opts->head_name, oid_to_hex(&opts->onto->object.oid));
> 
> ... so this will say "pick finished..." instead of "rebase
> finished..." while it may be better to have something like "rebase
> (pick) finished...", or am I missing something?
> 
>> -       strbuf_addf(&head_reflog, "rebase finished: returning to %s",
>> -                   opts->head_name);
>> +       strbuf_addf(&head_reflog, "%s finished: returning to %s",
>> +                   getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
> 
> In patch 2/7 GIT_REFLOG_ACTION, instead of
> GIT_REFLOG_ACTION_ENVIRONMENT, is used.
> 
> It looks like GIT_REFLOG_ACTION_ENVIRONMENT is defined in reset.h
> while GIT_REFLOG_ACTION is defined in sequencer.c. Maybe we could get
> rid of one of these two, and use the same everywhere?
> 
>>          ropts.branch = opts->head_name;
>>          ropts.flags = RESET_HEAD_REFS_ONLY;
>>          ropts.branch_msg = branch_reflog.buf;
Phillip Wood via GitGitGadget· Feb 21, 2022, 11:10 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH 7/7] rebase: cleanup action handling

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Treating the action as a string is a hang over from the scripted rebase. The last commit removed the only remaining use of the action that required a string so lets convert the other action users to use the existing action enum instead. If we ever need the action name as a string in the future the action_names array exists exactly for that purpose.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 91 +++++++++++++++++++++++-------------------------
 1 file changed, 43 insertions(+), 48 deletions(-)
Show changes to builtin/rebase.c +43 −48
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 70426e17b40..323f5154092 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -58,6 +58,26 @@ enum empty_type {
 	EMPTY_ASK
 };
 
+enum action {
+	ACTION_NONE = 0,
+	ACTION_CONTINUE,
+	ACTION_SKIP,
+	ACTION_ABORT,
+	ACTION_QUIT,
+	ACTION_EDIT_TODO,
+	ACTION_SHOW_CURRENT_PATCH
+};
+
+static const char *action_names[] = {
+	"undefined",
+	"continue",
+	"skip",
+	"abort",
+	"quit",
+	"edit_todo",
+	"show_current_patch"
+};
+
 struct rebase_options {
 	enum rebase_type type;
 	enum empty_type empty;
@@ -84,7 +104,7 @@ struct rebase_options {
 		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
 	} flags;
 	struct strvec git_am_opts;
-	const char *action;
+	enum action action;
 	int signoff;
 	int allow_rerere_autoupdate;
 	int keep_empty;
@@ -155,24 +175,6 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	return replay;
 }
 
-enum action {
-	ACTION_NONE = 0,
-	ACTION_CONTINUE,
-	ACTION_SKIP,
-	ACTION_ABORT,
-	ACTION_QUIT,
-	ACTION_EDIT_TODO,
-	ACTION_SHOW_CURRENT_PATCH
-};
-
-static const char *action_names[] = { "undefined",
-				      "continue",
-				      "skip",
-				      "abort",
-				      "quit",
-				      "edit_todo",
-				      "show_current_patch" };
-
 static int edit_todo_file(unsigned flags)
 {
 	const char *todo_file = rebase_path_todo();
@@ -309,8 +311,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	return ret;
 }
 
-static int run_sequencer_rebase(struct rebase_options *opts,
-				  enum action command)
+static int run_sequencer_rebase(struct rebase_options *opts)
 {
 	unsigned flags = 0;
 	int abbreviate_commands = 0, ret = 0;
@@ -325,7 +326,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 	flags |= opts->reapply_cherry_picks ? TODO_LIST_REAPPLY_CHERRY_PICKS : 0;
 	flags |= opts->flags & REBASE_NO_QUIET ? TODO_LIST_WARN_SKIPPED_CHERRY_PICKS : 0;
 
-	switch (command) {
+	switch (opts->action) {
 	case ACTION_NONE: {
 		if (!opts->onto && !opts->upstream)
 			die(_("a base commit must be provided with --upstream or --onto"));
@@ -358,7 +359,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 		break;
 	}
 	default:
-		BUG("invalid command '%d'", command);
+		BUG("invalid command '%d'", opts->action);
 	}
 
 	return ret;
@@ -615,7 +616,7 @@ static int run_am(struct rebase_options *opts)
 	strvec_push(&am.args, "am");
 	strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
 		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
-	if (opts->action && !strcmp("continue", opts->action)) {
+	if (opts->action == ACTION_CONTINUE) {
 		strvec_push(&am.args, "--resolved");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
 		if (opts->gpg_sign_opt)
@@ -626,7 +627,7 @@ static int run_am(struct rebase_options *opts)
 
 		return move_to_original_branch(opts);
 	}
-	if (opts->action && !strcmp("skip", opts->action)) {
+	if (opts->action == ACTION_SKIP) {
 		strvec_push(&am.args, "--skip");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
 		status = run_command(&am);
@@ -635,7 +636,7 @@ static int run_am(struct rebase_options *opts)
 
 		return move_to_original_branch(opts);
 	}
-	if (opts->action && !strcmp("show-current-patch", opts->action)) {
+	if (opts->action == ACTION_SHOW_CURRENT_PATCH) {
 		strvec_push(&am.args, "--show-current-patch");
 		return run_command(&am);
 	}
@@ -728,7 +729,7 @@ static int run_am(struct rebase_options *opts)
 	return status;
 }
 
-static int run_specific_rebase(struct rebase_options *opts, enum action action)
+static int run_specific_rebase(struct rebase_options *opts)
 {
 	int status;
 
@@ -746,7 +747,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
 			opts->gpg_sign_opt = tmp;
 		}
 
-		status = run_sequencer_rebase(opts, action);
+		status = run_sequencer_rebase(opts);
 	} else if (opts->type == REBASE_APPLY)
 		status = run_am(opts);
 	else
@@ -1016,7 +1017,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct strbuf buf = STRBUF_INIT;
 	struct object_id merge_base;
 	int ignore_whitespace = 0;
-	enum action action = ACTION_NONE;
 	const char *gpg_sign = NULL;
 	struct string_list exec = STRING_LIST_INIT_NODUP;
 	const char *rebase_merges = NULL;
@@ -1065,18 +1065,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "no-ff", &options.flags,
 			N_("cherry-pick all commits, even if unchanged"),
 			REBASE_FORCE),
-		OPT_CMDMODE(0, "continue", &action, N_("continue"),
+		OPT_CMDMODE(0, "continue", &options.action, N_("continue"),
 			    ACTION_CONTINUE),
-		OPT_CMDMODE(0, "skip", &action,
+		OPT_CMDMODE(0, "skip", &options.action,
 			    N_("skip current patch and continue"), ACTION_SKIP),
-		OPT_CMDMODE(0, "abort", &action,
+		OPT_CMDMODE(0, "abort", &options.action,
 			    N_("abort and check out the original branch"),
 			    ACTION_ABORT),
-		OPT_CMDMODE(0, "quit", &action,
+		OPT_CMDMODE(0, "quit", &options.action,
 			    N_("abort but keep HEAD where it is"), ACTION_QUIT),
-		OPT_CMDMODE(0, "edit-todo", &action, N_("edit the todo list "
+		OPT_CMDMODE(0, "edit-todo", &options.action, N_("edit the todo list "
 			    "during an interactive rebase"), ACTION_EDIT_TODO),
-		OPT_CMDMODE(0, "show-current-patch", &action,
+		OPT_CMDMODE(0, "show-current-patch", &options.action,
 			    N_("show the patch file being applied or merged"),
 			    ACTION_SHOW_CURRENT_PATCH),
 		OPT_CALLBACK_F(0, "apply", &options, NULL,
@@ -1189,7 +1189,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (preserve_merges_selected)
 		die(_("--preserve-merges was replaced by --rebase-merges"));
 
-	if (action != ACTION_NONE && total_argc != 2) {
+	if (options.action != ACTION_NONE && total_argc != 2) {
 		usage_with_options(builtin_rebase_usage,
 				   builtin_rebase_options);
 	}
@@ -1208,11 +1208,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.root && options.fork_point > 0)
 		die(_("cannot combine '--root' with '--fork-point'"));
 
-	if (action != ACTION_NONE && !in_progress)
+	if (options.action != ACTION_NONE && !in_progress)
 		die(_("No rebase in progress?"));
 	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
 
-	if (action == ACTION_EDIT_TODO && !is_merge(&options))
+	if (options.action == ACTION_EDIT_TODO && !is_merge(&options))
 		die(_("The --edit-todo action can only be used during "
 		      "interactive rebase."));
 
@@ -1222,16 +1222,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		else if (exec.nr)
 			trace2_cmd_mode("interactive-exec");
 		else
-			trace2_cmd_mode(action_names[action]);
+			trace2_cmd_mode(action_names[options.action]);
 	}
 
-	switch (action) {
+	switch (options.action) {
 	case ACTION_CONTINUE: {
 		struct object_id head;
 		struct lock_file lock_file = LOCK_INIT;
 		int fd;
 
-		options.action = "continue";
 		/* Sanity check */
 		if (get_oid("HEAD", &head))
 			die(_("Cannot read HEAD"));
@@ -1257,7 +1256,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	case ACTION_SKIP: {
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
-		options.action = "skip";
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 		ropts.flags = RESET_HEAD_HARD;
@@ -1272,7 +1270,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 		struct strbuf head_msg = STRBUF_INIT;
 
-		options.action = "abort";
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 
@@ -1312,17 +1309,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		goto cleanup;
 	}
 	case ACTION_EDIT_TODO:
-		options.action = "edit-todo";
 		options.dont_finish_rebase = 1;
 		goto run_rebase;
 	case ACTION_SHOW_CURRENT_PATCH:
-		options.action = "show-current-patch";
 		options.dont_finish_rebase = 1;
 		goto run_rebase;
 	case ACTION_NONE:
 		break;
 	default:
-		BUG("action: %d", action);
+		BUG("action: %d", options.action);
 	}
 
 	/* Make sure no rebase is in progress */
@@ -1346,7 +1341,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||
-	    (action != ACTION_NONE) ||
+	    (options.action != ACTION_NONE) ||
 	    (exec.nr > 0) ||
 	    options.autosquash) {
 		allow_preemptive_ff = 0;
@@ -1786,7 +1781,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	options.revisions = revisions.buf;
 
 run_rebase:
-	ret = run_specific_rebase(&options, action);
+	ret = run_specific_rebase(&options);
 
 cleanup:
 	strbuf_release(&buf);
-- 
gitgitgadget
Elijah Newren· Apr 17, 2022, 02:07 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH 7/7] rebase: cleanup action handling

On Mon, Feb 21, 2022 at 9:35 PM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 283 quoted lines
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Treating the action as a string is a hang over from the scripted
> rebase. The last commit removed the only remaining use of the action
> that required a string so lets convert the other action users to use
> the existing action enum instead. If we ever need the action name as a
> string in the future the action_names array exists exactly for that
> purpose.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c | 91 +++++++++++++++++++++++-------------------------
>  1 file changed, 43 insertions(+), 48 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 70426e17b40..323f5154092 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -58,6 +58,26 @@ enum empty_type {
>         EMPTY_ASK
>  };
>
> +enum action {
> +       ACTION_NONE = 0,
> +       ACTION_CONTINUE,
> +       ACTION_SKIP,
> +       ACTION_ABORT,
> +       ACTION_QUIT,
> +       ACTION_EDIT_TODO,
> +       ACTION_SHOW_CURRENT_PATCH
> +};
> +
> +static const char *action_names[] = {
> +       "undefined",
> +       "continue",
> +       "skip",
> +       "abort",
> +       "quit",
> +       "edit_todo",
> +       "show_current_patch"
> +};
> +
>  struct rebase_options {
>         enum rebase_type type;
>         enum empty_type empty;
> @@ -84,7 +104,7 @@ struct rebase_options {
>                 REBASE_INTERACTIVE_EXPLICIT = 1<<4,
>         } flags;
>         struct strvec git_am_opts;
> -       const char *action;
> +       enum action action;
>         int signoff;
>         int allow_rerere_autoupdate;
>         int keep_empty;
> @@ -155,24 +175,6 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>         return replay;
>  }
>
> -enum action {
> -       ACTION_NONE = 0,
> -       ACTION_CONTINUE,
> -       ACTION_SKIP,
> -       ACTION_ABORT,
> -       ACTION_QUIT,
> -       ACTION_EDIT_TODO,
> -       ACTION_SHOW_CURRENT_PATCH
> -};
> -
> -static const char *action_names[] = { "undefined",
> -                                     "continue",
> -                                     "skip",
> -                                     "abort",
> -                                     "quit",
> -                                     "edit_todo",
> -                                     "show_current_patch" };
> -
>  static int edit_todo_file(unsigned flags)
>  {
>         const char *todo_file = rebase_path_todo();
> @@ -309,8 +311,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>         return ret;
>  }
>
> -static int run_sequencer_rebase(struct rebase_options *opts,
> -                                 enum action command)
> +static int run_sequencer_rebase(struct rebase_options *opts)
>  {
>         unsigned flags = 0;
>         int abbreviate_commands = 0, ret = 0;
> @@ -325,7 +326,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
>         flags |= opts->reapply_cherry_picks ? TODO_LIST_REAPPLY_CHERRY_PICKS : 0;
>         flags |= opts->flags & REBASE_NO_QUIET ? TODO_LIST_WARN_SKIPPED_CHERRY_PICKS : 0;
>
> -       switch (command) {
> +       switch (opts->action) {
>         case ACTION_NONE: {
>                 if (!opts->onto && !opts->upstream)
>                         die(_("a base commit must be provided with --upstream or --onto"));
> @@ -358,7 +359,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
>                 break;
>         }
>         default:
> -               BUG("invalid command '%d'", command);
> +               BUG("invalid command '%d'", opts->action);
>         }
>
>         return ret;
> @@ -615,7 +616,7 @@ static int run_am(struct rebase_options *opts)
>         strvec_push(&am.args, "am");
>         strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
>                      getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
> -       if (opts->action && !strcmp("continue", opts->action)) {
> +       if (opts->action == ACTION_CONTINUE) {
>                 strvec_push(&am.args, "--resolved");
>                 strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
>                 if (opts->gpg_sign_opt)
> @@ -626,7 +627,7 @@ static int run_am(struct rebase_options *opts)
>
>                 return move_to_original_branch(opts);
>         }
> -       if (opts->action && !strcmp("skip", opts->action)) {
> +       if (opts->action == ACTION_SKIP) {
>                 strvec_push(&am.args, "--skip");
>                 strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
>                 status = run_command(&am);
> @@ -635,7 +636,7 @@ static int run_am(struct rebase_options *opts)
>
>                 return move_to_original_branch(opts);
>         }
> -       if (opts->action && !strcmp("show-current-patch", opts->action)) {
> +       if (opts->action == ACTION_SHOW_CURRENT_PATCH) {
>                 strvec_push(&am.args, "--show-current-patch");
>                 return run_command(&am);
>         }
> @@ -728,7 +729,7 @@ static int run_am(struct rebase_options *opts)
>         return status;
>  }
>
> -static int run_specific_rebase(struct rebase_options *opts, enum action action)
> +static int run_specific_rebase(struct rebase_options *opts)
>  {
>         int status;
>
> @@ -746,7 +747,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
>                         opts->gpg_sign_opt = tmp;
>                 }
>
> -               status = run_sequencer_rebase(opts, action);
> +               status = run_sequencer_rebase(opts);
>         } else if (opts->type == REBASE_APPLY)
>                 status = run_am(opts);
>         else
> @@ -1016,7 +1017,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>         struct strbuf buf = STRBUF_INIT;
>         struct object_id merge_base;
>         int ignore_whitespace = 0;
> -       enum action action = ACTION_NONE;
>         const char *gpg_sign = NULL;
>         struct string_list exec = STRING_LIST_INIT_NODUP;
>         const char *rebase_merges = NULL;
> @@ -1065,18 +1065,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                 OPT_BIT(0, "no-ff", &options.flags,
>                         N_("cherry-pick all commits, even if unchanged"),
>                         REBASE_FORCE),
> -               OPT_CMDMODE(0, "continue", &action, N_("continue"),
> +               OPT_CMDMODE(0, "continue", &options.action, N_("continue"),
>                             ACTION_CONTINUE),
> -               OPT_CMDMODE(0, "skip", &action,
> +               OPT_CMDMODE(0, "skip", &options.action,
>                             N_("skip current patch and continue"), ACTION_SKIP),
> -               OPT_CMDMODE(0, "abort", &action,
> +               OPT_CMDMODE(0, "abort", &options.action,
>                             N_("abort and check out the original branch"),
>                             ACTION_ABORT),
> -               OPT_CMDMODE(0, "quit", &action,
> +               OPT_CMDMODE(0, "quit", &options.action,
>                             N_("abort but keep HEAD where it is"), ACTION_QUIT),
> -               OPT_CMDMODE(0, "edit-todo", &action, N_("edit the todo list "
> +               OPT_CMDMODE(0, "edit-todo", &options.action, N_("edit the todo list "
>                             "during an interactive rebase"), ACTION_EDIT_TODO),
> -               OPT_CMDMODE(0, "show-current-patch", &action,
> +               OPT_CMDMODE(0, "show-current-patch", &options.action,
>                             N_("show the patch file being applied or merged"),
>                             ACTION_SHOW_CURRENT_PATCH),
>                 OPT_CALLBACK_F(0, "apply", &options, NULL,
> @@ -1189,7 +1189,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>         if (preserve_merges_selected)
>                 die(_("--preserve-merges was replaced by --rebase-merges"));
>
> -       if (action != ACTION_NONE && total_argc != 2) {
> +       if (options.action != ACTION_NONE && total_argc != 2) {
>                 usage_with_options(builtin_rebase_usage,
>                                    builtin_rebase_options);
>         }
> @@ -1208,11 +1208,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>         if (options.root && options.fork_point > 0)
>                 die(_("cannot combine '--root' with '--fork-point'"));
>
> -       if (action != ACTION_NONE && !in_progress)
> +       if (options.action != ACTION_NONE && !in_progress)
>                 die(_("No rebase in progress?"));
>         setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
>
> -       if (action == ACTION_EDIT_TODO && !is_merge(&options))
> +       if (options.action == ACTION_EDIT_TODO && !is_merge(&options))
>                 die(_("The --edit-todo action can only be used during "
>                       "interactive rebase."));
>
> @@ -1222,16 +1222,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                 else if (exec.nr)
>                         trace2_cmd_mode("interactive-exec");
>                 else
> -                       trace2_cmd_mode(action_names[action]);
> +                       trace2_cmd_mode(action_names[options.action]);
>         }
>
> -       switch (action) {
> +       switch (options.action) {
>         case ACTION_CONTINUE: {
>                 struct object_id head;
>                 struct lock_file lock_file = LOCK_INIT;
>                 int fd;
>
> -               options.action = "continue";
>                 /* Sanity check */
>                 if (get_oid("HEAD", &head))
>                         die(_("Cannot read HEAD"));
> @@ -1257,7 +1256,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>         case ACTION_SKIP: {
>                 struct string_list merge_rr = STRING_LIST_INIT_DUP;
>
> -               options.action = "skip";
>                 rerere_clear(the_repository, &merge_rr);
>                 string_list_clear(&merge_rr, 1);
>                 ropts.flags = RESET_HEAD_HARD;
> @@ -1272,7 +1270,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                 struct string_list merge_rr = STRING_LIST_INIT_DUP;
>                 struct strbuf head_msg = STRBUF_INIT;
>
> -               options.action = "abort";
>                 rerere_clear(the_repository, &merge_rr);
>                 string_list_clear(&merge_rr, 1);
>
> @@ -1312,17 +1309,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                 goto cleanup;
>         }
>         case ACTION_EDIT_TODO:
> -               options.action = "edit-todo";
>                 options.dont_finish_rebase = 1;
>                 goto run_rebase;
>         case ACTION_SHOW_CURRENT_PATCH:
> -               options.action = "show-current-patch";
>                 options.dont_finish_rebase = 1;
>                 goto run_rebase;
>         case ACTION_NONE:
>                 break;
>         default:
> -               BUG("action: %d", action);
> +               BUG("action: %d", options.action);
>         }
>
>         /* Make sure no rebase is in progress */
> @@ -1346,7 +1341,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>         }
>
>         if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||
> -           (action != ACTION_NONE) ||
> +           (options.action != ACTION_NONE) ||
>             (exec.nr > 0) ||
>             options.autosquash) {
>                 allow_preemptive_ff = 0;
> @@ -1786,7 +1781,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>         options.revisions = revisions.buf;
>
>  run_rebase:
> -       ret = run_specific_rebase(&options, action);
> +       ret = run_specific_rebase(&options);
>
>  cleanup:
>         strbuf_release(&buf);
> --
> gitgitgadget
Thanks for the nice cleanup!
Phillip Wood via GitGitGadget· Feb 21, 2022, 11:10 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH 5/7] rebase --apply: make reflog messages match rebase --merge

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The apply backend creates slightly different reflog messages to the merge backend when starting and finishing a rebase and when picking commits. The choice of backend is really an implementation detail so it is confusing to have the same command create different messages depending on which backend is selected. Change the apply backend so the reflog messages from the two backends match as closely as possible. Note that there is still a difference when committing a conflict resolution - the merge backend will use "(continue)" rather than "(pick)" in that case as it does not know which command created the conflict that it is committing.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 9 +++++----
 t/t3406-rebase-message.sh | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)
Show changes to 2 files +6 −5

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e50361fc2a9..678339c7bf7 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -580,10 +580,10 @@ static int move_to_original_branch(struct rebase_options *opts)
 	if (!opts->onto)
 		BUG("move_to_original_branch without onto");
 
-	strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
+	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
-	strbuf_addf(&head_reflog, "%s finished: returning to %s",
+	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
 	ropts.branch = opts->head_name;
 	ropts.flags = RESET_HEAD_REFS_ONLY;
@@ -613,7 +613,8 @@ static int run_am(struct rebase_options *opts)
 
 	am.git_cmd = 1;
 	strvec_push(&am.args, "am");
-
+	strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
+		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
 	if (opts->action && !strcmp("continue", opts->action)) {
 		strvec_push(&am.args, "--resolved");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
@@ -1763,7 +1764,7 @@ 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",
+	strbuf_addf(&msg, "%s (start): checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
 	ropts.oid = &options.onto->object.oid;
 	ropts.orig_head = &options.orig_head,
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index bb2a4949abc..5c6cd9af3bc 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
 write_reflog_expect () {
 	if test $mode = --apply
 	then
-		sed 's/(finish)/finished/; s/ ([^)]*)//'
+		sed 's/(continue)/(pick)/'
 	else
 		cat
 	fi >expect
-- 
gitgitgadget
Elijah Newren· Apr 17, 2022, 02:03 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH 5/7] rebase --apply: make reflog messages match rebase --merge

On Mon, Feb 21, 2022 at 3:19 PM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 13 quoted lines
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The apply backend creates slightly different reflog messages to the
> merge backend when starting and finishing a rebase and when picking
> commits. The choice of backend is really an implementation detail so
> it is confusing to have the same command create different messages
> depending on which backend is selected. Change the apply backend so
> the reflog messages from the two backends match as closely as
> possible. Note that there is still a difference when committing a
> conflict resolution - the merge backend will use "(continue)" rather
> than "(pick)" in that case as it does not know which command created
> the conflict that it is committing.

I've always been slightly annoyed by the various differences in these backends; it's nice to see another one disappearing.

Show 31 quoted lines
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c          | 9 +++++----
>  t/t3406-rebase-message.sh | 2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index e50361fc2a9..678339c7bf7 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -580,10 +580,10 @@ static int move_to_original_branch(struct rebase_options *opts)
>         if (!opts->onto)
>                 BUG("move_to_original_branch without onto");
>
> -       strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
> +       strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
>                     getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
>                     opts->head_name, oid_to_hex(&opts->onto->object.oid));
> -       strbuf_addf(&head_reflog, "%s finished: returning to %s",
> +       strbuf_addf(&head_reflog, "%s (finish): returning to %s",
>                     getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
>         ropts.branch = opts->head_name;
>         ropts.flags = RESET_HEAD_REFS_ONLY;
> @@ -613,7 +613,8 @@ static int run_am(struct rebase_options *opts)
>
>         am.git_cmd = 1;
>         strvec_push(&am.args, "am");
> -
> +       strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
> +                    getenv(GIT_REFLOG_ACTION_ENVIRONMENT));

Kinda sad that we're still forking subprocesses and passing things via environment variable, but that's outside the scope of this series. Fix looks fine.

Show 27 quoted lines
>         if (opts->action && !strcmp("continue", opts->action)) {
>                 strvec_push(&am.args, "--resolved");
>                 strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
> @@ -1763,7 +1764,7 @@ 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",
> +       strbuf_addf(&msg, "%s (start): checkout %s",
>                     getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
>         ropts.oid = &options.onto->object.oid;
>         ropts.orig_head = &options.orig_head,
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index bb2a4949abc..5c6cd9af3bc 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
>  write_reflog_expect () {
>         if test $mode = --apply
>         then
> -               sed 's/(finish)/finished/; s/ ([^)]*)//'
> +               sed 's/(continue)/(pick)/'
>         else
>                 cat
>         fi >expect
> --
> gitgitgadget
Phillip Wood via GitGitGadget· Feb 21, 2022, 11:10 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH 2/7] rebase --merge: fix reflog when continuing

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The reflog message for a conflict resolution committed by "rebase --continue" looks like

	rebase (continue): commit subject line
Unfortunately the reflog message each subsequent pick look like
	rebase (continue) (pick): commit subject line

Fix this by setting the reflog message for "rebase --continue" in sequencer_continue() so it does not affect subsequent commits. This introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION in pick_commits(). Both of these will be fixed in a future series that stops the sequencer calling setenv().

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          |   2 -
 sequencer.c               |   5 ++
 t/t3406-rebase-message.sh | 120 +++++++++++++++++++++++++-------------
 3 files changed, 86 insertions(+), 41 deletions(-)
Show changes to 3 files +86 −41

builtin/rebase.c, sequencer.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4832f16e675..cd9a4f3e2f1 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1247,8 +1247,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		int fd;
 
 		options.action = "continue";
-		set_reflog_action(&options);
-
 		/* Sanity check */
 		if (get_oid("HEAD", &head))
 			die(_("Cannot read HEAD"));
diff --git a/sequencer.c b/sequencer.c
index bdd66b4b67a..3634ad5baa9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4777,6 +4777,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 	if (read_populate_opts(opts))
 		return -1;
 	if (is_rebase_i(opts)) {
+		char *previous_reflog_action;
+
 		if ((res = read_populate_todo(r, &todo_list, opts)))
 			goto release_todo_list;
 
@@ -4787,10 +4789,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 			unlink(rebase_path_dropped());
 		}
 
+		previous_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
+		setenv(GIT_REFLOG_ACTION, reflog_message(opts, "continue", NULL), 1);
 		if (commit_staged_changes(r, opts, &todo_list)) {
 			res = -1;
 			goto release_todo_list;
 		}
+		setenv(GIT_REFLOG_ACTION, previous_reflog_action, 1);
 	} else if (!file_exists(get_todo_path(opts)))
 		return continue_single_pick(r, opts);
 	else if ((res = read_populate_todo(r, &todo_list, opts)))
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index d17b450e811..3ca2fbb0d59 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -10,10 +10,16 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 test_expect_success 'setup' '
 	test_commit O fileO &&
 	test_commit X fileX &&
+	git branch fast-forward &&
 	test_commit A fileA &&
 	test_commit B fileB &&
 	test_commit Y fileY &&
 
+	git checkout -b conflicts O &&
+	test_commit P &&
+	test_commit conflict-X fileX &&
+	test_commit Q &&
+
 	git checkout -b topic O &&
 	git cherry-pick A B &&
 	test_commit Z fileZ &&
@@ -79,54 +85,90 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
 	test_i18ngrep "Invalid whitespace option" err
 '
 
-test_expect_success 'GIT_REFLOG_ACTION' '
-	git checkout start &&
-	test_commit reflog-onto &&
-	git checkout -b reflog-topic start &&
-	test_commit reflog-to-rebase &&
-
-	git rebase reflog-onto &&
-	git log -g --format=%gs -3 >actual &&
-	cat >expect <<-\EOF &&
-	rebase (finish): returning to refs/heads/reflog-topic
-	rebase (pick): reflog-to-rebase
-	rebase (start): checkout reflog-onto
+write_reflog_expect () {
+	if test $mode = --apply
+	then
+		sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//'
+	else
+		cat
+	fi >expect
+}
+
+test_reflog () {
+	mode=$1
+	reflog_action="$2"
+
+	test_expect_success "rebase $mode reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout conflicts &&
+	test_when_finished "git reset --hard Q" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		test_must_fail git rebase $mode main &&
+		echo resolved >fileX &&
+		git add fileX &&
+		git rebase --continue
+	) &&
+
+	git log -g --format=%gs -5 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
+	${reflog_action:-rebase} (pick): Q
+	${reflog_action:-rebase} (continue): conflict-X
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
 	EOF
 	test_cmp expect actual &&
 
-	git checkout -b reflog-prefix reflog-to-rebase &&
-	GIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto &&
-	git log -g --format=%gs -3 >actual &&
-	cat >expect <<-\EOF &&
-	change-the-reflog (finish): returning to refs/heads/reflog-prefix
-	change-the-reflog (pick): reflog-to-rebase
-	change-the-reflog (start): checkout reflog-onto
+	git log -g --format=%gs -1 conflicts >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): refs/heads/conflicts onto $(git rev-parse main)
 	EOF
-	test_cmp expect actual
-'
-
-test_expect_success 'rebase --apply reflog' '
-	git checkout -b reflog-apply start &&
-	old_head_reflog="$(git log -g --format=%gs -1 HEAD)" &&
-
-	git rebase --apply Y &&
+	test_cmp expect actual &&
 
-	git log -g --format=%gs -4 HEAD >actual &&
-	cat >expect <<-EOF &&
-	rebase finished: returning to refs/heads/reflog-apply
-	rebase: Z
-	rebase: checkout Y
-	$old_head_reflog
+	# check there is only one new entry in the branch reflog
+	test_cmp_rev conflicts@{1} Q
+	'
+
+	test_expect_success "rebase $mode fast-forward reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout fast-forward &&
+	test_when_finished "git reset --hard X" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		git rebase $mode main
+	) &&
+
+	git log -g --format=%gs -2 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward
+	${reflog_action:-rebase} (start): checkout main
 	EOF
 	test_cmp expect actual &&
 
-	git log -g --format=%gs -2 reflog-apply >actual &&
-	cat >expect <<-EOF &&
-	rebase finished: refs/heads/reflog-apply onto $(git rev-parse Y)
-	branch: Created from start
+	git log -g --format=%gs -1 fast-forward >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): refs/heads/fast-forward onto $(git rev-parse main)
 	EOF
-	test_cmp expect actual
-'
+	test_cmp expect actual &&
+
+	# check there is only one new entry in the branch reflog
+	test_cmp_rev fast-forward@{1} X
+	'
+}
+
+test_reflog --merge
+test_reflog --merge my-reflog-action
+test_reflog --apply
+test_reflog --apply my-reflog-action
 
 test_expect_success 'rebase -i onto unrelated history' '
 	git init unrelated &&
-- 
gitgitgadget
Christian Couder· Apr 7, 2022, 13:49 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH 2/7] rebase --merge: fix reflog when continuing

On Tue, Feb 22, 2022 at 6:12 AM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 17 quoted lines
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The reflog message for a conflict resolution committed by "rebase
> --continue" looks like
>
>         rebase (continue): commit subject line
>
> Unfortunately the reflog message each subsequent pick look like
>
>         rebase (continue) (pick): commit subject line
>
> Fix this by setting the reflog message for "rebase --continue" in
> sequencer_continue() so it does not affect subsequent commits. This
> introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION
> in pick_commits(). Both of these will be fixed in a future series that
> stops the sequencer calling setenv().
Yeah, it looks like we will leak only a small string.
Show 5 quoted lines
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c          |   2 -
>  sequencer.c               |   5 ++
>  t/t3406-rebase-message.sh | 120 +++++++++++++++++++++++++-------------

The changes to the test script look a bit involved and aren't explained in the commit message. I wonder if some of those changes could have been made in a preparatory commit.

>  3 files changed, 86 insertions(+), 41 deletions(-)
Phillip Wood· Apr 15, 2022, 14:00 UTC · re: Christian Couder · lore

Re: [PATCH 2/7] rebase --merge: fix reflog when continuing

Hi Chirstian
Thanks for taking a look at this series
On 07/04/2022 14:49, Christian Couder wrote:
Show 31 quoted lines
> On Tue, Feb 22, 2022 at 6:12 AM Phillip Wood via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> The reflog message for a conflict resolution committed by "rebase
>> --continue" looks like
>>
>>          rebase (continue): commit subject line
>>
>> Unfortunately the reflog message each subsequent pick look like
>>
>>          rebase (continue) (pick): commit subject line
>>
>> Fix this by setting the reflog message for "rebase --continue" in
>> sequencer_continue() so it does not affect subsequent commits. This
>> introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION
>> in pick_commits(). Both of these will be fixed in a future series that
>> stops the sequencer calling setenv().
> 
> Yeah, it looks like we will leak only a small string.
> 
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   builtin/rebase.c          |   2 -
>>   sequencer.c               |   5 ++
>>   t/t3406-rebase-message.sh | 120 +++++++++++++++++++++++++-------------
> 
> The changes to the test script look a bit involved and aren't
> explained in the commit message. I wonder if some of those changes
> could have been made in a preparatory commit.

That's a good point. For some reason when I put the series together I thought it would be tricky to do that without the fixes in this commit but that is not actually the case so I'll split the test changes out.

Best Wishes
Phillip
> 
>>   3 files changed, 86 insertions(+), 41 deletions(-)
Elijah Newren· Apr 17, 2022, 01:57 UTC · re: Phillip Wood · lore

Re: [PATCH 2/7] rebase --merge: fix reflog when continuing

On Fri, Apr 15, 2022 at 5:23 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
Show 41 quoted lines
>
> Hi Chirstian
>
> Thanks for taking a look at this series
>
> On 07/04/2022 14:49, Christian Couder wrote:
> > On Tue, Feb 22, 2022 at 6:12 AM Phillip Wood via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >>
> >> The reflog message for a conflict resolution committed by "rebase
> >> --continue" looks like
> >>
> >>          rebase (continue): commit subject line
> >>
> >> Unfortunately the reflog message each subsequent pick look like
> >>
> >>          rebase (continue) (pick): commit subject line
> >>
> >> Fix this by setting the reflog message for "rebase --continue" in
> >> sequencer_continue() so it does not affect subsequent commits. This
> >> introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION
> >> in pick_commits(). Both of these will be fixed in a future series that
> >> stops the sequencer calling setenv().
> >
> > Yeah, it looks like we will leak only a small string.
> >
> >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> >> ---
> >>   builtin/rebase.c          |   2 -
> >>   sequencer.c               |   5 ++
> >>   t/t3406-rebase-message.sh | 120 +++++++++++++++++++++++++-------------
> >
> > The changes to the test script look a bit involved and aren't
> > explained in the commit message. I wonder if some of those changes
> > could have been made in a preparatory commit.
>
> That's a good point. For some reason when I put the series together I
> thought it would be tricky to do that without the fixes in this commit
> but that is not actually the case so I'll split the test changes out.
That would be very nice; I'd like to see it split out too.
Phillip Wood via GitGitGadget· Feb 21, 2022, 11:10 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH 1/7] rebase --apply: remove duplicated code

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.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)
Show changes to builtin/rebase.c +1 −10
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;
 	}
-- 
gitgitgadget
Christian Couder· Apr 7, 2022, 13:35 UTC · re: Phillip Wood via GitGitGadget · lore

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

On Tue, Feb 22, 2022 at 6:34 AM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 6 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.
Nit: maybe "rebase --apply: reuse move_to_original_branch()" would be
a better subject for this patch, as it's more specific.
Show 6 quoted lines
> 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.
It looks like the removed call is using strbuf_reset(), not strbuf_release().

Actually a call to strbuf_release(&msg) is also removed inside the `if (oideq(&merge_base, &options.orig_head)) { ... }` clause, but it doesn't look like you are talking about this one.

Show 14 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);

Yeah, it looks like `msg` isn't used in the function anymore so this call can be removed.

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;
>         }
Nice cleanup!
Elijah Newren· Apr 17, 2022, 01:56 UTC · re: Christian Couder · lore

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

On Thu, Apr 7, 2022 at 1:30 PM Christian Couder <christian.couder@gmail.com> wrote:

Show 25 quoted lines
>
> On Tue, Feb 22, 2022 at 6:34 AM Phillip Wood via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > 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.
>
> Nit: maybe "rebase --apply: reuse move_to_original_branch()" would be
> a better subject for this patch, as it's more specific.
>
> > 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.
>
> It looks like the removed call is using strbuf_reset(), not strbuf_release().
>
> Actually a call to strbuf_release(&msg) is also removed inside the `if
> (oideq(&merge_base, &options.orig_head)) { ... }` clause, but it
> doesn't look like you are talking about this one.

I think he is talking about that one and specifying why it's okay to drop that one, and omitted why it was okay to drop the strbuf_reset() because that one seemed more obvious.

Show 36 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);
>
> Yeah, it looks like `msg` isn't used in the function anymore so this
> call can be removed.
>
> >         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;
> >         }
>
> Nice cleanup!
Agreed.
Phillip Wood via GitGitGadget· Feb 21, 2022, 11:10 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH 6/7] rebase --abort: improve reflog message

From: Phillip Wood <phillip.wood@dunelm.org.uk>
When aborting a rebase the reflog message looks like
	rebase (abort): updating HEAD

which is not very informative. Improve the message by mentioning the branch that we are returning to as we do at the end of a successful rebase so it looks like.

	rebase (abort): returning to refs/heads/topic

If GIT_REFLOG_ACTION is set in the environment we no longer omit "(abort)" from the reflog message. We don't omit "(start)" and "(finish)" when starting and finishing a rebase in that case so we shouldn't omit "(abort)".

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 28 ++++++---------------
 t/t3406-rebase-message.sh | 51 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 20 deletions(-)
Show changes to 2 files +59 −20

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 678339c7bf7..70426e17b40 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -992,23 +992,6 @@ static void NORETURN error_on_missing_default_upstream(void)
 	exit(1);
 }
 
-static void set_reflog_action(struct rebase_options *options)
-{
-	const char *env;
-	struct strbuf buf = STRBUF_INIT;
-
-	if (!is_merge(options))
-		return;
-
-	env = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
-	if (env && strcmp("rebase", env))
-		return; /* only override it if it is "rebase" */
-
-	strbuf_addf(&buf, "rebase (%s)", options->action);
-	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, buf.buf, 1);
-	strbuf_release(&buf);
-}
-
 static int check_exec_cmd(const char *cmd)
 {
 	if (strchr(cmd, '\n'))
@@ -1287,18 +1270,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 	case ACTION_ABORT: {
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
-		options.action = "abort";
-		set_reflog_action(&options);
+		struct strbuf head_msg = STRBUF_INIT;
 
+		options.action = "abort";
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 
 		if (read_basic_state(&options))
 			exit(1);
+
+		strbuf_addf(&head_msg, "%s (abort): returning to %s",
+			    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
+			    options.head_name ? options.head_name
+					      : oid_to_hex(&options.orig_head));
 		ropts.oid = &options.orig_head;
+		ropts.head_msg = head_msg.buf;
 		ropts.branch = options.head_name;
 		ropts.flags = RESET_HEAD_HARD;
-		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
 		if (reset_head(the_repository, &ropts) < 0)
 			die(_("could not move back to %s"),
 			    oid_to_hex(&options.orig_head));
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 5c6cd9af3bc..ceca1600053 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -187,6 +187,57 @@ test_reflog () {
 	EOF
 	test_cmp expect actual
 	'
+
+	test_expect_success "rebase $mode --abort reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout conflicts &&
+	test_when_finished "git reset --hard Q" &&
+
+	git log -g -1 conflicts >branch-expect &&
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		test_must_fail git rebase $mode main &&
+		git rebase --abort
+	) &&
+
+	git log -g --format=%gs -3 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (abort): returning to refs/heads/conflicts
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
+	EOF
+	test_cmp expect actual &&
+
+	# check branch reflog is unchanged
+	git log -g -1 conflicts >branch-actual &&
+	test_cmp branch-expect branch-actual
+	'
+
+	test_expect_success "rebase $mode --abort detached HEAD reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout Q &&
+	test_when_finished "git reset --hard Q" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		test_must_fail git rebase $mode main &&
+		git rebase --abort
+	) &&
+
+	git log -g --format=%gs -3 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (abort): returning to $(git rev-parse Q)
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
+	EOF
+	test_cmp expect actual
+	'
 }
 
 test_reflog --merge
-- 
gitgitgadget
Elijah Newren· Apr 17, 2022, 02:07 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH 6/7] rebase --abort: improve reflog message

On Mon, Feb 21, 2022 at 3:19 PM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 12 quoted lines
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When aborting a rebase the reflog message looks like
>
>         rebase (abort): updating HEAD
>
> which is not very informative. Improve the message by mentioning the
> branch that we are returning to as we do at the end of a successful
> rebase so it looks like.
>
>         rebase (abort): returning to refs/heads/topic
That is _so_ much nicer.
> If GIT_REFLOG_ACTION is set in the environment we no longer omit
> "(abort)" from the reflog message. We don't omit "(start)" and
> "(finish)" when starting and finishing a rebase in that case so we
> shouldn't omit "(abort)".
I agree.
Show 92 quoted lines
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c          | 28 ++++++---------------
>  t/t3406-rebase-message.sh | 51 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+), 20 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 678339c7bf7..70426e17b40 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -992,23 +992,6 @@ static void NORETURN error_on_missing_default_upstream(void)
>         exit(1);
>  }
>
> -static void set_reflog_action(struct rebase_options *options)
> -{
> -       const char *env;
> -       struct strbuf buf = STRBUF_INIT;
> -
> -       if (!is_merge(options))
> -               return;
> -
> -       env = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
> -       if (env && strcmp("rebase", env))
> -               return; /* only override it if it is "rebase" */
> -
> -       strbuf_addf(&buf, "rebase (%s)", options->action);
> -       setenv(GIT_REFLOG_ACTION_ENVIRONMENT, buf.buf, 1);
> -       strbuf_release(&buf);
> -}
> -
>  static int check_exec_cmd(const char *cmd)
>  {
>         if (strchr(cmd, '\n'))
> @@ -1287,18 +1270,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>         }
>         case ACTION_ABORT: {
>                 struct string_list merge_rr = STRING_LIST_INIT_DUP;
> -               options.action = "abort";
> -               set_reflog_action(&options);
> +               struct strbuf head_msg = STRBUF_INIT;
>
> +               options.action = "abort";
>                 rerere_clear(the_repository, &merge_rr);
>                 string_list_clear(&merge_rr, 1);
>
>                 if (read_basic_state(&options))
>                         exit(1);
> +
> +               strbuf_addf(&head_msg, "%s (abort): returning to %s",
> +                           getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
> +                           options.head_name ? options.head_name
> +                                             : oid_to_hex(&options.orig_head));
>                 ropts.oid = &options.orig_head;
> +               ropts.head_msg = head_msg.buf;
>                 ropts.branch = options.head_name;
>                 ropts.flags = RESET_HEAD_HARD;
> -               ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
>                 if (reset_head(the_repository, &ropts) < 0)
>                         die(_("could not move back to %s"),
>                             oid_to_hex(&options.orig_head));
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 5c6cd9af3bc..ceca1600053 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -187,6 +187,57 @@ test_reflog () {
>         EOF
>         test_cmp expect actual
>         '
> +
> +       test_expect_success "rebase $mode --abort reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
> +       git checkout conflicts &&
> +       test_when_finished "git reset --hard Q" &&
> +
> +       git log -g -1 conflicts >branch-expect &&
> +       (
> +               if test -n "$reflog_action"
> +               then
> +                       GIT_REFLOG_ACTION="$reflog_action" &&
> +                       export GIT_REFLOG_ACTION
> +               fi &&
> +               test_must_fail git rebase $mode main &&
> +               git rebase --abort
> +       ) &&
> +
> +       git log -g --format=%gs -3 >actual &&
> +       write_reflog_expect <<-EOF &&
> +       ${reflog_action:-rebase} (abort): returning to refs/heads/conflicts
> +       ${reflog_action:-rebase} (pick): P
> +       ${reflog_action:-rebase} (start): checkout main
> +       EOF
> +       test_cmp expect actual &&

I was confused by the ordering at first, before I realized that `git log -g` is going to give you reflogs from newest to oldest instead of vice-versa. Looks good.

Show 29 quoted lines
> +
> +       # check branch reflog is unchanged
> +       git log -g -1 conflicts >branch-actual &&
> +       test_cmp branch-expect branch-actual
> +       '
> +
> +       test_expect_success "rebase $mode --abort detached HEAD reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
> +       git checkout Q &&
> +       test_when_finished "git reset --hard Q" &&
> +
> +       (
> +               if test -n "$reflog_action"
> +               then
> +                       GIT_REFLOG_ACTION="$reflog_action" &&
> +                       export GIT_REFLOG_ACTION
> +               fi &&
> +               test_must_fail git rebase $mode main &&
> +               git rebase --abort
> +       ) &&
> +
> +       git log -g --format=%gs -3 >actual &&
> +       write_reflog_expect <<-EOF &&
> +       ${reflog_action:-rebase} (abort): returning to $(git rev-parse Q)
> +       ${reflog_action:-rebase} (pick): P
> +       ${reflog_action:-rebase} (start): checkout main
> +       EOF
> +       test_cmp expect actual
> +       '
>  }
And thanks for checking the detached HEAD case too.
Phillip Wood· Apr 4, 2022, 15:34 UTC · re: Phillip Wood via GitGitGadget · lore

Review Request (was Re: [PATCH 0/7] rebase: make reflog messages independent of the backend)

If anyone has time to review these patches I'd be very grateful. I've cc'd this message to some list regulars who have reviewed rebase patches before but if anyone else fancies taking a look at them that would be great.

Thanks
Phillip
On 21/02/2022 11:10, Phillip Wood via GitGitGadget wrote:
Show 30 quoted lines
> This is a series of rebase reflog related patches with the aim of unifying
> the reflog messages from the two rebase backends.
> 
>   * improve rebase reflog test coverage
>   * rebase --merge: fix reflog messages for --continue and --skip
>   * rebase --apply: respect GIT_REFLOG_ACTION
>   * rebase --abort: improve reflog message
>   * unify reflog messages between the two rebase backends
> 
> This series is based on pw/use-inprocess-checkout-in-rebase
> 
> Phillip Wood (7):
>    rebase --apply: remove duplicated code
>    rebase --merge: fix reflog when continuing
>    rebase --merge: fix reflog message after skipping
>    rebase --apply: respect GIT_REFLOG_ACTION
>    rebase --apply: make reflog messages match rebase --merge
>    rebase --abort: improve reflog message
>    rebase: cleanup action handling
> 
>   builtin/rebase.c          | 144 ++++++++++++-----------------
>   sequencer.c               |   5 ++
>   t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++-------
>   3 files changed, 214 insertions(+), 120 deletions(-)
> 
> 
> base-commit: 38c541ce94048cf72aa4f465be9314423a57f445
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1150
Elijah Newren· Apr 17, 2022, 02:13 UTC · re: Phillip Wood · lore

Re: Review Request (was Re: [PATCH 0/7] rebase: make reflog messages independent of the backend)

On Mon, Apr 4, 2022 at 8:34 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> If anyone has time to review these patches I'd be very grateful. I've
> cc'd this message to some list regulars who have reviewed rebase patches
> before but if anyone else fancies taking a look at them that would be great.

I apologize for the delay here...especially when it came after you had already waited a month and a half. (This is the first git thing I've done in over a month, sadly.) Anyway, I just read through the series and left some comments; mostly looks good but I second the request to split up one of the patches.

Show 31 quoted lines
> On 21/02/2022 11:10, Phillip Wood via GitGitGadget wrote:
> > This is a series of rebase reflog related patches with the aim of unifying
> > the reflog messages from the two rebase backends.
> >
> >   * improve rebase reflog test coverage
> >   * rebase --merge: fix reflog messages for --continue and --skip
> >   * rebase --apply: respect GIT_REFLOG_ACTION
> >   * rebase --abort: improve reflog message
> >   * unify reflog messages between the two rebase backends
> >
> > This series is based on pw/use-inprocess-checkout-in-rebase
> >
> > Phillip Wood (7):
> >    rebase --apply: remove duplicated code
> >    rebase --merge: fix reflog when continuing
> >    rebase --merge: fix reflog message after skipping
> >    rebase --apply: respect GIT_REFLOG_ACTION
> >    rebase --apply: make reflog messages match rebase --merge
> >    rebase --abort: improve reflog message
> >    rebase: cleanup action handling
> >
> >   builtin/rebase.c          | 144 ++++++++++++-----------------
> >   sequencer.c               |   5 ++
> >   t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++-------
> >   3 files changed, 214 insertions(+), 120 deletions(-)
> >
> >
> > base-commit: 38c541ce94048cf72aa4f465be9314423a57f445
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1150
Phillip Wood· Apr 18, 2022, 18:56 UTC · re: Elijah Newren · lore

Re: Review Request (was Re: [PATCH 0/7] rebase: make reflog messages independent of the backend)

Hi Elijah
On 17/04/2022 03:13, Elijah Newren wrote:
Show 11 quoted lines
> On Mon, Apr 4, 2022 at 8:34 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> If anyone has time to review these patches I'd be very grateful. I've
>> cc'd this message to some list regulars who have reviewed rebase patches
>> before but if anyone else fancies taking a look at them that would be great.
> 
> I apologize for the delay here...especially when it came after you had
> already waited a month and a half.  (This is the first git thing I've
> done in over a month, sadly.)  Anyway, I just read through the series
> and left some comments; mostly looks good but I second the request to
> split up one of the patches.

There's no need to apologize, I'm very grateful for you comments. I'll re-roll with that patch split up.

Thanks
Phillip
Show 32 quoted lines
> 
>> On 21/02/2022 11:10, Phillip Wood via GitGitGadget wrote:
>>> This is a series of rebase reflog related patches with the aim of unifying
>>> the reflog messages from the two rebase backends.
>>>
>>>    * improve rebase reflog test coverage
>>>    * rebase --merge: fix reflog messages for --continue and --skip
>>>    * rebase --apply: respect GIT_REFLOG_ACTION
>>>    * rebase --abort: improve reflog message
>>>    * unify reflog messages between the two rebase backends
>>>
>>> This series is based on pw/use-inprocess-checkout-in-rebase
>>>
>>> Phillip Wood (7):
>>>     rebase --apply: remove duplicated code
>>>     rebase --merge: fix reflog when continuing
>>>     rebase --merge: fix reflog message after skipping
>>>     rebase --apply: respect GIT_REFLOG_ACTION
>>>     rebase --apply: make reflog messages match rebase --merge
>>>     rebase --abort: improve reflog message
>>>     rebase: cleanup action handling
>>>
>>>    builtin/rebase.c          | 144 ++++++++++++-----------------
>>>    sequencer.c               |   5 ++
>>>    t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++-------
>>>    3 files changed, 214 insertions(+), 120 deletions(-)
>>>
>>>
>>> base-commit: 38c541ce94048cf72aa4f465be9314423a57f445
>>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v1
>>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v1
>>> Pull-Request: https://github.com/gitgitgadget/git/pull/1150
Christian Couder· Apr 7, 2022, 14:15 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH 0/7] rebase: make reflog messages independent of the backend

On Tue, Feb 22, 2022 at 6:04 AM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:

>
> This is a series of rebase reflog related patches with the aim of unifying
> the reflog messages from the two rebase backends.

I took a look but might have been lost or confused in the middle, so I didn't look much at the last patches. I think this has a worthwhile goal though.

Elijah Newren· Apr 17, 2022, 02:09 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH 0/7] rebase: make reflog messages independent of the backend

On Mon, Feb 21, 2022 at 9:04 PM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 31 quoted lines
>
> This is a series of rebase reflog related patches with the aim of unifying
> the reflog messages from the two rebase backends.
>
>  * improve rebase reflog test coverage
>  * rebase --merge: fix reflog messages for --continue and --skip
>  * rebase --apply: respect GIT_REFLOG_ACTION
>  * rebase --abort: improve reflog message
>  * unify reflog messages between the two rebase backends
>
> This series is based on pw/use-inprocess-checkout-in-rebase
>
> Phillip Wood (7):
>   rebase --apply: remove duplicated code
>   rebase --merge: fix reflog when continuing
>   rebase --merge: fix reflog message after skipping
>   rebase --apply: respect GIT_REFLOG_ACTION
>   rebase --apply: make reflog messages match rebase --merge
>   rebase --abort: improve reflog message
>   rebase: cleanup action handling
>
>  builtin/rebase.c          | 144 ++++++++++++-----------------
>  sequencer.c               |   5 ++
>  t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++-------
>  3 files changed, 214 insertions(+), 120 deletions(-)
>
>
> base-commit: 38c541ce94048cf72aa4f465be9314423a57f445
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1150

I read through the seven patches, then went back and commented on all of them. Hard to find problems really, looks nicely written. I would second Christian's request to have the second patch split with a preparatory testcase cleanup, which it appears you already plan to do. Other than patch, the series looks good to me.

Phillip Wood via GitGitGadget· Apr 20, 2022, 09:56 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v2 0/8] rebase: make reflog messages independent of the backend

Thanks to Christian and Elijah for their comments on V1.

I've updated commit message for patch 1 to try and be clearer about the removal of a call to strbuf_release() and spilt out the test changes from the old patch 2 into a separate preparatory patch.

V1 Cover Letter:

This is a series of rebase reflog related patches with the aim of unifying the reflog messages from the two rebase backends.

 * improve rebase reflog test coverage
 * rebase --merge: fix reflog messages for --continue and --skip
 * rebase --apply: respect GIT_REFLOG_ACTION
 * rebase --abort: improve reflog message
 * unify reflog messages between the two rebase backends
This series is based on pw/use-inprocess-checkout-in-rebase
Phillip Wood (8):
  rebase --apply: remove duplicated code
  t3406: rework rebase reflog tests
  rebase --merge: fix reflog when continuing
  rebase --merge: fix reflog message after skipping
  rebase --apply: respect GIT_REFLOG_ACTION
  rebase --apply: make reflog messages match rebase --merge
  rebase --abort: improve reflog message
  rebase: cleanup action handling
 builtin/rebase.c          | 144 ++++++++++++-----------------
 sequencer.c               |   5 ++
 t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++-------
 3 files changed, 214 insertions(+), 120 deletions(-)
base-commit: 38c541ce94048cf72aa4f465be9314423a57f445
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1150
Range-diff vs v1:
 1:  243e7b0c372 ! 1:  a4320f2fcf3 rebase --apply: remove duplicated code
     @@ Commit message
          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.
     +    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>
      
 2:  493254ffbb8 ! 2:  0904b50a377 rebase --merge: fix reflog when continuing
     @@ Metadata
      Author: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## Commit message ##
     -    rebase --merge: fix reflog when continuing
     +    t3406: rework rebase reflog tests
      
     -    The reflog message for a conflict resolution committed by "rebase
     -    --continue" looks like
     +    Refactor the tests in preparation for adding more tests in the next
     +    few commits. The reworked tests use the same function for testing both
     +    the "merge" and "apply" backends. The test coverage for the "apply"
     +    backend now includes setting GIT_REFLOG_ACTION.
      
     -            rebase (continue): commit subject line
     -
     -    Unfortunately the reflog message each subsequent pick look like
     -
     -            rebase (continue) (pick): commit subject line
     -
     -    Fix this by setting the reflog message for "rebase --continue" in
     -    sequencer_continue() so it does not affect subsequent commits. This
     -    introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION
     -    in pick_commits(). Both of these will be fixed in a future series that
     -    stops the sequencer calling setenv().
     +    Note that rebasing the "conflicts" branch does not create any
     +    conflicts yet. A commit to do that will be added in the next commit
     +    and the diff ends up smaller if we have don't rename the branch when
     +    it is added.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     - ## builtin/rebase.c ##
     -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
     - 		int fd;
     - 
     - 		options.action = "continue";
     --		set_reflog_action(&options);
     --
     - 		/* Sanity check */
     - 		if (get_oid("HEAD", &head))
     - 			die(_("Cannot read HEAD"));
     -
     - ## sequencer.c ##
     -@@ sequencer.c: int sequencer_continue(struct repository *r, struct replay_opts *opts)
     - 	if (read_populate_opts(opts))
     - 		return -1;
     - 	if (is_rebase_i(opts)) {
     -+		char *previous_reflog_action;
     -+
     - 		if ((res = read_populate_todo(r, &todo_list, opts)))
     - 			goto release_todo_list;
     - 
     -@@ sequencer.c: int sequencer_continue(struct repository *r, struct replay_opts *opts)
     - 			unlink(rebase_path_dropped());
     - 		}
     - 
     -+		previous_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
     -+		setenv(GIT_REFLOG_ACTION, reflog_message(opts, "continue", NULL), 1);
     - 		if (commit_staged_changes(r, opts, &todo_list)) {
     - 			res = -1;
     - 			goto release_todo_list;
     - 		}
     -+		setenv(GIT_REFLOG_ACTION, previous_reflog_action, 1);
     - 	} else if (!file_exists(get_todo_path(opts)))
     - 		return continue_single_pick(r, opts);
     - 	else if ((res = read_populate_todo(r, &todo_list, opts)))
     -
       ## t/t3406-rebase-message.sh ##
      @@ t/t3406-rebase-message.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
       test_expect_success 'setup' '
     @@ t/t3406-rebase-message.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
       
      +	git checkout -b conflicts O &&
      +	test_commit P &&
     -+	test_commit conflict-X fileX &&
      +	test_commit Q &&
      +
       	git checkout -b topic O &&
     @@ t/t3406-rebase-message.sh: test_expect_success 'error out early upon -C<n> or --
      +			GIT_REFLOG_ACTION="$reflog_action" &&
      +			export GIT_REFLOG_ACTION
      +		fi &&
     -+		test_must_fail git rebase $mode main &&
     -+		echo resolved >fileX &&
     -+		git add fileX &&
     -+		git rebase --continue
     ++		git rebase $mode main
      +	) &&
      +
     -+	git log -g --format=%gs -5 >actual &&
     ++	git log -g --format=%gs -4 >actual &&
      +	write_reflog_expect <<-EOF &&
      +	${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
      +	${reflog_action:-rebase} (pick): Q
     -+	${reflog_action:-rebase} (continue): conflict-X
      +	${reflog_action:-rebase} (pick): P
      +	${reflog_action:-rebase} (start): checkout main
       	EOF
 -:  ----------- > 3:  6c15f00e170 rebase --merge: fix reflog when continuing
 3:  f4e6e94bc2c = 4:  d3afa85ffc5 rebase --merge: fix reflog message after skipping
 4:  58c560d0e19 = 5:  afa67abe01a rebase --apply: respect GIT_REFLOG_ACTION
 5:  1c3ec165422 = 6:  95161f21e00 rebase --apply: make reflog messages match rebase --merge
 6:  c863cebbdc8 = 7:  d2c1dfbcd5e rebase --abort: improve reflog message
 7:  d26a221a79d = 8:  b0d21affa78 rebase: cleanup action handling
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Apr 20, 2022, 09:56 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v2 2/8] t3406: rework rebase reflog tests

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Refactor the tests in preparation for adding more tests in the next few commits. The reworked tests use the same function for testing both the "merge" and "apply" backends. The test coverage for the "apply" backend now includes setting GIT_REFLOG_ACTION.

Note that rebasing the "conflicts" branch does not create any conflicts yet. A commit to do that will be added in the next commit and the diff ends up smaller if we have don't rename the branch when it is added.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3406-rebase-message.sh | 115 +++++++++++++++++++++++++-------------
 1 file changed, 76 insertions(+), 39 deletions(-)
Show changes to t/t3406-rebase-message.sh +76 −39
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index d17b450e811..5253dd1551d 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -10,10 +10,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 test_expect_success 'setup' '
 	test_commit O fileO &&
 	test_commit X fileX &&
+	git branch fast-forward &&
 	test_commit A fileA &&
 	test_commit B fileB &&
 	test_commit Y fileY &&
 
+	git checkout -b conflicts O &&
+	test_commit P &&
+	test_commit Q &&
+
 	git checkout -b topic O &&
 	git cherry-pick A B &&
 	test_commit Z fileZ &&
@@ -79,54 +84,86 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
 	test_i18ngrep "Invalid whitespace option" err
 '
 
-test_expect_success 'GIT_REFLOG_ACTION' '
-	git checkout start &&
-	test_commit reflog-onto &&
-	git checkout -b reflog-topic start &&
-	test_commit reflog-to-rebase &&
-
-	git rebase reflog-onto &&
-	git log -g --format=%gs -3 >actual &&
-	cat >expect <<-\EOF &&
-	rebase (finish): returning to refs/heads/reflog-topic
-	rebase (pick): reflog-to-rebase
-	rebase (start): checkout reflog-onto
+write_reflog_expect () {
+	if test $mode = --apply
+	then
+		sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//'
+	else
+		cat
+	fi >expect
+}
+
+test_reflog () {
+	mode=$1
+	reflog_action="$2"
+
+	test_expect_success "rebase $mode reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout conflicts &&
+	test_when_finished "git reset --hard Q" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		git rebase $mode main
+	) &&
+
+	git log -g --format=%gs -4 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
+	${reflog_action:-rebase} (pick): Q
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
 	EOF
 	test_cmp expect actual &&
 
-	git checkout -b reflog-prefix reflog-to-rebase &&
-	GIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto &&
-	git log -g --format=%gs -3 >actual &&
-	cat >expect <<-\EOF &&
-	change-the-reflog (finish): returning to refs/heads/reflog-prefix
-	change-the-reflog (pick): reflog-to-rebase
-	change-the-reflog (start): checkout reflog-onto
+	git log -g --format=%gs -1 conflicts >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): refs/heads/conflicts onto $(git rev-parse main)
 	EOF
-	test_cmp expect actual
-'
-
-test_expect_success 'rebase --apply reflog' '
-	git checkout -b reflog-apply start &&
-	old_head_reflog="$(git log -g --format=%gs -1 HEAD)" &&
-
-	git rebase --apply Y &&
+	test_cmp expect actual &&
 
-	git log -g --format=%gs -4 HEAD >actual &&
-	cat >expect <<-EOF &&
-	rebase finished: returning to refs/heads/reflog-apply
-	rebase: Z
-	rebase: checkout Y
-	$old_head_reflog
+	# check there is only one new entry in the branch reflog
+	test_cmp_rev conflicts@{1} Q
+	'
+
+	test_expect_success "rebase $mode fast-forward reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout fast-forward &&
+	test_when_finished "git reset --hard X" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		git rebase $mode main
+	) &&
+
+	git log -g --format=%gs -2 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward
+	${reflog_action:-rebase} (start): checkout main
 	EOF
 	test_cmp expect actual &&
 
-	git log -g --format=%gs -2 reflog-apply >actual &&
-	cat >expect <<-EOF &&
-	rebase finished: refs/heads/reflog-apply onto $(git rev-parse Y)
-	branch: Created from start
+	git log -g --format=%gs -1 fast-forward >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): refs/heads/fast-forward onto $(git rev-parse main)
 	EOF
-	test_cmp expect actual
-'
+	test_cmp expect actual &&
+
+	# check there is only one new entry in the branch reflog
+	test_cmp_rev fast-forward@{1} X
+	'
+}
+
+test_reflog --merge
+test_reflog --merge my-reflog-action
+test_reflog --apply
+test_reflog --apply my-reflog-action
 
 test_expect_success 'rebase -i onto unrelated history' '
 	git init unrelated &&
-- 
gitgitgadget
Junio C Hamano· Apr 20, 2022, 20:17 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH v2 2/8] t3406: rework rebase reflog tests

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 11 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Refactor the tests in preparation for adding more tests in the next
> few commits. The reworked tests use the same function for testing both
> the "merge" and "apply" backends. The test coverage for the "apply"
> backend now includes setting GIT_REFLOG_ACTION.
>
> Note that rebasing the "conflicts" branch does not create any
> conflicts yet. A commit to do that will be added in the next commit
> and the diff ends up smaller if we have don't rename the branch when
> it is added.
"if we have don't rename"?  Ecannotparse.
Show 26 quoted lines
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  t/t3406-rebase-message.sh | 115 +++++++++++++++++++++++++-------------
>  1 file changed, 76 insertions(+), 39 deletions(-)
>
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index d17b450e811..5253dd1551d 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -10,10 +10,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>  test_expect_success 'setup' '
>  	test_commit O fileO &&
>  	test_commit X fileX &&
> +	git branch fast-forward &&
>  	test_commit A fileA &&
>  	test_commit B fileB &&
>  	test_commit Y fileY &&
>  
> +	git checkout -b conflicts O &&
> +	test_commit P &&
> +	test_commit Q &&
> +
>  	git checkout -b topic O &&
>  	git cherry-pick A B &&
>  	test_commit Z fileZ &&
So, we'd create
	O---X---A---B---Y (main)
        |\
        | P---Q (conflicts)
        \
         A'--B'--Z (topic)
with the above.  The "fast-forward" branch is not yet used.

Without reading the rest of this patch (or the series), especially because you said you'll test multiple rebase strategies (and presumably make sure they result in the same state), my gut says that it may not buy us much to create such an unused branch. If we successfully run a fast-forward test on the branch for one strategy, we have to rewind the result before testing another strategy anyway, so forking a branch to be tested at X when we start testing each rebase strategy would be fine. But we'll see.

Show 28 quoted lines
> @@ -79,54 +84,86 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
>  	test_i18ngrep "Invalid whitespace option" err
>  '
>  
> -test_expect_success 'GIT_REFLOG_ACTION' '
> -	git checkout start &&
> -	test_commit reflog-onto &&
> -	git checkout -b reflog-topic start &&
> -	test_commit reflog-to-rebase &&
> -
> -	git rebase reflog-onto &&
> -	git log -g --format=%gs -3 >actual &&
> -	cat >expect <<-\EOF &&
> -	rebase (finish): returning to refs/heads/reflog-topic
> -	rebase (pick): reflog-to-rebase
> -	rebase (start): checkout reflog-onto
> +write_reflog_expect () {
> +	if test $mode = --apply
> +	then
> +		sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//'
> +	else
> +		cat
> +	fi >expect
> +}
> +
> +test_reflog () {
> +	mode=$1
> +	reflog_action="$2"

$1 being left unquoted while $2 being quoted makes them look incoherent. It is not wrong to enclose $2 in double quotes, but it is not necessary.

Show 12 quoted lines
> +	test_expect_success "rebase $mode reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
> +	git checkout conflicts &&
> +	test_when_finished "git reset --hard Q" &&
> +
> +	(
> +		if test -n "$reflog_action"
> +		then
> +			GIT_REFLOG_ACTION="$reflog_action" &&
> +			export GIT_REFLOG_ACTION
> +		fi &&
> +		git rebase $mode main
> +	) &&

Is the subshell because you may want to export the envirohnment, or is there anything more subtle going on?

Show 8 quoted lines
> +	git log -g --format=%gs -4 >actual &&
> +	write_reflog_expect <<-EOF &&
> +	${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
> +	${reflog_action:-rebase} (pick): Q
> +	${reflog_action:-rebase} (pick): P
> +	${reflog_action:-rebase} (start): checkout main
>  	EOF
>  	test_cmp expect actual &&

We reset to Q and rebase it onto main, which would mean that we'd replay P and then Q on top of Y. The 4 most recent events on HEAD are that we start at 'main', picking P and Q and then updating the conflicts branch with the result. OK.

It is not what we used to test, but with reference to Q that was established at the beginning, though.

Show 11 quoted lines
> -	git checkout -b reflog-prefix reflog-to-rebase &&
> -	GIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto &&
> -	git log -g --format=%gs -3 >actual &&
> -	cat >expect <<-\EOF &&
> -	change-the-reflog (finish): returning to refs/heads/reflog-prefix
> -	change-the-reflog (pick): reflog-to-rebase
> -	change-the-reflog (start): checkout reflog-onto
> +	git log -g --format=%gs -1 conflicts >actual &&
> +	write_reflog_expect <<-EOF &&
> +	${reflog_action:-rebase} (finish): refs/heads/conflicts onto $(git rev-parse main)
>  	EOF

And after doing that, there only is one reflog entry for the conflicts branch itself, i.e. record of finishing to rebase.

For both of the above two tests, I wonder if we want to make sure these four and one entries are the ONLY entries added by the operation. I.e. take the topmost reflog entry _before_ starting to run rebase for both conflicts branch and HEAD, grab five and two reflog entries after the operation from conflicts and HEAD and make sure the four and one are the truly new entries added by the operation, or something like that. Even easier would be to merely count the number of reflog entries of HEAD and conflicts before rebasing, and count them after. We can do subtraction in the shell just fine.

> +	test_cmp expect actual &&
>  
> +	# check there is only one new entry in the branch reflog
It is a valid thing to try verifying, but ...
> +	test_cmp_rev conflicts@{1} Q

... it is unclear how this can count the new reflog entries. If we added two reflog entries, one pointing at Y (i.e. tip of main), another that points at Q (i.e. tip of conflicts before the rebasing), conflicts@{1} would be the same as Q.

Show 21 quoted lines
> +	'
> +
> +	test_expect_success "rebase $mode fast-forward reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
> +	git checkout fast-forward &&
> +	test_when_finished "git reset --hard X" &&
> +
> +	(
> +		if test -n "$reflog_action"
> +		then
> +			GIT_REFLOG_ACTION="$reflog_action" &&
> +			export GIT_REFLOG_ACTION
> +		fi &&
> +		git rebase $mode main
> +	) &&
> +
> +	git log -g --format=%gs -2 >actual &&
> +	write_reflog_expect <<-EOF &&
> +	${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward
> +	${reflog_action:-rebase} (start): checkout main
>  	EOF
>  	test_cmp expect actual &&

Likewise. This only makes sure that the topmost two are what you expect without checking if it added anything extra before these two. Actually counting the entries would give you an easy fix.

Show 14 quoted lines
> -	git log -g --format=%gs -2 reflog-apply >actual &&
> -	cat >expect <<-EOF &&
> -	rebase finished: refs/heads/reflog-apply onto $(git rev-parse Y)
> -	branch: Created from start
> +	git log -g --format=%gs -1 fast-forward >actual &&
> +	write_reflog_expect <<-EOF &&
> +	${reflog_action:-rebase} (finish): refs/heads/fast-forward onto $(git rev-parse main)
>  	EOF
> -	test_cmp expect actual
> -'
> +	test_cmp expect actual &&
> +
> +	# check there is only one new entry in the branch reflog
> +	test_cmp_rev fast-forward@{1} X

The same. This does not ensure there is only one new entry. It merely says that the set of entries you cared about (which is a set of one) is preceded by an entry that happened to point at X.

Show 10 quoted lines
> +	'
> +}
> +
> +test_reflog --merge
> +test_reflog --merge my-reflog-action
> +test_reflog --apply
> +test_reflog --apply my-reflog-action
>  
>  test_expect_success 'rebase -i onto unrelated history' '
>  	git init unrelated &&
Thanks.
Phillip Wood via GitGitGadget· Apr 20, 2022, 09:56 UTC · re: Phillip Wood via GitGitGadget · lore

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

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.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)
Show changes to builtin/rebase.c +1 −10
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;
 	}
-- 
gitgitgadget
Ævar Arnfjörð Bjarmason· Apr 20, 2022, 10:10 UTC · re: Phillip Wood via GitGitGadget · lore

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

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;
>  	}
Junio C Hamano· Apr 20, 2022, 18:25 UTC · re: Phillip Wood via GitGitGadget · lore

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

"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;
>  	}
Junio C Hamano· Apr 20, 2022, 18:39 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> 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".

Phillip Wood via GitGitGadget· Apr 20, 2022, 09:56 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v2 3/8] rebase --merge: fix reflog when continuing

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The reflog message for a conflict resolution committed by "rebase --continue" looks like

	rebase (continue): commit subject line
Unfortunately the reflog message each subsequent pick look like
	rebase (continue) (pick): commit subject line

Fix this by setting the reflog message for "rebase --continue" in sequencer_continue() so it does not affect subsequent commits. This introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION in pick_commits(). Both of these will be fixed in a future series that stops the sequencer calling setenv().

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 2 --
 sequencer.c               | 5 +++++
 t/t3406-rebase-message.sh | 9 +++++++--
 3 files changed, 12 insertions(+), 4 deletions(-)
Show changes to 3 files +12 −4

builtin/rebase.c, sequencer.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4832f16e675..cd9a4f3e2f1 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1247,8 +1247,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		int fd;
 
 		options.action = "continue";
-		set_reflog_action(&options);
-
 		/* Sanity check */
 		if (get_oid("HEAD", &head))
 			die(_("Cannot read HEAD"));
diff --git a/sequencer.c b/sequencer.c
index bdd66b4b67a..3634ad5baa9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4777,6 +4777,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 	if (read_populate_opts(opts))
 		return -1;
 	if (is_rebase_i(opts)) {
+		char *previous_reflog_action;
+
 		if ((res = read_populate_todo(r, &todo_list, opts)))
 			goto release_todo_list;
 
@@ -4787,10 +4789,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 			unlink(rebase_path_dropped());
 		}
 
+		previous_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
+		setenv(GIT_REFLOG_ACTION, reflog_message(opts, "continue", NULL), 1);
 		if (commit_staged_changes(r, opts, &todo_list)) {
 			res = -1;
 			goto release_todo_list;
 		}
+		setenv(GIT_REFLOG_ACTION, previous_reflog_action, 1);
 	} else if (!file_exists(get_todo_path(opts)))
 		return continue_single_pick(r, opts);
 	else if ((res = read_populate_todo(r, &todo_list, opts)))
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 5253dd1551d..3ca2fbb0d59 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -17,6 +17,7 @@ test_expect_success 'setup' '
 
 	git checkout -b conflicts O &&
 	test_commit P &&
+	test_commit conflict-X fileX &&
 	test_commit Q &&
 
 	git checkout -b topic O &&
@@ -107,13 +108,17 @@ test_reflog () {
 			GIT_REFLOG_ACTION="$reflog_action" &&
 			export GIT_REFLOG_ACTION
 		fi &&
-		git rebase $mode main
+		test_must_fail git rebase $mode main &&
+		echo resolved >fileX &&
+		git add fileX &&
+		git rebase --continue
 	) &&
 
-	git log -g --format=%gs -4 >actual &&
+	git log -g --format=%gs -5 >actual &&
 	write_reflog_expect <<-EOF &&
 	${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
 	${reflog_action:-rebase} (pick): Q
+	${reflog_action:-rebase} (continue): conflict-X
 	${reflog_action:-rebase} (pick): P
 	${reflog_action:-rebase} (start): checkout main
 	EOF
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Apr 20, 2022, 09:56 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v2 4/8] rebase --merge: fix reflog message after skipping

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The reflog message for every pick after running "rebase --skip" looks like

	rebase (skip) (pick): commit subject line
Fix this by not appending " (skip)" to the reflog action.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          |  2 --
 t/t3406-rebase-message.sh | 24 ++++++++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)
Show changes to 2 files +24 −2

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index cd9a4f3e2f1..36863117fba 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1273,8 +1273,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
 		options.action = "skip";
-		set_reflog_action(&options);
-
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 		ropts.flags = RESET_HEAD_HARD;
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 3ca2fbb0d59..8aa6a79acc1 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -163,6 +163,30 @@ test_reflog () {
 	# check there is only one new entry in the branch reflog
 	test_cmp_rev fast-forward@{1} X
 	'
+
+	test_expect_success "rebase $mode --skip reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout conflicts &&
+	test_when_finished "git reset --hard Q" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		test_must_fail git rebase $mode main &&
+		git rebase --skip
+	) &&
+
+	git log -g --format=%gs -4 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
+	${reflog_action:-rebase} (pick): Q
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
+	EOF
+	test_cmp expect actual
+	'
 }
 
 test_reflog --merge
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Apr 20, 2022, 09:56 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v2 5/8] rebase --apply: respect GIT_REFLOG_ACTION

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The reflog messages when finishing a rebase hard code "rebase" rather than using GIT_REFLOG_ACTION.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 7 ++++---
 t/t3406-rebase-message.sh | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)
Show changes to 2 files +5 −4

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 36863117fba..e50361fc2a9 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -580,10 +580,11 @@ static int move_to_original_branch(struct rebase_options *opts)
 	if (!opts->onto)
 		BUG("move_to_original_branch without onto");
 
-	strbuf_addf(&branch_reflog, "rebase finished: %s onto %s",
+	strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
+		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
-	strbuf_addf(&head_reflog, "rebase finished: returning to %s",
-		    opts->head_name);
+	strbuf_addf(&head_reflog, "%s finished: returning to %s",
+		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
 	ropts.branch = opts->head_name;
 	ropts.flags = RESET_HEAD_REFS_ONLY;
 	ropts.branch_msg = branch_reflog.buf;
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 8aa6a79acc1..bb2a4949abc 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
 write_reflog_expect () {
 	if test $mode = --apply
 	then
-		sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//'
+		sed 's/(finish)/finished/; s/ ([^)]*)//'
 	else
 		cat
 	fi >expect
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Apr 20, 2022, 09:56 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v2 6/8] rebase --apply: make reflog messages match rebase --merge

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The apply backend creates slightly different reflog messages to the merge backend when starting and finishing a rebase and when picking commits. The choice of backend is really an implementation detail so it is confusing to have the same command create different messages depending on which backend is selected. Change the apply backend so the reflog messages from the two backends match as closely as possible. Note that there is still a difference when committing a conflict resolution - the merge backend will use "(continue)" rather than "(pick)" in that case as it does not know which command created the conflict that it is committing.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 9 +++++----
 t/t3406-rebase-message.sh | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)
Show changes to 2 files +6 −5

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e50361fc2a9..678339c7bf7 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -580,10 +580,10 @@ static int move_to_original_branch(struct rebase_options *opts)
 	if (!opts->onto)
 		BUG("move_to_original_branch without onto");
 
-	strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
+	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
-	strbuf_addf(&head_reflog, "%s finished: returning to %s",
+	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
 	ropts.branch = opts->head_name;
 	ropts.flags = RESET_HEAD_REFS_ONLY;
@@ -613,7 +613,8 @@ static int run_am(struct rebase_options *opts)
 
 	am.git_cmd = 1;
 	strvec_push(&am.args, "am");
-
+	strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
+		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
 	if (opts->action && !strcmp("continue", opts->action)) {
 		strvec_push(&am.args, "--resolved");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
@@ -1763,7 +1764,7 @@ 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",
+	strbuf_addf(&msg, "%s (start): checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
 	ropts.oid = &options.onto->object.oid;
 	ropts.orig_head = &options.orig_head,
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index bb2a4949abc..5c6cd9af3bc 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
 write_reflog_expect () {
 	if test $mode = --apply
 	then
-		sed 's/(finish)/finished/; s/ ([^)]*)//'
+		sed 's/(continue)/(pick)/'
 	else
 		cat
 	fi >expect
-- 
gitgitgadget
Ævar Arnfjörð Bjarmason· Apr 20, 2022, 10:22 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH v2 6/8] rebase --apply: make reflog messages match rebase --merge

On Wed, Apr 20 2022, Phillip Wood via GitGitGadget wrote:
Show 68 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The apply backend creates slightly different reflog messages to the
> merge backend when starting and finishing a rebase and when picking
> commits. The choice of backend is really an implementation detail so
> it is confusing to have the same command create different messages
> depending on which backend is selected. Change the apply backend so
> the reflog messages from the two backends match as closely as
> possible. Note that there is still a difference when committing a
> conflict resolution - the merge backend will use "(continue)" rather
> than "(pick)" in that case as it does not know which command created
> the conflict that it is committing.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c          | 9 +++++----
>  t/t3406-rebase-message.sh | 2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index e50361fc2a9..678339c7bf7 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -580,10 +580,10 @@ static int move_to_original_branch(struct rebase_options *opts)
>  	if (!opts->onto)
>  		BUG("move_to_original_branch without onto");
>  
> -	strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
> +	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
>  		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
>  		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
> -	strbuf_addf(&head_reflog, "%s finished: returning to %s",
> +	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
>  		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
>  	ropts.branch = opts->head_name;
>  	ropts.flags = RESET_HEAD_REFS_ONLY;
> @@ -613,7 +613,8 @@ static int run_am(struct rebase_options *opts)
>  
>  	am.git_cmd = 1;
>  	strvec_push(&am.args, "am");
> -
> +	strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
> +		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
>  	if (opts->action && !strcmp("continue", opts->action)) {
>  		strvec_push(&am.args, "--resolved");
>  		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
> @@ -1763,7 +1764,7 @@ 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",
> +	strbuf_addf(&msg, "%s (start): checkout %s",
>  		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
>  	ropts.oid = &options.onto->object.oid;
>  	ropts.orig_head = &options.orig_head,
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index bb2a4949abc..5c6cd9af3bc 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
>  write_reflog_expect () {
>  	if test $mode = --apply
>  	then
> -		sed 's/(finish)/finished/; s/ ([^)]*)//'
> +		sed 's/(continue)/(pick)/'
>  	else
>  		cat
>  	fi >expect

I like the new message better, but I wonder given that the old message has been with us since 6fd2f5e60d4 (rebase: operate on a detached HEAD, 2007-11-08) whether this will break anything in the wild.

Junio C Hamano· Apr 20, 2022, 22:15 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH v2 6/8] rebase --apply: make reflog messages match rebase --merge

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 9 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The apply backend creates slightly different reflog messages to the
> merge backend when starting and finishing a rebase and when picking
> commits. The choice of backend is really an implementation detail so
> it is confusing to have the same command create different messages
> depending on which backend is selected. Change the apply backend so
> the reflog messages from the two backends match as closely as
> possible.
I do not agree with that line of thought.

When choosing backend, the user clearly cares which one is used to do the job, so it is far from an "implementation detail" that is better hidden from the users. In other words, the message being different is not source of confusion in this case. Producing different messages depending on the version of Git may create worse confusion, though.

> Note that there is still a difference when committing a
> conflict resolution - the merge backend will use "(continue)" rather
> than "(pick)" in that case as it does not know which command created
> the conflict that it is committing.

That is OK, unless we can teach the backend to produce a more proper one (if 'continue' is considered less proper, that is, than 'pick'; I do not think I would personally care between the two words too much).

Show 9 quoted lines
> -	strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
> +	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
>  		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
>  		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
> -	strbuf_addf(&head_reflog, "%s finished: returning to %s",
> +	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
>  		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
>  	ropts.branch = opts->head_name;
>  	ropts.flags = RESET_HEAD_REFS_ONLY;

These convey the same information so it is "Meh---I do not care too strongly about them" kind of change, unless the justification is "let's show the (word) in parehtheses consistently in these reflog entries", in which case they are welcome change.

Overall, I'd rather see us not to apply this patch at this point in time, especially ...

> +	strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",

... this new use of .env_array member semantically conflicts with the removal of .env member from the structure being proposed in another topic in flight.

Show 11 quoted lines
> +		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
>  	if (opts->action && !strcmp("continue", opts->action)) {
>  		strvec_push(&am.args, "--resolved");
>  		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
> @@ -1763,7 +1764,7 @@ 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",
> +	strbuf_addf(&msg, "%s (start): checkout %s",
>  		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);

I think this one is a strict improvement with or without () around the word that tells us what phase of the rebase operation we are at, and if it happens to match what the other backend produces, that would be a tolerable small regression, even though it makes it hard to tell them apart.

Phillip Wood via GitGitGadget· Apr 20, 2022, 09:56 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v2 7/8] rebase --abort: improve reflog message

From: Phillip Wood <phillip.wood@dunelm.org.uk>
When aborting a rebase the reflog message looks like
	rebase (abort): updating HEAD

which is not very informative. Improve the message by mentioning the branch that we are returning to as we do at the end of a successful rebase so it looks like.

	rebase (abort): returning to refs/heads/topic

If GIT_REFLOG_ACTION is set in the environment we no longer omit "(abort)" from the reflog message. We don't omit "(start)" and "(finish)" when starting and finishing a rebase in that case so we shouldn't omit "(abort)".

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 28 ++++++---------------
 t/t3406-rebase-message.sh | 51 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 20 deletions(-)
Show changes to 2 files +59 −20

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 678339c7bf7..70426e17b40 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -992,23 +992,6 @@ static void NORETURN error_on_missing_default_upstream(void)
 	exit(1);
 }
 
-static void set_reflog_action(struct rebase_options *options)
-{
-	const char *env;
-	struct strbuf buf = STRBUF_INIT;
-
-	if (!is_merge(options))
-		return;
-
-	env = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
-	if (env && strcmp("rebase", env))
-		return; /* only override it if it is "rebase" */
-
-	strbuf_addf(&buf, "rebase (%s)", options->action);
-	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, buf.buf, 1);
-	strbuf_release(&buf);
-}
-
 static int check_exec_cmd(const char *cmd)
 {
 	if (strchr(cmd, '\n'))
@@ -1287,18 +1270,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 	case ACTION_ABORT: {
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
-		options.action = "abort";
-		set_reflog_action(&options);
+		struct strbuf head_msg = STRBUF_INIT;
 
+		options.action = "abort";
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 
 		if (read_basic_state(&options))
 			exit(1);
+
+		strbuf_addf(&head_msg, "%s (abort): returning to %s",
+			    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
+			    options.head_name ? options.head_name
+					      : oid_to_hex(&options.orig_head));
 		ropts.oid = &options.orig_head;
+		ropts.head_msg = head_msg.buf;
 		ropts.branch = options.head_name;
 		ropts.flags = RESET_HEAD_HARD;
-		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
 		if (reset_head(the_repository, &ropts) < 0)
 			die(_("could not move back to %s"),
 			    oid_to_hex(&options.orig_head));
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 5c6cd9af3bc..ceca1600053 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -187,6 +187,57 @@ test_reflog () {
 	EOF
 	test_cmp expect actual
 	'
+
+	test_expect_success "rebase $mode --abort reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout conflicts &&
+	test_when_finished "git reset --hard Q" &&
+
+	git log -g -1 conflicts >branch-expect &&
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		test_must_fail git rebase $mode main &&
+		git rebase --abort
+	) &&
+
+	git log -g --format=%gs -3 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (abort): returning to refs/heads/conflicts
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
+	EOF
+	test_cmp expect actual &&
+
+	# check branch reflog is unchanged
+	git log -g -1 conflicts >branch-actual &&
+	test_cmp branch-expect branch-actual
+	'
+
+	test_expect_success "rebase $mode --abort detached HEAD reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout Q &&
+	test_when_finished "git reset --hard Q" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		test_must_fail git rebase $mode main &&
+		git rebase --abort
+	) &&
+
+	git log -g --format=%gs -3 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (abort): returning to $(git rev-parse Q)
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
+	EOF
+	test_cmp expect actual
+	'
 }
 
 test_reflog --merge
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Apr 20, 2022, 09:56 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v2 8/8] rebase: cleanup action handling

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Treating the action as a string is a hang over from the scripted rebase. The last commit removed the only remaining use of the action that required a string so lets convert the other action users to use the existing action enum instead. If we ever need the action name as a string in the future the action_names array exists exactly for that purpose.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 91 +++++++++++++++++++++++-------------------------
 1 file changed, 43 insertions(+), 48 deletions(-)
Show changes to builtin/rebase.c +43 −48
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 70426e17b40..323f5154092 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -58,6 +58,26 @@ enum empty_type {
 	EMPTY_ASK
 };
 
+enum action {
+	ACTION_NONE = 0,
+	ACTION_CONTINUE,
+	ACTION_SKIP,
+	ACTION_ABORT,
+	ACTION_QUIT,
+	ACTION_EDIT_TODO,
+	ACTION_SHOW_CURRENT_PATCH
+};
+
+static const char *action_names[] = {
+	"undefined",
+	"continue",
+	"skip",
+	"abort",
+	"quit",
+	"edit_todo",
+	"show_current_patch"
+};
+
 struct rebase_options {
 	enum rebase_type type;
 	enum empty_type empty;
@@ -84,7 +104,7 @@ struct rebase_options {
 		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
 	} flags;
 	struct strvec git_am_opts;
-	const char *action;
+	enum action action;
 	int signoff;
 	int allow_rerere_autoupdate;
 	int keep_empty;
@@ -155,24 +175,6 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	return replay;
 }
 
-enum action {
-	ACTION_NONE = 0,
-	ACTION_CONTINUE,
-	ACTION_SKIP,
-	ACTION_ABORT,
-	ACTION_QUIT,
-	ACTION_EDIT_TODO,
-	ACTION_SHOW_CURRENT_PATCH
-};
-
-static const char *action_names[] = { "undefined",
-				      "continue",
-				      "skip",
-				      "abort",
-				      "quit",
-				      "edit_todo",
-				      "show_current_patch" };
-
 static int edit_todo_file(unsigned flags)
 {
 	const char *todo_file = rebase_path_todo();
@@ -309,8 +311,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	return ret;
 }
 
-static int run_sequencer_rebase(struct rebase_options *opts,
-				  enum action command)
+static int run_sequencer_rebase(struct rebase_options *opts)
 {
 	unsigned flags = 0;
 	int abbreviate_commands = 0, ret = 0;
@@ -325,7 +326,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 	flags |= opts->reapply_cherry_picks ? TODO_LIST_REAPPLY_CHERRY_PICKS : 0;
 	flags |= opts->flags & REBASE_NO_QUIET ? TODO_LIST_WARN_SKIPPED_CHERRY_PICKS : 0;
 
-	switch (command) {
+	switch (opts->action) {
 	case ACTION_NONE: {
 		if (!opts->onto && !opts->upstream)
 			die(_("a base commit must be provided with --upstream or --onto"));
@@ -358,7 +359,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 		break;
 	}
 	default:
-		BUG("invalid command '%d'", command);
+		BUG("invalid command '%d'", opts->action);
 	}
 
 	return ret;
@@ -615,7 +616,7 @@ static int run_am(struct rebase_options *opts)
 	strvec_push(&am.args, "am");
 	strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
 		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
-	if (opts->action && !strcmp("continue", opts->action)) {
+	if (opts->action == ACTION_CONTINUE) {
 		strvec_push(&am.args, "--resolved");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
 		if (opts->gpg_sign_opt)
@@ -626,7 +627,7 @@ static int run_am(struct rebase_options *opts)
 
 		return move_to_original_branch(opts);
 	}
-	if (opts->action && !strcmp("skip", opts->action)) {
+	if (opts->action == ACTION_SKIP) {
 		strvec_push(&am.args, "--skip");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
 		status = run_command(&am);
@@ -635,7 +636,7 @@ static int run_am(struct rebase_options *opts)
 
 		return move_to_original_branch(opts);
 	}
-	if (opts->action && !strcmp("show-current-patch", opts->action)) {
+	if (opts->action == ACTION_SHOW_CURRENT_PATCH) {
 		strvec_push(&am.args, "--show-current-patch");
 		return run_command(&am);
 	}
@@ -728,7 +729,7 @@ static int run_am(struct rebase_options *opts)
 	return status;
 }
 
-static int run_specific_rebase(struct rebase_options *opts, enum action action)
+static int run_specific_rebase(struct rebase_options *opts)
 {
 	int status;
 
@@ -746,7 +747,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
 			opts->gpg_sign_opt = tmp;
 		}
 
-		status = run_sequencer_rebase(opts, action);
+		status = run_sequencer_rebase(opts);
 	} else if (opts->type == REBASE_APPLY)
 		status = run_am(opts);
 	else
@@ -1016,7 +1017,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct strbuf buf = STRBUF_INIT;
 	struct object_id merge_base;
 	int ignore_whitespace = 0;
-	enum action action = ACTION_NONE;
 	const char *gpg_sign = NULL;
 	struct string_list exec = STRING_LIST_INIT_NODUP;
 	const char *rebase_merges = NULL;
@@ -1065,18 +1065,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "no-ff", &options.flags,
 			N_("cherry-pick all commits, even if unchanged"),
 			REBASE_FORCE),
-		OPT_CMDMODE(0, "continue", &action, N_("continue"),
+		OPT_CMDMODE(0, "continue", &options.action, N_("continue"),
 			    ACTION_CONTINUE),
-		OPT_CMDMODE(0, "skip", &action,
+		OPT_CMDMODE(0, "skip", &options.action,
 			    N_("skip current patch and continue"), ACTION_SKIP),
-		OPT_CMDMODE(0, "abort", &action,
+		OPT_CMDMODE(0, "abort", &options.action,
 			    N_("abort and check out the original branch"),
 			    ACTION_ABORT),
-		OPT_CMDMODE(0, "quit", &action,
+		OPT_CMDMODE(0, "quit", &options.action,
 			    N_("abort but keep HEAD where it is"), ACTION_QUIT),
-		OPT_CMDMODE(0, "edit-todo", &action, N_("edit the todo list "
+		OPT_CMDMODE(0, "edit-todo", &options.action, N_("edit the todo list "
 			    "during an interactive rebase"), ACTION_EDIT_TODO),
-		OPT_CMDMODE(0, "show-current-patch", &action,
+		OPT_CMDMODE(0, "show-current-patch", &options.action,
 			    N_("show the patch file being applied or merged"),
 			    ACTION_SHOW_CURRENT_PATCH),
 		OPT_CALLBACK_F(0, "apply", &options, NULL,
@@ -1189,7 +1189,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (preserve_merges_selected)
 		die(_("--preserve-merges was replaced by --rebase-merges"));
 
-	if (action != ACTION_NONE && total_argc != 2) {
+	if (options.action != ACTION_NONE && total_argc != 2) {
 		usage_with_options(builtin_rebase_usage,
 				   builtin_rebase_options);
 	}
@@ -1208,11 +1208,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.root && options.fork_point > 0)
 		die(_("cannot combine '--root' with '--fork-point'"));
 
-	if (action != ACTION_NONE && !in_progress)
+	if (options.action != ACTION_NONE && !in_progress)
 		die(_("No rebase in progress?"));
 	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
 
-	if (action == ACTION_EDIT_TODO && !is_merge(&options))
+	if (options.action == ACTION_EDIT_TODO && !is_merge(&options))
 		die(_("The --edit-todo action can only be used during "
 		      "interactive rebase."));
 
@@ -1222,16 +1222,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		else if (exec.nr)
 			trace2_cmd_mode("interactive-exec");
 		else
-			trace2_cmd_mode(action_names[action]);
+			trace2_cmd_mode(action_names[options.action]);
 	}
 
-	switch (action) {
+	switch (options.action) {
 	case ACTION_CONTINUE: {
 		struct object_id head;
 		struct lock_file lock_file = LOCK_INIT;
 		int fd;
 
-		options.action = "continue";
 		/* Sanity check */
 		if (get_oid("HEAD", &head))
 			die(_("Cannot read HEAD"));
@@ -1257,7 +1256,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	case ACTION_SKIP: {
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
-		options.action = "skip";
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 		ropts.flags = RESET_HEAD_HARD;
@@ -1272,7 +1270,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 		struct strbuf head_msg = STRBUF_INIT;
 
-		options.action = "abort";
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 
@@ -1312,17 +1309,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		goto cleanup;
 	}
 	case ACTION_EDIT_TODO:
-		options.action = "edit-todo";
 		options.dont_finish_rebase = 1;
 		goto run_rebase;
 	case ACTION_SHOW_CURRENT_PATCH:
-		options.action = "show-current-patch";
 		options.dont_finish_rebase = 1;
 		goto run_rebase;
 	case ACTION_NONE:
 		break;
 	default:
-		BUG("action: %d", action);
+		BUG("action: %d", options.action);
 	}
 
 	/* Make sure no rebase is in progress */
@@ -1346,7 +1341,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||
-	    (action != ACTION_NONE) ||
+	    (options.action != ACTION_NONE) ||
 	    (exec.nr > 0) ||
 	    options.autosquash) {
 		allow_preemptive_ff = 0;
@@ -1786,7 +1781,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	options.revisions = revisions.buf;
 
 run_rebase:
-	ret = run_specific_rebase(&options, action);
+	ret = run_specific_rebase(&options);
 
 cleanup:
 	strbuf_release(&buf);
-- 
gitgitgadget
Ævar Arnfjörð Bjarmason· Apr 20, 2022, 10:34 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH v2 8/8] rebase: cleanup action handling

On Wed, Apr 20 2022, Phillip Wood via GitGitGadget wrote:
Show 8 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Treating the action as a string is a hang over from the scripted
> rebase. The last commit removed the only remaining use of the action
> that required a string so lets convert the other action users to use
> the existing action enum instead. If we ever need the action name as a
> string in the future the action_names array exists exactly for that
> purpose.

This is very much an existing issue, just noting this since I read this over: We really should just carry that "action_names" in the "switch" statement immediately below the trace2_cmd_mode() call, see "git grep trace2_cmd_mode". I.e. usually we do it in the "callback".

Or actually just add a OPT_CMDMODE_TRACE2(), which would be an OPT_CALLBACK (see parse_opt_show_current_patch for such an example for a cmdmode callback) that would take the "--continue" string, log "continue" etc (do we really need to munge "edit-todo" to "edit_todo").

But I digress, this code looks fine, thanks.
Phillip Wood via GitGitGadget· Oct 12, 2022, 09:35 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v3 0/8] rebase: make reflog messages independent of the backend

This series fixes some bugs in the reflog messages when rebasing and changes the reflog messages of "rebase --apply" to match "rebase --merge" with the aim of making the reflog easier to parse.

Thanks to everyone who commented on V2, I've added the review club participants that I've got address for to the cc list. I've rebased onto pw/rebase-keep-base-fixes.

Change since V2:
 * Patch 1: Reworded the commit message to address the concerns in [1,2]
   about the behavior when head_name is NULL. There is also a small change
   due to being rebased.
 * Patch 2: Unchanged. There wasn't much love for parameterized tests in
   review club but we want to ensure both backends produce the same messages
   I think this is the safest way to achieve that. Using separate tests
   makes it too easy to introduce subtle differences in the testing of the
   two backends.
 * Patch 3: Added a note to the commit message to address the concerns in
   [1] about not resetting GIT_REFLOG_ACTION when we return early.
 * Patches 4 & 5: Unchanged.
 * Patch 6: Reworded the commit message to make a stronger argument for this
   change. There are concerns about backwards compatibility in [1,3,4] but
   (i) we have made similar changes in the past without complaints and (ii)
   we're changing the message to an existing format. There is also a small
   change due to being rebased.
 * Patches 7 & 8: Small changes due to rebase.

[1] https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?pli=1#heading=h.t6g5l2t5ibzw [2] https://lore.kernel.org/git/xmqq35i7r4rj.fsf@gitster.g/ [3] https://lore.kernel.org/git/xmqq4k2nmmeg.fsf@gitster.g/ [4] https://lore.kernel.org/git/220420.865yn4833u.gmgdl@evledraar.gmail.com/

V2 Cover Letter:
Thanks to Christian and Elijah for their comments on V1.

I've updated commit message for patch 1 to try and be clearer about the removal of a call to strbuf_release() and spilt out the test changes from the old patch 2 into a separate preparatory patch.

V1 Cover Letter:

This is a series of rebase reflog related patches with the aim of unifying the reflog messages from the two rebase backends.

 * improve rebase reflog test coverage
 * rebase --merge: fix reflog messages for --continue and --skip
 * rebase --apply: respect GIT_REFLOG_ACTION
 * rebase --abort: improve reflog message
 * unify reflog messages between the two rebase backends
This series is based on pw/use-inprocess-checkout-in-rebase
Phillip Wood (8):
  rebase --apply: remove duplicated code
  t3406: rework rebase reflog tests
  rebase --merge: fix reflog when continuing
  rebase --merge: fix reflog message after skipping
  rebase --apply: respect GIT_REFLOG_ACTION
  rebase --apply: make reflog messages match rebase --merge
  rebase --abort: improve reflog message
  rebase: cleanup action handling
 builtin/rebase.c          | 146 ++++++++++++------------------
 sequencer.c               |   5 ++
 t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++-------
 3 files changed, 215 insertions(+), 121 deletions(-)
base-commit: 0203c679871112c78adc6428a8ed6c04c30ccad9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1150
Range-diff vs v2:
 1:  a4320f2fcf3 ! 1:  a84cf971a75 rebase --apply: remove duplicated code
     @@ Metadata
       ## Commit message ##
          rebase --apply: remove duplicated code
      
     -    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.
     +    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. 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
          identical call just above this hunk which can be seen by viewing the
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       	 * we just fast-forwarded.
       	 */
      -	strbuf_reset(&msg);
     - 	if (oideq(&merge_base, &options.orig_head)) {
     + 	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",
 2:  0904b50a377 = 2:  b9255ad35d2 t3406: rework rebase reflog tests
 3:  6c15f00e170 ! 3:  ea4da25a19c rebase --merge: fix reflog when continuing
     @@ Commit message
          in pick_commits(). Both of these will be fixed in a future series that
          stops the sequencer calling setenv().
      
     +    If we fail to commit the staged changes then we error out so
     +    GIT_REFLOG_ACTION does not need to be reset in that case.
     +
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## builtin/rebase.c ##
 4:  d3afa85ffc5 = 4:  225ff4baef7 rebase --merge: fix reflog message after skipping
 5:  afa67abe01a = 5:  1094681eb11 rebase --apply: respect GIT_REFLOG_ACTION
 6:  95161f21e00 ! 6:  a5338e6bdd8 rebase --apply: make reflog messages match rebase --merge
     @@ Commit message
          rebase --apply: make reflog messages match rebase --merge
      
          The apply backend creates slightly different reflog messages to the
     -    merge backend when starting and finishing a rebase and when picking
     -    commits. The choice of backend is really an implementation detail so
     -    it is confusing to have the same command create different messages
     -    depending on which backend is selected. Change the apply backend so
     -    the reflog messages from the two backends match as closely as
     -    possible. Note that there is still a difference when committing a
     -    conflict resolution - the merge backend will use "(continue)" rather
     -    than "(pick)" in that case as it does not know which command created
     -    the conflict that it is committing.
     +    merge backend when starting or finishing a rebase and when picking
     +    commits. These differences make it harder than it needs to be to parse
     +    the reflog (I have a script that reads the finishing messages from
     +    rebase and it is a pain to have to accommodate two different message
     +    formats). While it is possible to determine the backend used for a
     +    rebase from the reflog messages, the differences are not designed for
     +    that purpose. c2417d3af7 (rebase: drop '-i' from the reflog for
     +    interactive-based rebases, 2020-02-15) removed the clear distinction
     +    between the reflog messages of the two backends without complaint.
     +
     +    As the merge backend is the default it is likely to be the format most
     +    common in existing reflogs. For that reason the apply backend is changed
     +    to format its reflog messages to match the merge backend as closely as
     +    possible. Note that there is still a difference as when committing a
     +    conflict resolution the apply backend will use "(pick)" rather than
     +    "(continue)" because it is not currently possible to change the message
     +    for a single commit.
     +
     +    In addition to c2417d3af7 we also changed the reflog messages in
     +    68aa495b59 (rebase: implement --merge via the interactive machinery,
     +    2018-12-11) and 2ac0d6273f (rebase: change the default backend from "am"
     +    to "merge", 2020-02-15). This commit makes the same change to "git
     +    rebase --apply" that 2ac0d6273f made to "git rebase" without any backend
     +    specific options. As the messages are changed to use an existing format
     +    any scripts that can parse the reflog messages of the default rebase
     +    backend should be unaffected by this change.
     +
     +    There are existing tests for the messages from both backends which are
     +    adjusted to ensure that they do not get out of sync in the future.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     @@ builtin/rebase.c: static int run_am(struct rebase_options *opts)
       	am.git_cmd = 1;
       	strvec_push(&am.args, "am");
      -
     -+	strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
     ++	strvec_pushf(&am.env, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
      +		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
       	if (opts->action && !strcmp("continue", opts->action)) {
       		strvec_push(&am.args, "--resolved");
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
      +	strbuf_addf(&msg, "%s (start): checkout %s",
       		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
       	ropts.oid = &options.onto->object.oid;
     - 	ropts.orig_head = &options.orig_head,
     + 	ropts.orig_head = &options.orig_head->object.oid,
      
       ## t/t3406-rebase-message.sh ##
      @@ t/t3406-rebase-message.sh: test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
 7:  d2c1dfbcd5e ! 7:  aa808725fb8 rebase --abort: improve reflog message
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
      +		strbuf_addf(&head_msg, "%s (abort): returning to %s",
      +			    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
      +			    options.head_name ? options.head_name
     -+					      : oid_to_hex(&options.orig_head));
     - 		ropts.oid = &options.orig_head;
     ++					      : oid_to_hex(&options.orig_head->object.oid));
     + 		ropts.oid = &options.orig_head->object.oid;
      +		ropts.head_msg = head_msg.buf;
       		ropts.branch = options.head_name;
       		ropts.flags = RESET_HEAD_HARD;
      -		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
       		if (reset_head(the_repository, &ropts) < 0)
       			die(_("could not move back to %s"),
     - 			    oid_to_hex(&options.orig_head));
     + 			    oid_to_hex(&options.orig_head->object.oid));
      
       ## t/t3406-rebase-message.sh ##
      @@ t/t3406-rebase-message.sh: test_reflog () {
 8:  b0d21affa78 ! 8:  f9c8664b883 rebase: cleanup action handling
     @@ builtin/rebase.c: static int run_sequencer_rebase(struct rebase_options *opts,
       	return ret;
      @@ builtin/rebase.c: static int run_am(struct rebase_options *opts)
       	strvec_push(&am.args, "am");
     - 	strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
     + 	strvec_pushf(&am.env, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
       		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
      -	if (opts->action && !strcmp("continue", opts->action)) {
      +	if (opts->action == ACTION_CONTINUE) {
     @@ builtin/rebase.c: static int run_specific_rebase(struct rebase_options *opts, en
       	else
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
       	struct strbuf buf = STRBUF_INIT;
     - 	struct object_id merge_base;
     + 	struct object_id branch_base;
       	int ignore_whitespace = 0;
      -	enum action action = ACTION_NONE;
       	const char *gpg_sign = NULL;
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       			    ACTION_SHOW_CURRENT_PATCH),
       		OPT_CALLBACK_F(0, "apply", &options, NULL,
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
     - 	if (preserve_merges_selected)
     - 		die(_("--preserve-merges was replaced by --rebase-merges"));
     + 	} else if (is_directory(merge_dir())) {
     + 		strbuf_reset(&buf);
     + 		strbuf_addf(&buf, "%s/rewritten", merge_dir());
     +-		if (!(action == ACTION_ABORT) && is_directory(buf.buf)) {
     ++		if (!(options.action == ACTION_ABORT) && is_directory(buf.buf)) {
     + 			die(_("`rebase --preserve-merges` (-p) is no longer supported.\n"
     + 			"Use `git rebase --abort` to terminate current rebase.\n"
     + 			"Or downgrade to v2.33, or earlier, to complete the rebase."));
     +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
     + 			"Note: Your `pull.rebase` configuration may also be set to 'preserve',\n"
     + 			"which is no longer supported; use 'merges' instead"));
       
      -	if (action != ACTION_NONE && total_argc != 2) {
      +	if (options.action != ACTION_NONE && total_argc != 2) {
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       	}
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
       	if (options.root && options.fork_point > 0)
     - 		die(_("cannot combine '--root' with '--fork-point'"));
     + 		die(_("options '%s' and '%s' cannot be used together"), "--root", "--fork-point");
       
      -	if (action != ACTION_NONE && !in_progress)
      +	if (options.action != ACTION_NONE && !in_progress)
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Oct 12, 2022, 09:35 UTC · re: Phillip Wood via GitGitGadget · lore

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

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. 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 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(-)
Show changes to builtin/rebase.c +1 −10
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;
 	}
-- 
gitgitgadget
Junio C Hamano· Oct 12, 2022, 20:36 UTC · re: Phillip Wood via GitGitGadget · lore

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

"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;
>  	}
Junio C Hamano· Oct 13, 2022, 18:13 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 18 quoted lines
> "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?

I think "git rebase --apply A B" when B is already an descendant of A with a single strand of pearls would trigger the new logic, and instead of the old "rebase finished: %s onto %s" message used for both reflogs, calling move_to_original_branch() will give us "rebase finished: %s onto %s" in the branch reflog, while "rebase finished: returning to %s" in the HEAD reflog.

Note that I am not saying we should not change the behaviour. Saying "returning to X" in the reflog of HEAD may arguably be better than using the same "rebased X onto Y" for reflogs for both HEAD and the underlying branch.

But if that is what is going on, we should record it as improving the reflog message, not removing duplicated code.

Also, it would be good to have a test that demonstrates how the contents of the reflog changes with this change. It took me some time to figure out how to reach that codepath, even though it was relatively easy to see how the reflog message(s) used before and after the patch are different.

Show 12 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.
Thanks.
Phillip Wood· Oct 20, 2022, 09:50 UTC · re: Junio C Hamano · lore

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

Hi Junio
On 13/10/2022 19:13, Junio C Hamano wrote:
Show 20 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> "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?
Yes, you're right
Show 20 quoted lines
> I think "git rebase --apply A B" when B is already an descendant of
> A with a single strand of pearls would trigger the new logic, and
> instead of the old "rebase finished: %s onto %s" message used for
> both reflogs, calling move_to_original_branch() will give us "rebase
> finished: %s onto %s" in the branch reflog, while "rebase finished:
> returning to %s" in the HEAD reflog.
> 
> Note that I am not saying we should not change the behaviour.
> Saying "returning to X" in the reflog of HEAD may arguably be better
> than using the same "rebased X onto Y" for reflogs for both HEAD and
> the underlying branch.
> 
> But if that is what is going on, we should record it as improving
> the reflog message, not removing duplicated code.
> 
> Also, it would be good to have a test that demonstrates how the
> contents of the reflog changes with this change.  It took me some
> time to figure out how to reach that codepath, even though it was
> relatively easy to see how the reflog message(s) used before and
> after the patch are different.

I've just checked and the tests added in the next patch do test this path (they fail if I revert this commit). I should be able to swap the two patches round to demonstrate the change in behavior and rework the commit commit message for this patch.

Best Wishes
Phillip
Show 14 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.
> 
> Thanks.
Phillip Wood via GitGitGadget· Oct 12, 2022, 09:35 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v3 2/8] t3406: rework rebase reflog tests

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Refactor the tests in preparation for adding more tests in the next few commits. The reworked tests use the same function for testing both the "merge" and "apply" backends. The test coverage for the "apply" backend now includes setting GIT_REFLOG_ACTION.

Note that rebasing the "conflicts" branch does not create any conflicts yet. A commit to do that will be added in the next commit and the diff ends up smaller if we have don't rename the branch when it is added.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3406-rebase-message.sh | 115 +++++++++++++++++++++++++-------------
 1 file changed, 76 insertions(+), 39 deletions(-)
Show changes to t/t3406-rebase-message.sh +76 −39
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index d17b450e811..5253dd1551d 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -10,10 +10,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 test_expect_success 'setup' '
 	test_commit O fileO &&
 	test_commit X fileX &&
+	git branch fast-forward &&
 	test_commit A fileA &&
 	test_commit B fileB &&
 	test_commit Y fileY &&
 
+	git checkout -b conflicts O &&
+	test_commit P &&
+	test_commit Q &&
+
 	git checkout -b topic O &&
 	git cherry-pick A B &&
 	test_commit Z fileZ &&
@@ -79,54 +84,86 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
 	test_i18ngrep "Invalid whitespace option" err
 '
 
-test_expect_success 'GIT_REFLOG_ACTION' '
-	git checkout start &&
-	test_commit reflog-onto &&
-	git checkout -b reflog-topic start &&
-	test_commit reflog-to-rebase &&
-
-	git rebase reflog-onto &&
-	git log -g --format=%gs -3 >actual &&
-	cat >expect <<-\EOF &&
-	rebase (finish): returning to refs/heads/reflog-topic
-	rebase (pick): reflog-to-rebase
-	rebase (start): checkout reflog-onto
+write_reflog_expect () {
+	if test $mode = --apply
+	then
+		sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//'
+	else
+		cat
+	fi >expect
+}
+
+test_reflog () {
+	mode=$1
+	reflog_action="$2"
+
+	test_expect_success "rebase $mode reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout conflicts &&
+	test_when_finished "git reset --hard Q" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		git rebase $mode main
+	) &&
+
+	git log -g --format=%gs -4 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
+	${reflog_action:-rebase} (pick): Q
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
 	EOF
 	test_cmp expect actual &&
 
-	git checkout -b reflog-prefix reflog-to-rebase &&
-	GIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto &&
-	git log -g --format=%gs -3 >actual &&
-	cat >expect <<-\EOF &&
-	change-the-reflog (finish): returning to refs/heads/reflog-prefix
-	change-the-reflog (pick): reflog-to-rebase
-	change-the-reflog (start): checkout reflog-onto
+	git log -g --format=%gs -1 conflicts >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): refs/heads/conflicts onto $(git rev-parse main)
 	EOF
-	test_cmp expect actual
-'
-
-test_expect_success 'rebase --apply reflog' '
-	git checkout -b reflog-apply start &&
-	old_head_reflog="$(git log -g --format=%gs -1 HEAD)" &&
-
-	git rebase --apply Y &&
+	test_cmp expect actual &&
 
-	git log -g --format=%gs -4 HEAD >actual &&
-	cat >expect <<-EOF &&
-	rebase finished: returning to refs/heads/reflog-apply
-	rebase: Z
-	rebase: checkout Y
-	$old_head_reflog
+	# check there is only one new entry in the branch reflog
+	test_cmp_rev conflicts@{1} Q
+	'
+
+	test_expect_success "rebase $mode fast-forward reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout fast-forward &&
+	test_when_finished "git reset --hard X" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		git rebase $mode main
+	) &&
+
+	git log -g --format=%gs -2 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward
+	${reflog_action:-rebase} (start): checkout main
 	EOF
 	test_cmp expect actual &&
 
-	git log -g --format=%gs -2 reflog-apply >actual &&
-	cat >expect <<-EOF &&
-	rebase finished: refs/heads/reflog-apply onto $(git rev-parse Y)
-	branch: Created from start
+	git log -g --format=%gs -1 fast-forward >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): refs/heads/fast-forward onto $(git rev-parse main)
 	EOF
-	test_cmp expect actual
-'
+	test_cmp expect actual &&
+
+	# check there is only one new entry in the branch reflog
+	test_cmp_rev fast-forward@{1} X
+	'
+}
+
+test_reflog --merge
+test_reflog --merge my-reflog-action
+test_reflog --apply
+test_reflog --apply my-reflog-action
 
 test_expect_success 'rebase -i onto unrelated history' '
 	git init unrelated &&
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Oct 12, 2022, 09:35 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v3 3/8] rebase --merge: fix reflog when continuing

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The reflog message for a conflict resolution committed by "rebase --continue" looks like

	rebase (continue): commit subject line
Unfortunately the reflog message each subsequent pick look like
	rebase (continue) (pick): commit subject line

Fix this by setting the reflog message for "rebase --continue" in sequencer_continue() so it does not affect subsequent commits. This introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION in pick_commits(). Both of these will be fixed in a future series that stops the sequencer calling setenv().

If we fail to commit the staged changes then we error out so GIT_REFLOG_ACTION does not need to be reset in that case.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 2 --
 sequencer.c               | 5 +++++
 t/t3406-rebase-message.sh | 9 +++++++--
 3 files changed, 12 insertions(+), 4 deletions(-)
Show changes to 3 files +12 −4

builtin/rebase.c, sequencer.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 51accb4fd61..488e6bdfd3e 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1271,8 +1271,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		int fd;
 
 		options.action = "continue";
-		set_reflog_action(&options);
-
 		/* Sanity check */
 		if (get_oid("HEAD", &head))
 			die(_("Cannot read HEAD"));
diff --git a/sequencer.c b/sequencer.c
index 61a8e0020d5..5790b35d763 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4785,6 +4785,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 	if (read_populate_opts(opts))
 		return -1;
 	if (is_rebase_i(opts)) {
+		char *previous_reflog_action;
+
 		if ((res = read_populate_todo(r, &todo_list, opts)))
 			goto release_todo_list;
 
@@ -4795,10 +4797,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 			unlink(rebase_path_dropped());
 		}
 
+		previous_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
+		setenv(GIT_REFLOG_ACTION, reflog_message(opts, "continue", NULL), 1);
 		if (commit_staged_changes(r, opts, &todo_list)) {
 			res = -1;
 			goto release_todo_list;
 		}
+		setenv(GIT_REFLOG_ACTION, previous_reflog_action, 1);
 	} else if (!file_exists(get_todo_path(opts)))
 		return continue_single_pick(r, opts);
 	else if ((res = read_populate_todo(r, &todo_list, opts)))
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 5253dd1551d..3ca2fbb0d59 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -17,6 +17,7 @@ test_expect_success 'setup' '
 
 	git checkout -b conflicts O &&
 	test_commit P &&
+	test_commit conflict-X fileX &&
 	test_commit Q &&
 
 	git checkout -b topic O &&
@@ -107,13 +108,17 @@ test_reflog () {
 			GIT_REFLOG_ACTION="$reflog_action" &&
 			export GIT_REFLOG_ACTION
 		fi &&
-		git rebase $mode main
+		test_must_fail git rebase $mode main &&
+		echo resolved >fileX &&
+		git add fileX &&
+		git rebase --continue
 	) &&
 
-	git log -g --format=%gs -4 >actual &&
+	git log -g --format=%gs -5 >actual &&
 	write_reflog_expect <<-EOF &&
 	${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
 	${reflog_action:-rebase} (pick): Q
+	${reflog_action:-rebase} (continue): conflict-X
 	${reflog_action:-rebase} (pick): P
 	${reflog_action:-rebase} (start): checkout main
 	EOF
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Oct 12, 2022, 09:35 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v3 4/8] rebase --merge: fix reflog message after skipping

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The reflog message for every pick after running "rebase --skip" looks like

	rebase (skip) (pick): commit subject line
Fix this by not appending " (skip)" to the reflog action.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          |  2 --
 t/t3406-rebase-message.sh | 24 ++++++++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)
Show changes to 2 files +24 −2

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 488e6bdfd3e..9a40a5e27f8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1297,8 +1297,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
 		options.action = "skip";
-		set_reflog_action(&options);
-
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 		ropts.flags = RESET_HEAD_HARD;
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 3ca2fbb0d59..8aa6a79acc1 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -163,6 +163,30 @@ test_reflog () {
 	# check there is only one new entry in the branch reflog
 	test_cmp_rev fast-forward@{1} X
 	'
+
+	test_expect_success "rebase $mode --skip reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout conflicts &&
+	test_when_finished "git reset --hard Q" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		test_must_fail git rebase $mode main &&
+		git rebase --skip
+	) &&
+
+	git log -g --format=%gs -4 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
+	${reflog_action:-rebase} (pick): Q
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
+	EOF
+	test_cmp expect actual
+	'
 }
 
 test_reflog --merge
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Oct 12, 2022, 09:35 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v3 5/8] rebase --apply: respect GIT_REFLOG_ACTION

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The reflog messages when finishing a rebase hard code "rebase" rather than using GIT_REFLOG_ACTION.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 7 ++++---
 t/t3406-rebase-message.sh | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)
Show changes to 2 files +5 −4

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 9a40a5e27f8..f7b00efeb92 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -582,10 +582,11 @@ static int move_to_original_branch(struct rebase_options *opts)
 	if (!opts->onto)
 		BUG("move_to_original_branch without onto");
 
-	strbuf_addf(&branch_reflog, "rebase finished: %s onto %s",
+	strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
+		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
-	strbuf_addf(&head_reflog, "rebase finished: returning to %s",
-		    opts->head_name);
+	strbuf_addf(&head_reflog, "%s finished: returning to %s",
+		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
 	ropts.branch = opts->head_name;
 	ropts.flags = RESET_HEAD_REFS_ONLY;
 	ropts.branch_msg = branch_reflog.buf;
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 8aa6a79acc1..bb2a4949abc 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
 write_reflog_expect () {
 	if test $mode = --apply
 	then
-		sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//'
+		sed 's/(finish)/finished/; s/ ([^)]*)//'
 	else
 		cat
 	fi >expect
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Oct 12, 2022, 09:35 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v3 6/8] rebase --apply: make reflog messages match rebase --merge

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The apply backend creates slightly different reflog messages to the merge backend when starting or finishing a rebase and when picking commits. These differences make it harder than it needs to be to parse the reflog (I have a script that reads the finishing messages from rebase and it is a pain to have to accommodate two different message formats). While it is possible to determine the backend used for a rebase from the reflog messages, the differences are not designed for that purpose. c2417d3af7 (rebase: drop '-i' from the reflog for interactive-based rebases, 2020-02-15) removed the clear distinction between the reflog messages of the two backends without complaint.

As the merge backend is the default it is likely to be the format most common in existing reflogs. For that reason the apply backend is changed to format its reflog messages to match the merge backend as closely as possible. Note that there is still a difference as when committing a conflict resolution the apply backend will use "(pick)" rather than "(continue)" because it is not currently possible to change the message for a single commit.

In addition to c2417d3af7 we also changed the reflog messages in 68aa495b59 (rebase: implement --merge via the interactive machinery, 2018-12-11) and 2ac0d6273f (rebase: change the default backend from "am" to "merge", 2020-02-15). This commit makes the same change to "git rebase --apply" that 2ac0d6273f made to "git rebase" without any backend specific options. As the messages are changed to use an existing format any scripts that can parse the reflog messages of the default rebase backend should be unaffected by this change.

There are existing tests for the messages from both backends which are adjusted to ensure that they do not get out of sync in the future.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 9 +++++----
 t/t3406-rebase-message.sh | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)
Show changes to 2 files +6 −5

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index f7b00efeb92..ed612c2d0c3 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -582,10 +582,10 @@ static int move_to_original_branch(struct rebase_options *opts)
 	if (!opts->onto)
 		BUG("move_to_original_branch without onto");
 
-	strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
+	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
-	strbuf_addf(&head_reflog, "%s finished: returning to %s",
+	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
 	ropts.branch = opts->head_name;
 	ropts.flags = RESET_HEAD_REFS_ONLY;
@@ -615,7 +615,8 @@ static int run_am(struct rebase_options *opts)
 
 	am.git_cmd = 1;
 	strvec_push(&am.args, "am");
-
+	strvec_pushf(&am.env, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
+		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
 	if (opts->action && !strcmp("continue", opts->action)) {
 		strvec_push(&am.args, "--resolved");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
@@ -1789,7 +1790,7 @@ 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",
+	strbuf_addf(&msg, "%s (start): checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
 	ropts.oid = &options.onto->object.oid;
 	ropts.orig_head = &options.orig_head->object.oid,
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index bb2a4949abc..5c6cd9af3bc 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
 write_reflog_expect () {
 	if test $mode = --apply
 	then
-		sed 's/(finish)/finished/; s/ ([^)]*)//'
+		sed 's/(continue)/(pick)/'
 	else
 		cat
 	fi >expect
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Oct 12, 2022, 09:35 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v3 7/8] rebase --abort: improve reflog message

From: Phillip Wood <phillip.wood@dunelm.org.uk>
When aborting a rebase the reflog message looks like
	rebase (abort): updating HEAD

which is not very informative. Improve the message by mentioning the branch that we are returning to as we do at the end of a successful rebase so it looks like.

	rebase (abort): returning to refs/heads/topic

If GIT_REFLOG_ACTION is set in the environment we no longer omit "(abort)" from the reflog message. We don't omit "(start)" and "(finish)" when starting and finishing a rebase in that case so we shouldn't omit "(abort)".

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 28 ++++++---------------
 t/t3406-rebase-message.sh | 51 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 20 deletions(-)
Show changes to 2 files +59 −20

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index ed612c2d0c3..e4620935129 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1001,23 +1001,6 @@ static void NORETURN error_on_missing_default_upstream(void)
 	exit(1);
 }
 
-static void set_reflog_action(struct rebase_options *options)
-{
-	const char *env;
-	struct strbuf buf = STRBUF_INIT;
-
-	if (!is_merge(options))
-		return;
-
-	env = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
-	if (env && strcmp("rebase", env))
-		return; /* only override it if it is "rebase" */
-
-	strbuf_addf(&buf, "rebase (%s)", options->action);
-	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, buf.buf, 1);
-	strbuf_release(&buf);
-}
-
 static int check_exec_cmd(const char *cmd)
 {
 	if (strchr(cmd, '\n'))
@@ -1311,18 +1294,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 	case ACTION_ABORT: {
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
-		options.action = "abort";
-		set_reflog_action(&options);
+		struct strbuf head_msg = STRBUF_INIT;
 
+		options.action = "abort";
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 
 		if (read_basic_state(&options))
 			exit(1);
+
+		strbuf_addf(&head_msg, "%s (abort): returning to %s",
+			    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
+			    options.head_name ? options.head_name
+					      : oid_to_hex(&options.orig_head->object.oid));
 		ropts.oid = &options.orig_head->object.oid;
+		ropts.head_msg = head_msg.buf;
 		ropts.branch = options.head_name;
 		ropts.flags = RESET_HEAD_HARD;
-		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
 		if (reset_head(the_repository, &ropts) < 0)
 			die(_("could not move back to %s"),
 			    oid_to_hex(&options.orig_head->object.oid));
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 5c6cd9af3bc..ceca1600053 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -187,6 +187,57 @@ test_reflog () {
 	EOF
 	test_cmp expect actual
 	'
+
+	test_expect_success "rebase $mode --abort reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout conflicts &&
+	test_when_finished "git reset --hard Q" &&
+
+	git log -g -1 conflicts >branch-expect &&
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		test_must_fail git rebase $mode main &&
+		git rebase --abort
+	) &&
+
+	git log -g --format=%gs -3 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (abort): returning to refs/heads/conflicts
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
+	EOF
+	test_cmp expect actual &&
+
+	# check branch reflog is unchanged
+	git log -g -1 conflicts >branch-actual &&
+	test_cmp branch-expect branch-actual
+	'
+
+	test_expect_success "rebase $mode --abort detached HEAD reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout Q &&
+	test_when_finished "git reset --hard Q" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		test_must_fail git rebase $mode main &&
+		git rebase --abort
+	) &&
+
+	git log -g --format=%gs -3 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (abort): returning to $(git rev-parse Q)
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
+	EOF
+	test_cmp expect actual
+	'
 }
 
 test_reflog --merge
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Oct 12, 2022, 09:35 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v3 8/8] rebase: cleanup action handling

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Treating the action as a string is a hang over from the scripted rebase. The last commit removed the only remaining use of the action that required a string so lets convert the other action users to use the existing action enum instead. If we ever need the action name as a string in the future the action_names array exists exactly for that purpose.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 93 +++++++++++++++++++++++-------------------------
 1 file changed, 44 insertions(+), 49 deletions(-)
Show changes to builtin/rebase.c +44 −49
diff --git a/builtin/rebase.c b/builtin/rebase.c
index e4620935129..eb1fb8e3551 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -59,6 +59,26 @@ enum empty_type {
 	EMPTY_ASK
 };
 
+enum action {
+	ACTION_NONE = 0,
+	ACTION_CONTINUE,
+	ACTION_SKIP,
+	ACTION_ABORT,
+	ACTION_QUIT,
+	ACTION_EDIT_TODO,
+	ACTION_SHOW_CURRENT_PATCH
+};
+
+static const char *action_names[] = {
+	"undefined",
+	"continue",
+	"skip",
+	"abort",
+	"quit",
+	"edit_todo",
+	"show_current_patch"
+};
+
 struct rebase_options {
 	enum rebase_type type;
 	enum empty_type empty;
@@ -85,7 +105,7 @@ struct rebase_options {
 		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
 	} flags;
 	struct strvec git_am_opts;
-	const char *action;
+	enum action action;
 	int signoff;
 	int allow_rerere_autoupdate;
 	int keep_empty;
@@ -156,24 +176,6 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	return replay;
 }
 
-enum action {
-	ACTION_NONE = 0,
-	ACTION_CONTINUE,
-	ACTION_SKIP,
-	ACTION_ABORT,
-	ACTION_QUIT,
-	ACTION_EDIT_TODO,
-	ACTION_SHOW_CURRENT_PATCH
-};
-
-static const char *action_names[] = { "undefined",
-				      "continue",
-				      "skip",
-				      "abort",
-				      "quit",
-				      "edit_todo",
-				      "show_current_patch" };
-
 static int edit_todo_file(unsigned flags)
 {
 	const char *todo_file = rebase_path_todo();
@@ -310,8 +312,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	return ret;
 }
 
-static int run_sequencer_rebase(struct rebase_options *opts,
-				  enum action command)
+static int run_sequencer_rebase(struct rebase_options *opts)
 {
 	unsigned flags = 0;
 	int abbreviate_commands = 0, ret = 0;
@@ -326,7 +327,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 	flags |= opts->reapply_cherry_picks ? TODO_LIST_REAPPLY_CHERRY_PICKS : 0;
 	flags |= opts->flags & REBASE_NO_QUIET ? TODO_LIST_WARN_SKIPPED_CHERRY_PICKS : 0;
 
-	switch (command) {
+	switch (opts->action) {
 	case ACTION_NONE: {
 		if (!opts->onto && !opts->upstream)
 			die(_("a base commit must be provided with --upstream or --onto"));
@@ -359,7 +360,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 		break;
 	}
 	default:
-		BUG("invalid command '%d'", command);
+		BUG("invalid command '%d'", opts->action);
 	}
 
 	return ret;
@@ -617,7 +618,7 @@ static int run_am(struct rebase_options *opts)
 	strvec_push(&am.args, "am");
 	strvec_pushf(&am.env, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
 		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
-	if (opts->action && !strcmp("continue", opts->action)) {
+	if (opts->action == ACTION_CONTINUE) {
 		strvec_push(&am.args, "--resolved");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
 		if (opts->gpg_sign_opt)
@@ -628,7 +629,7 @@ static int run_am(struct rebase_options *opts)
 
 		return move_to_original_branch(opts);
 	}
-	if (opts->action && !strcmp("skip", opts->action)) {
+	if (opts->action == ACTION_SKIP) {
 		strvec_push(&am.args, "--skip");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
 		status = run_command(&am);
@@ -637,7 +638,7 @@ static int run_am(struct rebase_options *opts)
 
 		return move_to_original_branch(opts);
 	}
-	if (opts->action && !strcmp("show-current-patch", opts->action)) {
+	if (opts->action == ACTION_SHOW_CURRENT_PATCH) {
 		strvec_push(&am.args, "--show-current-patch");
 		return run_command(&am);
 	}
@@ -730,7 +731,7 @@ static int run_am(struct rebase_options *opts)
 	return status;
 }
 
-static int run_specific_rebase(struct rebase_options *opts, enum action action)
+static int run_specific_rebase(struct rebase_options *opts)
 {
 	int status;
 
@@ -748,7 +749,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
 			opts->gpg_sign_opt = tmp;
 		}
 
-		status = run_sequencer_rebase(opts, action);
+		status = run_sequencer_rebase(opts);
 	} else if (opts->type == REBASE_APPLY)
 		status = run_am(opts);
 	else
@@ -1025,7 +1026,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct strbuf buf = STRBUF_INIT;
 	struct object_id branch_base;
 	int ignore_whitespace = 0;
-	enum action action = ACTION_NONE;
 	const char *gpg_sign = NULL;
 	struct string_list exec = STRING_LIST_INIT_NODUP;
 	const char *rebase_merges = NULL;
@@ -1074,18 +1074,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "no-ff", &options.flags,
 			N_("cherry-pick all commits, even if unchanged"),
 			REBASE_FORCE),
-		OPT_CMDMODE(0, "continue", &action, N_("continue"),
+		OPT_CMDMODE(0, "continue", &options.action, N_("continue"),
 			    ACTION_CONTINUE),
-		OPT_CMDMODE(0, "skip", &action,
+		OPT_CMDMODE(0, "skip", &options.action,
 			    N_("skip current patch and continue"), ACTION_SKIP),
-		OPT_CMDMODE(0, "abort", &action,
+		OPT_CMDMODE(0, "abort", &options.action,
 			    N_("abort and check out the original branch"),
 			    ACTION_ABORT),
-		OPT_CMDMODE(0, "quit", &action,
+		OPT_CMDMODE(0, "quit", &options.action,
 			    N_("abort but keep HEAD where it is"), ACTION_QUIT),
-		OPT_CMDMODE(0, "edit-todo", &action, N_("edit the todo list "
+		OPT_CMDMODE(0, "edit-todo", &options.action, N_("edit the todo list "
 			    "during an interactive rebase"), ACTION_EDIT_TODO),
-		OPT_CMDMODE(0, "show-current-patch", &action,
+		OPT_CMDMODE(0, "show-current-patch", &options.action,
 			    N_("show the patch file being applied or merged"),
 			    ACTION_SHOW_CURRENT_PATCH),
 		OPT_CALLBACK_F(0, "apply", &options, NULL,
@@ -1174,7 +1174,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	} else if (is_directory(merge_dir())) {
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "%s/rewritten", merge_dir());
-		if (!(action == ACTION_ABORT) && is_directory(buf.buf)) {
+		if (!(options.action == ACTION_ABORT) && is_directory(buf.buf)) {
 			die(_("`rebase --preserve-merges` (-p) is no longer supported.\n"
 			"Use `git rebase --abort` to terminate current rebase.\n"
 			"Or downgrade to v2.33, or earlier, to complete the rebase."));
@@ -1201,7 +1201,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			"Note: Your `pull.rebase` configuration may also be set to 'preserve',\n"
 			"which is no longer supported; use 'merges' instead"));
 
-	if (action != ACTION_NONE && total_argc != 2) {
+	if (options.action != ACTION_NONE && total_argc != 2) {
 		usage_with_options(builtin_rebase_usage,
 				   builtin_rebase_options);
 	}
@@ -1232,11 +1232,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.root && options.fork_point > 0)
 		die(_("options '%s' and '%s' cannot be used together"), "--root", "--fork-point");
 
-	if (action != ACTION_NONE && !in_progress)
+	if (options.action != ACTION_NONE && !in_progress)
 		die(_("No rebase in progress?"));
 	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
 
-	if (action == ACTION_EDIT_TODO && !is_merge(&options))
+	if (options.action == ACTION_EDIT_TODO && !is_merge(&options))
 		die(_("The --edit-todo action can only be used during "
 		      "interactive rebase."));
 
@@ -1246,16 +1246,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		else if (exec.nr)
 			trace2_cmd_mode("interactive-exec");
 		else
-			trace2_cmd_mode(action_names[action]);
+			trace2_cmd_mode(action_names[options.action]);
 	}
 
-	switch (action) {
+	switch (options.action) {
 	case ACTION_CONTINUE: {
 		struct object_id head;
 		struct lock_file lock_file = LOCK_INIT;
 		int fd;
 
-		options.action = "continue";
 		/* Sanity check */
 		if (get_oid("HEAD", &head))
 			die(_("Cannot read HEAD"));
@@ -1281,7 +1280,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	case ACTION_SKIP: {
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
-		options.action = "skip";
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 		ropts.flags = RESET_HEAD_HARD;
@@ -1296,7 +1294,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 		struct strbuf head_msg = STRBUF_INIT;
 
-		options.action = "abort";
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 
@@ -1336,17 +1333,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		goto cleanup;
 	}
 	case ACTION_EDIT_TODO:
-		options.action = "edit-todo";
 		options.dont_finish_rebase = 1;
 		goto run_rebase;
 	case ACTION_SHOW_CURRENT_PATCH:
-		options.action = "show-current-patch";
 		options.dont_finish_rebase = 1;
 		goto run_rebase;
 	case ACTION_NONE:
 		break;
 	default:
-		BUG("action: %d", action);
+		BUG("action: %d", options.action);
 	}
 
 	/* Make sure no rebase is in progress */
@@ -1370,7 +1365,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||
-	    (action != ACTION_NONE) ||
+	    (options.action != ACTION_NONE) ||
 	    (exec.nr > 0) ||
 	    options.autosquash) {
 		allow_preemptive_ff = 0;
@@ -1812,7 +1807,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	options.revisions = revisions.buf;
 
 run_rebase:
-	ret = run_specific_rebase(&options, action);
+	ret = run_specific_rebase(&options);
 
 cleanup:
 	strbuf_release(&buf);
-- 
gitgitgadget
Junio C Hamano· Oct 12, 2022, 20:37 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH v3 0/8] rebase: make reflog messages independent of the backend

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 7 quoted lines
> This series fixes some bugs in the reflog messages when rebasing and changes
> the reflog messages of "rebase --apply" to match "rebase --merge" with the
> aim of making the reflog easier to parse.
>
> Thanks to everyone who commented on V2, I've added the review club
> participants that I've got address for to the cc list. I've rebased onto
> pw/rebase-keep-base-fixes.
Thanks for identifying the base clearly.

If "What's cooking" is correct, the base topic is waiting for an update? I'd prefer to see us build new stuff on top of solidified ground.

Phillip Wood· Oct 13, 2022, 08:44 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 0/8] rebase: make reflog messages independent of the backend

On 12/10/2022 21:37, Junio C Hamano wrote:
Show 15 quoted lines
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> This series fixes some bugs in the reflog messages when rebasing and changes
>> the reflog messages of "rebase --apply" to match "rebase --merge" with the
>> aim of making the reflog easier to parse.
>>
>> Thanks to everyone who commented on V2, I've added the review club
>> participants that I've got address for to the cc list. I've rebased onto
>> pw/rebase-keep-base-fixes.
> 
> Thanks for identifying the base clearly.
> 
> If "What's cooking" is correct, the base topic is waiting for an
> update?  I'd prefer to see us build new stuff on top of solidified
> ground.

Oh sorry, I thought I'd submitted a new version last week but it seems I pushed it to gitgitgadet, updated the cover letter but forgot to say "/submit". I'm hoping that will be the final version and provide a stable base for this series.

Best Wishes
Phillip
Junio C Hamano· Oct 13, 2022, 15:31 UTC · re: Phillip Wood · lore

Re: [PATCH v3 0/8] rebase: make reflog messages independent of the backend

Phillip Wood <phillip.wood123@gmail.com> writes:
> I pushed it to gitgitgadet, updated the cover letter but forgot to say
> "/submit". I'm hoping that will be the final version and provide a
> stable base for this series.
Thanks, will look forward to.
Phillip Wood via GitGitGadget· Oct 21, 2022, 09:21 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v4 0/8] rebase: make reflog messages independent of the backend

This series fixes some bugs in the reflog messages when rebasing and changes the reflog messages of "rebase --apply" to match "rebase --merge" with the aim of making the reflog easier to parse.

Thanks to Junio for his comments, here are the changes since V3:
 * Patch 1: Swapped with patch 2. Small change to fast-forward tests to make
   them pass
 * Patch 2: Swapped with patch 1. Reworded commit message and added a change
   to the fast-forward test to reflect the change in reflog message
   introduced by this patch
The rest of the patches are unchanged.
V2 Cover Letter

Thanks to everyone who commented on V2, I've added the review club participants that I've got address for to the cc list. I've rebased onto pw/rebase-keep-base-fixes.

Change since V2:
 * Patch 1: Reworded the commit message to address the concerns in [1,2]
   about the behavior when head_name is NULL. There is also a small change
   due to being rebased.
 * Patch 2: Unchanged. There wasn't much love for parameterized tests in
   review club but we want to ensure both backends produce the same messages
   I think this is the safest way to achieve that. Using separate tests
   makes it too easy to introduce subtle differences in the testing of the
   two backends.
 * Patch 3: Added a note to the commit message to address the concerns in
   [1] about not resetting GIT_REFLOG_ACTION when we return early.
 * Patches 4 & 5: Unchanged.
 * Patch 6: Reworded the commit message to make a stronger argument for this
   change. There are concerns about backwards compatibility in [1,3,4] but
   (i) we have made similar changes in the past without complaints and (ii)
   we're changing the message to an existing format. There is also a small
   change due to being rebased.
 * Patches 7 & 8: Small changes due to rebase.

[1] https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?pli=1#heading=h.t6g5l2t5ibzw [2] https://lore.kernel.org/git/xmqq35i7r4rj.fsf@gitster.g/ [3] https://lore.kernel.org/git/xmqq4k2nmmeg.fsf@gitster.g/ [4] https://lore.kernel.org/git/220420.865yn4833u.gmgdl@evledraar.gmail.com/

V2 Cover Letter:
Thanks to Christian and Elijah for their comments on V1.

I've updated commit message for patch 1 to try and be clearer about the removal of a call to strbuf_release() and spilt out the test changes from the old patch 2 into a separate preparatory patch.

V1 Cover Letter:

This is a series of rebase reflog related patches with the aim of unifying the reflog messages from the two rebase backends.

 * improve rebase reflog test coverage
 * rebase --merge: fix reflog messages for --continue and --skip
 * rebase --apply: respect GIT_REFLOG_ACTION
 * rebase --abort: improve reflog message
 * unify reflog messages between the two rebase backends
This series is based on pw/use-inprocess-checkout-in-rebase
Phillip Wood (8):
  t3406: rework rebase reflog tests
  rebase --apply: improve fast-forward reflog message
  rebase --merge: fix reflog when continuing
  rebase --merge: fix reflog message after skipping
  rebase --apply: respect GIT_REFLOG_ACTION
  rebase --apply: make reflog messages match rebase --merge
  rebase --abort: improve reflog message
  rebase: cleanup action handling
 builtin/rebase.c          | 146 ++++++++++++------------------
 sequencer.c               |   5 ++
 t/t3406-rebase-message.sh | 185 +++++++++++++++++++++++++++++++-------
 3 files changed, 215 insertions(+), 121 deletions(-)
base-commit: aa1df8146d70bb85c63b0999868fe29aebc1173e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1150%2Fphillipwood%2Fwip%2Frebase-reflog-fixes-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1150/phillipwood/wip/rebase-reflog-fixes-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1150
Range-diff vs v3:
 2:  b9255ad35d2 ! 1:  8d5ae067ce3 t3406: rework rebase reflog tests
     @@ Commit message
          the "merge" and "apply" backends. The test coverage for the "apply"
          backend now includes setting GIT_REFLOG_ACTION.
      
     -    Note that rebasing the "conflicts" branch does not create any
     -    conflicts yet. A commit to do that will be added in the next commit
     -    and the diff ends up smaller if we have don't rename the branch when
     -    it is added.
     +    Note that rebasing the "conflicts" branch does not create any conflicts
     +    yet. A commit to do that will be added shortly and the diff ends up
     +    smaller if we have don't rename the branch when it is added.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     @@ t/t3406-rebase-message.sh: test_expect_success 'error out early upon -C<n> or --
      +	) &&
      +
      +	git log -g --format=%gs -2 >actual &&
     ++	if test "$mode" = "--apply"
     ++	then
     ++		finish_msg="refs/heads/fast-forward onto $(git rev-parse main)"
     ++	else
     ++		finish_msg="returning to refs/heads/fast-forward"
     ++	fi &&
      +	write_reflog_expect <<-EOF &&
     -+	${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward
     ++	${reflog_action:-rebase} (finish): $finish_msg
      +	${reflog_action:-rebase} (start): checkout main
       	EOF
       	test_cmp expect actual &&
 1:  a84cf971a75 ! 2:  12701ef7c7c rebase --apply: remove duplicated code
     @@ Metadata
      Author: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## Commit message ##
     -    rebase --apply: remove duplicated code
     +    rebase --apply: improve fast-forward reflog message
      
     -    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. 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.
     +    Using move_to_original_branch() when reattaching HEAD after a
     +    fast-forward improves HEAD's reflog message when rebasing a branch. This
     +    is because it uses separate reflog messages for "HEAD" and
     +    "branch". HEAD's reflog will now record which branch we're returning to.
      
     -    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.
     +    When rebasing a detached HEAD there is no change in behavior. We
     +    currently call reset_head() when head_name is NULL but
     +    move_to_original_branch() does not. However the existing 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() skips the update.
      
     +    Note that the removal of "strbuf_reset(&msg)" is safe as there is a call
     +    to strbuf_release(&msf) just above this hunk which can be seen by
     +    viewing the diff with -U6.
     +
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## builtin/rebase.c ##
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       		ret = finish_rebase(&options);
       		goto cleanup;
       	}
     +
     + ## t/t3406-rebase-message.sh ##
     +@@ t/t3406-rebase-message.sh: test_reflog () {
     + 	) &&
     + 
     + 	git log -g --format=%gs -2 >actual &&
     +-	if test "$mode" = "--apply"
     +-	then
     +-		finish_msg="refs/heads/fast-forward onto $(git rev-parse main)"
     +-	else
     +-		finish_msg="returning to refs/heads/fast-forward"
     +-	fi &&
     + 	write_reflog_expect <<-EOF &&
     +-	${reflog_action:-rebase} (finish): $finish_msg
     ++	${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward
     + 	${reflog_action:-rebase} (start): checkout main
     + 	EOF
     + 	test_cmp expect actual &&
 3:  ea4da25a19c = 3:  2c965f4b97c rebase --merge: fix reflog when continuing
 4:  225ff4baef7 = 4:  17138d910f0 rebase --merge: fix reflog message after skipping
 5:  1094681eb11 = 5:  0c71c732904 rebase --apply: respect GIT_REFLOG_ACTION
 6:  a5338e6bdd8 = 6:  3f6b2e39f40 rebase --apply: make reflog messages match rebase --merge
 7:  aa808725fb8 = 7:  c8fa57f129d rebase --abort: improve reflog message
 8:  f9c8664b883 = 8:  ed800844ba1 rebase: cleanup action handling
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Oct 21, 2022, 09:21 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v4 1/8] t3406: rework rebase reflog tests

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Refactor the tests in preparation for adding more tests in the next few commits. The reworked tests use the same function for testing both the "merge" and "apply" backends. The test coverage for the "apply" backend now includes setting GIT_REFLOG_ACTION.

Note that rebasing the "conflicts" branch does not create any conflicts yet. A commit to do that will be added shortly and the diff ends up smaller if we have don't rename the branch when it is added.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3406-rebase-message.sh | 121 ++++++++++++++++++++++++++------------
 1 file changed, 82 insertions(+), 39 deletions(-)
Show changes to t/t3406-rebase-message.sh +82 −39
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index d17b450e811..413d6132ea2 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -10,10 +10,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 test_expect_success 'setup' '
 	test_commit O fileO &&
 	test_commit X fileX &&
+	git branch fast-forward &&
 	test_commit A fileA &&
 	test_commit B fileB &&
 	test_commit Y fileY &&
 
+	git checkout -b conflicts O &&
+	test_commit P &&
+	test_commit Q &&
+
 	git checkout -b topic O &&
 	git cherry-pick A B &&
 	test_commit Z fileZ &&
@@ -79,54 +84,92 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
 	test_i18ngrep "Invalid whitespace option" err
 '
 
-test_expect_success 'GIT_REFLOG_ACTION' '
-	git checkout start &&
-	test_commit reflog-onto &&
-	git checkout -b reflog-topic start &&
-	test_commit reflog-to-rebase &&
-
-	git rebase reflog-onto &&
-	git log -g --format=%gs -3 >actual &&
-	cat >expect <<-\EOF &&
-	rebase (finish): returning to refs/heads/reflog-topic
-	rebase (pick): reflog-to-rebase
-	rebase (start): checkout reflog-onto
+write_reflog_expect () {
+	if test $mode = --apply
+	then
+		sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//'
+	else
+		cat
+	fi >expect
+}
+
+test_reflog () {
+	mode=$1
+	reflog_action="$2"
+
+	test_expect_success "rebase $mode reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout conflicts &&
+	test_when_finished "git reset --hard Q" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		git rebase $mode main
+	) &&
+
+	git log -g --format=%gs -4 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
+	${reflog_action:-rebase} (pick): Q
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
 	EOF
 	test_cmp expect actual &&
 
-	git checkout -b reflog-prefix reflog-to-rebase &&
-	GIT_REFLOG_ACTION=change-the-reflog git rebase reflog-onto &&
-	git log -g --format=%gs -3 >actual &&
-	cat >expect <<-\EOF &&
-	change-the-reflog (finish): returning to refs/heads/reflog-prefix
-	change-the-reflog (pick): reflog-to-rebase
-	change-the-reflog (start): checkout reflog-onto
+	git log -g --format=%gs -1 conflicts >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): refs/heads/conflicts onto $(git rev-parse main)
 	EOF
-	test_cmp expect actual
-'
-
-test_expect_success 'rebase --apply reflog' '
-	git checkout -b reflog-apply start &&
-	old_head_reflog="$(git log -g --format=%gs -1 HEAD)" &&
-
-	git rebase --apply Y &&
+	test_cmp expect actual &&
 
-	git log -g --format=%gs -4 HEAD >actual &&
-	cat >expect <<-EOF &&
-	rebase finished: returning to refs/heads/reflog-apply
-	rebase: Z
-	rebase: checkout Y
-	$old_head_reflog
+	# check there is only one new entry in the branch reflog
+	test_cmp_rev conflicts@{1} Q
+	'
+
+	test_expect_success "rebase $mode fast-forward reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout fast-forward &&
+	test_when_finished "git reset --hard X" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		git rebase $mode main
+	) &&
+
+	git log -g --format=%gs -2 >actual &&
+	if test "$mode" = "--apply"
+	then
+		finish_msg="refs/heads/fast-forward onto $(git rev-parse main)"
+	else
+		finish_msg="returning to refs/heads/fast-forward"
+	fi &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): $finish_msg
+	${reflog_action:-rebase} (start): checkout main
 	EOF
 	test_cmp expect actual &&
 
-	git log -g --format=%gs -2 reflog-apply >actual &&
-	cat >expect <<-EOF &&
-	rebase finished: refs/heads/reflog-apply onto $(git rev-parse Y)
-	branch: Created from start
+	git log -g --format=%gs -1 fast-forward >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): refs/heads/fast-forward onto $(git rev-parse main)
 	EOF
-	test_cmp expect actual
-'
+	test_cmp expect actual &&
+
+	# check there is only one new entry in the branch reflog
+	test_cmp_rev fast-forward@{1} X
+	'
+}
+
+test_reflog --merge
+test_reflog --merge my-reflog-action
+test_reflog --apply
+test_reflog --apply my-reflog-action
 
 test_expect_success 'rebase -i onto unrelated history' '
 	git init unrelated &&
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Oct 21, 2022, 09:21 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v4 2/8] rebase --apply: improve fast-forward reflog message

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Using move_to_original_branch() when reattaching HEAD after a fast-forward improves HEAD's reflog message when rebasing a branch. This is because it uses separate reflog messages for "HEAD" and "branch". HEAD's reflog will now record which branch we're returning to.

When rebasing a detached HEAD there is no change in behavior. We currently call reset_head() when head_name is NULL but move_to_original_branch() does not. However the existing 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() skips the update.

Note that the removal of "strbuf_reset(&msg)" is safe as there is a call to strbuf_release(&msf) just above this hunk which can be seen by viewing the diff with -U6.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 11 +----------
 t/t3406-rebase-message.sh |  8 +-------
 2 files changed, 2 insertions(+), 17 deletions(-)
Show changes to 2 files +2 −17

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index ef520f66fb8..e67020b3586 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1811,19 +1811,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;
 	}
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 413d6132ea2..5253dd1551d 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -143,14 +143,8 @@ test_reflog () {
 	) &&
 
 	git log -g --format=%gs -2 >actual &&
-	if test "$mode" = "--apply"
-	then
-		finish_msg="refs/heads/fast-forward onto $(git rev-parse main)"
-	else
-		finish_msg="returning to refs/heads/fast-forward"
-	fi &&
 	write_reflog_expect <<-EOF &&
-	${reflog_action:-rebase} (finish): $finish_msg
+	${reflog_action:-rebase} (finish): returning to refs/heads/fast-forward
 	${reflog_action:-rebase} (start): checkout main
 	EOF
 	test_cmp expect actual &&
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Oct 21, 2022, 09:21 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v4 3/8] rebase --merge: fix reflog when continuing

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The reflog message for a conflict resolution committed by "rebase --continue" looks like

	rebase (continue): commit subject line
Unfortunately the reflog message each subsequent pick look like
	rebase (continue) (pick): commit subject line

Fix this by setting the reflog message for "rebase --continue" in sequencer_continue() so it does not affect subsequent commits. This introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION in pick_commits(). Both of these will be fixed in a future series that stops the sequencer calling setenv().

If we fail to commit the staged changes then we error out so GIT_REFLOG_ACTION does not need to be reset in that case.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 2 --
 sequencer.c               | 5 +++++
 t/t3406-rebase-message.sh | 9 +++++++--
 3 files changed, 12 insertions(+), 4 deletions(-)
Show changes to 3 files +12 −4

builtin/rebase.c, sequencer.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e67020b3586..414526f83a8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1271,8 +1271,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		int fd;
 
 		options.action = "continue";
-		set_reflog_action(&options);
-
 		/* Sanity check */
 		if (get_oid("HEAD", &head))
 			die(_("Cannot read HEAD"));
diff --git a/sequencer.c b/sequencer.c
index 61a8e0020d5..5790b35d763 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4785,6 +4785,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 	if (read_populate_opts(opts))
 		return -1;
 	if (is_rebase_i(opts)) {
+		char *previous_reflog_action;
+
 		if ((res = read_populate_todo(r, &todo_list, opts)))
 			goto release_todo_list;
 
@@ -4795,10 +4797,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 			unlink(rebase_path_dropped());
 		}
 
+		previous_reflog_action = xstrdup(getenv(GIT_REFLOG_ACTION));
+		setenv(GIT_REFLOG_ACTION, reflog_message(opts, "continue", NULL), 1);
 		if (commit_staged_changes(r, opts, &todo_list)) {
 			res = -1;
 			goto release_todo_list;
 		}
+		setenv(GIT_REFLOG_ACTION, previous_reflog_action, 1);
 	} else if (!file_exists(get_todo_path(opts)))
 		return continue_single_pick(r, opts);
 	else if ((res = read_populate_todo(r, &todo_list, opts)))
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 5253dd1551d..3ca2fbb0d59 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -17,6 +17,7 @@ test_expect_success 'setup' '
 
 	git checkout -b conflicts O &&
 	test_commit P &&
+	test_commit conflict-X fileX &&
 	test_commit Q &&
 
 	git checkout -b topic O &&
@@ -107,13 +108,17 @@ test_reflog () {
 			GIT_REFLOG_ACTION="$reflog_action" &&
 			export GIT_REFLOG_ACTION
 		fi &&
-		git rebase $mode main
+		test_must_fail git rebase $mode main &&
+		echo resolved >fileX &&
+		git add fileX &&
+		git rebase --continue
 	) &&
 
-	git log -g --format=%gs -4 >actual &&
+	git log -g --format=%gs -5 >actual &&
 	write_reflog_expect <<-EOF &&
 	${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
 	${reflog_action:-rebase} (pick): Q
+	${reflog_action:-rebase} (continue): conflict-X
 	${reflog_action:-rebase} (pick): P
 	${reflog_action:-rebase} (start): checkout main
 	EOF
-- 
gitgitgadget
Junio C Hamano· Oct 21, 2022, 17:37 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH v4 3/8] rebase --merge: fix reflog when continuing

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> ... This
> introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION
> in pick_commits().

Is it just the matter of freeing previous_reflog_action after you call setenv(), or does it take much more involved changes?

> Both of these will be fixed in a future series that
> stops the sequencer calling setenv().

If it gets fixed in a future step in the same series, that is a different matter, but if it is easy enough not to deliberately introduce a new leak, we'd prefer to do so.

Phillip Wood· Oct 25, 2022, 10:08 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 3/8] rebase --merge: fix reflog when continuing

Hi Junio
On 21/10/2022 18:37, Junio C Hamano wrote:
Show 8 quoted lines
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> ... This
>> introduces a memory leak similar to the one leaking GIT_REFLOG_ACTION
>> in pick_commits().
> 
> Is it just the matter of freeing previous_reflog_action after you
> call setenv(), or does it take much more involved changes?

We should be freeing previous_reflog_action - I had misremembered a previous discussion about setenv() but the manual page makes it quite clear that it copies the strings passed to it. However we call setenv() each time we pick a commit and that leaks the previous value. The solution is to avoid calling setenv() at all and instead use the env member of struct child_process when we run "git commit".

Show 6 quoted lines
>> Both of these will be fixed in a future series that
>> stops the sequencer calling setenv().
> 
> If it gets fixed in a future step in the same series, that is a
> different matter, but if it is easy enough not to deliberately
> introduce a new leak, we'd prefer to do so.

It's a couple of patches to fix which are more or less finished, I'm planning to send them once this series is in next.

Best Wishes
Phillip
Junio C Hamano· Oct 25, 2022, 16:11 UTC · re: Phillip Wood · lore

Re: [PATCH v4 3/8] rebase --merge: fix reflog when continuing

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 8 quoted lines
>>> Both of these will be fixed in a future series that
>>> stops the sequencer calling setenv().
>> If it gets fixed in a future step in the same series, that is a
>> different matter, but if it is easy enough not to deliberately
>> introduce a new leak, we'd prefer to do so.
>
> It's a couple of patches to fix which are more or less finished, I'm
> planning to send them once this series is in next.

So we will do the "add a known breakage of the same kind as there exists others, and then later fix them all up, including the one that is added by this series, because fixes are non-trivial and this topic is easier to finish if we allowed to add a known breakage" approach? Just making sure it is what you plan to do.

Thanks.
Phillip Wood· Oct 26, 2022, 15:17 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 3/8] rebase --merge: fix reflog when continuing

Hi Junio
On 25/10/2022 17:11, Junio C Hamano wrote:
Show 16 quoted lines
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>>> Both of these will be fixed in a future series that
>>>> stops the sequencer calling setenv().
>>> If it gets fixed in a future step in the same series, that is a
>>> different matter, but if it is easy enough not to deliberately
>>> introduce a new leak, we'd prefer to do so.
>>
>> It's a couple of patches to fix which are more or less finished, I'm
>> planning to send them once this series is in next.
> 
> So we will do the "add a known breakage of the same kind as there
> exists others, and then later fix them all up, including the one
> that is added by this series, because fixes are non-trivial and this
> topic is easier to finish if we allowed to add a known breakage"
> approach?  Just making sure it is what you plan to do.
Yes, that's right
Thanks
Phillip
> Thanks.
Junio C Hamano· Oct 26, 2022, 16:55 UTC · re: Phillip Wood · lore

Re: [PATCH v4 3/8] rebase --merge: fix reflog when continuing

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 20 quoted lines
> Hi Junio
>
> On 25/10/2022 17:11, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>> 
>>>>> Both of these will be fixed in a future series that
>>>>> stops the sequencer calling setenv().
>>>> If it gets fixed in a future step in the same series, that is a
>>>> different matter, but if it is easy enough not to deliberately
>>>> introduce a new leak, we'd prefer to do so.
>>>
>>> It's a couple of patches to fix which are more or less finished, I'm
>>> planning to send them once this series is in next.
>> So we will do the "add a known breakage of the same kind as there
>> exists others, and then later fix them all up, including the one
>> that is added by this series, because fixes are non-trivial and this
>> topic is easier to finish if we allowed to add a known breakage"
>> approach?  Just making sure it is what you plan to do.
>
> Yes, that's right

OK, I do not mind as long as we leave a NEEDSWORK note to tell others that we know the leak and promise to fix it soon (so they do not waste their effort to fix it independently).

Thanks.
Phillip Wood via GitGitGadget· Oct 21, 2022, 09:21 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v4 4/8] rebase --merge: fix reflog message after skipping

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The reflog message for every pick after running "rebase --skip" looks like

	rebase (skip) (pick): commit subject line
Fix this by not appending " (skip)" to the reflog action.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          |  2 --
 t/t3406-rebase-message.sh | 24 ++++++++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)
Show changes to 2 files +24 −2

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 414526f83a8..c1e68173b5f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1297,8 +1297,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
 		options.action = "skip";
-		set_reflog_action(&options);
-
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 		ropts.flags = RESET_HEAD_HARD;
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 3ca2fbb0d59..8aa6a79acc1 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -163,6 +163,30 @@ test_reflog () {
 	# check there is only one new entry in the branch reflog
 	test_cmp_rev fast-forward@{1} X
 	'
+
+	test_expect_success "rebase $mode --skip reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout conflicts &&
+	test_when_finished "git reset --hard Q" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		test_must_fail git rebase $mode main &&
+		git rebase --skip
+	) &&
+
+	git log -g --format=%gs -4 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (finish): returning to refs/heads/conflicts
+	${reflog_action:-rebase} (pick): Q
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
+	EOF
+	test_cmp expect actual
+	'
 }
 
 test_reflog --merge
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Oct 21, 2022, 09:21 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v4 5/8] rebase --apply: respect GIT_REFLOG_ACTION

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The reflog messages when finishing a rebase hard code "rebase" rather than using GIT_REFLOG_ACTION.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 7 ++++---
 t/t3406-rebase-message.sh | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)
Show changes to 2 files +5 −4

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index c1e68173b5f..ea246c6bb3a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -582,10 +582,11 @@ static int move_to_original_branch(struct rebase_options *opts)
 	if (!opts->onto)
 		BUG("move_to_original_branch without onto");
 
-	strbuf_addf(&branch_reflog, "rebase finished: %s onto %s",
+	strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
+		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
-	strbuf_addf(&head_reflog, "rebase finished: returning to %s",
-		    opts->head_name);
+	strbuf_addf(&head_reflog, "%s finished: returning to %s",
+		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
 	ropts.branch = opts->head_name;
 	ropts.flags = RESET_HEAD_REFS_ONLY;
 	ropts.branch_msg = branch_reflog.buf;
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 8aa6a79acc1..bb2a4949abc 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
 write_reflog_expect () {
 	if test $mode = --apply
 	then
-		sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//'
+		sed 's/(finish)/finished/; s/ ([^)]*)//'
 	else
 		cat
 	fi >expect
-- 
gitgitgadget
Junio C Hamano· Oct 21, 2022, 17:38 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH v4 5/8] rebase --apply: respect GIT_REFLOG_ACTION

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The reflog messages when finishing a rebase hard code "rebase" rather
> than using GIT_REFLOG_ACTION.
Makes sense.
Show 39 quoted lines
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c          | 7 ++++---
>  t/t3406-rebase-message.sh | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index c1e68173b5f..ea246c6bb3a 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -582,10 +582,11 @@ static int move_to_original_branch(struct rebase_options *opts)
>  	if (!opts->onto)
>  		BUG("move_to_original_branch without onto");
>  
> -	strbuf_addf(&branch_reflog, "rebase finished: %s onto %s",
> +	strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
> +		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
>  		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
> -	strbuf_addf(&head_reflog, "rebase finished: returning to %s",
> -		    opts->head_name);
> +	strbuf_addf(&head_reflog, "%s finished: returning to %s",
> +		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
>  	ropts.branch = opts->head_name;
>  	ropts.flags = RESET_HEAD_REFS_ONLY;
>  	ropts.branch_msg = branch_reflog.buf;
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 8aa6a79acc1..bb2a4949abc 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
>  write_reflog_expect () {
>  	if test $mode = --apply
>  	then
> -		sed 's/.*(finish)/rebase finished/; s/ ([^)]*)//'
> +		sed 's/(finish)/finished/; s/ ([^)]*)//'
>  	else
>  		cat
>  	fi >expect
Phillip Wood via GitGitGadget· Oct 21, 2022, 09:21 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v4 6/8] rebase --apply: make reflog messages match rebase --merge

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The apply backend creates slightly different reflog messages to the merge backend when starting or finishing a rebase and when picking commits. These differences make it harder than it needs to be to parse the reflog (I have a script that reads the finishing messages from rebase and it is a pain to have to accommodate two different message formats). While it is possible to determine the backend used for a rebase from the reflog messages, the differences are not designed for that purpose. c2417d3af7 (rebase: drop '-i' from the reflog for interactive-based rebases, 2020-02-15) removed the clear distinction between the reflog messages of the two backends without complaint.

As the merge backend is the default it is likely to be the format most common in existing reflogs. For that reason the apply backend is changed to format its reflog messages to match the merge backend as closely as possible. Note that there is still a difference as when committing a conflict resolution the apply backend will use "(pick)" rather than "(continue)" because it is not currently possible to change the message for a single commit.

In addition to c2417d3af7 we also changed the reflog messages in 68aa495b59 (rebase: implement --merge via the interactive machinery, 2018-12-11) and 2ac0d6273f (rebase: change the default backend from "am" to "merge", 2020-02-15). This commit makes the same change to "git rebase --apply" that 2ac0d6273f made to "git rebase" without any backend specific options. As the messages are changed to use an existing format any scripts that can parse the reflog messages of the default rebase backend should be unaffected by this change.

There are existing tests for the messages from both backends which are adjusted to ensure that they do not get out of sync in the future.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 9 +++++----
 t/t3406-rebase-message.sh | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)
Show changes to 2 files +6 −5

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index ea246c6bb3a..5253ba66861 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -582,10 +582,10 @@ static int move_to_original_branch(struct rebase_options *opts)
 	if (!opts->onto)
 		BUG("move_to_original_branch without onto");
 
-	strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
+	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
-	strbuf_addf(&head_reflog, "%s finished: returning to %s",
+	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
 	ropts.branch = opts->head_name;
 	ropts.flags = RESET_HEAD_REFS_ONLY;
@@ -615,7 +615,8 @@ static int run_am(struct rebase_options *opts)
 
 	am.git_cmd = 1;
 	strvec_push(&am.args, "am");
-
+	strvec_pushf(&am.env, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
+		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
 	if (opts->action && !strcmp("continue", opts->action)) {
 		strvec_push(&am.args, "--resolved");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
@@ -1792,7 +1793,7 @@ 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",
+	strbuf_addf(&msg, "%s (start): checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
 	ropts.oid = &options.onto->object.oid;
 	ropts.orig_head = &options.orig_head->object.oid,
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index bb2a4949abc..5c6cd9af3bc 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
 write_reflog_expect () {
 	if test $mode = --apply
 	then
-		sed 's/(finish)/finished/; s/ ([^)]*)//'
+		sed 's/(continue)/(pick)/'
 	else
 		cat
 	fi >expect
-- 
gitgitgadget
Junio C Hamano· Oct 21, 2022, 17:39 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH v4 6/8] rebase --apply: make reflog messages match rebase --merge

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 7 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The apply backend creates slightly different reflog messages to the
> merge backend when starting or finishing a rebase and when picking
> commits. ...
> There are existing tests for the messages from both backends which are
> adjusted to ensure that they do not get out of sync in the future.
Nice.
Phillip Wood via GitGitGadget· Oct 21, 2022, 09:21 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v4 7/8] rebase --abort: improve reflog message

From: Phillip Wood <phillip.wood@dunelm.org.uk>
When aborting a rebase the reflog message looks like
	rebase (abort): updating HEAD

which is not very informative. Improve the message by mentioning the branch that we are returning to as we do at the end of a successful rebase so it looks like.

	rebase (abort): returning to refs/heads/topic

If GIT_REFLOG_ACTION is set in the environment we no longer omit "(abort)" from the reflog message. We don't omit "(start)" and "(finish)" when starting and finishing a rebase in that case so we shouldn't omit "(abort)".

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c          | 28 ++++++---------------
 t/t3406-rebase-message.sh | 51 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 20 deletions(-)
Show changes to 2 files +59 −20

builtin/rebase.c, t/t3406-rebase-message.sh

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5253ba66861..d63d77757e8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1001,23 +1001,6 @@ static void NORETURN error_on_missing_default_upstream(void)
 	exit(1);
 }
 
-static void set_reflog_action(struct rebase_options *options)
-{
-	const char *env;
-	struct strbuf buf = STRBUF_INIT;
-
-	if (!is_merge(options))
-		return;
-
-	env = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
-	if (env && strcmp("rebase", env))
-		return; /* only override it if it is "rebase" */
-
-	strbuf_addf(&buf, "rebase (%s)", options->action);
-	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, buf.buf, 1);
-	strbuf_release(&buf);
-}
-
 static int check_exec_cmd(const char *cmd)
 {
 	if (strchr(cmd, '\n'))
@@ -1311,18 +1294,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 	case ACTION_ABORT: {
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
-		options.action = "abort";
-		set_reflog_action(&options);
+		struct strbuf head_msg = STRBUF_INIT;
 
+		options.action = "abort";
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 
 		if (read_basic_state(&options))
 			exit(1);
+
+		strbuf_addf(&head_msg, "%s (abort): returning to %s",
+			    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
+			    options.head_name ? options.head_name
+					      : oid_to_hex(&options.orig_head->object.oid));
 		ropts.oid = &options.orig_head->object.oid;
+		ropts.head_msg = head_msg.buf;
 		ropts.branch = options.head_name;
 		ropts.flags = RESET_HEAD_HARD;
-		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
 		if (reset_head(the_repository, &ropts) < 0)
 			die(_("could not move back to %s"),
 			    oid_to_hex(&options.orig_head->object.oid));
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 5c6cd9af3bc..ceca1600053 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -187,6 +187,57 @@ test_reflog () {
 	EOF
 	test_cmp expect actual
 	'
+
+	test_expect_success "rebase $mode --abort reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout conflicts &&
+	test_when_finished "git reset --hard Q" &&
+
+	git log -g -1 conflicts >branch-expect &&
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		test_must_fail git rebase $mode main &&
+		git rebase --abort
+	) &&
+
+	git log -g --format=%gs -3 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (abort): returning to refs/heads/conflicts
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
+	EOF
+	test_cmp expect actual &&
+
+	# check branch reflog is unchanged
+	git log -g -1 conflicts >branch-actual &&
+	test_cmp branch-expect branch-actual
+	'
+
+	test_expect_success "rebase $mode --abort detached HEAD reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
+	git checkout Q &&
+	test_when_finished "git reset --hard Q" &&
+
+	(
+		if test -n "$reflog_action"
+		then
+			GIT_REFLOG_ACTION="$reflog_action" &&
+			export GIT_REFLOG_ACTION
+		fi &&
+		test_must_fail git rebase $mode main &&
+		git rebase --abort
+	) &&
+
+	git log -g --format=%gs -3 >actual &&
+	write_reflog_expect <<-EOF &&
+	${reflog_action:-rebase} (abort): returning to $(git rev-parse Q)
+	${reflog_action:-rebase} (pick): P
+	${reflog_action:-rebase} (start): checkout main
+	EOF
+	test_cmp expect actual
+	'
 }
 
 test_reflog --merge
-- 
gitgitgadget
Junio C Hamano· Oct 21, 2022, 17:44 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH v4 7/8] rebase --abort: improve reflog message

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 16 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When aborting a rebase the reflog message looks like
>
> 	rebase (abort): updating HEAD
>
> which is not very informative. Improve the message by mentioning the
> branch that we are returning to as we do at the end of a successful
> rebase so it looks like.
>
> 	rebase (abort): returning to refs/heads/topic
>
> If GIT_REFLOG_ACTION is set in the environment we no longer omit
> "(abort)" from the reflog message. We don't omit "(start)" and
> "(finish)" when starting and finishing a rebase in that case so we
> shouldn't omit "(abort)".
Super nice.
Phillip Wood via GitGitGadget· Oct 21, 2022, 09:21 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v4 8/8] rebase: cleanup action handling

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Treating the action as a string is a hang over from the scripted rebase. The last commit removed the only remaining use of the action that required a string so lets convert the other action users to use the existing action enum instead. If we ever need the action name as a string in the future the action_names array exists exactly for that purpose.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 93 +++++++++++++++++++++++-------------------------
 1 file changed, 44 insertions(+), 49 deletions(-)
Show changes to builtin/rebase.c +44 −49
diff --git a/builtin/rebase.c b/builtin/rebase.c
index d63d77757e8..d5e684a7cec 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -59,6 +59,26 @@ enum empty_type {
 	EMPTY_ASK
 };
 
+enum action {
+	ACTION_NONE = 0,
+	ACTION_CONTINUE,
+	ACTION_SKIP,
+	ACTION_ABORT,
+	ACTION_QUIT,
+	ACTION_EDIT_TODO,
+	ACTION_SHOW_CURRENT_PATCH
+};
+
+static const char *action_names[] = {
+	"undefined",
+	"continue",
+	"skip",
+	"abort",
+	"quit",
+	"edit_todo",
+	"show_current_patch"
+};
+
 struct rebase_options {
 	enum rebase_type type;
 	enum empty_type empty;
@@ -85,7 +105,7 @@ struct rebase_options {
 		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
 	} flags;
 	struct strvec git_am_opts;
-	const char *action;
+	enum action action;
 	int signoff;
 	int allow_rerere_autoupdate;
 	int keep_empty;
@@ -156,24 +176,6 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	return replay;
 }
 
-enum action {
-	ACTION_NONE = 0,
-	ACTION_CONTINUE,
-	ACTION_SKIP,
-	ACTION_ABORT,
-	ACTION_QUIT,
-	ACTION_EDIT_TODO,
-	ACTION_SHOW_CURRENT_PATCH
-};
-
-static const char *action_names[] = { "undefined",
-				      "continue",
-				      "skip",
-				      "abort",
-				      "quit",
-				      "edit_todo",
-				      "show_current_patch" };
-
 static int edit_todo_file(unsigned flags)
 {
 	const char *todo_file = rebase_path_todo();
@@ -310,8 +312,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	return ret;
 }
 
-static int run_sequencer_rebase(struct rebase_options *opts,
-				  enum action command)
+static int run_sequencer_rebase(struct rebase_options *opts)
 {
 	unsigned flags = 0;
 	int abbreviate_commands = 0, ret = 0;
@@ -326,7 +327,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 	flags |= opts->reapply_cherry_picks ? TODO_LIST_REAPPLY_CHERRY_PICKS : 0;
 	flags |= opts->flags & REBASE_NO_QUIET ? TODO_LIST_WARN_SKIPPED_CHERRY_PICKS : 0;
 
-	switch (command) {
+	switch (opts->action) {
 	case ACTION_NONE: {
 		if (!opts->onto && !opts->upstream)
 			die(_("a base commit must be provided with --upstream or --onto"));
@@ -359,7 +360,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 		break;
 	}
 	default:
-		BUG("invalid command '%d'", command);
+		BUG("invalid command '%d'", opts->action);
 	}
 
 	return ret;
@@ -617,7 +618,7 @@ static int run_am(struct rebase_options *opts)
 	strvec_push(&am.args, "am");
 	strvec_pushf(&am.env, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
 		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
-	if (opts->action && !strcmp("continue", opts->action)) {
+	if (opts->action == ACTION_CONTINUE) {
 		strvec_push(&am.args, "--resolved");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
 		if (opts->gpg_sign_opt)
@@ -628,7 +629,7 @@ static int run_am(struct rebase_options *opts)
 
 		return move_to_original_branch(opts);
 	}
-	if (opts->action && !strcmp("skip", opts->action)) {
+	if (opts->action == ACTION_SKIP) {
 		strvec_push(&am.args, "--skip");
 		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
 		status = run_command(&am);
@@ -637,7 +638,7 @@ static int run_am(struct rebase_options *opts)
 
 		return move_to_original_branch(opts);
 	}
-	if (opts->action && !strcmp("show-current-patch", opts->action)) {
+	if (opts->action == ACTION_SHOW_CURRENT_PATCH) {
 		strvec_push(&am.args, "--show-current-patch");
 		return run_command(&am);
 	}
@@ -730,7 +731,7 @@ static int run_am(struct rebase_options *opts)
 	return status;
 }
 
-static int run_specific_rebase(struct rebase_options *opts, enum action action)
+static int run_specific_rebase(struct rebase_options *opts)
 {
 	int status;
 
@@ -748,7 +749,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
 			opts->gpg_sign_opt = tmp;
 		}
 
-		status = run_sequencer_rebase(opts, action);
+		status = run_sequencer_rebase(opts);
 	} else if (opts->type == REBASE_APPLY)
 		status = run_am(opts);
 	else
@@ -1025,7 +1026,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct strbuf buf = STRBUF_INIT;
 	struct object_id branch_base;
 	int ignore_whitespace = 0;
-	enum action action = ACTION_NONE;
 	const char *gpg_sign = NULL;
 	struct string_list exec = STRING_LIST_INIT_NODUP;
 	const char *rebase_merges = NULL;
@@ -1074,18 +1074,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "no-ff", &options.flags,
 			N_("cherry-pick all commits, even if unchanged"),
 			REBASE_FORCE),
-		OPT_CMDMODE(0, "continue", &action, N_("continue"),
+		OPT_CMDMODE(0, "continue", &options.action, N_("continue"),
 			    ACTION_CONTINUE),
-		OPT_CMDMODE(0, "skip", &action,
+		OPT_CMDMODE(0, "skip", &options.action,
 			    N_("skip current patch and continue"), ACTION_SKIP),
-		OPT_CMDMODE(0, "abort", &action,
+		OPT_CMDMODE(0, "abort", &options.action,
 			    N_("abort and check out the original branch"),
 			    ACTION_ABORT),
-		OPT_CMDMODE(0, "quit", &action,
+		OPT_CMDMODE(0, "quit", &options.action,
 			    N_("abort but keep HEAD where it is"), ACTION_QUIT),
-		OPT_CMDMODE(0, "edit-todo", &action, N_("edit the todo list "
+		OPT_CMDMODE(0, "edit-todo", &options.action, N_("edit the todo list "
 			    "during an interactive rebase"), ACTION_EDIT_TODO),
-		OPT_CMDMODE(0, "show-current-patch", &action,
+		OPT_CMDMODE(0, "show-current-patch", &options.action,
 			    N_("show the patch file being applied or merged"),
 			    ACTION_SHOW_CURRENT_PATCH),
 		OPT_CALLBACK_F(0, "apply", &options, NULL,
@@ -1174,7 +1174,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	} else if (is_directory(merge_dir())) {
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "%s/rewritten", merge_dir());
-		if (!(action == ACTION_ABORT) && is_directory(buf.buf)) {
+		if (!(options.action == ACTION_ABORT) && is_directory(buf.buf)) {
 			die(_("`rebase --preserve-merges` (-p) is no longer supported.\n"
 			"Use `git rebase --abort` to terminate current rebase.\n"
 			"Or downgrade to v2.33, or earlier, to complete the rebase."));
@@ -1201,7 +1201,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			"Note: Your `pull.rebase` configuration may also be set to 'preserve',\n"
 			"which is no longer supported; use 'merges' instead"));
 
-	if (action != ACTION_NONE && total_argc != 2) {
+	if (options.action != ACTION_NONE && total_argc != 2) {
 		usage_with_options(builtin_rebase_usage,
 				   builtin_rebase_options);
 	}
@@ -1232,11 +1232,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.root && options.fork_point > 0)
 		die(_("options '%s' and '%s' cannot be used together"), "--root", "--fork-point");
 
-	if (action != ACTION_NONE && !in_progress)
+	if (options.action != ACTION_NONE && !in_progress)
 		die(_("No rebase in progress?"));
 	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
 
-	if (action == ACTION_EDIT_TODO && !is_merge(&options))
+	if (options.action == ACTION_EDIT_TODO && !is_merge(&options))
 		die(_("The --edit-todo action can only be used during "
 		      "interactive rebase."));
 
@@ -1246,16 +1246,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		else if (exec.nr)
 			trace2_cmd_mode("interactive-exec");
 		else
-			trace2_cmd_mode(action_names[action]);
+			trace2_cmd_mode(action_names[options.action]);
 	}
 
-	switch (action) {
+	switch (options.action) {
 	case ACTION_CONTINUE: {
 		struct object_id head;
 		struct lock_file lock_file = LOCK_INIT;
 		int fd;
 
-		options.action = "continue";
 		/* Sanity check */
 		if (get_oid("HEAD", &head))
 			die(_("Cannot read HEAD"));
@@ -1281,7 +1280,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	case ACTION_SKIP: {
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
-		options.action = "skip";
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 		ropts.flags = RESET_HEAD_HARD;
@@ -1296,7 +1294,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
 		struct strbuf head_msg = STRBUF_INIT;
 
-		options.action = "abort";
 		rerere_clear(the_repository, &merge_rr);
 		string_list_clear(&merge_rr, 1);
 
@@ -1336,17 +1333,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		goto cleanup;
 	}
 	case ACTION_EDIT_TODO:
-		options.action = "edit-todo";
 		options.dont_finish_rebase = 1;
 		goto run_rebase;
 	case ACTION_SHOW_CURRENT_PATCH:
-		options.action = "show-current-patch";
 		options.dont_finish_rebase = 1;
 		goto run_rebase;
 	case ACTION_NONE:
 		break;
 	default:
-		BUG("action: %d", action);
+		BUG("action: %d", options.action);
 	}
 
 	/* Make sure no rebase is in progress */
@@ -1370,7 +1365,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||
-	    (action != ACTION_NONE) ||
+	    (options.action != ACTION_NONE) ||
 	    (exec.nr > 0) ||
 	    options.autosquash) {
 		allow_preemptive_ff = 0;
@@ -1815,7 +1810,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	options.revisions = revisions.buf;
 
 run_rebase:
-	ret = run_specific_rebase(&options, action);
+	ret = run_specific_rebase(&options);
 
 cleanup:
 	strbuf_release(&buf);
-- 
gitgitgadget
Junio C Hamano· Oct 21, 2022, 17:54 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH v4 8/8] rebase: cleanup action handling

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 8 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Treating the action as a string is a hang over from the scripted
> rebase. The last commit removed the only remaining use of the action
> that required a string so lets convert the other action users to use
> the existing action enum instead. If we ever need the action name as a
> string in the future the action_names array exists exactly for that
> purpose.
Nice.
#leftoverbit

Perhaps a clean-up worth making after the dust settles from this series would be to use designated initialisers to avoid names and their string values going out of sync, perhaps like

	static const char *action_names[] = {
		[ACTION_NONE] = "undefined",
		[ACTION_CONTINUE] = "continue",
		...
		[ACTION_SHOW_CURRENT_PATCH] = "show_current_patch",
	};

Unless the final element is something that must stay at the end even when adding new member to a collection, it is a good idea to keep a (seemingly unnecessary) comma at the end. That would make it easier to add a new member without unnecessary patch noise.

← back to recent threads