From: "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: derrickstolee@github.com, Victoria Dye <vdye@github.com>
Subject: [PATCH v3 0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash'
Date: Tue, 15 Mar 2022 01:49:37 +0000 [thread overview]
Message-ID: <pull.1170.v3.git.1647308982.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1170.v2.git.1647274230.gitgitgadget@gmail.com>
In the process of working on tests for 'git stash' sparse index integration,
I found that the '--quiet' option in 'git stash' does not suppress all
non-error output when used with '--index'. Specifically, this comes from an
invocation of 'git reset' without the '--quiet' flag in 'reset_head()'. Upon
enabling that flag, though, I discovered that 1) 'reset' does not refresh
the index if '--quiet' is specified (as of [1]) and 2) 'git stash' needs the
index to be refreshed after the reset.
This series aims to decouple the "suppress logging" and "skip index refresh"
behaviors in 'git reset --mixed', then allow 'stash' to internally use reset
with logs suppressed but index refresh enabled. This is accomplished by
introducing the '--[no-]refresh' option and 'reset.refresh' config setting
to 'git reset'. Additionally, in the spirit of backward-compatibility,
'--quiet' and/or 'reset.quiet=true' without any specified "refresh"
option/config will continue to skip 'refresh_index(...)'.
There are also some minor updates to the advice that suggests skipping the
index refresh:
* replace recommendation to use "--quiet" with "--no-refresh"
* use 'advise()' rather than 'printf()'
* rename the advice config setting from 'advice.resetQuiet' to to
'advice.resetNoRefresh'
* suppress advice if '--quiet' is specified in 'reset'
Finally, tests are added to 't7102-reset.sh' verifying whether index refresh
happens when expected and to 't3903-stash.sh' verifying that 'apply --quiet'
no longer prints extraneous logs.
Changes since V2
================
* Rename 'test_index_refreshed' to 'test_reset_refreshes_index'
* Move comments explaining the "reset refreshes index" test logic inline
with execution
* Replace 'git read-tree' with 'git diff-files' in
'test_reset_refreshes_index' for a (hopefully) more future-proofed
command verifying for index refresh
* Add tests for ensuring the index is refreshed after 'git stash push
--staged' and 'git stash apply --index'
Changes since V1
================
* Update cover letter title
* Squash 't7102' test fixes from patch 5 into patch 2
* Refactor 't7102' tests to properly use inline config settings
[1] https://lore.kernel.org/git/20181023190423.5772-2-peartben@gmail.com/
Thanks! -Victoria
Victoria Dye (5):
reset: revise index refresh advice
reset: introduce --[no-]refresh option to --mixed
reset: replace '--quiet' with '--no-refresh' in performance advice
reset: suppress '--no-refresh' advice if logging is silenced
stash: make internal resets quiet and refresh index
Documentation/config/advice.txt | 8 ++--
Documentation/git-reset.txt | 9 ++++
advice.c | 2 +-
advice.h | 2 +-
builtin/reset.c | 21 +++++++---
builtin/stash.c | 5 ++-
t/t3903-stash.sh | 33 +++++++++++++++
t/t7102-reset.sh | 73 +++++++++++++++++++++++++++++----
8 files changed, 133 insertions(+), 20 deletions(-)
base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1170%2Fvdye%2Fstash%2Fmake-reset-quiet-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1170/vdye/stash/make-reset-quiet-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1170
Range-diff vs v2:
1: 7206ef8dd8a = 1: 7206ef8dd8a reset: revise index refresh advice
2: 7f0226bc3e6 ! 2: 101cee42dd6 reset: introduce --[no-]refresh option to --mixed
@@ Commit message
whether logs are silenced and do not affect index refresh.
Helped-by: Derrick Stolee <derrickstolee@github.com>
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Victoria Dye <vdye@github.com>
## Documentation/git-reset.txt ##
@@ t/t7102-reset.sh: test_expect_success 'resetting an unmodified path is a no-op'
git diff-index --cached --exit-code HEAD
'
-+test_index_refreshed () {
-+
-+ # To test whether the index is refresh, create a scenario where a
-+ # command will fail if the index is *not* refreshed:
-+ # 1. update the worktree to match HEAD & remove file2 in the index
-+ # 2. reset --mixed to unstage the change from step 1
-+ # 3. read-tree HEAD~1 (which differs from HEAD in file2)
-+ # If the index is refreshed in step 2, then file2 in the index will be
-+ # up-to-date with HEAD and read-tree will succeed (thus failing the
-+ # test). If the index is *not* refreshed, however, the staged deletion
-+ # of file2 from step 1 will conflict with the changes from the tree read
-+ # in step 3, resulting in a failure.
++test_reset_refreshes_index () {
++
++ # To test whether the index is refreshed in `git reset --mixed` with
++ # the given options, create a scenario where we clearly see different
++ # results depending on whether the refresh occurred or not.
+
+ # Step 0: start with a clean index
+ git reset --hard HEAD &&
+
-+ # Step 1
++ # Step 1: remove file2, but only in the index (no change to worktree)
+ git rm --cached file2 &&
+
-+ # Step 2
++ # Step 2: reset index & leave worktree unchanged from HEAD
+ git $1 reset $2 --mixed HEAD &&
+
-+ # Step 3
-+ git read-tree -m HEAD~1
++ # Step 3: verify whether the index is refreshed by checking whether
++ # file2 still has staged changes in the index differing from HEAD (if
++ # the refresh occurred, there should be no such changes)
++ git diff-files >output.log &&
++ test_must_be_empty output.log
+}
+
test_expect_success '--mixed refreshes the index' '
@@ t/t7102-reset.sh: test_expect_success 'resetting an unmodified path is a no-op'
- test_cmp expect output
+ # Verify default behavior (with no config settings or command line
+ # options)
-+ test_index_refreshed
++ test_reset_refreshes_index
+'
+test_expect_success '--mixed --[no-]quiet sets default refresh behavior' '
+ # Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh)
+ # determine refresh behavior
+
+ # Config setting
-+ ! test_index_refreshed "-c reset.quiet=true" &&
-+ test_index_refreshed "-c reset.quiet=false" &&
++ ! test_reset_refreshes_index "-c reset.quiet=true" &&
++ test_reset_refreshes_index "-c reset.quiet=false" &&
+
+ # Command line option
-+ ! test_index_refreshed "" --quiet &&
-+ test_index_refreshed "" --no-quiet &&
++ ! test_reset_refreshes_index "" --quiet &&
++ test_reset_refreshes_index "" --no-quiet &&
+
+ # Command line option overrides config setting
-+ ! test_index_refreshed "-c reset.quiet=false" --quiet &&
-+ test_index_refreshed "-c reset.refresh=true" --no-quiet
++ ! test_reset_refreshes_index "-c reset.quiet=false" --quiet &&
++ test_reset_refreshes_index "-c reset.refresh=true" --no-quiet
+'
+
+test_expect_success '--mixed --[no-]refresh sets refresh behavior' '
+ # Verify that --[no-]refresh and `reset.refresh` control index refresh
+
+ # Config setting
-+ test_index_refreshed "-c reset.refresh=true" &&
-+ ! test_index_refreshed "-c reset.refresh=false" &&
++ test_reset_refreshes_index "-c reset.refresh=true" &&
++ ! test_reset_refreshes_index "-c reset.refresh=false" &&
+
+ # Command line option
-+ test_index_refreshed "" --refresh &&
-+ ! test_index_refreshed "" --no-refresh &&
++ test_reset_refreshes_index "" --refresh &&
++ ! test_reset_refreshes_index "" --no-refresh &&
+
+ # Command line option overrides config setting
-+ test_index_refreshed "-c reset.refresh=false" --refresh &&
-+ ! test_index_refreshed "-c reset.refresh=true" --no-refresh
++ test_reset_refreshes_index "-c reset.refresh=false" --refresh &&
++ ! test_reset_refreshes_index "-c reset.refresh=true" --no-refresh
+'
+
+test_expect_success '--mixed --refresh overrides --quiet refresh behavior' '
+ # Verify that *both* --refresh and `reset.refresh` override the
+ # default non-refresh behavior of --quiet
-+ test_index_refreshed "" "--quiet --refresh" &&
-+ test_index_refreshed "-c reset.quiet=true" --refresh &&
-+ test_index_refreshed "-c reset.refresh=true" --quiet &&
-+ test_index_refreshed "-c reset.refresh=true -c reset.quiet=true"
++ test_reset_refreshes_index "" "--quiet --refresh" &&
++ test_reset_refreshes_index "-c reset.quiet=true" --refresh &&
++ test_reset_refreshes_index "-c reset.refresh=true" --quiet &&
++ test_reset_refreshes_index "-c reset.refresh=true -c reset.quiet=true"
'
test_expect_success '--mixed preserves skip-worktree' '
3: f869723d64f = 3: eb5958194ee reset: replace '--quiet' with '--no-refresh' in performance advice
4: cffca0ea5c6 = 4: 548c9303c44 reset: suppress '--no-refresh' advice if logging is silenced
5: 3334d4cb6f3 ! 5: 4c45351a0c4 stash: make internal resets quiet and refresh index
@@ Commit message
refreshing the index, but later operations in 'git stash' subcommands expect
a non-stale index, enable '--refresh' as well.
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Victoria Dye <vdye@github.com>
## builtin/stash.c ##
@@ t/t3903-stash.sh: test_expect_success 'apply -q is quiet' '
test_expect_success 'save -q is quiet' '
git stash save --quiet >output.out 2>&1 &&
test_must_be_empty output.out
+@@ t/t3903-stash.sh: 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 diff-files >output.out &&
++ test_must_be_empty output.out
++'
++
++test_expect_success 'stash apply -q --index refreshes the index' '
++ echo test >other-file &&
++ git add other-file &&
++ echo another-change >other-file &&
++ git diff-files >expect &&
++ git stash &&
++
++ git stash apply -q --index &&
++ git diff-files >actual &&
++ test_cmp expect actual
++'
++
+ test_expect_success 'stash -k' '
+ echo bar3 >file &&
+ echo bar4 >file2 &&
--
gitgitgadget
next prev parent reply other threads:[~2022-03-15 1:49 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 ` Victoria Dye via GitGitGadget [this message]
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
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=pull.1170.v3.git.1647308982.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=vdye@github.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).