Re: [PATCH] reset: allow "-" short hand for previous commit

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 5, 2015, 00:34 UTC
Message-ID: <xmqq1tl4705j.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAODo60qimjiMfWY_FNuOcVaYc=JdbTv_4SJ3BROC-9Jo5qWkdA@mail.gmail.com>

Sudhanshu Shekhar <sudshekhar02@gmail.com> writes:

Show 6 quoted lines

>> What should worry us even more is what the user would get when @{-1}
>> does not resolve to something the command can use.  It would be bad
>> if we give an error message with @{-1} in it that the user never
>> typed (and may not even understand what it means).
>
> I apologize for having overlooked this use case.

Thanking is fine, but there is no need to apologize in response to review comments. We are imperfect humans and review exchanges are designed to allow us cover points each of us missed in our collective effort to make Git better.

> Another thing, can someone guide me regarding the right place to add a
> test case? Should it be t7102-reset.sh or some other file?

At the end of that file sounds like a reasonable choice. You would want to test various cases, such as (1) what happens when there is no @{-1} at all (you would need a newly initialied test repository, just like the last test in that file creates mixed_worktree repository for its own use), (2) what happens when there is, split into two , i.e. (1-a) @{-1} does not exist but there is a file whose name is "-"; (1-b) @{-1} does not exist and there is no file whose name is "-"; (2-a) @{-1} exists but there is a file whose name is "-"; and (2-b) @{-1} exists and there is no file whose name is "-".

Do not just test (2-b) and declare victory---making sure that a feature does not trigger when it should not is also important.

Thanks.