Hi,
On Sat, Mar 14, 2015 at 2:40 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> 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).
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.
>
>> 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.
>
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
--
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