Re: [PATCH 01/30] Tighten and correct a few testcases for merging and cherry-picking

From: Stefan Beller <sbeller@google.com>
To: Elijah Newren <newren@gmail.com>
Cc: git <git@vger.kernel.org>
Subject: Re: [PATCH 01/30] Tighten and correct a few testcases for merging and cherry-picking
Date: Mon, 13 Nov 2017 11:32:42 -0800	[thread overview]
Message-ID: <CAGZ79kZtOjB43d9kT8yjnwA9ii60VsyzRxM7XHQBtMs70S2zGQ@mail.gmail.com> (raw)
In-Reply-To: <20171110190550.27059-2-newren@gmail.com>

On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren <newren@gmail.com> wrote:
> t3501 had a testcase originally added

... goes and looks ...

  "in 05f2dfb965 (cherry-pick: demonstrate a segmentation fault, 2016-11-26)"

would have helped me here in the commit message.

> to ensure cherry-pick wouldn't
> segfault when working with a dirty file involved in a rename.  While
> the segfault was fixed, there was another problem this test demonstrated:
> namely, that git would overwrite a dirty file involved in a rename.
> Further, the test encoded a "successful merge" and overwriting of this
> file as correct behavior.  Modify the test so that it would still catch
> the segfault, but to require the correct behavior.

As the correct behavior is not yet implemented, mark it as
test_expect_failure, too. (probably this reads implicit)

>
> t7607 had a test

... added in 30fd3a5425 (merge overwrites unstaged changes in renamed file,
2012-04-15) ...

> specific to looking for a merge overwriting a dirty file
> involved in a rename, but it too actually encoded what I would term
> incorrect behavior: it expected the merge to succeed.  Fix that, and add
> a few more checks to make sure that the merge really does produce the
> expected results.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  t/t3501-revert-cherry-pick.sh | 7 +++++--
>  t/t7607-merge-overwrite.sh    | 5 ++++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 4f2a263b63..783bdbf59d 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with arguments' '
>         test_cmp expect actual
>  '
>
> -test_expect_success 'cherry-pick works with dirty renamed file' '
> +test_expect_failure 'cherry-pick works with dirty renamed file' '
>         test_commit to-rename &&
>         git checkout -b unrelated &&
>         test_commit unrelated &&
> @@ -150,7 +150,10 @@ test_expect_success 'cherry-pick works with dirty renamed file' '
>         test_tick &&
>         git commit -m renamed &&
>         echo modified >renamed &&
> -       git cherry-pick refs/heads/unrelated
> +       test_must_fail git cherry-pick refs/heads/unrelated >out &&
> +       test_i18ngrep "Refusing to lose dirty file at renamed" out &&
> +       test $(git rev-parse :0:renamed) = $(git rev-parse HEAD^:to-rename.t) &&
> +       grep -q "^modified$" renamed
>  '
>
>  test_done
> diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
> index 9444d6a9b9..00617dadf8 100755
> --- a/t/t7607-merge-overwrite.sh
> +++ b/t/t7607-merge-overwrite.sh
> @@ -97,7 +97,10 @@ test_expect_failure 'will not overwrite unstaged changes in renamed file' '
>         git mv c1.c other.c &&
>         git commit -m rename &&
>         cp important other.c &&
> -       git merge c1a &&
> +       test_must_fail git merge c1a >out &&
> +       test_i18ngrep "Refusing to lose dirty file at other.c" out &&
> +       test -f other.c~HEAD &&
> +       test $(git hash-object other.c~HEAD) = $(git rev-parse c1a:c1.c) &&
>         test_cmp important other.c

Code looks good,

Thanks,
Stefan