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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Mar 13, 2015 at 2:18 PM, Sudhanshu Shekhar
<sudshekhar02@xxxxxxxxx> wrote:
> 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
>
> Helped-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
> Helped-by: Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx>
> Helped-by: David Aguilar <davvid@xxxxxxxxx>
> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@xxxxxxxxx>
> ---
> Eric: Thank you for pointing out the mistake. The '&&' after the Here
> Docs was causing the issue. I have removed the concatenation from
> there, hope that's okay.

The && needs to go on the first line, not the last line of the here-doc.

However, that was not my main concern in the previous review. What
disturbed me was that the new tests, which were supposed to be
checking if "-" behaved as @{-1}, were succeeding even without patch
1/2 applied which implemented the "-" alias for @{-1}. That seems
wrong. I don't think you particularly addressed that issue in this
version (even though the first couple tests will now fail without 1/2
due to the changed error message).

> Regarding the @{-1} test case, I created it as a check for Junio's
> comment on the error message generated by "git reset -" when a file
> named @{-1} is there.  Since, in this situation "git reset @{-1}" will
> return an error (but "reset -" shouldn't).

Reminder: Wrap commentary to about column 72, as you would the commit
message. (I re-wrapped it manually to reply to it.)

> I have renamed the folder to 'dash' as suggested by you, keeping the
> old name only where it made sense.

Considering that the test titles already tell us the intent of the
tests, I don't find that the directory name "no_previous" adds much
value to tests checking the behavior of "-" with no previous branch. A
single, consistent name used throughout all these tests would be less
surprising and place smaller cognitive load on the reader.

More below.

> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 98bcfe2..18523c1 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -568,4 +568,162 @@ 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 &&
> +       test_when_finished rm -rf no_previous &&
> +       (
> +               cd no_previous &&
> +               test_must_fail git reset - 2>actual
> +       ) &&
> +       test_i18ngrep "ambiguous argument" 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 "ambiguous argument" no_previous/actual
> +'
> +
> +

Style: Unnecessary extra blank line.

> +test_expect_success \
> +       'reset - in the presence of file named - with previous branch resets commit' '
> +       cat >expect <<-EOF

Place the && at the end of this line. Also, prefix EOF with a
backslash to indicate that you don't intend any interpolation to occur
within the here-doc. So:

    cat >expect <<-\EOF &&

Ditto for the remaining tests.

> +       Unstaged changes after reset:
> +       M       -
> +       M       file
> +       EOF
> +       git init dash &&
> +       test_when_finished rm -rf dash &&
> +       (
> +               cd dash &&
> +               >- &&
> +               >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 - in the presence of file named - with -- option resets commit' '
> +       cat >expect <<-EOF
> +       Unstaged changes after reset:
> +       M       -
> +       M       file
> +       EOF
> +       git init dash &&
> +       test_when_finished rm -rf dash &&
> +       (
> +               cd dash &&
> +               >- &&
> +               >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 - in the presence of file named - with -- file option resets file' '
> +       cat >expect <<-EOF
> +       Unstaged changes after reset:
> +       M       -
> +       EOF
> +       git init dash &&
> +       test_when_finished rm -rf dash &&
> +       (
> +               cd dash &&
> +               >- &&
> +               >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 - in the presence of file named - with both pre and post -- option resets file' '
> +       cat >expect <<-EOF
> +       Unstaged changes after reset:
> +       M       -
> +       EOF
> +       git init dash &&
> +       test_when_finished rm -rf dash &&
> +       (
> +               cd dash &&
> +               >- &&
> +               >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 - works same as reset @{-1}' '
> +       git init dash &&
> +       test_when_finished rm -rf dash &&
> +       (
> +               cd dash &&
> +               echo "file1" >file1 &&
> +               git add file1 &&
> +               git commit -m "base commit" &&
> +               git checkout -b temp &&
> +               echo "new file" >file &&
> +               git add file &&
> +               git commit -m "added file" &&
> +               git reset - &&
> +               git status --porcelain >../actual &&
> +               git add file &&
> +               git commit -m "added file" &&
> +               git reset @{-1} &&
> +               git status --porcelain >../expect
> +       ) &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'reset - with file named @{-1} succeeds' '
> +       cat >expect <<-EOF
> +       Unstaged changes after reset:
> +       M       @{-1}
> +       M       file
> +       EOF
> +       git init dash &&
> +       test_when_finished rm -rf dash &&
> +       (
> +               cd dash &&
> +               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.277.gd67f9d5.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]