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

From: Victoria Dye <vdye@github.com>
Date: Mar 16, 2022, 20:07 UTC
Message-ID: <a4a7c3ea-3a40-1110-1c1e-a089daf01281@github.com>
In-Reply-To: <xmqqee33h5ty.fsf@gitster.g>

Junio C Hamano wrote:

Show 7 quoted lines

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

Yes, because the changes in this series only correct the "quietness" of 'git apply --index -q'. Similarly, there are two instances where '--refresh' was added to 'reset's in 'stash', and two corresponding tests added here.

Show 29 quoted lines

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

I like having the "quietness" and "refresh" checks separate because they're not inextricably linked. One could fail while the other doesn't (or both could independently fail), where two separate results would be slightly more helpful information for a developer debugging the tests. The same reasoning applies to 'stash push' with 'stash apply' (they can fail independently, so they're tested separately).

I'm not normally super adherent to a "test each mode of failure separately" paradigm (evidenced by some of the more complicated tests in 't1092-sparse-checkout-compatibility'), but 't3903-stash.sh' already has dedicated'-q' tests separate from other functionality tests, so I followed the precedent.

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

Yes and no, I think. There is a gap in that stash is not otherwise tested for index refresh, but in the context of this series, that's primarily a concern where 'git reset [--mixed] -q' is used internally - if the refresh didn't happen, the command could (incorrectly) fail altogether. The new tests cover usage of 'stash' wherever a unique 'git reset [--mixed]' exists, and those are theoretically the main sources of a failure-inducing non-refreshed index.

Show 5 quoted lines

> 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)?

The goal of this series was to fix 'git stash apply --index -q' specifically
- it's a somewhat narrow scope, to the point where I would have included it
in another series I'm working on if it weren't for the other changes needed
in 'reset'. In line with that, I tried to keep the tests here tied
one-to-one with the changes made in 'stash':

* add '-q' to 'reset_head()' -> 'apply --index -q is quiet'
* add '--refresh' to 'reset_head()' -> 'stash apply -q --index refreshes the
  index'
* add '--refresh' to 'do_push_stash()' -> 'stash push -q --staged refreshes
  the index'

So while I do think 'stash' testing could be expanded, my intention with this particular series was to unblock changes relying on 'git stash apply --index -q' to work properly. If I'm making this too narrowly-scoped, though, I can submit a patch on top of this one that more broadly expands testing in 'stash' for '-q'. Let me know what you think!