Re: [PATCH v3 0/2] rebase -i: extend rebase.missingCommitsCheck

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 6607 bytes --]

Hi Alban,

On Fri, 6 Dec 2019, Phillip Wood wrote:

> On 05/12/2019 23:15, Alban Gruin wrote:
> >
> > Le 04/12/2019 à 22:51, Junio C Hamano a écrit :
> > > Alban Gruin <alban.gruin@gmail.com> writes:
> > >
> > > > To prevent mistakes when editing a branch, rebase features a knob,
> > > > rebase.missingCommitsCheck, to warn the user if a commit was dropped.
> > > > Unfortunately, this check is only effective for the initial edit, which
> > > > means that if you edit the todo list at a later point of the rebase and
> > > > drop a commit, no warnings or errors would be issued.
> > > > ...
> > > >   rebase-interactive.c          | 57 ++++++++++++++++++++----
> > > >   rebase-interactive.h          |  2 +
> > > >   sequencer.c                   | 53 ++++++----------------
> > > >   sequencer.h                   |  1 -
> > > >   t/t3404-rebase-interactive.sh | 83 +++++++++++++++++++++++++++++++++++
> > > >   5 files changed, 147 insertions(+), 49 deletions(-)
> > >
> > > This passes the self-test when tested by itself, but when merged
> > > near the tip of 'pu', it breaks t3404.116, it seems.
> > >
> >
> > After a quick investigation, it comes from
> > pw/post-commit-from-sequencer.  Since then, tests are expected to setup
> > the editor and run the commands using it in a subshell.  So the fix is
> > straightforward.
> >
> > Perhaps I should take ag/sequencer-todo-updates, merge
> > pw/post-commit-from-sequencer, rebase this series onto the result, fix
> > the issue, and reroll the series?
>
> If the issue is just using a subshell to set the editor then (assuming you're
> only adding new tests) I don't think you need to rebase - just change your
> tests and it should be fine when Junio merges it into pu. I'm sorry I've not
> looked at the latest version yet, I'll try and get round to it next week.

Just squash this in:

-- snipsnap --
Subject: [PATCH] fixup??? rebase-interactive: warn if commit is dropped with
 `rebase --edit-todo'

This is required to appease the interaction with
`pw/post-commit-from-sequencer`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3404-rebase-interactive.sh | 74 +++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 34 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e3f64bc2a59..72718d0d839 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1453,10 +1453,12 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
 test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ignore' '
 	test_config rebase.missingCommitsCheck ignore &&
 	rebase_setup_and_clean missing-commit &&
-	set_fake_editor &&
-	FAKE_LINES="break 1 2 3 4 5" git rebase -i --root &&
-	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual &&
-	git rebase --continue 2>actual &&
+	(
+		set_fake_editor &&
+		FAKE_LINES="break 1 2 3 4 5" git rebase -i --root &&
+		FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual &&
+		git rebase --continue 2>actual
+	) &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
 	test_i18ngrep \
 		"Successfully rebased and updated refs/heads/missing-commit" \
@@ -1474,18 +1476,20 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = wa
 	tail -n4 expect >expect.2 &&
 	test_config rebase.missingCommitsCheck warn &&
 	rebase_setup_and_clean missing-commit &&
-	set_fake_editor &&
-	test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \
-		git rebase -i --root &&
-	cp .git/rebase-merge/git-rebase-todo.backup orig &&
-	FAKE_LINES="2 3 4" git rebase --edit-todo 2>actual.2 &&
-	head -n5 actual.2 >actual &&
-	test_i18ncmp expect actual &&
-	cp orig .git/rebase-merge/git-rebase-todo &&
-	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
-	head -n4 actual.2 >actual &&
-	test_i18ncmp expect.2 actual &&
-	git rebase --continue 2>actual &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \
+			git rebase -i --root &&
+		cp .git/rebase-merge/git-rebase-todo.backup orig &&
+		FAKE_LINES="2 3 4" git rebase --edit-todo 2>actual.2 &&
+		head -n5 actual.2 >actual &&
+		test_i18ncmp expect actual &&
+		cp orig .git/rebase-merge/git-rebase-todo &&
+		FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 &&
+		head -n4 actual.2 >actual &&
+		test_i18ncmp expect.2 actual &&
+		git rebase --continue 2>actual
+	) &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
 	test_i18ngrep \
 		"Successfully rebased and updated refs/heads/missing-commit" \
@@ -1509,24 +1513,26 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = er
 	tail -n10 expect >expect.2 &&
 	test_config rebase.missingCommitsCheck error &&
 	rebase_setup_and_clean missing-commit &&
-	set_fake_editor &&
-	test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \
-		git rebase -i --root &&
-	cp .git/rebase-merge/git-rebase-todo.backup orig &&
-	test_must_fail env FAKE_LINES="2 3 4" \
-		git rebase --edit-todo 2>actual &&
-	test_i18ncmp expect actual &&
-	test_must_fail git rebase --continue 2>actual &&
-	test_i18ncmp expect actual &&
-	cp orig .git/rebase-merge/git-rebase-todo &&
-	test_must_fail env FAKE_LINES="1 2 3 4" \
-		git rebase --edit-todo 2>actual &&
-	test_i18ncmp expect.2 actual &&
-	test_must_fail git rebase --continue 2>actual &&
-	test_i18ncmp expect.2 actual &&
-	cp orig .git/rebase-merge/git-rebase-todo &&
-	FAKE_LINES="1 2 3 4 drop 5" git rebase --edit-todo &&
-	git rebase --continue 2>actual &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \
+			git rebase -i --root &&
+		cp .git/rebase-merge/git-rebase-todo.backup orig &&
+		test_must_fail env FAKE_LINES="2 3 4" \
+			git rebase --edit-todo 2>actual &&
+		test_i18ncmp expect actual &&
+		test_must_fail git rebase --continue 2>actual &&
+		test_i18ncmp expect actual &&
+		cp orig .git/rebase-merge/git-rebase-todo &&
+		test_must_fail env FAKE_LINES="1 2 3 4" \
+			git rebase --edit-todo 2>actual &&
+		test_i18ncmp expect.2 actual &&
+		test_must_fail git rebase --continue 2>actual &&
+		test_i18ncmp expect.2 actual &&
+		cp orig .git/rebase-merge/git-rebase-todo &&
+		FAKE_LINES="1 2 3 4 drop 5" git rebase --edit-todo &&
+		git rebase --continue 2>actual
+	) &&
 	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
 	test_i18ngrep \
 		"Successfully rebased and updated refs/heads/missing-commit" \
--
2.24.0.windows.2.611.ge9aced84530