Re: [PATCH v3 5/5] stash: make internal resets quiet and refresh index

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 15, 2022, 10:23 UTC
Message-ID: <xmqqee33h5ty.fsf@gitster.g>
In-Reply-To: <4c45351a0c4b8b955a9101ed9445a763750ca3c3.1647308982.git.gitgitgadget@gmail.com>

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +test_expect_success 'apply --index -q is quiet' '

Hmph. So being quiet and refreshing index are tested separately in different tests, and this one is only about being quiet?

I wonder if a single test that checks chattiness and refreshing of "git stash -q" and "git apply --index -q" (that's 2x2 which is 4) would be sufficient?

Show 7 quoted lines

> +	# Added file, deleted file, modified file all staged for commit
> +	echo foo >new-file &&
> +	echo test >file &&
> +	git add new-file file &&
> +	git rm other-file &&
> +
> +	git stash &&

As this is only about chattiness about "apply --index -q", this command goes unchecked (4 - 2 = 2).

> +	git stash apply --index -q >output.out 2>&1 &&

And this is only about chattiness so we do not test if the index gets refreshed after this operation (2 - 1 = 1).

> +	test_must_be_empty output.out

This ensures that "git stash apply --index -q" is silent, as that is the main objective of this step: make sure reset won't chatter, especially when stash is told to be --quiet, which is good.

But with a few more lines, this set-up can also test the other three with minimum additional effort, no?

Show 14 quoted lines

> +'
> +
>  test_expect_success 'save -q is quiet' '
>  	git stash save --quiet >output.out 2>&1 &&
>  	test_must_be_empty output.out
> @@ -291,6 +303,27 @@ test_expect_success 'drop -q is quiet' '
>  	test_must_be_empty output.out
>  '
>  
> +test_expect_success 'stash push -q --staged refreshes the index' '
> +	git reset --hard &&
> +	echo test >file &&
> +	git add file &&
> +	git stash push -q --staged &&

"git stash" and "git stash push -q --staged" may do different things, so leaving the plain "git stash" untested for refreshing in an earlier test, and "git stash" with different options being tested for refreshing here, makes me wonder about a gap in test coverage.

The overall theme of the whole topic was that chatty output from "git reset" run as an implementation detail seeps through from "git stash", IIUC. So, making sure that our index is refreshed after the operation is good, but at the same time, wouldn't we want to see what the output of this command says (or be silent)?