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

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

[Phillip Wood via GitGitGadget]

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Christian Couder]

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.

> 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!

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

[Elijah Newren]

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

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Christian Couder]

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.


>  3 files changed, 86 insertions(+), 41 deletions(-)

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

[Phillip Wood]

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.

Best Wishes

Phillip

> 
>>   3 files changed, 86 insertions(+), 41 deletions(-)

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

[Elijah Newren]

On Fri, Apr 15, 2022 at 5:23 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> 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.

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Elijah Newren]

On Mon, Feb 21, 2022 at 3:19 PM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Christian Couder]

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" ...

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

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

[Phillip Wood]

Hi Christian

On 07/04/2022 14:59, Christian Couder wrote:
> 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

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

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

[Phillip Wood via GitGitGadget]

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
-- 
gitgitgadget

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

[Elijah Newren]

On Mon, Feb 21, 2022 at 3:19 PM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.

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

>         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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Elijah Newren]

On Mon, Feb 21, 2022 at 3:19 PM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.

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

> +
> +       # 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.

[PATCH 7/7] rebase: cleanup action handling

[Phillip Wood via GitGitGadget]

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

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

[Elijah Newren]

On Mon, Feb 21, 2022 at 9:35 PM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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!

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

[Phillip Wood]

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:
> 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

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

[Elijah Newren]

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.


> 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

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

[Phillip Wood]

Hi Elijah

On 17/04/2022 03:13, Elijah Newren wrote:
> 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

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

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

[Christian Couder]

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.

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

[Elijah Newren]

On Mon, Feb 21, 2022 at 9:04 PM 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.
>
>  * 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.

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

[Phillip Wood via GitGitGadget]

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 20 2022, Phillip Wood via GitGitGadget 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. The old code appears to handle the case where the rebase is
> started from a detached HEAD but in fact in that case there is nothing
> to do as we have already updated HEAD.
>
> Note that the removal of "strbuf_release(&msg)" is safe as there is an
> identical call just above this hunk which can be seen by viewing the
> diff with -U6.

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

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

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

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

I.e. the "ropts" earlier in the function wasn't shared with the code
below, we'd "goto" past it...
>
> Signed-off-by: Phillip Wood <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;
>  	}

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

[Junio C Hamano]

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

> -	strbuf_reset(&msg);

This is unnecessary, because we have released immediately before.

> 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?

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "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".

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Junio C Hamano]

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

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

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

> @@ -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.

> +	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?

> +	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.

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

> +	'
> +
> +	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.

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

> +	'
> +}
> +
> +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.

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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
-- 
gitgitgadget

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Junio C Hamano]

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

> 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).

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

> +		     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.

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

[Phillip Wood via GitGitGadget]

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Junio C Hamano]

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Use move_to_original_branch() when reattaching HEAD after a fast-forward
> rather than open coding a copy of that code. move_to_original_branch()
> does not call reset_head() if head_name is NULL but there should be no
> user visible changes even though we currently call reset_head() in that
> case.

move_to_original_branch() uses both .head_msg and .branch_msg and
uses different messages for them, but the original code below only
feeds .head_msg while .branch_msg leaves NULL, which leads
reset.c::update_refs() to use the same message as .head_msg when it
wants to use .branch_msg (i.e. the message recorded in the reflog of
the branch).  

Doesn't this difference result in a different behaviour?

> The reason for this is that the reset_head() call does not add a
> message to the reflog because we're not changing the commit that HEAD
> points to and so lock_ref_for_update() elides the update. When head_name
> is not NULL then reset_head() behaves like "git symbolic-ref" and so the
> reflog is updated.

> Note that the removal of "strbuf_release(&msg)" is safe as there is an

The patch is removing strbuf_reset(), not _release(), here, though.

We have released already so there is no point to reset it again, so
the removal is still safe.

> identical call just above this hunk which can be seen by viewing the
> diff with -U6.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index a2ca66b54be..51accb4fd61 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1808,19 +1808,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	 * If the onto is a proper descendant of the tip of the branch, then
>  	 * we just fast-forwarded.
>  	 */
> -	strbuf_reset(&msg);
>  	if (oideq(&branch_base, &options.orig_head->object.oid)) {
>  		printf(_("Fast-forwarded %s to %s.\n"),
>  			branch_name, options.onto_name);
> -		strbuf_addf(&msg, "rebase finished: %s onto %s",
> -			options.head_name ? options.head_name : "detached HEAD",
> -			oid_to_hex(&options.onto->object.oid));
> -		memset(&ropts, 0, sizeof(ropts));
> -		ropts.branch = options.head_name;
> -		ropts.flags = RESET_HEAD_REFS_ONLY;
> -		ropts.head_msg = msg.buf;
> -		reset_head(the_repository, &ropts);
> -		strbuf_release(&msg);
> +		move_to_original_branch(&options);
>  		ret = finish_rebase(&options);
>  		goto cleanup;
>  	}

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

[Junio C Hamano]

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?

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.

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

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

[Phillip Wood]

Hi Junio

On 13/10/2022 19:13, Junio C Hamano wrote:
> 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

> 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

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Junio C Hamano]

"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.

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

[Phillip Wood]

On 12/10/2022 21:37, Junio C Hamano wrote:
> "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

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

[Junio C Hamano]

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.

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

[Phillip Wood via GitGitGadget]

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Junio C Hamano]

"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.

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

[Phillip Wood]

Hi Junio

On 21/10/2022 18:37, Junio C Hamano wrote:
> "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".

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

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

[Junio C Hamano]

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.

Thanks.

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

[Phillip Wood]

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

Thanks

Phillip

> Thanks.

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Junio C Hamano]

"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.

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Junio C Hamano]

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

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Junio C Hamano]

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

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Junio C Hamano]

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

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