Re: [PATCH v2 1/6] stash: Add tests for passing in too many refs

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Joel,

On Sun, 25 Mar 2018, Joel Teichroeb wrote:

> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>

I could imagine that the commit message would benefit from this body:

	In preparation for converting the stash command incrementally to
	a builtin command, this patch improves test coverage of the option
	parsing.

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index aefde7b172..7146e27bb5 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -45,6 +45,12 @@ test_expect_success 'applying bogus stash does nothing' '
>  	test_cmp expect file
>  '
>  
> +test_expect_success 'applying with too many agruments does nothing' '
> +	test_must_fail git stash apply stash@{0} bar &&
> +	echo 1 >expect &&
> +	test_cmp expect file
> +'

I suppose you encountered a problem where `stash apply a b` would modify
the file?

And if you really want to verify that the command does nothing, I guess
you will have to use

	test-chmtime =123456789 file &&
	test_must_fail git stash apply stash@{0} bar &&
	test 123456789 = $(test-chmtime -v +0 file | sed 's/[^0-9].*$//')

> @@ -97,6 +103,10 @@ test_expect_success 'stash drop complains of extra options' '
>  	test_must_fail git stash drop --foo
>  '
>  
> +test_expect_success 'stash drop complains with too many refs' '
> +	test_must_fail git stash drop stash@{1} stash@{2}

I wonder whether you might want to verify that the error message is
printed, e.g. via

	test_must_fail git stash drop stash@{1} stash@{2} 2>err &&
	test_i18ngrep "Too many" err

Also, since the added tests look very similar, it might make sense to use
a loop (with fixed revision arguments).

Ciao,
Dscho