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

Commit Message

Victoria Dye March 12, 2022, 12:08 a.m. UTC

From: Victoria Dye <[e-mail]>

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 <[e-mail]>
---
 Documentation/git-reset.txt |  9 +++++
 builtin/reset.c             | 13 ++++++-
 t/t7102-reset.sh            | 77 +++++++++++++++++++++++++++++++++----
 3 files changed, 91 insertions(+), 8 deletions(-)

Comments

Derrick Stolee March 14, 2022, 3:05 p.m. UTC | #1

On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <[e-mail]>
> 
> 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

Derrick Stolee March 14, 2022, 3:13 p.m. UTC | #2

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

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

Victoria Dye March 14, 2022, 3:55 p.m. UTC | #3

Derrick Stolee wrote:
> On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <[e-mail]>
>>
>> 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

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