Re: [PATCH 2/2] Added test cases for git reset -

From: David Aguilar <davvid@gmail.com>
Date: Mar 8, 2015, 18:09 UTC
Message-ID: <20150308180934.GA18215@gmail.com>
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:

Show 14 quoted lines

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

Show 12 quoted lines

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

Show 11 quoted lines

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

Show 10 quoted lines

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

Show 8 quoted lines

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

Show 9 quoted lines

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

Show 18 quoted lines

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