Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 3, 2015, 19:14 UTC
Message-ID: <CAPig+cR=xKy-WSZSRgC8UbEbyY+=t7yDZo3O-2GquWXWWPrGVA@mail.gmail.com>
In-Reply-To: <1433331859-24832-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr>

On Wednesday, June 3, 2015, Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:

> Check if commits were removed (i.e. a line was deleted) and print
> warnings or abort git rebase depending on the value of the
> configuration variable rebase.missingCommits.

A few comments below in addition to those already made by Matthieu...

Show 16 quoted lines

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 8960083..f369d2c 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1112,4 +1112,67 @@ test_expect_success 'drop' '
>         test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
>  '
>
> +cat >expect <<EOF
> +Successfully rebased and updated refs/heads/tmp2.
> +EOF
> +
> +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=ignore' '
> +       test_config rebase.missingCommitsCheck ignore &&
> +       test_when_finished "git checkout master &&
> +               git branch -D tmp2" &&

Strange indentation.

> +       git checkout -b tmp2 master &&
> +       set_fake_editor &&
> +       FAKE_LINES="1 2 3 4" \
> +               git rebase -i --root 2>warning &&

The file containing the actual output is usually spelled "actual".

> +       test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> +       test_cmp warning expect

The arguments to test_cmp are usually reversed so that 'expect' comes before 'actual', which results in a more natural-feeling diff when test_cmp detects that the files differ.

These comments apply to remaining new tests, as well.

Show 52 quoted lines

> +'
> +
> +cat >expect <<EOF
> +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 (ignore, warn, error).
> +
> +Successfully rebased and updated refs/heads/tmp2.
> +EOF
> +
> +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=warn' '
> +       test_config rebase.missingCommitsCheck warn &&
> +       test_when_finished "git checkout master &&
> +               git branch -D tmp2" &&
> +       git checkout -b tmp2 master &&
> +       set_fake_editor &&
> +       FAKE_LINES="1 2 3 4" \
> +               git rebase -i --root 2>warning &&
> +       test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> +       test_cmp warning expect
> +'
> +
> +cat >expect <<EOF
> +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 (ignore, warn, error).
> +
> +Rebase aborted due to dropped commits.
> +EOF
> +
> +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=error' '
> +       test_config rebase.missingCommitsCheck error &&
> +       test_when_finished "git checkout master &&
> +               git branch -D tmp2" &&
> +       git checkout -b tmp2 master &&
> +       set_fake_editor &&
> +       test_must_fail env FAKE_LINES="1 2 4" \
> +               git rebase -i --root 2>warning &&
> +       test E = $(git cat-file commit HEAD | sed -ne \$p) &&
> +       test_cmp warning expect
> +'
> +
>  test_done
> --
> 2.4.2.389.geaf7ccf