diff mbox series

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

Message ID 493254ffbb8b11f325f5995790341d70198b5c97.1645441854.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series rebase: make reflog messages independent of the backend | expand

Commit Message

Phillip Wood Feb. 21, 2022, 11:10 a.m. UTC
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(-)

Comments

Christian Couder April 7, 2022, 1:49 p.m. UTC | #1
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(-)
Phillip Wood April 15, 2022, 2 p.m. UTC | #2
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(-)
Elijah Newren April 17, 2022, 1:57 a.m. UTC | #3
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.
diff mbox series

Patch

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