[v3,0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash'

Message

Bruce Perry via GitGitGadget March 15, 2022, 1:49 a.m. UTC

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/[e-mail]/

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 <[e-mail]>
     +    Helped-by: Junio C Hamano <[e-mail]>
          Signed-off-by: Victoria Dye <[e-mail]>
      
       ## 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 <[e-mail]>
          Signed-off-by: Victoria Dye <[e-mail]>
      
       ## 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 &&