Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash'

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.

[1] https://lore.kernel.org/git/[email protected]/

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                | 12 +++++
 t/t7102-reset.sh                | 95 ++++++++++++++++++++++++++++++---
 8 files changed, 134 insertions(+), 20 deletions(-)

base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1170%2Fvdye%2Fstash%2Fmake-reset-quiet-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1170/vdye/stash/make-reset-quiet-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1170

gitgitgadget

From: Victoria Dye <[email protected]>

Update the advice describing index refresh from "enumerate unstaged changes" to "refresh the index." Describing 'refresh_index(...)' as "enumerating unstaged changes" is not fully representative of what an index refresh is doing; more generally, it updates the properties of index entries that are affected by outside-of-index state, e.g. CE_UPTODATE, which is affected by the file contents on-disk. This distinction is relevant to operations that read the index but do not refresh first - e.g., 'git read-tree' - where a stale index may cause incorrect behavior.

In addition to changing the advice message, use the "advise" function to print advice.

---
 Documentation/config/advice.txt | 4 ++--
 builtin/reset.c                 | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

index c40eb09cb7e..971aad2f237 100644
@@ -69,8 +69,8 @@ advice.*::
 		merge to avoid overwriting local changes.
 	resetQuiet::
 		Advice to consider using the `--quiet` option to linkgit:git-reset[1]
-		when the command takes more than 2 seconds to enumerate unstaged
-		changes after reset.
+		when the command takes more than 2 seconds to refresh the index
+		after reset.
 	resolveConflict::
 		Advice shown by various commands when conflicts
 		prevent the operation from being performed.

index 6e65e90c5db..a420497a14f 100644
@@ -525,9 +525,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 					      _("Unstaged changes after reset:"));
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
 				if (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
-					printf(_("\nIt took %.2f seconds to enumerate unstaged changes after reset.  You can\n"
+					advise(_("It took %.2f seconds to refresh the index after reset.  You can\n"
 						"use '--quiet' to avoid this.  Set the config setting reset.quiet to true\n"
-						"to make this the default.\n"), t_delta_in_ms / 1000.0);
+						"to make this the default."), t_delta_in_ms / 1000.0);
 				}
 			}
 		} else {

gitgitgadget

From: Victoria Dye <[email protected]>

Add a new --[no-]refresh option that is intended to explicitly determine whether a mixed reset should end in an index refresh.

A few years ago, [1] introduced behavior to the '--quiet' option to skip the call to 'refresh_index(...)' at the end of a mixed reset with the goal of improving performance. However, by coupling behavior that modifies the index with the option that silences logs, there is no way for users to have one without the other (i.e., silenced logs with a refreshed index) without incurring the overhead of a separate call to 'git update-index --refresh'. Furthermore, there is minimal user-facing documentation indicating that --quiet skips the index refresh, potentially leading to unexpected issues executing commands after 'git reset --quiet' that do not themselves refresh the index (e.g., internals of 'git stash', 'git read-tree').

To mitigate these issues, '--[no-]refresh' and 'reset.refresh' are introduced to provide a dedicated mechanism for refreshing the index. When either is set, '--quiet' and 'reset.quiet' revert to controlling only whether logs are silenced and do not affect index refresh.

[1] 9ac8125d1a (reset: don't compute unstaged changes after reset when

    --quiet, 2018-10-23)

---
 Documentation/git-reset.txt |  9 +++++
 builtin/reset.c             | 13 ++++++-
 t/t7102-reset.sh            | 77 +++++++++++++++++++++++++++++++++----
 3 files changed, 91 insertions(+), 8 deletions(-)

index 6f7685f53d5..89ddc85c2e4 100644
@@ -110,6 +110,15 @@ OPTIONS
 	`reset.quiet` config option. `--quiet` and `--no-quiet` will
 	override the default behavior.
 
+--refresh::
+--no-refresh::
+	Proactively refresh the index after a mixed reset. If unspecified, the
+	behavior falls back on the `reset.refresh` config option. If neither
+	`--[no-]refresh` nor `reset.refresh` are set, the default behavior is
+	decided by the `--[no-]quiet` option and/or `reset.quiet` config.
+	If `--quiet` is specified or `reset.quiet` is set with no command-line
+	"quiet" setting, refresh is disabled. Otherwise, refresh is enabled.
+
 --pathspec-from-file=<file>::
 	Pathspec is passed in `<file>` instead of commandline args. If
 	`<file>` is exactly `-` then standard input is used. Pathspec

index a420497a14f..7f667e13d71 100644
@@ -392,6 +392,7 @@ static int git_reset_config(const char *var, const char *value, void *cb)
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
 	int reset_type = NONE, update_ref_status = 0, quiet = 0;
+	int refresh = -1;
 	int patch_mode = 0, pathspec_file_nul = 0, unborn;
 	const char *rev, *pathspec_from_file = NULL;
 	struct object_id oid;

@@ -399,6 +400,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	int intent_to_add = 0;
 	const struct option options[] = {
 		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_BOOL(0, "refresh", &refresh,
+				N_("skip refreshing the index after reset")),
 		OPT_SET_INT(0, "mixed", &reset_type,
 						N_("reset HEAD and index"), MIXED),
 		OPT_SET_INT(0, "soft", &reset_type, N_("reset only HEAD"), SOFT),

@@ -421,11 +424,19 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 	git_config(git_reset_config, NULL);
 	git_config_get_bool("reset.quiet", &quiet);
+	git_config_get_bool("reset.refresh", &refresh);
 
 	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
 						PARSE_OPT_KEEP_DASHDASH);
 	parse_args(&pathspec, argv, prefix, patch_mode, &rev);
 
+	/*
+	 * If refresh is completely unspecified (either by config or by command
+	 * line option), decide based on 'quiet'.
+	 */
+	if (refresh < 0)
+		refresh = !quiet;
+
 	if (pathspec_from_file) {
 		if (patch_mode)
 			die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--patch");

@@ -517,7 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 			if (read_from_tree(&pathspec, &oid, intent_to_add))
 				return 1;
 			the_index.updated_skipworktree = 1;
-			if (!quiet && get_git_work_tree()) {
+			if (refresh && get_git_work_tree()) {
 				uint64_t t_begin, t_delta_in_ms;
 
 				t_begin = getnanotime();

gitgitgadget

From: Victoria Dye <[email protected]>

Replace references to '--quiet' with '--no-refresh' in the advice on how to skip refreshing the index. When the advice was introduced, '--quiet' was the only way to avoid the expensive 'refresh_index(...)' at the end of a mixed reset. After introducing '--no-refresh', however, '--quiet' became only a fallback option for determining refresh behavior, overridden by '--[no-]refresh' or 'reset.refresh' if either is set. To ensure users are advised to use the most reliable option for avoiding 'refresh_index(...)', replace recommendation of '--quiet' with '--[no-]refresh'.

---
 Documentation/config/advice.txt | 8 ++++----
 advice.c                        | 2 +-
 advice.h                        | 2 +-
 builtin/reset.c                 | 8 ++++----
 4 files changed, 10 insertions(+), 10 deletions(-)

index 971aad2f237..83c2a956611 100644
@@ -67,10 +67,10 @@ advice.*::
 	commitBeforeMerge::
 		Advice shown when linkgit:git-merge[1] refuses to
 		merge to avoid overwriting local changes.
-	resetQuiet::
-		Advice to consider using the `--quiet` option to linkgit:git-reset[1]
-		when the command takes more than 2 seconds to refresh the index
-		after reset.
+	resetNoRefresh::
+		Advice to consider using the `--no-refresh` option to
+		linkgit:git-reset[1] when the command takes more than 2 seconds
+		to refresh the index after reset.
 	resolveConflict::
 		Advice shown by various commands when conflicts
 		prevent the operation from being performed.

index 2e1fd483040..cb755c551a2 100644
@@ -61,7 +61,7 @@ static struct {
 	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching", 1 },
 	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName", 1 },
 	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected", 1 },
-	[ADVICE_RESET_QUIET_WARNING]			= { "resetQuiet", 1 },
+	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh", 1 },
 	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict", 1 },
 	[ADVICE_RM_HINTS]				= { "rmHints", 1 },
 	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse", 1 },

index a3957123a16..f95af70ced4 100644
@@ -36,7 +36,7 @@ struct string_list;
 	ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
 	ADVICE_PUSH_UPDATE_REJECTED,
 	ADVICE_PUSH_REF_NEEDS_UPDATE,
-	ADVICE_RESET_QUIET_WARNING,
+	ADVICE_RESET_NO_REFRESH_WARNING,
 	ADVICE_RESOLVE_CONFLICT,
 	ADVICE_RM_HINTS,
 	ADVICE_SEQUENCER_IN_USE,

index 7f667e13d71..feab85e03de 100644
@@ -535,10 +535,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
-				if (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
-					advise(_("It took %.2f seconds to refresh the index after reset.  You can\n"
-						"use '--quiet' to avoid this.  Set the config setting reset.quiet to true\n"
-						"to make this the default."), t_delta_in_ms / 1000.0);
+				if (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+					advise(_("It took %.2f seconds to refresh the index after reset.  You can use\n"
+						 "'--no-refresh' to avoid this.  Set the config setting reset.refresh to false\n"
+						 "to make this the default."), t_delta_in_ms / 1000.0);
 				}
 			}
 		} else {

gitgitgadget

From: Victoria Dye <[email protected]>

If using '--quiet' or 'reset.quiet=true', do not print the 'resetnoRefresh' advice string. For applications that rely on '--quiet' disabling all non-error logs, the advice message should be suppressed accordingly.

---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index feab85e03de..c8a356ec5b0 100644
@@ -535,7 +535,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
-				if (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+				if (!quiet && advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
 					advise(_("It took %.2f seconds to refresh the index after reset.  You can use\n"
 						 "'--no-refresh' to avoid this.  Set the config setting reset.refresh to false\n"
 						 "to make this the default."), t_delta_in_ms / 1000.0);

gitgitgadget

From: Victoria Dye <[email protected]>

Add the options '-q' and '--refresh' to the 'git reset' executed in 'reset_head()', and '--refresh' to the 'git reset -q' executed in 'do_push_stash(...)'.

'stash' is implemented such that git commands invoked as part of it (e.g., 'clean', 'read-tree', 'reset', etc.) have their informational output silenced. However, the 'reset' in 'reset_head()' is *not* called with '-q', leading to the potential for a misleading printout from 'git stash apply --index' if the stash included a removed file:

Unstaged changes after reset: D <deleted file>

Not only is this confusing in its own right (since, after the reset, 'git stash' execution would stage the deletion in the index), it would be printed even when the stash was applied with the '-q' option. As a result, the messaging is removed entirely by calling 'git status' with '-q'.

Additionally, because the default behavior of 'git reset -q' is to skip refreshing the index, but later operations in 'git stash' subcommands expect a non-stale index, enable '--refresh' as well.

---
 builtin/stash.c  |  5 ++--
 t/t3903-stash.sh | 12 ++++++++++
 t/t7102-reset.sh | 60 +++++++++++++++++++++++++++++++-----------------
 3 files changed, 54 insertions(+), 23 deletions(-)

index 3e8af210fde..91407d9bbe0 100644
@@ -310,7 +310,7 @@ static int reset_head(void)
 	 * API for resetting.
 	 */
 	cp.git_cmd = 1;
-	strvec_push(&cp.args, "reset");
+	strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL);
 
 	return run_command(&cp);
 }

@@ -1633,7 +1633,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 			struct child_process cp = CHILD_PROCESS_INIT;
 
 			cp.git_cmd = 1;
-			strvec_pushl(&cp.args, "reset", "-q", "--", NULL);
+			strvec_pushl(&cp.args, "reset", "-q", "--refresh", "--",
+				     NULL);
 			add_pathspecs(&cp.args, ps);
 			if (run_command(&cp)) {
 				ret = -1;

index f36e121210e..17f2ad2344c 100755
@@ -261,6 +261,18 @@ test_expect_success 'apply -q is quiet' '
 	test_must_be_empty output.out
 '
 
+test_expect_success 'apply --index -q is quiet' '
+	# 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 &&
+	git stash apply --index -q >output.out 2>&1 &&
+	test_must_be_empty output.out
+'
+
 test_expect_success 'save -q is quiet' '
 	git stash save --quiet >output.out 2>&1 &&
 	test_must_be_empty output.out

gitgitgadget

It looks like I submitted without updating the cover letter title from a previous iteration (should be "Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash' "). I'll update it in the next version along with any code changes.

Sorry about that!

The latter is not surprising, but I have to agree with the series that the former is unbelievably bad design taste, and I am happy to see that this series separates out refreshing decision from the verbosity.

...

I believe convention would normally have this listing of the commit in-line with your discussion of it. The "[1]" probably works, too, but I can't say that I've seen that used except for URLs. Something like:

Starting at <commit>, the '--quiet' option skips refresh_index()...

Well motivated change.

	git reset -c --mixed HEAD

	git $1 reset $2 --mixed HEAD

	test_index_refreshed "-c reset.quiet=true" "" &&

	test_index_refreshed "" --no-quiet &&

After looking at your other examples, I thought of another example that you might want to consider.

In the helper, use:

	git $@ --mixed HEAD &&

and then for the callers, use

	test_index_refreshed refresh &&

or

	test_index_refreshed -c reset.quiet=true refresh &&

or

	test_index_refreshed refresh --quiet &&

and similarly through the other tests.

Thanks,
-Stolee

Good find! Code looks great.

There are some test issues in Patch 2 that you correct in patch 5, so maybe we just need to squash the changes in the right order.

However, I recommended two alternate ways to design your helper so you can test config and command-line options more easily.

Thanks,
-Stolee

As you noted in patch 5, I switched to using test_config mostly because I couldn't figure out how to get this syntax right (although, now that you point it out, I *definitely* should have seen that). I prefer using the inline config like this over test_config, so I'll update to do what you suggest here.

That's exactly what happened, and it will be fixed (and triple-checked) in V2. :)

This was mostly organizational, but I'll remove them entirely in favor of the line config option in V2.

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 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/[email protected]/

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                | 12 +++++
 t/t7102-reset.sh                | 77 ++++++++++++++++++++++++++++++---
 8 files changed, 116 insertions(+), 20 deletions(-)

base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1170%2Fvdye%2Fstash%2Fmake-reset-quiet-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1170/vdye/stash/make-reset-quiet-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1170

Range-diff vs v1:

 1:  7206ef8dd8a = 1:  7206ef8dd8a reset: revise index refresh advice
 2:  bda93703013 ! 2:  7f0226bc3e6 reset: introduce --[no-]refresh option to --mixed
     @@ Commit message
          Add a new --[no-]refresh option that is intended to explicitly determine
          whether a mixed reset should end in an index refresh.
      
     -    A few years ago, [1] introduced behavior to the '--quiet' option to skip the
     -    call to 'refresh_index(...)' at the end of a mixed reset with the goal of
     -    improving performance. However, by coupling behavior that modifies the index
     -    with the option that silences logs, there is no way for users to have one
     -    without the other (i.e., silenced logs with a refreshed index) without
     +    Starting at 9ac8125d1a (reset: don't compute unstaged changes after reset
     +    when --quiet, 2018-10-23), using the '--quiet' option results in skipping
     +    the call to 'refresh_index(...)' at the end of a mixed reset with the goal
     +    of improving performance. However, by coupling behavior that modifies the
     +    index with the option that silences logs, there is no way for users to have
     +    one without the other (i.e., silenced logs with a refreshed index) without
          incurring the overhead of a separate call to 'git update-index --refresh'.
          Furthermore, there is minimal user-facing documentation indicating that
          --quiet skips the index refresh, potentially leading to unexpected issues
     @@ Commit message
          either is set, '--quiet' and 'reset.quiet' revert to controlling only
          whether logs are silenced and do not affect index refresh.
      
     -    [1] 9ac8125d1a (reset: don't compute unstaged changes after reset when
     -        --quiet, 2018-10-23)
     -
     +    Helped-by: Derrick Stolee <[email protected]>
          Signed-off-by: Victoria Dye <[email protected]>
      
       ## Documentation/git-reset.txt ##
     @@ t/t7102-reset.sh: test_expect_success 'resetting an unmodified path is a no-op'
      +	git rm --cached file2 &&
      +
      +	# Step 2
     -+	git reset $1 --mixed HEAD &&
     ++	git $1 reset $2 --mixed HEAD &&
      +
      +	# Step 3
      +	git read-tree -m HEAD~1
     @@ 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_index_refreshed
      +'
      +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_must_fail test_index_refreshed -c reset.quiet=true &&
     -+	test_index_refreshed -c reset.quiet=true &&
     ++	! test_index_refreshed "-c reset.quiet=true" &&
     ++	test_index_refreshed "-c reset.quiet=false" &&
      +
      +	# Command line option
     -+	test_must_fail test_index_refreshed --quiet &&
     -+	test_index_refreshed --no-quiet &&
     ++	! test_index_refreshed "" --quiet &&
     ++	test_index_refreshed "" --no-quiet &&
      +
      +	# Command line option overrides config setting
     -+	test_must_fail test_index_refreshed -c reset.quiet=false --quiet &&
     -+	test_index_refreshed -c reset.refresh=true --no-quiet
     ++	! test_index_refreshed "-c reset.quiet=false" --quiet &&
     ++	test_index_refreshed "-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_must_fail test_index_refreshed -c reset.refresh=false &&
     ++	test_index_refreshed "-c reset.refresh=true" &&
     ++	! test_index_refreshed "-c reset.refresh=false" &&
      +
      +	# Command line option
     -+	test_index_refreshed --refresh &&
     -+	test_must_fail test_index_refreshed --no-refresh &&
     ++	test_index_refreshed "" --refresh &&
     ++	! test_index_refreshed "" --no-refresh &&
      +
      +	# Command line option overrides config setting
     -+	test_index_refreshed -c reset.refresh=false --refresh &&
     -+	test_must_fail test_index_refreshed -c reset.refresh=true --no-refresh
     ++	test_index_refreshed "-c reset.refresh=false" --refresh &&
     ++	! test_index_refreshed "-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 --refresh --quiet &&
     -+	test_index_refreshed --refresh -c reset.quiet=true &&
     -+	test_index_refreshed -c reset.refresh=true --quiet &&
     -+	test_index_refreshed -c reset.refresh=true -c reset.quiet=true
     ++	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_expect_success '--mixed preserves skip-worktree' '
 3:  63c5ee36feb = 3:  f869723d64f reset: replace '--quiet' with '--no-refresh' in performance advice
 4:  3c65a9f1993 = 4:  cffca0ea5c6 reset: suppress '--no-refresh' advice if logging is silenced
 5:  052499bbc93 ! 5:  3334d4cb6f3 stash: make internal resets quiet and refresh index
     @@ 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/t7102-reset.sh ##
     -@@ t/t7102-reset.sh: test_index_refreshed () {
     - 	git rm --cached file2 &&
     - 
     - 	# Step 2
     --	git reset $1 --mixed HEAD &&
     -+	git reset $@ --mixed HEAD &&
     - 
     - 	# Step 3
     - 	git read-tree -m HEAD~1
     -@@ t/t7102-reset.sh: test_index_refreshed () {
     - test_expect_success '--mixed refreshes the index' '
     - 	# Verify default behavior (with no config settings or command line
     - 	# options)
     --	test_index_refreshed &&
     -+	test_index_refreshed
     - '
     - 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_must_fail test_index_refreshed -c reset.quiet=true &&
     --	test_index_refreshed -c reset.quiet=true &&
     --
     - 	# Command line option
     --	test_must_fail test_index_refreshed --quiet &&
     -+	! test_index_refreshed --quiet &&
     - 	test_index_refreshed --no-quiet &&
     - 
     --	# Command line option overrides config setting
     --	test_must_fail test_index_refreshed -c reset.quiet=false --quiet &&
     --	test_index_refreshed -c reset.refresh=true --no-quiet
     -+	# Config: reset.quiet=false
     -+	test_config reset.quiet false &&
     -+	(
     -+		test_index_refreshed &&
     -+		! test_index_refreshed --quiet
     -+	) &&
     -+
     -+	# Config: reset.quiet=true
     -+	test_config reset.quiet true &&
     -+	(
     -+		! test_index_refreshed &&
     -+		test_index_refreshed --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_must_fail test_index_refreshed -c reset.refresh=false &&
     --
     - 	# Command line option
     - 	test_index_refreshed --refresh &&
     --	test_must_fail test_index_refreshed --no-refresh &&
     -+	! test_index_refreshed --no-refresh &&
     -+
     -+	# Config: reset.refresh=false
     -+	test_config reset.refresh false &&
     -+	(
     -+		! test_index_refreshed &&
     -+		test_index_refreshed --refresh
     -+	) &&
     - 
     --	# Command line option overrides config setting
     --	test_index_refreshed -c reset.refresh=false --refresh &&
     --	test_must_fail test_index_refreshed -c reset.refresh=true --no-refresh
     -+	# Config: reset.refresh=true
     -+	test_config reset.refresh true &&
     -+	(
     -+		test_index_refreshed &&
     -+		! test_index_refreshed --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 --refresh --quiet &&
     --	test_index_refreshed --refresh -c reset.quiet=true &&
     --	test_index_refreshed -c reset.refresh=true --quiet &&
     --	test_index_refreshed -c reset.refresh=true -c reset.quiet=true
     -+
     -+	# Config: reset.quiet=true
     -+	test_config reset.quiet true &&
     -+	test_index_refreshed --refresh &&
     -+
     -+	# Config: reset.quiet=true, reset.refresh=true
     -+	test_config reset.refresh true &&
     -+	test_index_refreshed
     - '
     - 
     - test_expect_success '--mixed preserves skip-worktree' '

gitgitgadget

From: Victoria Dye <[email protected]>

Add a new --[no-]refresh option that is intended to explicitly determine whether a mixed reset should end in an index refresh.

Starting at 9ac8125d1a (reset: don't compute unstaged changes after reset when --quiet, 2018-10-23), using the '--quiet' option results in skipping the call to 'refresh_index(...)' at the end of a mixed reset with the goal of improving performance. However, by coupling behavior that modifies the index with the option that silences logs, there is no way for users to have one without the other (i.e., silenced logs with a refreshed index) without incurring the overhead of a separate call to 'git update-index --refresh'. Furthermore, there is minimal user-facing documentation indicating that --quiet skips the index refresh, potentially leading to unexpected issues executing commands after 'git reset --quiet' that do not themselves refresh the index (e.g., internals of 'git stash', 'git read-tree').

To mitigate these issues, '--[no-]refresh' and 'reset.refresh' are introduced to provide a dedicated mechanism for refreshing the index. When either is set, '--quiet' and 'reset.quiet' revert to controlling only whether logs are silenced and do not affect index refresh.

---
 Documentation/git-reset.txt |  9 +++++
 builtin/reset.c             | 13 ++++++-
 t/t7102-reset.sh            | 77 +++++++++++++++++++++++++++++++++----
 3 files changed, 91 insertions(+), 8 deletions(-)

index 6f7685f53d5..89ddc85c2e4 100644
@@ -110,6 +110,15 @@ OPTIONS
 	`reset.quiet` config option. `--quiet` and `--no-quiet` will
 	override the default behavior.
 
+--refresh::
+--no-refresh::
+	Proactively refresh the index after a mixed reset. If unspecified, the
+	behavior falls back on the `reset.refresh` config option. If neither
+	`--[no-]refresh` nor `reset.refresh` are set, the default behavior is
+	decided by the `--[no-]quiet` option and/or `reset.quiet` config.
+	If `--quiet` is specified or `reset.quiet` is set with no command-line
+	"quiet" setting, refresh is disabled. Otherwise, refresh is enabled.
+
 --pathspec-from-file=<file>::
 	Pathspec is passed in `<file>` instead of commandline args. If
 	`<file>` is exactly `-` then standard input is used. Pathspec

index a420497a14f..7f667e13d71 100644
@@ -392,6 +392,7 @@ static int git_reset_config(const char *var, const char *value, void *cb)
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
 	int reset_type = NONE, update_ref_status = 0, quiet = 0;
+	int refresh = -1;
 	int patch_mode = 0, pathspec_file_nul = 0, unborn;
 	const char *rev, *pathspec_from_file = NULL;
 	struct object_id oid;

@@ -399,6 +400,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	int intent_to_add = 0;
 	const struct option options[] = {
 		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_BOOL(0, "refresh", &refresh,
+				N_("skip refreshing the index after reset")),
 		OPT_SET_INT(0, "mixed", &reset_type,
 						N_("reset HEAD and index"), MIXED),
 		OPT_SET_INT(0, "soft", &reset_type, N_("reset only HEAD"), SOFT),

@@ -421,11 +424,19 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 	git_config(git_reset_config, NULL);
 	git_config_get_bool("reset.quiet", &quiet);
+	git_config_get_bool("reset.refresh", &refresh);
 
 	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
 						PARSE_OPT_KEEP_DASHDASH);
 	parse_args(&pathspec, argv, prefix, patch_mode, &rev);
 
+	/*
+	 * If refresh is completely unspecified (either by config or by command
+	 * line option), decide based on 'quiet'.
+	 */
+	if (refresh < 0)
+		refresh = !quiet;
+
 	if (pathspec_from_file) {
 		if (patch_mode)
 			die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--patch");

@@ -517,7 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 			if (read_from_tree(&pathspec, &oid, intent_to_add))
 				return 1;
 			the_index.updated_skipworktree = 1;
-			if (!quiet && get_git_work_tree()) {
+			if (refresh && get_git_work_tree()) {
 				uint64_t t_begin, t_delta_in_ms;
 
 				t_begin = getnanotime();

gitgitgadget

From: Victoria Dye <[email protected]>

Update the advice describing index refresh from "enumerate unstaged changes" to "refresh the index." Describing 'refresh_index(...)' as "enumerating unstaged changes" is not fully representative of what an index refresh is doing; more generally, it updates the properties of index entries that are affected by outside-of-index state, e.g. CE_UPTODATE, which is affected by the file contents on-disk. This distinction is relevant to operations that read the index but do not refresh first - e.g., 'git read-tree' - where a stale index may cause incorrect behavior.

In addition to changing the advice message, use the "advise" function to print advice.

---
 Documentation/config/advice.txt | 4 ++--
 builtin/reset.c                 | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

index c40eb09cb7e..971aad2f237 100644
@@ -69,8 +69,8 @@ advice.*::
 		merge to avoid overwriting local changes.
 	resetQuiet::
 		Advice to consider using the `--quiet` option to linkgit:git-reset[1]
-		when the command takes more than 2 seconds to enumerate unstaged
-		changes after reset.
+		when the command takes more than 2 seconds to refresh the index
+		after reset.
 	resolveConflict::
 		Advice shown by various commands when conflicts
 		prevent the operation from being performed.

index 6e65e90c5db..a420497a14f 100644
@@ -525,9 +525,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 					      _("Unstaged changes after reset:"));
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
 				if (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
-					printf(_("\nIt took %.2f seconds to enumerate unstaged changes after reset.  You can\n"
+					advise(_("It took %.2f seconds to refresh the index after reset.  You can\n"
 						"use '--quiet' to avoid this.  Set the config setting reset.quiet to true\n"
-						"to make this the default.\n"), t_delta_in_ms / 1000.0);
+						"to make this the default."), t_delta_in_ms / 1000.0);
 				}
 			}
 		} else {

gitgitgadget

From: Victoria Dye <[email protected]>

If using '--quiet' or 'reset.quiet=true', do not print the 'resetnoRefresh' advice string. For applications that rely on '--quiet' disabling all non-error logs, the advice message should be suppressed accordingly.

---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index feab85e03de..c8a356ec5b0 100644
@@ -535,7 +535,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
-				if (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+				if (!quiet && advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
 					advise(_("It took %.2f seconds to refresh the index after reset.  You can use\n"
 						 "'--no-refresh' to avoid this.  Set the config setting reset.refresh to false\n"
 						 "to make this the default."), t_delta_in_ms / 1000.0);

gitgitgadget

From: Victoria Dye <[email protected]>

Replace references to '--quiet' with '--no-refresh' in the advice on how to skip refreshing the index. When the advice was introduced, '--quiet' was the only way to avoid the expensive 'refresh_index(...)' at the end of a mixed reset. After introducing '--no-refresh', however, '--quiet' became only a fallback option for determining refresh behavior, overridden by '--[no-]refresh' or 'reset.refresh' if either is set. To ensure users are advised to use the most reliable option for avoiding 'refresh_index(...)', replace recommendation of '--quiet' with '--[no-]refresh'.

---
 Documentation/config/advice.txt | 8 ++++----
 advice.c                        | 2 +-
 advice.h                        | 2 +-
 builtin/reset.c                 | 8 ++++----
 4 files changed, 10 insertions(+), 10 deletions(-)

index 971aad2f237..83c2a956611 100644
@@ -67,10 +67,10 @@ advice.*::
 	commitBeforeMerge::
 		Advice shown when linkgit:git-merge[1] refuses to
 		merge to avoid overwriting local changes.
-	resetQuiet::
-		Advice to consider using the `--quiet` option to linkgit:git-reset[1]
-		when the command takes more than 2 seconds to refresh the index
-		after reset.
+	resetNoRefresh::
+		Advice to consider using the `--no-refresh` option to
+		linkgit:git-reset[1] when the command takes more than 2 seconds
+		to refresh the index after reset.
 	resolveConflict::
 		Advice shown by various commands when conflicts
 		prevent the operation from being performed.

index 2e1fd483040..cb755c551a2 100644
@@ -61,7 +61,7 @@ static struct {
 	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching", 1 },
 	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName", 1 },
 	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected", 1 },
-	[ADVICE_RESET_QUIET_WARNING]			= { "resetQuiet", 1 },
+	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh", 1 },
 	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict", 1 },
 	[ADVICE_RM_HINTS]				= { "rmHints", 1 },
 	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse", 1 },

index a3957123a16..f95af70ced4 100644
@@ -36,7 +36,7 @@ struct string_list;
 	ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
 	ADVICE_PUSH_UPDATE_REJECTED,
 	ADVICE_PUSH_REF_NEEDS_UPDATE,
-	ADVICE_RESET_QUIET_WARNING,
+	ADVICE_RESET_NO_REFRESH_WARNING,
 	ADVICE_RESOLVE_CONFLICT,
 	ADVICE_RM_HINTS,
 	ADVICE_SEQUENCER_IN_USE,

index 7f667e13d71..feab85e03de 100644
@@ -535,10 +535,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
-				if (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
-					advise(_("It took %.2f seconds to refresh the index after reset.  You can\n"
-						"use '--quiet' to avoid this.  Set the config setting reset.quiet to true\n"
-						"to make this the default."), t_delta_in_ms / 1000.0);
+				if (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+					advise(_("It took %.2f seconds to refresh the index after reset.  You can use\n"
+						 "'--no-refresh' to avoid this.  Set the config setting reset.refresh to false\n"
+						 "to make this the default."), t_delta_in_ms / 1000.0);
 				}
 			}
 		} else {

gitgitgadget

From: Victoria Dye <[email protected]>

Add the options '-q' and '--refresh' to the 'git reset' executed in 'reset_head()', and '--refresh' to the 'git reset -q' executed in 'do_push_stash(...)'.

'stash' is implemented such that git commands invoked as part of it (e.g., 'clean', 'read-tree', 'reset', etc.) have their informational output silenced. However, the 'reset' in 'reset_head()' is *not* called with '-q', leading to the potential for a misleading printout from 'git stash apply --index' if the stash included a removed file:

Unstaged changes after reset: D <deleted file>

Not only is this confusing in its own right (since, after the reset, 'git stash' execution would stage the deletion in the index), it would be printed even when the stash was applied with the '-q' option. As a result, the messaging is removed entirely by calling 'git status' with '-q'.

Additionally, because the default behavior of 'git reset -q' is to skip refreshing the index, but later operations in 'git stash' subcommands expect a non-stale index, enable '--refresh' as well.

---
 builtin/stash.c  |  5 +++--
 t/t3903-stash.sh | 12 ++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

index 3e8af210fde..91407d9bbe0 100644
@@ -310,7 +310,7 @@ static int reset_head(void)
 	 * API for resetting.
 	 */
 	cp.git_cmd = 1;
-	strvec_push(&cp.args, "reset");
+	strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL);
 
 	return run_command(&cp);
 }

@@ -1633,7 +1633,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 			struct child_process cp = CHILD_PROCESS_INIT;
 
 			cp.git_cmd = 1;
-			strvec_pushl(&cp.args, "reset", "-q", "--", NULL);
+			strvec_pushl(&cp.args, "reset", "-q", "--refresh", "--",
+				     NULL);
 			add_pathspecs(&cp.args, ps);
 			if (run_command(&cp)) {
 				ret = -1;

index f36e121210e..17f2ad2344c 100755
@@ -261,6 +261,18 @@ test_expect_success 'apply -q is quiet' '
 	test_must_be_empty output.out
 '
 
+test_expect_success 'apply --index -q is quiet' '
+	# 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 &&
+	git stash apply --index -q >output.out 2>&1 &&
+	test_must_be_empty output.out
+'
+
 test_expect_success 'save -q is quiet' '
 	git stash save --quiet >output.out 2>&1 &&
 	test_must_be_empty output.out

gitgitgadget

Thanks. This version answers all of my concerns and looks ready to go from my perspective.

Thanks,
-Stolee

No complaints, but it is somewhat unsatisfying that we need these two steps that keep --quiet tied to the decision to or not to refresh. In the longer term, it may be cleaner to completely dissociate them, but it probably is not a huge deal.

	# remove file2 from the index

	# resurrect file2 to the index from HEAD; if the cached stat
	# info gets refreshed, this brings us back to the state
        # after Step 0.  If not, "diff-files" would report file2 is
	# different.

Funny, but it is true that we would want to squelch advice messages under 'quiet' mode.

But why did we even spend 2 seconds to refresh the index if --quiet is given in the first place? Isn't it because an explicit --refresh or reset.refresh=yes was in effect? IOW, the user wanted the command to be quiet but still wanted it to refresh the index.

So it would be yet another reason why we do not want to show the
message: the user knows what they are doing and explicitly asked us to spend cycles to refresh.

I've read this iteration, too, and except for "we should test not just 'apply -index' but 'push'" in [5/5], everything looked good.

Thanks.

These are all helpful suggestions, I'll include them in a re-roll (specifically: rename 'test_index_refreshed' to something mentioning 'reset', move the test comments inline with the steps they execute, and use 'diff-files' rather than 'read-tree').

Thanks!

The '-q' option was already present in the reset in 'do_push_stash()', but I did add the '--refresh' option to ensure that (for example) 'git stash push --staged' refreshes the index. With that in mind...

yes, both cases where '--refresh' was added should be tested (since both will fail if they don't include that option). I'll add them in the next iteration.

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/[email protected]/

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

From: Victoria Dye <[email protected]>

Update the advice describing index refresh from "enumerate unstaged changes" to "refresh the index." Describing 'refresh_index(...)' as "enumerating unstaged changes" is not fully representative of what an index refresh is doing; more generally, it updates the properties of index entries that are affected by outside-of-index state, e.g. CE_UPTODATE, which is affected by the file contents on-disk. This distinction is relevant to operations that read the index but do not refresh first - e.g., 'git read-tree' - where a stale index may cause incorrect behavior.

In addition to changing the advice message, use the "advise" function to print advice.

---
 Documentation/config/advice.txt | 4 ++--
 builtin/reset.c                 | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

index c40eb09cb7e..971aad2f237 100644
@@ -69,8 +69,8 @@ advice.*::
 		merge to avoid overwriting local changes.
 	resetQuiet::
 		Advice to consider using the `--quiet` option to linkgit:git-reset[1]
-		when the command takes more than 2 seconds to enumerate unstaged
-		changes after reset.
+		when the command takes more than 2 seconds to refresh the index
+		after reset.
 	resolveConflict::
 		Advice shown by various commands when conflicts
 		prevent the operation from being performed.

index 6e65e90c5db..a420497a14f 100644
@@ -525,9 +525,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 					      _("Unstaged changes after reset:"));
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
 				if (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
-					printf(_("\nIt took %.2f seconds to enumerate unstaged changes after reset.  You can\n"
+					advise(_("It took %.2f seconds to refresh the index after reset.  You can\n"
 						"use '--quiet' to avoid this.  Set the config setting reset.quiet to true\n"
-						"to make this the default.\n"), t_delta_in_ms / 1000.0);
+						"to make this the default."), t_delta_in_ms / 1000.0);
 				}
 			}
 		} else {

gitgitgadget

From: Victoria Dye <[email protected]>

Add a new --[no-]refresh option that is intended to explicitly determine whether a mixed reset should end in an index refresh.

Starting at 9ac8125d1a (reset: don't compute unstaged changes after reset when --quiet, 2018-10-23), using the '--quiet' option results in skipping the call to 'refresh_index(...)' at the end of a mixed reset with the goal of improving performance. However, by coupling behavior that modifies the index with the option that silences logs, there is no way for users to have one without the other (i.e., silenced logs with a refreshed index) without incurring the overhead of a separate call to 'git update-index --refresh'. Furthermore, there is minimal user-facing documentation indicating that --quiet skips the index refresh, potentially leading to unexpected issues executing commands after 'git reset --quiet' that do not themselves refresh the index (e.g., internals of 'git stash', 'git read-tree').

To mitigate these issues, '--[no-]refresh' and 'reset.refresh' are introduced to provide a dedicated mechanism for refreshing the index. When either is set, '--quiet' and 'reset.quiet' revert to controlling only whether logs are silenced and do not affect index refresh.

---
 Documentation/git-reset.txt |  9 +++++
 builtin/reset.c             | 13 ++++++-
 t/t7102-reset.sh            | 73 +++++++++++++++++++++++++++++++++----
 3 files changed, 87 insertions(+), 8 deletions(-)

index 6f7685f53d5..89ddc85c2e4 100644
@@ -110,6 +110,15 @@ OPTIONS
 	`reset.quiet` config option. `--quiet` and `--no-quiet` will
 	override the default behavior.
 
+--refresh::
+--no-refresh::
+	Proactively refresh the index after a mixed reset. If unspecified, the
+	behavior falls back on the `reset.refresh` config option. If neither
+	`--[no-]refresh` nor `reset.refresh` are set, the default behavior is
+	decided by the `--[no-]quiet` option and/or `reset.quiet` config.
+	If `--quiet` is specified or `reset.quiet` is set with no command-line
+	"quiet" setting, refresh is disabled. Otherwise, refresh is enabled.
+
 --pathspec-from-file=<file>::
 	Pathspec is passed in `<file>` instead of commandline args. If
 	`<file>` is exactly `-` then standard input is used. Pathspec

index a420497a14f..7f667e13d71 100644
@@ -392,6 +392,7 @@ static int git_reset_config(const char *var, const char *value, void *cb)
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
 	int reset_type = NONE, update_ref_status = 0, quiet = 0;
+	int refresh = -1;
 	int patch_mode = 0, pathspec_file_nul = 0, unborn;
 	const char *rev, *pathspec_from_file = NULL;
 	struct object_id oid;

@@ -399,6 +400,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	int intent_to_add = 0;
 	const struct option options[] = {
 		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_BOOL(0, "refresh", &refresh,
+				N_("skip refreshing the index after reset")),
 		OPT_SET_INT(0, "mixed", &reset_type,
 						N_("reset HEAD and index"), MIXED),
 		OPT_SET_INT(0, "soft", &reset_type, N_("reset only HEAD"), SOFT),

@@ -421,11 +424,19 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 	git_config(git_reset_config, NULL);
 	git_config_get_bool("reset.quiet", &quiet);
+	git_config_get_bool("reset.refresh", &refresh);
 
 	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
 						PARSE_OPT_KEEP_DASHDASH);
 	parse_args(&pathspec, argv, prefix, patch_mode, &rev);
 
+	/*
+	 * If refresh is completely unspecified (either by config or by command
+	 * line option), decide based on 'quiet'.
+	 */
+	if (refresh < 0)
+		refresh = !quiet;
+
 	if (pathspec_from_file) {
 		if (patch_mode)
 			die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--patch");

@@ -517,7 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 			if (read_from_tree(&pathspec, &oid, intent_to_add))
 				return 1;
 			the_index.updated_skipworktree = 1;
-			if (!quiet && get_git_work_tree()) {
+			if (refresh && get_git_work_tree()) {
 				uint64_t t_begin, t_delta_in_ms;
 
 				t_begin = getnanotime();

gitgitgadget

From: Victoria Dye <[email protected]>

Replace references to '--quiet' with '--no-refresh' in the advice on how to skip refreshing the index. When the advice was introduced, '--quiet' was the only way to avoid the expensive 'refresh_index(...)' at the end of a mixed reset. After introducing '--no-refresh', however, '--quiet' became only a fallback option for determining refresh behavior, overridden by '--[no-]refresh' or 'reset.refresh' if either is set. To ensure users are advised to use the most reliable option for avoiding 'refresh_index(...)', replace recommendation of '--quiet' with '--[no-]refresh'.

---
 Documentation/config/advice.txt | 8 ++++----
 advice.c                        | 2 +-
 advice.h                        | 2 +-
 builtin/reset.c                 | 8 ++++----
 4 files changed, 10 insertions(+), 10 deletions(-)

index 971aad2f237..83c2a956611 100644
@@ -67,10 +67,10 @@ advice.*::
 	commitBeforeMerge::
 		Advice shown when linkgit:git-merge[1] refuses to
 		merge to avoid overwriting local changes.
-	resetQuiet::
-		Advice to consider using the `--quiet` option to linkgit:git-reset[1]
-		when the command takes more than 2 seconds to refresh the index
-		after reset.
+	resetNoRefresh::
+		Advice to consider using the `--no-refresh` option to
+		linkgit:git-reset[1] when the command takes more than 2 seconds
+		to refresh the index after reset.
 	resolveConflict::
 		Advice shown by various commands when conflicts
 		prevent the operation from being performed.

index 2e1fd483040..cb755c551a2 100644
@@ -61,7 +61,7 @@ static struct {
 	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching", 1 },
 	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName", 1 },
 	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected", 1 },
-	[ADVICE_RESET_QUIET_WARNING]			= { "resetQuiet", 1 },
+	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh", 1 },
 	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict", 1 },
 	[ADVICE_RM_HINTS]				= { "rmHints", 1 },
 	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse", 1 },

index a3957123a16..f95af70ced4 100644
@@ -36,7 +36,7 @@ struct string_list;
 	ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
 	ADVICE_PUSH_UPDATE_REJECTED,
 	ADVICE_PUSH_REF_NEEDS_UPDATE,
-	ADVICE_RESET_QUIET_WARNING,
+	ADVICE_RESET_NO_REFRESH_WARNING,
 	ADVICE_RESOLVE_CONFLICT,
 	ADVICE_RM_HINTS,
 	ADVICE_SEQUENCER_IN_USE,

index 7f667e13d71..feab85e03de 100644
@@ -535,10 +535,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
-				if (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
-					advise(_("It took %.2f seconds to refresh the index after reset.  You can\n"
-						"use '--quiet' to avoid this.  Set the config setting reset.quiet to true\n"
-						"to make this the default."), t_delta_in_ms / 1000.0);
+				if (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+					advise(_("It took %.2f seconds to refresh the index after reset.  You can use\n"
+						 "'--no-refresh' to avoid this.  Set the config setting reset.refresh to false\n"
+						 "to make this the default."), t_delta_in_ms / 1000.0);
 				}
 			}
 		} else {

gitgitgadget

From: Victoria Dye <[email protected]>

If using '--quiet' or 'reset.quiet=true', do not print the 'resetnoRefresh' advice string. For applications that rely on '--quiet' disabling all non-error logs, the advice message should be suppressed accordingly.

---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index feab85e03de..c8a356ec5b0 100644
@@ -535,7 +535,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				refresh_index(&the_index, flags, NULL, NULL,
 					      _("Unstaged changes after reset:"));
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
-				if (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+				if (!quiet && advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
 					advise(_("It took %.2f seconds to refresh the index after reset.  You can use\n"
 						 "'--no-refresh' to avoid this.  Set the config setting reset.refresh to false\n"
 						 "to make this the default."), t_delta_in_ms / 1000.0);

gitgitgadget

From: Victoria Dye <[email protected]>

Add the options '-q' and '--refresh' to the 'git reset' executed in 'reset_head()', and '--refresh' to the 'git reset -q' executed in 'do_push_stash(...)'.

'stash' is implemented such that git commands invoked as part of it (e.g., 'clean', 'read-tree', 'reset', etc.) have their informational output silenced. However, the 'reset' in 'reset_head()' is *not* called with '-q', leading to the potential for a misleading printout from 'git stash apply --index' if the stash included a removed file:

Unstaged changes after reset: D <deleted file>

Not only is this confusing in its own right (since, after the reset, 'git stash' execution would stage the deletion in the index), it would be printed even when the stash was applied with the '-q' option. As a result, the messaging is removed entirely by calling 'git status' with '-q'.

Additionally, because the default behavior of 'git reset -q' is to skip refreshing the index, but later operations in 'git stash' subcommands expect a non-stale index, enable '--refresh' as well.

---
 builtin/stash.c  |  5 +++--
 t/t3903-stash.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

index 3e8af210fde..91407d9bbe0 100644
@@ -310,7 +310,7 @@ static int reset_head(void)
 	 * API for resetting.
 	 */
 	cp.git_cmd = 1;
-	strvec_push(&cp.args, "reset");
+	strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL);
 
 	return run_command(&cp);
 }

@@ -1633,7 +1633,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 			struct child_process cp = CHILD_PROCESS_INIT;
 
 			cp.git_cmd = 1;
-			strvec_pushl(&cp.args, "reset", "-q", "--", NULL);
+			strvec_pushl(&cp.args, "reset", "-q", "--refresh", "--",
+				     NULL);
 			add_pathspecs(&cp.args, ps);
 			if (run_command(&cp)) {
 				ret = -1;

index f36e121210e..1a5c1bd3109 100755
@@ -261,6 +261,18 @@ test_expect_success 'apply -q is quiet' '
 	test_must_be_empty output.out
 '
 
+test_expect_success 'apply --index -q is quiet' '
+	# 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 &&
+	git stash apply --index -q >output.out 2>&1 &&
+	test_must_be_empty output.out
+'
+
 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 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

Hmph. So being quiet and refreshing index are tested separately in different tests, and this one is only about being quiet?

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?

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

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

OK.

Hi Victoria

[replying to v3 rather than v4 as gmail still hasn't delivered that version]

I would very much prefer to see "--quiet" not making contribution to the decision to refresh or not in the longer term. Many plumbing commands expect that the calling scripts refresh the index with an explicit use of "update-index --refresh" and leave the index not refreshed, but working on unrefreshed index is a trade-off between performance and correctness.

* Turning "--quiet" not to refresh may incur performance regression for shorter term. It will not hurt correctness.

* Introducing "--no-refresh" to mark "reset --quiet" invocations, where the freshness of the index does not matter for correctness, would help regain performance without breaking scripts. All "reset --quiet" invocations in scripts written before this series are supposed to be safe (as they lived with their "reset --quiet" that does not refresh), but newly written scripts may start expecting that "reset --quiet" would refresh for correctness.

* If we allow reset.refresh to be set to "no", however, that would affect _all_ uses of "reset --quiet", including the ones in newly written scripts that expect "reset --quiet" to refresh. They would be forced to say "reset --quiet --refresh", just in case the user has such a configuration; otherwise these scripts will be declared "buggy" for not explicitly saying "--refresh".

I do not think reset.refresh is a good idea, but I very much like the idea to making "reset" (regardless of "--quiet") to refresh by default.

Thanks.

I tend to agree with you and Phillip on this. I took a more conservative approach with the intention of preserving as much backward compatibility as possible, but having '--quiet' disable refresh (to me) actively hurts its correctness. If backwards-compatibility isn't a huge concern, I'll gladly make that change.

I added the option as a "replacement" for 'reset.quiet' (specifically, its ability to summarily disable refresh), but I can definitely see how it would lead to issues in the future. I'm happy to remove it, but should 'reset.quiet' be removed as well? No other commands have a config option for 'quiet', and it presents a similar issue of potentially suppressing a helpful feature (in this case, informational printouts) across all invocations unless otherwise specified.

I was hesitant to go this far because it would force people that were comfortably relying on 'reset.quiet' to need to always use '--no-refresh' to get the same behavior. But, to Phillip's point earlier, there are other options now (like sparse index) that could provide a just-as-substantial (if not greater) performance boost without sacrificing the refresh.

Since this is already in 'next' (and, in its current state, I don't think it does any more damage than the pre-series state), I'll send a new series on top of this that deprecates 'reset.refresh' and 'reset.quiet', and makes '--refresh' the default for all of 'reset'.

Thanks, both!