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@xxxxxxxxx/ 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@xxxxxxxxxx> + Helped-by: Junio C Hamano <gitster@xxxxxxxxx> Signed-off-by: Victoria Dye <vdye@xxxxxxxxxx> ## 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@xxxxxxxxx> Signed-off-by: Victoria Dye <vdye@xxxxxxxxxx> ## 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
