git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Victoria Dye <vdye@github.com>
To: Junio C Hamano <gitster@pobox.com>,
	Victoria Dye via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, derrickstolee@github.com
Subject: Re: [PATCH v3 5/5] stash: make internal resets quiet and refresh index
Date: Wed, 16 Mar 2022 13:07:09 -0700	[thread overview]
Message-ID: <a4a7c3ea-3a40-1110-1c1e-a089daf01281@github.com> (raw)
In-Reply-To: <xmqqee33h5ty.fsf@gitster.g>

Junio C Hamano wrote:
> "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.

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

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

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

  reply	other threads:[~2022-03-16 20:07 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-12  0:08 [PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash' Victoria Dye via GitGitGadget
2022-03-12  0:08 ` [PATCH 1/5] reset: revise index refresh advice Victoria Dye via GitGitGadget
2022-03-12  0:08 ` [PATCH 2/5] reset: introduce --[no-]refresh option to --mixed Victoria Dye via GitGitGadget
2022-03-14 15:05   ` Derrick Stolee
2022-03-14 15:13     ` Derrick Stolee
2022-03-14 15:55     ` Victoria Dye
2022-03-12  0:08 ` [PATCH 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice Victoria Dye via GitGitGadget
2022-03-12  0:08 ` [PATCH 4/5] reset: suppress '--no-refresh' advice if logging is silenced Victoria Dye via GitGitGadget
2022-03-12  0:08 ` [PATCH 5/5] stash: make internal resets quiet and refresh index Victoria Dye via GitGitGadget
2022-03-14 15:10   ` Derrick Stolee
2022-03-14 15:56     ` Victoria Dye
2022-03-12 17:12 ` [PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash' Victoria Dye
2022-03-14  6:22 ` Junio C Hamano
2022-03-14 15:13 ` Derrick Stolee
2022-03-14 16:10 ` [PATCH v2 0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' " Victoria Dye via GitGitGadget
2022-03-14 16:10   ` [PATCH v2 1/5] reset: revise index refresh advice Victoria Dye via GitGitGadget
2022-03-14 16:10   ` [PATCH v2 2/5] reset: introduce --[no-]refresh option to --mixed Victoria Dye via GitGitGadget
2022-03-14 19:27     ` Junio C Hamano
2022-03-14 23:48       ` Victoria Dye
2022-03-14 16:10   ` [PATCH v2 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice Victoria Dye via GitGitGadget
2022-03-14 16:10   ` [PATCH v2 4/5] reset: suppress '--no-refresh' advice if logging is silenced Victoria Dye via GitGitGadget
2022-03-14 19:38     ` Junio C Hamano
2022-03-14 16:10   ` [PATCH v2 5/5] stash: make internal resets quiet and refresh index Victoria Dye via GitGitGadget
2022-03-14 19:42     ` Junio C Hamano
2022-03-14 23:54       ` Victoria Dye
2022-03-14 16:30   ` [PATCH v2 0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash' Derrick Stolee
2022-03-14 23:17   ` Junio C Hamano
2022-03-15  1:49   ` [PATCH v3 " Victoria Dye via GitGitGadget
2022-03-15  1:49     ` [PATCH v3 1/5] reset: revise index refresh advice Victoria Dye via GitGitGadget
2022-03-15  1:49     ` [PATCH v3 2/5] reset: introduce --[no-]refresh option to --mixed Victoria Dye via GitGitGadget
2022-03-18 11:08       ` Phillip Wood
2022-03-18 17:17         ` Junio C Hamano
2022-03-18 19:19           ` Victoria Dye
2022-03-15  1:49     ` [PATCH v3 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice Victoria Dye via GitGitGadget
2022-03-15  1:49     ` [PATCH v3 4/5] reset: suppress '--no-refresh' advice if logging is silenced Victoria Dye via GitGitGadget
2022-03-15  1:49     ` [PATCH v3 5/5] stash: make internal resets quiet and refresh index Victoria Dye via GitGitGadget
2022-03-15 10:23       ` Junio C Hamano
2022-03-16 20:07         ` Victoria Dye [this message]
2022-03-16 20:55           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a4a7c3ea-3a40-1110-1c1e-a089daf01281@github.com \
    --to=vdye@github.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).