[PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash'

[PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash'

[Victoria Dye via GitGitGadget]

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/20181023190423.5772-2-peartben@gmail.com/

Thanks! -Victoria

Victoria Dye (5):
  reset: revise index refresh advice
  reset: introduce --[no-]refresh option to --mixed
  reset: replace '--quiet' with '--no-refresh' in performance advice
  reset: suppress '--no-refresh' advice if logging is silenced
  stash: make internal resets quiet and refresh index

 Documentation/config/advice.txt |  8 +--
 Documentation/git-reset.txt     |  9 ++++
 advice.c                        |  2 +-
 advice.h                        |  2 +-
 builtin/reset.c                 | 21 ++++++--
 builtin/stash.c                 |  5 +-
 t/t3903-stash.sh                | 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

[PATCH 1/5] reset: revise index refresh advice

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/config/advice.txt | 4 ++--
 builtin/reset.c                 | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c40eb09cb7e..971aad2f237 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -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.
diff --git a/builtin/reset.c b/builtin/reset.c
index 6e65e90c5db..a420497a14f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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

[PATCH 2/5] reset: introduce --[no-]refresh option to --mixed

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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)

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/git-reset.txt |  9 +++++
 builtin/reset.c             | 13 ++++++-
 t/t7102-reset.sh            | 77 +++++++++++++++++++++++++++++++++----
 3 files changed, 91 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 6f7685f53d5..89ddc85c2e4 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -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
diff --git a/builtin/reset.c b/builtin/reset.c
index a420497a14f..7f667e13d71 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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();
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index d05426062ec..5e68180f3b2 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -462,14 +462,77 @@ 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.
+
+	# Step 0: start with a clean index
+	git reset --hard HEAD &&
+
+	# Step 1
+	git rm --cached file2 &&
+
+	# Step 2
+	git reset $1 --mixed HEAD &&
+
+	# Step 3
+	git read-tree -m HEAD~1
+}
+
 test_expect_success '--mixed refreshes the index' '
-	cat >expect <<-\EOF &&
-	Unstaged changes after reset:
-	M	file2
-	EOF
-	echo 123 >>file2 &&
-	git reset --mixed HEAD >output &&
-	test_cmp expect output
+	# Verify default behavior (with no config settings or command line
+	# options)
+	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 --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_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 &&
+
+	# 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_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_expect_success '--mixed preserves skip-worktree' '
-- 
gitgitgadget

Re: [PATCH 2/5] reset: introduce --[no-]refresh option to --mixed

[Derrick Stolee]

On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
> 
> 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
...
> [1] 9ac8125d1a (reset: don't compute unstaged changes after reset when
>     --quiet, 2018-10-23)

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

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

Well motivated change.

> +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.
> +
> +	# Step 0: start with a clean index
> +	git reset --hard HEAD &&
> +
> +	# Step 1
> +	git rm --cached file2 &&
> +
> +	# Step 2
> +	git reset $1 --mixed HEAD &&
> +
> +	# Step 3
> +	git read-tree -m HEAD~1
> +}
> +
>  test_expect_success '--mixed refreshes the index' '
> -	cat >expect <<-\EOF &&
> -	Unstaged changes after reset:
> -	M	file2
> -	EOF
> -	echo 123 >>file2 &&
> -	git reset --mixed HEAD >output &&
> -	test_cmp expect output
> +	# Verify default behavior (with no config settings or command line
> +	# options)
> +	test_index_refreshed &&
> +'

It looks like this test ends with an &&. There's also a missing newline
after the test.

> +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 &&

This is failing, but not for the reason you want: It is running

	git reset -c --mixed HEAD

and failing to parse the "-c", I bet.

Perhaps you want to have two arguments: one for config settings
and another for arguments, meaning your call in test_index_refreshed
should be

	git $1 reset $2 --mixed HEAD

and calls like this should be

	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 --no-quiet &&

If you take a change like I recommend above, these would be

	test_index_refreshed "" --no-quiet &&

Thanks,
-Stolee

Re: [PATCH 2/5] reset: introduce --[no-]refresh option to --mixed

[Derrick Stolee]

On 3/14/2022 11:05 AM, Derrick Stolee wrote:
> On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <vdye@github.com>

>> +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.
>> +
>> +	# Step 0: start with a clean index
>> +	git reset --hard HEAD &&
>> +
>> +	# Step 1
>> +	git rm --cached file2 &&
>> +
>> +	# Step 2
>> +	git reset $1 --mixed HEAD &&

> Perhaps you want to have two arguments: one for config settings
> and another for arguments, meaning your call in test_index_refreshed
> should be
> 
> 	git $1 reset $2 --mixed HEAD
> 
> and calls like this should be
> 
> 	test_index_refreshed "-c reset.quiet=true" "" &&

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

Re: [PATCH 2/5] reset: introduce --[no-]refresh option to --mixed

[Victoria Dye]

Derrick Stolee wrote:
> On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <vdye@github.com>
>>
>> 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
> ...
>> [1] 9ac8125d1a (reset: don't compute unstaged changes after reset when
>>     --quiet, 2018-10-23)
> 
> 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()...
> 
>> 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.
> 
> Well motivated change.
> 
>> +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.
>> +
>> +	# Step 0: start with a clean index
>> +	git reset --hard HEAD &&
>> +
>> +	# Step 1
>> +	git rm --cached file2 &&
>> +
>> +	# Step 2
>> +	git reset $1 --mixed HEAD &&
>> +
>> +	# Step 3
>> +	git read-tree -m HEAD~1
>> +}
>> +
>>  test_expect_success '--mixed refreshes the index' '
>> -	cat >expect <<-\EOF &&
>> -	Unstaged changes after reset:
>> -	M	file2
>> -	EOF
>> -	echo 123 >>file2 &&
>> -	git reset --mixed HEAD >output &&
>> -	test_cmp expect output
>> +	# Verify default behavior (with no config settings or command line
>> +	# options)
>> +	test_index_refreshed &&
>> +'
> 
> It looks like this test ends with an &&. There's also a missing newline
> after the test.
> 
>> +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 &&
> 
> This is failing, but not for the reason you want: It is running
> 
> 	git reset -c --mixed HEAD
> 
> and failing to parse the "-c", I bet.
> 
> Perhaps you want to have two arguments: one for config settings
> and another for arguments, meaning your call in test_index_refreshed
> should be
> 
> 	git $1 reset $2 --mixed HEAD
> 
> and calls like this should be
> 
> 	test_index_refreshed "-c reset.quiet=true" "" &&
> 

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.

>> +	test_index_refreshed -c reset.quiet=true &&
>> +
>> +	# Command line option
>> +	test_must_fail test_index_refreshed --quiet &&
>> +	test_index_refreshed --no-quiet &&
> 
> If you take a change like I recommend above, these would be
> 
> 	test_index_refreshed "" --no-quiet &&
> 
> Thanks,
> -Stolee

[PATCH 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/config/advice.txt | 8 ++++----
 advice.c                        | 2 +-
 advice.h                        | 2 +-
 builtin/reset.c                 | 8 ++++----
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 971aad2f237..83c2a956611 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -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.
diff --git a/advice.c b/advice.c
index 2e1fd483040..cb755c551a2 100644
--- a/advice.c
+++ b/advice.c
@@ -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 },
diff --git a/advice.h b/advice.h
index a3957123a16..f95af70ced4 100644
--- a/advice.h
+++ b/advice.h
@@ -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,
diff --git a/builtin/reset.c b/builtin/reset.c
index 7f667e13d71..feab85e03de 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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

[PATCH 4/5] reset: suppress '--no-refresh' advice if logging is silenced

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index feab85e03de..c8a356ec5b0 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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

[PATCH 5/5] stash: make internal resets quiet and refresh index

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/stash.c  |  5 ++--
 t/t3903-stash.sh | 12 ++++++++++
 t/t7102-reset.sh | 60 +++++++++++++++++++++++++++++++-----------------
 3 files changed, 54 insertions(+), 23 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 3e8af210fde..91407d9bbe0 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -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;
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index f36e121210e..17f2ad2344c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 5e68180f3b2..f2076545133 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -482,7 +482,7 @@ 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
@@ -491,48 +491,66 @@ 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

Re: [PATCH 5/5] stash: make internal resets quiet and refresh index

[Derrick Stolee]

On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
> 
> 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(...)'.

> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 5e68180f3b2..f2076545133 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -482,7 +482,7 @@ test_index_refreshed () {
>  	git rm --cached file2 &&
>  
>  	# Step 2
> -	git reset $1 --mixed HEAD &&
> +	git reset $@ --mixed HEAD &&

I see you change this from "$1" to "$@", which won't help with
the "-c key=value" issues from earlier.

>  
>  	# Step 3
>  	git read-tree -m HEAD~1
> @@ -491,48 +491,66 @@ 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

Ah, I see you fixed this here, probably just a rebase issue, then.

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

Ah, and the -c changes are removed here. You could still test them
using the trick I mention in reply to patch 2.

>  	# 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
> +	)

I'm not sure why you need sub-shells here. The test_config is
not scoped to the shell. These lines could be avoided with the
-c trick, which should make it a bit simpler to show what you
intend to be testing here.

Thanks,
-Stolee

Re: [PATCH 5/5] stash: make internal resets quiet and refresh index

[Victoria Dye]

Derrick Stolee wrote:
> On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <vdye@github.com>
>>
>> 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(...)'.
> 
>> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
>> index 5e68180f3b2..f2076545133 100755
>> --- a/t/t7102-reset.sh
>> +++ b/t/t7102-reset.sh
>> @@ -482,7 +482,7 @@ test_index_refreshed () {
>>  	git rm --cached file2 &&
>>  
>>  	# Step 2
>> -	git reset $1 --mixed HEAD &&
>> +	git reset $@ --mixed HEAD &&
> 
> I see you change this from "$1" to "$@", which won't help with
> the "-c key=value" issues from earlier.
> 
>>  
>>  	# Step 3
>>  	git read-tree -m HEAD~1
>> @@ -491,48 +491,66 @@ 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
> 
> Ah, I see you fixed this here, probably just a rebase issue, then.
> 

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

>>  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 &&
>> -
> 
> Ah, and the -c changes are removed here. You could still test them
> using the trick I mention in reply to patch 2.
> 
>>  	# 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
>> +	)
> 
> I'm not sure why you need sub-shells here. The test_config is
> not scoped to the shell. These lines could be avoided with the
> -c trick, which should make it a bit simpler to show what you
> intend to be testing here.
> 

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

> Thanks,
> -Stolee

Re: [PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash'

[Victoria Dye]

Victoria Dye via GitGitGadget wrote:
> 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/20181023190423.5772-2-peartben@gmail.com/
> 
> Thanks! -Victoria
> 
> Victoria Dye (5):
>   reset: revise index refresh advice
>   reset: introduce --[no-]refresh option to --mixed
>   reset: replace '--quiet' with '--no-refresh' in performance advice
>   reset: suppress '--no-refresh' advice if logging is silenced
>   stash: make internal resets quiet and refresh index
> 
>  Documentation/config/advice.txt |  8 +--
>  Documentation/git-reset.txt     |  9 ++++
>  advice.c                        |  2 +-
>  advice.h                        |  2 +-
>  builtin/reset.c                 | 21 ++++++--
>  builtin/stash.c                 |  5 +-
>  t/t3903-stash.sh                | 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

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!

Re: [PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash'

[Junio C Hamano]

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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.

Re: [PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash'

[Derrick Stolee]

On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:
> 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.

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

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

[Victoria Dye via GitGitGadget]

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/20181023190423.5772-2-peartben@gmail.com/

Thanks! -Victoria

Victoria Dye (5):
  reset: revise index refresh advice
  reset: introduce --[no-]refresh option to --mixed
  reset: replace '--quiet' with '--no-refresh' in performance advice
  reset: suppress '--no-refresh' advice if logging is silenced
  stash: make internal resets quiet and refresh index

 Documentation/config/advice.txt |  8 ++--
 Documentation/git-reset.txt     |  9 ++++
 advice.c                        |  2 +-
 advice.h                        |  2 +-
 builtin/reset.c                 | 21 ++++++---
 builtin/stash.c                 |  5 ++-
 t/t3903-stash.sh                | 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 <derrickstolee@github.com>
          Signed-off-by: Victoria Dye <vdye@github.com>
      
       ## Documentation/git-reset.txt ##
     @@ t/t7102-reset.sh: test_expect_success 'resetting an unmodified path is a no-op'
      +	git 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

[PATCH v2 1/5] reset: revise index refresh advice

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/config/advice.txt | 4 ++--
 builtin/reset.c                 | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c40eb09cb7e..971aad2f237 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -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.
diff --git a/builtin/reset.c b/builtin/reset.c
index 6e65e90c5db..a420497a14f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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

[PATCH v2 2/5] reset: introduce --[no-]refresh option to --mixed

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/git-reset.txt |  9 +++++
 builtin/reset.c             | 13 ++++++-
 t/t7102-reset.sh            | 77 +++++++++++++++++++++++++++++++++----
 3 files changed, 91 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 6f7685f53d5..89ddc85c2e4 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -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
diff --git a/builtin/reset.c b/builtin/reset.c
index a420497a14f..7f667e13d71 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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();
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index d05426062ec..005940778b7 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -462,14 +462,77 @@ 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.
+
+	# Step 0: start with a clean index
+	git reset --hard HEAD &&
+
+	# Step 1
+	git rm --cached file2 &&
+
+	# Step 2
+	git $1 reset $2 --mixed HEAD &&
+
+	# Step 3
+	git read-tree -m HEAD~1
+}
+
 test_expect_success '--mixed refreshes the index' '
-	cat >expect <<-\EOF &&
-	Unstaged changes after reset:
-	M	file2
-	EOF
-	echo 123 >>file2 &&
-	git reset --mixed HEAD >output &&
-	test_cmp expect output
+	# Verify default behavior (with no config settings or command line
+	# options)
+	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_index_refreshed "-c reset.quiet=true" &&
+	test_index_refreshed "-c reset.quiet=false" &&
+
+	# Command line option
+	! test_index_refreshed "" --quiet &&
+	test_index_refreshed "" --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_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" &&
+
+	# Command line option
+	test_index_refreshed "" --refresh &&
+	! test_index_refreshed "" --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_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_expect_success '--mixed preserves skip-worktree' '
-- 
gitgitgadget

Re: [PATCH v2 2/5] reset: introduce --[no-]refresh option to --mixed

[Junio C Hamano]

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Victoria Dye <vdye@github.com>
>
> 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.
>
> Helped-by: Derrick Stolee <derrickstolee@github.com>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  Documentation/git-reset.txt |  9 +++++
>  builtin/reset.c             | 13 ++++++-
>  t/t7102-reset.sh            | 77 +++++++++++++++++++++++++++++++++----
>  3 files changed, 91 insertions(+), 8 deletions(-)

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.

> +	/*
> +	 * If refresh is completely unspecified (either by config or by command
> +	 * line option), decide based on 'quiet'.
> +	 */
> +	if (refresh < 0)
> +		refresh = !quiet;

OK.

> @@ -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();

Quite sensible.

> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index d05426062ec..005940778b7 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -462,14 +462,77 @@ 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

Doesn't the verb "refresh" refer to the act of making it "fresh"
(again)?  i.e. update the cached stat info to up-to-date?

"To test whether the index has been refreshed" or "To test whether
the cached stat info is up-to-date", perhaps?

> +	# command will fail if the index is *not* refreshed:
> +	#   1. update the worktree to match HEAD & remove file2 in the index

In other words, file2 tentatively becomes untracked.

> +	#   2. reset --mixed to unstage the change from step 1

But then, file2 is "added" to the index again, but added from the
HEAD.  If this did not refresh, then we do not know if the contents
of the file in the working tree is the same, and "diff-files" may
say "file2 may be modified".  If "reset" refreshes, this will take
us back to the same state as "reset --hard HEAD", and "diff-files"
will not report that "file2" is different.

> +	#   3. read-tree HEAD~1 (which differs from HEAD in file2)

With "-m" option, I presume?  Do we want "-u" here, too?

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

This feels a bit brittle.  The implementation of "read-tree -m" may
choose to refresh beforehand to avoid such a failure.

In any case, the name of the helper alone wasn't of any help to
realize that this is about checking if "reset" refreshes the index
or not.  Perhaps call it more like

	reset_refreshes_index

or something?

In any case, instead of the big comment block, comments interspersed
in the steps may be easier to follow.  

> +	# Step 0: start with a clean index
> +	git reset --hard HEAD &&
> +
> +	# Step 1
	# remove file2 from the index
> +	git rm --cached file2 &&
> +
> +	# Step 2
	# 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.
> +	git $1 reset $2 --mixed HEAD &&
> +
> +	# Step 3
> +	git read-tree -m HEAD~1

And use "diff-files file2" here?  Then you do not even have to rely
on HEAD and HEAD~1 being different at file2.

Re: [PATCH v2 2/5] reset: introduce --[no-]refresh option to --mixed

[Victoria Dye]

Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Victoria Dye <vdye@github.com>
>>
>> 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.
>>
>> Helped-by: Derrick Stolee <derrickstolee@github.com>
>> Signed-off-by: Victoria Dye <vdye@github.com>
>> ---
>>  Documentation/git-reset.txt |  9 +++++
>>  builtin/reset.c             | 13 ++++++-
>>  t/t7102-reset.sh            | 77 +++++++++++++++++++++++++++++++++----
>>  3 files changed, 91 insertions(+), 8 deletions(-)
> 
> 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.
> 
>> +	/*
>> +	 * If refresh is completely unspecified (either by config or by command
>> +	 * line option), decide based on 'quiet'.
>> +	 */
>> +	if (refresh < 0)
>> +		refresh = !quiet;
> 
> OK.
> 
>> @@ -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();
> 
> Quite sensible.
> 
>> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
>> index d05426062ec..005940778b7 100755
>> --- a/t/t7102-reset.sh
>> +++ b/t/t7102-reset.sh
>> @@ -462,14 +462,77 @@ 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
> 
> Doesn't the verb "refresh" refer to the act of making it "fresh"
> (again)?  i.e. update the cached stat info to up-to-date?
> 
> "To test whether the index has been refreshed" or "To test whether
> the cached stat info is up-to-date", perhaps?
> 
>> +	# command will fail if the index is *not* refreshed:
>> +	#   1. update the worktree to match HEAD & remove file2 in the index
> 
> In other words, file2 tentatively becomes untracked.
> 
>> +	#   2. reset --mixed to unstage the change from step 1
> 
> But then, file2 is "added" to the index again, but added from the
> HEAD.  If this did not refresh, then we do not know if the contents
> of the file in the working tree is the same, and "diff-files" may
> say "file2 may be modified".  If "reset" refreshes, this will take
> us back to the same state as "reset --hard HEAD", and "diff-files"
> will not report that "file2" is different.
> 
>> +	#   3. read-tree HEAD~1 (which differs from HEAD in file2)
> 
> With "-m" option, I presume?  Do we want "-u" here, too?
> 
>> +	# 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.
> 
> This feels a bit brittle.  The implementation of "read-tree -m" may
> choose to refresh beforehand to avoid such a failure.
> 
> In any case, the name of the helper alone wasn't of any help to
> realize that this is about checking if "reset" refreshes the index
> or not.  Perhaps call it more like
> 
> 	reset_refreshes_index
> 
> or something?
> 
> In any case, instead of the big comment block, comments interspersed
> in the steps may be easier to follow.  
> 
>> +	# Step 0: start with a clean index
>> +	git reset --hard HEAD &&
>> +
>> +	# Step 1
> 	# remove file2 from the index
>> +	git rm --cached file2 &&
>> +
>> +	# Step 2
> 	# 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.
>> +	git $1 reset $2 --mixed HEAD &&
>> +
>> +	# Step 3
>> +	git read-tree -m HEAD~1
> 
> And use "diff-files file2" here?  Then you do not even have to rely
> on HEAD and HEAD~1 being different at file2.
> 

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!

[PATCH v2 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/config/advice.txt | 8 ++++----
 advice.c                        | 2 +-
 advice.h                        | 2 +-
 builtin/reset.c                 | 8 ++++----
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 971aad2f237..83c2a956611 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -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.
diff --git a/advice.c b/advice.c
index 2e1fd483040..cb755c551a2 100644
--- a/advice.c
+++ b/advice.c
@@ -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 },
diff --git a/advice.h b/advice.h
index a3957123a16..f95af70ced4 100644
--- a/advice.h
+++ b/advice.h
@@ -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,
diff --git a/builtin/reset.c b/builtin/reset.c
index 7f667e13d71..feab85e03de 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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

[PATCH v2 4/5] reset: suppress '--no-refresh' advice if logging is silenced

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index feab85e03de..c8a356ec5b0 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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

Re: [PATCH v2 4/5] reset: suppress '--no-refresh' advice if logging is silenced

[Junio C Hamano]

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Victoria Dye <vdye@github.com>
>
> 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.

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.


> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  builtin/reset.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index feab85e03de..c8a356ec5b0 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -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);

[PATCH v2 5/5] stash: make internal resets quiet and refresh index

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/stash.c  |  5 +++--
 t/t3903-stash.sh | 12 ++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 3e8af210fde..91407d9bbe0 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -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;
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index f36e121210e..17f2ad2344c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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

Re: [PATCH v2 5/5] stash: make internal resets quiet and refresh index

[Junio C Hamano]

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Victoria Dye <vdye@github.com>
>
> 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.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  builtin/stash.c  |  5 +++--
>  t/t3903-stash.sh | 12 ++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 3e8af210fde..91407d9bbe0 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -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;
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index f36e121210e..17f2ad2344c 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -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 &&

Hpmh.  The hunk that updates reset_head() does get exercised by
"apply --index", so testing that is OK, but we are also patching
"do_push_stash()" to be quiet, so don't we care the chattyness of
this step, too?

In these steps, we also want the same "did the command refresh the
index?" tests, no?

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

Other than these nits I noticed, the overall idea of the topic is
well presented.  Thanks for working on this.

Re: [PATCH v2 5/5] stash: make internal resets quiet and refresh index

[Victoria Dye]

Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Victoria Dye <vdye@github.com>
>>
>> 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.
>>
>> Signed-off-by: Victoria Dye <vdye@github.com>
>> ---
>>  builtin/stash.c  |  5 +++--
>>  t/t3903-stash.sh | 12 ++++++++++++
>>  2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/stash.c b/builtin/stash.c
>> index 3e8af210fde..91407d9bbe0 100644
>> --- a/builtin/stash.c
>> +++ b/builtin/stash.c
>> @@ -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;
>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
>> index f36e121210e..17f2ad2344c 100755
>> --- a/t/t3903-stash.sh
>> +++ b/t/t3903-stash.sh
>> @@ -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 &&
> 
> Hpmh.  The hunk that updates reset_head() does get exercised by
> "apply --index", so testing that is OK, but we are also patching
> "do_push_stash()" to be quiet, so don't we care the chattyness of
> this step, too?
> 

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

> In these steps, we also want the same "did the command refresh the
> index?" tests, no?
> 

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.

>> +	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
> 
> Other than these nits I noticed, the overall idea of the topic is
> well presented.  Thanks for working on this.
> 

Re: [PATCH v2 0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash'

[Derrick Stolee]

On 3/14/2022 12:10 PM, Victoria Dye via GitGitGadget wrote:
 
> 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

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

Thanks,
-Stolee

Re: [PATCH v2 0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash'

[Junio C Hamano]

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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.

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

[Victoria Dye via GitGitGadget]

In the process of working on tests for 'git stash' sparse index integration,
I found that the '--quiet' option in 'git stash' does not suppress all
non-error output when used with '--index'. Specifically, this comes from an
invocation of 'git reset' without the '--quiet' flag in 'reset_head()'. Upon
enabling that flag, though, I discovered that 1) 'reset' does not refresh
the index if '--quiet' is specified (as of [1]) and 2) 'git stash' needs the
index to be refreshed after the reset.

This series aims to decouple the "suppress logging" and "skip index refresh"
behaviors in 'git reset --mixed', then allow 'stash' to internally use reset
with logs suppressed but index refresh enabled. This is accomplished by
introducing the '--[no-]refresh' option and 'reset.refresh' config setting
to 'git reset'. Additionally, in the spirit of backward-compatibility,
'--quiet' and/or 'reset.quiet=true' without any specified "refresh"
option/config will continue to skip 'refresh_index(...)'.

There are also some minor updates to the advice that suggests skipping the
index refresh:

 * replace recommendation to use "--quiet" with "--no-refresh"
 * use 'advise()' rather than 'printf()'
 * rename the advice config setting from 'advice.resetQuiet' to to
   'advice.resetNoRefresh'
 * suppress advice if '--quiet' is specified in 'reset'

Finally, tests are added to 't7102-reset.sh' verifying whether index refresh
happens when expected and to 't3903-stash.sh' verifying that 'apply --quiet'
no longer prints extraneous logs.


Changes since V2
================

 * Rename 'test_index_refreshed' to 'test_reset_refreshes_index'
 * Move comments explaining the "reset refreshes index" test logic inline
   with execution
 * Replace 'git read-tree' with 'git diff-files' in
   'test_reset_refreshes_index' for a (hopefully) more future-proofed
   command verifying for index refresh
 * Add tests for ensuring the index is refreshed after 'git stash push
   --staged' and 'git stash apply --index'


Changes since V1
================

 * Update cover letter title
 * Squash 't7102' test fixes from patch 5 into patch 2
 * Refactor 't7102' tests to properly use inline config settings

[1] https://lore.kernel.org/git/20181023190423.5772-2-peartben@gmail.com/

Thanks! -Victoria

Victoria Dye (5):
  reset: revise index refresh advice
  reset: introduce --[no-]refresh option to --mixed
  reset: replace '--quiet' with '--no-refresh' in performance advice
  reset: suppress '--no-refresh' advice if logging is silenced
  stash: make internal resets quiet and refresh index

 Documentation/config/advice.txt |  8 ++--
 Documentation/git-reset.txt     |  9 ++++
 advice.c                        |  2 +-
 advice.h                        |  2 +-
 builtin/reset.c                 | 21 +++++++---
 builtin/stash.c                 |  5 ++-
 t/t3903-stash.sh                | 33 +++++++++++++++
 t/t7102-reset.sh                | 73 +++++++++++++++++++++++++++++----
 8 files changed, 133 insertions(+), 20 deletions(-)


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

Range-diff vs v2:

 1:  7206ef8dd8a = 1:  7206ef8dd8a reset: revise index refresh advice
 2:  7f0226bc3e6 ! 2:  101cee42dd6 reset: introduce --[no-]refresh option to --mixed
     @@ Commit message
          whether logs are silenced and do not affect index refresh.
      
          Helped-by: Derrick Stolee <derrickstolee@github.com>
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: Victoria Dye <vdye@github.com>
      
       ## Documentation/git-reset.txt ##
     @@ t/t7102-reset.sh: test_expect_success 'resetting an unmodified path is a no-op'
       	git diff-index --cached --exit-code HEAD
       '
       
     -+test_index_refreshed () {
     -+
     -+	# To test whether the index is refresh, create a scenario where a
     -+	# command will fail if the index is *not* refreshed:
     -+	#   1. update the worktree to match HEAD & remove file2 in the index
     -+	#   2. reset --mixed to unstage the change from step 1
     -+	#   3. read-tree HEAD~1 (which differs from HEAD in file2)
     -+	# If the index is refreshed in step 2, then file2 in the index will be
     -+	# up-to-date with HEAD and read-tree will succeed (thus failing the
     -+	# test). If the index is *not* refreshed, however, the staged deletion
     -+	# of file2 from step 1 will conflict with the changes from the tree read
     -+	# in step 3, resulting in a failure.
     ++test_reset_refreshes_index () {
     ++
     ++	# To test whether the index is refreshed in `git reset --mixed` with
     ++	# the given options, create a scenario where we clearly see different
     ++	# results depending on whether the refresh occurred or not.
      +
      +	# Step 0: start with a clean index
      +	git reset --hard HEAD &&
      +
     -+	# Step 1
     ++	# Step 1: remove file2, but only in the index (no change to worktree)
      +	git rm --cached file2 &&
      +
     -+	# Step 2
     ++	# Step 2: reset index & leave worktree unchanged from HEAD
      +	git $1 reset $2 --mixed HEAD &&
      +
     -+	# Step 3
     -+	git read-tree -m HEAD~1
     ++	# Step 3: verify whether the index is refreshed by checking whether
     ++	# file2 still has staged changes in the index differing from HEAD (if
     ++	# the refresh occurred, there should be no such changes)
     ++	git diff-files >output.log &&
     ++	test_must_be_empty output.log
      +}
      +
       test_expect_success '--mixed refreshes the index' '
     @@ t/t7102-reset.sh: test_expect_success 'resetting an unmodified path is a no-op'
      -	test_cmp expect output
      +	# Verify default behavior (with no config settings or command line
      +	# options)
     -+	test_index_refreshed
     ++	test_reset_refreshes_index
      +'
      +test_expect_success '--mixed --[no-]quiet sets default refresh behavior' '
      +	# Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh)
      +	# determine refresh behavior
      +
      +	# Config setting
     -+	! test_index_refreshed "-c reset.quiet=true" &&
     -+	test_index_refreshed "-c reset.quiet=false" &&
     ++	! test_reset_refreshes_index "-c reset.quiet=true" &&
     ++	test_reset_refreshes_index "-c reset.quiet=false" &&
      +
      +	# Command line option
     -+	! test_index_refreshed "" --quiet &&
     -+	test_index_refreshed "" --no-quiet &&
     ++	! test_reset_refreshes_index "" --quiet &&
     ++	test_reset_refreshes_index "" --no-quiet &&
      +
      +	# Command line option overrides config setting
     -+	! test_index_refreshed "-c reset.quiet=false" --quiet &&
     -+	test_index_refreshed "-c reset.refresh=true" --no-quiet
     ++	! test_reset_refreshes_index "-c reset.quiet=false" --quiet &&
     ++	test_reset_refreshes_index "-c reset.refresh=true" --no-quiet
      +'
      +
      +test_expect_success '--mixed --[no-]refresh sets refresh behavior' '
      +	# Verify that --[no-]refresh and `reset.refresh` control index refresh
      +
      +	# Config setting
     -+	test_index_refreshed "-c reset.refresh=true" &&
     -+	! test_index_refreshed "-c reset.refresh=false" &&
     ++	test_reset_refreshes_index "-c reset.refresh=true" &&
     ++	! test_reset_refreshes_index "-c reset.refresh=false" &&
      +
      +	# Command line option
     -+	test_index_refreshed "" --refresh &&
     -+	! test_index_refreshed "" --no-refresh &&
     ++	test_reset_refreshes_index "" --refresh &&
     ++	! test_reset_refreshes_index "" --no-refresh &&
      +
      +	# Command line option overrides config setting
     -+	test_index_refreshed "-c reset.refresh=false" --refresh &&
     -+	! test_index_refreshed "-c reset.refresh=true" --no-refresh
     ++	test_reset_refreshes_index "-c reset.refresh=false" --refresh &&
     ++	! test_reset_refreshes_index "-c reset.refresh=true" --no-refresh
      +'
      +
      +test_expect_success '--mixed --refresh overrides --quiet refresh behavior' '
      +	# Verify that *both* --refresh and `reset.refresh` override the
      +	# default non-refresh behavior of --quiet
     -+	test_index_refreshed "" "--quiet --refresh" &&
     -+	test_index_refreshed "-c reset.quiet=true" --refresh &&
     -+	test_index_refreshed "-c reset.refresh=true" --quiet &&
     -+	test_index_refreshed "-c reset.refresh=true -c reset.quiet=true"
     ++	test_reset_refreshes_index "" "--quiet --refresh" &&
     ++	test_reset_refreshes_index "-c reset.quiet=true" --refresh &&
     ++	test_reset_refreshes_index "-c reset.refresh=true" --quiet &&
     ++	test_reset_refreshes_index "-c reset.refresh=true -c reset.quiet=true"
       '
       
       test_expect_success '--mixed preserves skip-worktree' '
 3:  f869723d64f = 3:  eb5958194ee reset: replace '--quiet' with '--no-refresh' in performance advice
 4:  cffca0ea5c6 = 4:  548c9303c44 reset: suppress '--no-refresh' advice if logging is silenced
 5:  3334d4cb6f3 ! 5:  4c45351a0c4 stash: make internal resets quiet and refresh index
     @@ Commit message
          refreshing the index, but later operations in 'git stash' subcommands expect
          a non-stale index, enable '--refresh' as well.
      
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: Victoria Dye <vdye@github.com>
      
       ## builtin/stash.c ##
     @@ t/t3903-stash.sh: test_expect_success 'apply -q is quiet' '
       test_expect_success 'save -q is quiet' '
       	git stash save --quiet >output.out 2>&1 &&
       	test_must_be_empty output.out
     +@@ t/t3903-stash.sh: test_expect_success 'drop -q is quiet' '
     + 	test_must_be_empty output.out
     + '
     + 
     ++test_expect_success 'stash push -q --staged refreshes the index' '
     ++	git reset --hard &&
     ++	echo test >file &&
     ++	git add file &&
     ++	git stash push -q --staged &&
     ++	git diff-files >output.out &&
     ++	test_must_be_empty output.out
     ++'
     ++
     ++test_expect_success 'stash apply -q --index refreshes the index' '
     ++	echo test >other-file &&
     ++	git add other-file &&
     ++	echo another-change >other-file &&
     ++	git diff-files >expect &&
     ++	git stash &&
     ++
     ++	git stash apply -q --index &&
     ++	git diff-files >actual &&
     ++	test_cmp expect actual
     ++'
     ++
     + test_expect_success 'stash -k' '
     + 	echo bar3 >file &&
     + 	echo bar4 >file2 &&

-- 
gitgitgadget

[PATCH v3 1/5] reset: revise index refresh advice

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/config/advice.txt | 4 ++--
 builtin/reset.c                 | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c40eb09cb7e..971aad2f237 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -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.
diff --git a/builtin/reset.c b/builtin/reset.c
index 6e65e90c5db..a420497a14f 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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

[PATCH v3 2/5] reset: introduce --[no-]refresh option to --mixed

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/git-reset.txt |  9 +++++
 builtin/reset.c             | 13 ++++++-
 t/t7102-reset.sh            | 73 +++++++++++++++++++++++++++++++++----
 3 files changed, 87 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 6f7685f53d5..89ddc85c2e4 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -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
diff --git a/builtin/reset.c b/builtin/reset.c
index a420497a14f..7f667e13d71 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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();
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index d05426062ec..1dc3911a060 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -462,14 +462,73 @@ test_expect_success 'resetting an unmodified path is a no-op' '
 	git diff-index --cached --exit-code HEAD
 '
 
+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: remove file2, but only in the index (no change to worktree)
+	git rm --cached file2 &&
+
+	# Step 2: reset index & leave worktree unchanged from HEAD
+	git $1 reset $2 --mixed HEAD &&
+
+	# 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' '
-	cat >expect <<-\EOF &&
-	Unstaged changes after reset:
-	M	file2
-	EOF
-	echo 123 >>file2 &&
-	git reset --mixed HEAD >output &&
-	test_cmp expect output
+	# Verify default behavior (with no config settings or command line
+	# options)
+	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_reset_refreshes_index "-c reset.quiet=true" &&
+	test_reset_refreshes_index "-c reset.quiet=false" &&
+
+	# Command line option
+	! test_reset_refreshes_index "" --quiet &&
+	test_reset_refreshes_index "" --no-quiet &&
+
+	# Command line option overrides config setting
+	! 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_reset_refreshes_index "-c reset.refresh=true" &&
+	! test_reset_refreshes_index "-c reset.refresh=false" &&
+
+	# Command line option
+	test_reset_refreshes_index "" --refresh &&
+	! test_reset_refreshes_index "" --no-refresh &&
+
+	# Command line option overrides config setting
+	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_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' '
-- 
gitgitgadget

Re: [PATCH v3 2/5] reset: introduce --[no-]refresh option to --mixed

[Phillip Wood]

Hi Victoria

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

On 15/03/2022 01:49, Victoria Dye via GitGitGadget wrote:
> +	/*
> +	 * If refresh is completely unspecified (either by config or by command
> +	 * line option), decide based on 'quiet'.
> +	 */
> +	if (refresh < 0)
> +		refresh = !quiet;

This keeps the current behavior of not refreshing when --quiet is given. 
I wonder how disruptive it would be to take the opportunity to get rid 
of that hack and go back the the original behavior of refreshing when 
--quiet is given. There are a couple of assumptions that make me think 
it might be acceptable

1 - anyone using a sparse index wont notice as refreshing the index
     should be fast for them

2 - the large repositories that are affected exist in managed
     environments where an admin who reads the release notes could set
     reset.refresh in a central config so individual users are not
     inconvenienced.

Best Wishes

Phillip

Re: [PATCH v3 2/5] reset: introduce --[no-]refresh option to --mixed

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> This keeps the current behavior of not refreshing when --quiet is
> given. I wonder how disruptive it would be to take the opportunity to
> get rid of that hack and go back the the original behavior of
> refreshing when --quiet is given. There are a couple of assumptions
> that make me think it might be acceptable
>
> 1 - anyone using a sparse index wont notice as refreshing the index
>     should be fast for them
>
> 2 - the large repositories that are affected exist in managed
>     environments where an admin who reads the release notes could set
>     reset.refresh in a central config so individual users are not
>     inconvenienced.

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.

Re: [PATCH v3 2/5] reset: introduce --[no-]refresh option to --mixed

[Victoria Dye]

Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> This keeps the current behavior of not refreshing when --quiet is
>> given. I wonder how disruptive it would be to take the opportunity to
>> get rid of that hack and go back the the original behavior of
>> refreshing when --quiet is given. There are a couple of assumptions
>> that make me think it might be acceptable
>>
>> 1 - anyone using a sparse index wont notice as refreshing the index
>>     should be fast for them
>>
>> 2 - the large repositories that are affected exist in managed
>>     environments where an admin who reads the release notes could set
>>     reset.refresh in a central config so individual users are not
>>     inconvenienced.
> 
> 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.
> 

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.

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

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.

> Thanks.
> 
> 

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!

[PATCH v3 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/config/advice.txt | 8 ++++----
 advice.c                        | 2 +-
 advice.h                        | 2 +-
 builtin/reset.c                 | 8 ++++----
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 971aad2f237..83c2a956611 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -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.
diff --git a/advice.c b/advice.c
index 2e1fd483040..cb755c551a2 100644
--- a/advice.c
+++ b/advice.c
@@ -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 },
diff --git a/advice.h b/advice.h
index a3957123a16..f95af70ced4 100644
--- a/advice.h
+++ b/advice.h
@@ -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,
diff --git a/builtin/reset.c b/builtin/reset.c
index 7f667e13d71..feab85e03de 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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

[PATCH v3 4/5] reset: suppress '--no-refresh' advice if logging is silenced

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index feab85e03de..c8a356ec5b0 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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

[PATCH v3 5/5] stash: make internal resets quiet and refresh index

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

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.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 builtin/stash.c  |  5 +++--
 t/t3903-stash.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 3e8af210fde..91407d9bbe0 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -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;
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index f36e121210e..1a5c1bd3109 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -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

Re: [PATCH v3 5/5] stash: make internal resets quiet and refresh index

[Junio C Hamano]

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +test_expect_success 'apply --index -q is quiet' '

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

I wonder if a single test that checks chattiness and refreshing of
"git stash -q" and "git apply --index -q" (that's 2x2 which is 4)
would be sufficient?

> +	# Added file, deleted file, modified file all staged for commit
> +	echo foo >new-file &&
> +	echo test >file &&
> +	git add new-file file &&
> +	git rm other-file &&
> +
> +	git stash &&

As this is only about chattiness about "apply --index -q", this
command goes unchecked (4 - 2 = 2).

> +	git stash apply --index -q >output.out 2>&1 &&

And this is only about chattiness so we do not test if the index
gets refreshed after this operation (2 - 1 = 1).

> +	test_must_be_empty output.out

This ensures that "git stash apply --index -q" is silent, as that is
the main objective of this step: make sure reset won't chatter,
especially when stash is told to be --quiet, which is good.

But with a few more lines, this set-up can also test the other three
with minimum additional effort, no?

> +'
> +
>  test_expect_success 'save -q is quiet' '
>  	git stash save --quiet >output.out 2>&1 &&
>  	test_must_be_empty output.out
> @@ -291,6 +303,27 @@ test_expect_success 'drop -q is quiet' '
>  	test_must_be_empty output.out
>  '
>  
> +test_expect_success 'stash push -q --staged refreshes the index' '
> +	git reset --hard &&
> +	echo test >file &&
> +	git add file &&
> +	git stash push -q --staged &&

"git stash" and "git stash push -q --staged" may do different
things, so leaving the plain "git stash" untested for refreshing in
an earlier test, and "git stash" with different options being tested
for refreshing here, makes me wonder about a gap in test coverage.

The overall theme of the whole topic was that chatty output from
"git reset" run as an implementation detail seeps through from "git
stash", IIUC.  So, making sure that our index is refreshed after the
operation is good, but at the same time, wouldn't we want to see
what the output of this command says (or be silent)?

Re: [PATCH v3 5/5] stash: make internal resets quiet and refresh index

[Victoria Dye]

Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +test_expect_success 'apply --index -q is quiet' '
> 
> Hmph.  So being quiet and refreshing index are tested separately in
> different tests, and this one is only about being quiet?
> 

Yes, because the changes in this series only correct the "quietness" of 'git
apply --index -q'. Similarly, there are two instances where '--refresh' was
added to 'reset's in 'stash', and two corresponding tests added here.

> I wonder if a single test that checks chattiness and refreshing of
> "git stash -q" and "git apply --index -q" (that's 2x2 which is 4)
> would be sufficient?
> 
>> +	# Added file, deleted file, modified file all staged for commit
>> +	echo foo >new-file &&
>> +	echo test >file &&
>> +	git add new-file file &&
>> +	git rm other-file &&
>> +
>> +	git stash &&
> 
> As this is only about chattiness about "apply --index -q", this
> command goes unchecked (4 - 2 = 2).
> 
>> +	git stash apply --index -q >output.out 2>&1 &&
> 
> And this is only about chattiness so we do not test if the index
> gets refreshed after this operation (2 - 1 = 1).
> 
>> +	test_must_be_empty output.out
> 
> This ensures that "git stash apply --index -q" is silent, as that is
> the main objective of this step: make sure reset won't chatter,
> especially when stash is told to be --quiet, which is good.
> 
> But with a few more lines, this set-up can also test the other three
> with minimum additional effort, no?
> 

I like having the "quietness" and "refresh" checks separate because they're
not inextricably linked. One could fail while the other doesn't (or both
could independently fail), where two separate results would be slightly more
helpful information for a developer debugging the tests. The same reasoning
applies to 'stash push' with 'stash apply' (they can fail independently, so
they're tested separately). 

I'm not normally super adherent to a "test each mode of failure separately"
paradigm (evidenced by some of the more complicated tests in
't1092-sparse-checkout-compatibility'), but 't3903-stash.sh' already has
dedicated'-q' tests separate from other functionality tests, so I followed
the precedent.

>> +'
>> +
>>  test_expect_success 'save -q is quiet' '
>>  	git stash save --quiet >output.out 2>&1 &&
>>  	test_must_be_empty output.out
>> @@ -291,6 +303,27 @@ test_expect_success 'drop -q is quiet' '
>>  	test_must_be_empty output.out
>>  '
>>  
>> +test_expect_success 'stash push -q --staged refreshes the index' '
>> +	git reset --hard &&
>> +	echo test >file &&
>> +	git add file &&
>> +	git stash push -q --staged &&
> 
> "git stash" and "git stash push -q --staged" may do different
> things, so leaving the plain "git stash" untested for refreshing in
> an earlier test, and "git stash" with different options being tested
> for refreshing here, makes me wonder about a gap in test coverage.
> 

Yes and no, I think. There is a gap in that stash is not otherwise tested for index refresh, but in the context of this series, that's primarily a concern where 'git reset [--mixed] -q' is used internally - if the refresh didn't happen, the command could (incorrectly) fail altogether. The new tests cover usage of 'stash' wherever a unique 'git reset [--mixed]' exists, and those are theoretically the main sources of a failure-inducing non-refreshed index.

> The overall theme of the whole topic was that chatty output from
> "git reset" run as an implementation detail seeps through from "git
> stash", IIUC.  So, making sure that our index is refreshed after the
> operation is good, but at the same time, wouldn't we want to see
> what the output of this command says (or be silent)?

The goal of this series was to fix 'git stash apply --index -q' specifically
- it's a somewhat narrow scope, to the point where I would have included it
in another series I'm working on if it weren't for the other changes needed
in 'reset'. In line with that, I tried to keep the tests here tied
one-to-one with the changes made in 'stash':

* add '-q' to 'reset_head()' -> 'apply --index -q is quiet'
* add '--refresh' to 'reset_head()' -> 'stash apply -q --index refreshes the
  index'
* add '--refresh' to 'do_push_stash()' -> 'stash push -q --staged refreshes
  the index'

So while I do think 'stash' testing could be expanded, my intention with
this particular series was to unblock changes relying on 'git stash apply
--index -q' to work properly. If I'm making this too narrowly-scoped,
though, I can submit a patch on top of this one that more broadly expands
testing in 'stash' for '-q'. Let me know what you think!

Re: [PATCH v3 5/5] stash: make internal resets quiet and refresh index

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

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

OK.