Re: [PATCH v3 2/2] t7102: add 'reset -' tests

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 13, 2015, 10:27 UTC
Message-ID: <CAPig+cSxuS0Khg2a0K1+K3UXH-TTgMREH30qYMt4cs0eCMbjiQ@mail.gmail.com>
In-Reply-To: <1426025409-18758-1-git-send-email-sudshekhar02@gmail.com>

On Tue, Mar 10, 2015 at 6:10 PM, Sudhanshu Shekhar <sudshekhar02@gmail.com> wrote:

Show 6 quoted lines

> Add following test cases:
>
> 1) Confirm error message when git reset is used with no previous branch
> 2) Confirm git reset - works like git reset @{-1}
> 3) Confirm "-" is always treated as a commit unless the -- file option is specified
> 4) Confirm "git reset -" works normally even when a file named @{-1} is present

Does it concern you that all new tests pass except the last one even when patch 1/2 is not applied? I would have expected most or all of these tests to fail without patch 1/2.

Show 15 quoted lines

> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> Helped-by: David Aguilar <davvid@gmail.com>
> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
> ---
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 98bcfe2..d3a5874 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -568,4 +568,163 @@ test_expect_success 'reset --mixed sets up work tree' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'reset - with no previous branch fails' '
> +       git init no_previous &&

I understand the intention of the name "no_previous" in tests for which there is no @{-1}, however...

Show 27 quoted lines

> +       test_when_finished rm -rf no_previous &&
> +       (
> +               cd no_previous &&
> +               test_must_fail git reset - 2>actual
> +       ) &&
> +       test_i18ngrep "bad flag" no_previous/actual
> +'
> +
> +test_expect_success 'reset - while having file named - and no previous branch fails' '
> +       git init no_previous &&
> +       test_when_finished rm -rf no_previous &&
> +       (
> +               cd no_previous &&
> +               >- &&
> +               test_must_fail git reset - 2>actual
> +       ) &&
> +       test_i18ngrep "bad flag" no_previous/actual
> +'
> +
> +test_expect_success \
> +       'reset - in the presence of file named - with previous branch resets commit' '
> +       cat >expect <<-\EOF
> +       Unstaged changes after reset:
> +       M       -
> +       M       file
> +       EOF &&
> +       git init no_previous &&

I don't understand the name "no_previous" in tests for which @{-1} can resolve. It probably would be better to choose a more generic name and use it for all these tests. For instance, "resetdash" or just "dash" or something better.

Show 17 quoted lines

> +       test_when_finished rm -rf no_previous &&
> +       (
> +               cd no_previous &&
> +               >- &&
> +               >file &&
> +               git add file - &&
> +               git commit -m "add base files" &&
> +               git checkout -b new_branch &&
> +               echo "random" >- &&
> +               echo "wow" >file &&
> +               git add file - &&
> +               git reset - >../actual
> +       ) &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'reset - with file named @{-1} succeeds' '

I may be missing something obvious, but why is it necessary to single out a file named @{-1} at all, particuarly when there are so many other ways to specify revisions, and there may be files mirroring those spellings, as well?

Show 25 quoted lines

> +       cat >expect <<EOF
> +       Unstaged changes after reset:
> +       M       file
> +       M       @{-1}
> +       EOF &&
> +       git init no_previous &&
> +       test_when_finished rm -rf no_previous &&
> +       (
> +               cd no_previous &&
> +               echo "random" >@{-1} &&
> +               echo "random" >file &&
> +               git add @{-1} file &&
> +               git commit -m "base commit" &&
> +               git checkout -b new_branch &&
> +               echo "additional stuff" >>file &&
> +               echo "additional stuff" >>@{-1} &&
> +               git add file @{-1} &&
> +               git reset - >../actual
> +       ) &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.3.1.278.ge5c7b1f.dirty