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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 26, 2018, 19:21 UTC
Message-ID: <nycvar.QRO.7.76.6.1803262110000.77@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
In-Reply-To: <20180326011426.19159-2-joel@teichroeb.net>

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.

Show 13 quoted lines

> 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].*$//')

Show 6 quoted lines

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