From: David Aguilar <davvid@gmail.com>
To: Sudhanshu Shekhar <sudshekhar02@gmail.com>
Cc: git@vger.kernel.org, Matthieu.Moy@grenoble-inp.fr, gitster@pobox.com
Subject: Re: [PATCH 2/2] Added test cases for git reset -
Date: Sun, 8 Mar 2015 11:09:37 -0700 [thread overview]
Message-ID: <20150308180934.GA18215@gmail.com> (raw)
In-Reply-To: <1425826720-5899-2-git-send-email-sudshekhar02@gmail.com>
Hi,
On Sun, Mar 08, 2015 at 08:28:40PM +0530, Sudhanshu Shekhar wrote:
> Four test cases have been added
>
> 1) when user does reset - without any previous branch => Leads to error
> 2) when user does reset - with a previous branch => Ensure it
> behaves like <at> {-1}
>
> Other two deal with the situation when we have a file named '-'. We
> ignore such a file and - is always treated either as a previous branch
> or a bad filename. Users who wish to reset a file named '-' should
> specify
> it as './-'
>
> Signed-off-by: Sudhanshu Shekhar <sudshekhar02@gmail.com>
> ---
Please reword the commit message so that it uses an imperative
mood; see ~line 112 in Documentation/SubmittingPatches for an
explanation.
> t/t7102-reset.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 98bcfe2..ade3d6a 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -568,4 +568,66 @@ test_expect_success 'reset --mixed sets up work tree' '
> test_cmp expect actual
> '
>
> +cat > expect << EOF
Please drop the space after ">" and "<<".
> +fatal: bad flag '-' used after filename
> +EOF
> +
> +test_expect_success 'reset - with no previous branch' '
> + git init no_previous --quiet &&
> + (
> + cd no_previous
> + ) &&
> + test_must_fail git reset - 2>output &&
> + test_cmp expect output
> +'
Please indent lines inside the (...) parens so that it's easier
to notice that they are executing within a subshell, e.g.
git init no_previous --quiet &&
(
cd no_previous &&
...
) &&
...
As written, the above test verifies that we can "cd" into
"no_previous", but because it's a subshell the subsequent
commands after the parens will not be run within that
subdirectory.
Thus, the "git reset -" that we assert must fail is happening in
the outer directory, not the inner no_previous repo.
If that's all we wanted to verify then the (...) sub-shelled cd
command could be replaced entirely by "test -d no_previous", but
I don't think that's the intention of this test.
I believe you may have intended to write the following instead:
test_expect_success 'reset - with no previous branch' '
git init no_previous --quiet &&
(
cd no_previous &&
test_must_fail git reset - 2>../output
) &&
test_cmp expect output
'
"output" becomes "../output" because we're one directory down.
> +
> +test_expect_success 'reset - while having file named - and no previous branch' '
> + git init no_previous --quiet &&
> + (
> + cd no_previous &&
> + touch ./-
> + ) &&
> + test_must_fail git reset - 2>output &&
> + test_cmp expect output
> +'
Ditto; please indent (...) subshells and move the "git reset"
invocation into the subshell so that it runs in the context of
the no_previous repo. The output path will need the same
adjustment as above.
> +
> +cat > expect << EOF
Ditto; no space after > and <<.
> +Unstaged changes after reset:
> +M -
> +M 1
> +EOF
> +
> +test_expect_success 'reset - in the prescence of file named - with previou branch' '
> + git init no_previous --quiet &&
> + cd no_previous &&
Tests that need to change the current directory with "cd" should
generally always use a (...) subshell. It prevents them from
needing to manually undo the "cd" before running subsequent
commands that need to be in the original, parent directory.
> + touch ./- 1 &&
> + git add 1 - &&
> + git commit -m "add base files" &&
> + git checkout -b new_branch &&
> + echo "random" >./- &&
> + echo "wow" >1 &&
> + git add 1 - &&
> + git reset - >output &&
> + test_cmp output ../expect
When applying the previous note, if we keep the test_cmp line
outside of the (...) subshell then we won't need to use "../"
when referring to "expect" here, but we will need it for
"../output" file on the line above it.
> +'
> +test_expect_success 'reset - works same as reset @{-1}' '
> + git init no_previous --quiet &&
> + cd no_previous &&
Ditto; please use a subshell when entering "no_previous".
> + echo "random" >random &&
> + git add random &&
> + git commit -m "base commit" &&
> + git checkout -b temp &&
> + echo new-file >new-file &&
> + git add new-file &&
> + git commit -m "added new-file" &&
> + git reset - &&
> +
> + git status >../first &&
> + git add new-file &&
> + git commit -m "added new-file" &&
> + git reset @{-1} &&
> + git status >../second &&
> + test_cmp ../first ../second
> +'
> +
> test_done
This test uses "git status" to capture the worktree state so
that we can verify that calling "reset -" and "reset @{-1}" are
equivalent.
Bare "git status" is not a plumbing command. This doesn't make a
practical difference for the purpose of this test, but it's
probably a good idea to use the plumbing form of "git status" by
passing the "--porcelain" flag when calling it here.
In addition to these tests, it might also be worth adding test
cases to ensure that we unambiguously differentiate the "-"
shortcut from a file when the "--" marker is used in a repo that
contains a "-" file. When running the following two commands,
git reset - --
git reset -- -
we should test that these are unambiguous because of the "--".
The first command should notice the dash-dash and treat "-" like
a shortcut despite the existence of a file named "-".
The second command should operate on the "-" file only and
otherwise leave the repo state as-is.
If we want to be especially thorough then we should also test
this invocation:
git reset - -- -
This invocation should reset the index to the previous commit
for the "-" file only.
I don't want to increase the scope of this commit so it might
not hurt to add these additional tests as a separate patch.
--
David
prev parent reply other threads:[~2015-03-08 18:09 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-03 20:51 [PATCH] reset: allow "-" short hand for previous commit Sudhanshu Shekhar
2015-03-03 22:10 ` Matthieu Moy
2015-03-03 23:17 ` Junio C Hamano
2015-03-04 7:07 ` Sudhanshu Shekhar
2015-03-04 7:09 ` Sudhanshu Shekhar
2015-03-04 7:10 ` Eric Sunshine
2015-03-05 0:34 ` Junio C Hamano
2015-03-07 21:04 ` [PATCH 1/2] " Sudhanshu Shekhar
2015-03-08 10:33 ` Matthieu Moy
2015-03-08 14:58 ` [PATCH 1/2] Teach reset the same short-hand as checkout Sudhanshu Shekhar
2015-03-08 14:58 ` [PATCH 2/2] Added test cases for git reset - Sudhanshu Shekhar
2015-03-08 18:09 ` David Aguilar [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150308180934.GA18215@gmail.com \
--to=davvid@gmail.com \
--cc=Matthieu.Moy@grenoble-inp.fr \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=sudshekhar02@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).