Re: [RFC PATCH 1/9] t3404: demonstrate that --edit-todo does not check for dropped commits

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 18, 2019, 18:31 UTC
Message-ID: <xmqqwogfxkhx.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190717143918.7406-2-alban.gruin@gmail.com>

Alban Gruin <alban.gruin@gmail.com> writes:

Show 29 quoted lines

> When set to "warn" or "error", `rebase.missingCommitCheck' would make
> rebase -i warn if the user removed commits from the todo list to prevent
> mistakes.  Unfortunately, rebase --edit-todo and rebase --continue don't
> take it into account.
>
> This adds three tests to t3404 to demonstrate this.  The first one is
> not broken, as when `rebase.missingCommitsCheck' is not set, nothing in
> particular must be done towards dropped commits.  The two others are
> broken, demonstrating the problem.
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  t/t3404-rebase-interactive.sh | 82 +++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 461dd539ff..f5c0a8d2bb 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1345,6 +1345,88 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
>  	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>  '
>  
> +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ignore' '
> +	test_config rebase.missingCommitsCheck ignore &&
> +	rebase_setup_and_clean missing-commit &&
> +	set_fake_editor &&
> +	test_must_fail env FAKE_LINES="1 2 bad 3 4" \
> +		git rebase -i --root >/dev/null 2>stderr &&

Do you need to capture into stderr?  Nobody seems to use it.

Show 21 quoted lines

> +	FAKE_LINES="1 2 4" 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" \
> +		actual
> +'
> +
> +cat >expect <<EOF
> +error: invalid line 5: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master)
> +Warning: some commits may have been dropped accidentally.
> +Dropped commits (newer to older):
> + - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
> +To avoid this message, use "drop" to explicitly remove a commit.
> +
> +Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
> +The possible behaviours are: ignore, warn, error.
> +
> +EOF
> +
> +tail -n 8 <expect >expect.2

Having this outside the test_expect_success block that uses the file is bad. You may have mimicked other tests in the same script, but that is not a good excuse to make things worse. Just make sure these new stuff follow the best-current-practice pattern without touching the existing bad examples (and later fix them up after the dust settles, but don't let it distract you from the theme these patches are addressing).

Show 7 quoted lines

> +
> +test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' '
> +	test_config rebase.missingCommitsCheck warn &&
> +	rebase_setup_and_clean missing-commit &&
> +	set_fake_editor &&
> +	test_must_fail env FAKE_LINES="1 2 3 4 bad 5" \
> +		git rebase -i --root >/dev/null 2>stderr &&

Ditto.

> +	FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual &&
> +	test_i18ncmp expect actual &&

So, after "--edit-todo", you are supposed to get an error and a warning, but ...

> +	git rebase --continue 2>actual.2 &&
> +	head -n 8 <actual.2 >actual &&
> +	test_i18ncmp expect.2 actual &&

... after "--continue", you do not get any error, as you removed 'bad' from the input, but you still get a warning, followed by a report of the fact that a commit has been dropped. OK.

Show 31 quoted lines

> +	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> +	test_i18ngrep \
> +		"Successfully rebased and updated refs/heads/missing-commit" \
> +		actual.2
> +'
> +
> +cat >expect <<EOF
> +error: invalid line 3: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
> +Warning: some commits may have been dropped accidentally.
> +Dropped commits (newer to older):
> + - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
> + - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
> +To avoid this message, use "drop" to explicitly remove a commit.
> +
> +Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
> +The possible behaviours are: ignore, warn, error.
> +
> +EOF
> +
> +tail -n 9 <expect >expect.2
> +
> +test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = error' '
> +	test_config rebase.missingCommitsCheck error &&
> +	rebase_setup_and_clean missing-commit &&
> +	set_fake_editor &&
> +	test_must_fail env FAKE_LINES="1 2 bad 3 4" \
> +		git rebase -i --root >/dev/null 2>stderr &&
> +	test_must_fail env FAKE_LINES="1 2 4" \
> +		git rebase --edit-todo 2>actual &&
> +	test_i18ncmp expect actual &&
> +	test_must_fail git rebase --continue 2>actual &&

OK, and this one fails as the configuration is set to 'error'.

> +	test_i18ncmp expect.2 actual &&
> +	cp .git/rebase-merge/git-rebase-todo.backup \
> +		.git/rebase-merge/git-rebase-todo &&

Why?  Who uses this copy?

Show 12 quoted lines

> +	FAKE_LINES="1 2 drop 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" \
> +		actual
> +'
> +
>  test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' '
>  	rebase_setup_and_clean abbrevcmd &&
>  	test_commit "first" file1.txt "first line" first &&