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@xxxxxxxxx>
> ---
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
--
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