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

Hi,
On Sat, Mar 14, 2015 at 2:40 AM, Eric Sunshine [off-list ref] wrote:

On Fri, Mar 13, 2015 at 2:18 PM, Sudhanshu Shekhar
[off-list ref] wrote:

quoted

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 [off-list ref]
Helped-by: Matthieu Moy [off-list ref]
Helped-by: David Aguilar [off-list ref]
Signed-off-by: Sudhanshu Shekhar <redacted>
---
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).

Actually, The issue was caused due a HERE docs error. If you run this
patch now, you will see that 7 out of the 8 test cases fail.

quoted

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.)

quoted

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.

quoted

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.

quoted

+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.

Thank you for taking the time out to point out these style changes to
me. There is a lot I have to learn about open source contribution yet
and I believe during the course of this microproject, I did learn
something about this (By making some very silly mistakes). Thank you
to all the developers who took time out to review my code and point
out the mistakes I had done.

Regards,
Sudhanshu