[PATCH 0/4] reset: make --no-refresh the only way to skip index refresh

[PATCH 0/4] reset: make --no-refresh the only way to skip index refresh

[Victoria Dye via GitGitGadget]

Maintainer's note: this is based on vd/stash-silence-reset

----------------------------------------------------------------------------

This is a follow-up to the changes in vd/stash-silence-reset [1], in which
index refreshing behavior was decoupled from log silencing in the '--quiet'
option to 'git reset --mixed' by introducing a '--[no-]refresh' option and
'reset.refresh' config setting.

After some discussion [2] on the mailing list, both the
backward-compatibility and use of global options in that series came into
question:

 * '--quiet' still skipped refresh if neither '--[no-]refresh' nor
   'reset.refresh' were specified, meaning that users could still be left
   with an incorrect index state after reset.
 * Having 'reset.quiet' and/or 'reset.refresh' potentially disable index
   refresh by default meant that developers would need to defensively add
   '--refresh' to all internal uses of 'git reset --mixed'. Without that
   option, different config setups could cause variability in index
   correctness from user to user.

In response, this series deprecates all instances of skipping index refresh
in 'git reset --mixed' except for '--no-refresh' itself:

 * Patch [1/4] removes the "fallback" behavior of 'reset.quiet' and
   '--quiet' implying '--no-refresh' if neither '--[no-]refresh' nor
   'config.refresh' were specified. In other words, '--quiet' no longer does
   anything other than log silencing.
 * Patch [2/4] deprecates 'reset.quiet', since its main use was to disable
   index refresh until it was deprecated in [1/4].
 * Patch [3/4] deprecates 'reset.refresh' to avoid users accidentally ending
   up with an incorrect index state after all resets as a result of a global
   setting's passive effects.
 * Patch [4/4] removes the '--refresh' option, leaving only '--no-refresh'.
   Because nothing but '--no-refresh' can skip the 'reset' index refresh
   anymore, '--refresh' would never be needed.

[1]
https://lore.kernel.org/git/pull.1170.v3.git.1647308982.gitgitgadget@gmail.com/
[2]
https://lore.kernel.org/git/80a2a5a2-256f-6c3b-2430-10bef99ce1e9@github.com/

Thanks! -Victoria

Victoria Dye (4):
  reset: do not make '--quiet' disable index refresh
  reset: deprecate 'reset.quiet' config option
  reset: deprecate 'reset.refresh' config option
  reset: deprecate '--refresh', leaving only '--no-refresh'

 Documentation/config.txt       |  2 --
 Documentation/config/reset.txt |  2 --
 Documentation/git-reset.txt    | 13 ++-------
 builtin/reset.c                | 18 +++----------
 builtin/stash.c                |  4 +--
 contrib/scalar/scalar.c        |  1 -
 t/t7102-reset.sh               | 48 +++++-----------------------------
 7 files changed, 15 insertions(+), 73 deletions(-)
 delete mode 100644 Documentation/config/reset.txt


base-commit: 4b8b0f6fa2778c1f9c373620e3f07787543914c6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1184%2Fvdye%2Freset%2Fclean-up-refresh-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1184/vdye/reset/clean-up-refresh-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1184
-- 
gitgitgadget

[PATCH 1/4] reset: do not make '--quiet' disable index refresh

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Update '--quiet' to no longer implicitly skip refreshing the index in a
mixed reset. Users now have the ability to explicitly disable refreshing the
index with the '--no-refresh' option, so they no longer need to use
'--quiet' to do so. Moreover, we explicitly remove the refresh-skipping
behavior from '--quiet' because it is completely unrelated to the stated
purpose of the option: "Be quiet, only report errors."

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/git-reset.txt |  5 +----
 builtin/reset.c             |  7 -------
 t/t7102-reset.sh            | 32 +++++---------------------------
 3 files changed, 6 insertions(+), 38 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 89ddc85c2e4..bc1646c3016 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -114,10 +114,7 @@ OPTIONS
 --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.
+	`--[no-]refresh` nor `reset.refresh` are set, refresh is enabled.
 
 --pathspec-from-file=<file>::
 	Pathspec is passed in `<file>` instead of commandline args. If
diff --git a/builtin/reset.c b/builtin/reset.c
index c8a356ec5b0..7c3828f6fc5 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -430,13 +430,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 						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");
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 1dc3911a060..8b62bb39b3d 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -485,25 +485,12 @@ test_reset_refreshes_index () {
 }
 
 test_expect_success '--mixed refreshes the index' '
-	# 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 &&
+	# Verify default behavior (without --[no-]refresh or reset.refresh)
+	test_reset_refreshes_index &&
 
-	# 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
+	# With --quiet & reset.quiet
+	test_reset_refreshes_index "-c reset.quiet=true" &&
+	test_reset_refreshes_index "" --quiet
 '
 
 test_expect_success '--mixed --[no-]refresh sets refresh behavior' '
@@ -522,15 +509,6 @@ test_expect_success '--mixed --[no-]refresh sets refresh behavior' '
 	! 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' '
 	echo 123 >>file2 &&
 	git add file2 &&
-- 
gitgitgadget

Re: [PATCH 1/4] reset: do not make '--quiet' disable index refresh

[Phillip Wood]

Hi Victoria

On 21/03/2022 20:34, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
> 
> Update '--quiet' to no longer implicitly skip refreshing the index in a
> mixed reset. Users now have the ability to explicitly disable refreshing the
> index with the '--no-refresh' option, so they no longer need to use
> '--quiet' to do so. Moreover, we explicitly remove the refresh-skipping
> behavior from '--quiet' because it is completely unrelated to the stated
> purpose of the option: "Be quiet, only report errors."
> 
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>   Documentation/git-reset.txt |  5 +----
>   builtin/reset.c             |  7 -------
>   t/t7102-reset.sh            | 32 +++++---------------------------
>   3 files changed, 6 insertions(+), 38 deletions(-)
> 
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index 89ddc85c2e4..bc1646c3016 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -114,10 +114,7 @@ OPTIONS
>   --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.
> +	`--[no-]refresh` nor `reset.refresh` are set, refresh is enabled.
>   
>   --pathspec-from-file=<file>::
>   	Pathspec is passed in `<file>` instead of commandline args. If
> diff --git a/builtin/reset.c b/builtin/reset.c
> index c8a356ec5b0..7c3828f6fc5 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -430,13 +430,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>   						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;

Does this mean we can stop initializing refresh to -1?

Best Wishes

Phillip

Re: [PATCH 1/4] reset: do not make '--quiet' disable index refresh

[Victoria Dye]

Phillip Wood wrote:
> Hi Victoria
> 
> On 21/03/2022 20:34, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <vdye@github.com>
>>
>> Update '--quiet' to no longer implicitly skip refreshing the index in a
>> mixed reset. Users now have the ability to explicitly disable refreshing the
>> index with the '--no-refresh' option, so they no longer need to use
>> '--quiet' to do so. Moreover, we explicitly remove the refresh-skipping
>> behavior from '--quiet' because it is completely unrelated to the stated
>> purpose of the option: "Be quiet, only report errors."
>>
>> Signed-off-by: Victoria Dye <vdye@github.com>
>> ---
>>   Documentation/git-reset.txt |  5 +----
>>   builtin/reset.c             |  7 -------
>>   t/t7102-reset.sh            | 32 +++++---------------------------
>>   3 files changed, 6 insertions(+), 38 deletions(-)
>>
>> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
>> index 89ddc85c2e4..bc1646c3016 100644
>> --- a/Documentation/git-reset.txt
>> +++ b/Documentation/git-reset.txt
>> @@ -114,10 +114,7 @@ OPTIONS
>>   --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.
>> +    `--[no-]refresh` nor `reset.refresh` are set, refresh is enabled.
>>     --pathspec-from-file=<file>::
>>       Pathspec is passed in `<file>` instead of commandline args. If
>> diff --git a/builtin/reset.c b/builtin/reset.c
>> index c8a356ec5b0..7c3828f6fc5 100644
>> --- a/builtin/reset.c
>> +++ b/builtin/reset.c
>> @@ -430,13 +430,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>>                           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;
> 
> Does this mean we can stop initializing refresh to -1?
> 
> Best Wishes
> 
> Phillip

Yes, thanks for catching that! It should be initialized to 1 in this patch
(rather than later in patch 4).

[PATCH 2/4] reset: deprecate 'reset.quiet' config option

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Remove the 'reset.quiet' config option, remove '--no-quiet' documentation in
'Documentation/git-reset.txt'. In 4c3abd0551 (reset: add new reset.quiet
config setting, 2018-10-23), 'reset.quiet' was introduced as a way to
globally change the default behavior of 'git reset --mixed' to skip index
refresh.

However, now that '--quiet' does not affect index refresh, 'reset.quiet'
would only serve to globally silence logging. This was not the original
intention of the config setting, and there's no precedent for such a setting
in other commands with a '--quiet' option, so it appears to be obsolete.

In addition to the options & its documentation, remove 'reset.quiet' from
the recommended config for 'scalar'.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/config.txt       | 2 --
 Documentation/config/reset.txt | 2 --
 Documentation/git-reset.txt    | 5 +----
 builtin/reset.c                | 1 -
 contrib/scalar/scalar.c        | 1 -
 t/t7102-reset.sh               | 3 +--
 6 files changed, 2 insertions(+), 12 deletions(-)
 delete mode 100644 Documentation/config/reset.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f0fb25a371c..43f5e6fd6d3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -495,8 +495,6 @@ include::config/repack.txt[]
 
 include::config/rerere.txt[]
 
-include::config/reset.txt[]
-
 include::config/sendemail.txt[]
 
 include::config/sequencer.txt[]
diff --git a/Documentation/config/reset.txt b/Documentation/config/reset.txt
deleted file mode 100644
index 63b7c45aac2..00000000000
--- a/Documentation/config/reset.txt
+++ /dev/null
@@ -1,2 +0,0 @@
-reset.quiet::
-	When set to true, 'git reset' will default to the '--quiet' option.
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index bc1646c3016..f4aca9dd35c 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -105,10 +105,7 @@ OPTIONS
 
 -q::
 --quiet::
---no-quiet::
-	Be quiet, only report errors. The default behavior is set by the
-	`reset.quiet` config option. `--quiet` and `--no-quiet` will
-	override the default behavior.
+	Be quiet, only report errors.
 
 --refresh::
 --no-refresh::
diff --git a/builtin/reset.c b/builtin/reset.c
index 7c3828f6fc5..e824aad3604 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -423,7 +423,6 @@ 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,
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 7db2a97416e..58ca0e56f14 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -152,7 +152,6 @@ static int set_recommended_config(int reconfigure)
 		{ "pack.useBitmaps", "false", 1 },
 		{ "pack.useSparse", "true", 1 },
 		{ "receive.autoGC", "false", 1 },
-		{ "reset.quiet", "true", 1 },
 		{ "feature.manyFiles", "false", 1 },
 		{ "feature.experimental", "false", 1 },
 		{ "fetch.unpackLimit", "1", 1 },
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 8b62bb39b3d..9e4c4deee35 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -488,8 +488,7 @@ test_expect_success '--mixed refreshes the index' '
 	# Verify default behavior (without --[no-]refresh or reset.refresh)
 	test_reset_refreshes_index &&
 
-	# With --quiet & reset.quiet
-	test_reset_refreshes_index "-c reset.quiet=true" &&
+	# With --quiet
 	test_reset_refreshes_index "" --quiet
 '
 
-- 
gitgitgadget

Re: [PATCH 2/4] reset: deprecate 'reset.quiet' config option

[Phillip Wood]

Hi Victoria

On 21/03/2022 20:34, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>

Small nit-pick about the title, I think "deprecate" is a bit misleading 
as we're actually removing the config option.

> Remove the 'reset.quiet' config option, remove '--no-quiet' documentation in
> 'Documentation/git-reset.txt'. In 4c3abd0551 (reset: add new reset.quiet
> config setting, 2018-10-23), 'reset.quiet' was introduced as a way to
> globally change the default behavior of 'git reset --mixed' to skip index
> refresh.
> 
> However, now that '--quiet' does not affect index refresh, 'reset.quiet'
> would only serve to globally silence logging. This was not the original
> intention of the config setting, and there's no precedent for such a setting
> in other commands with a '--quiet' option, so it appears to be obsolete.
> 
> In addition to the options & its documentation, remove 'reset.quiet' from
> the recommended config for 'scalar'.

Nice attention to detail


The code changes look good

Best Wishes

Phillip

> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>   Documentation/config.txt       | 2 --
>   Documentation/config/reset.txt | 2 --
>   Documentation/git-reset.txt    | 5 +----
>   builtin/reset.c                | 1 -
>   contrib/scalar/scalar.c        | 1 -
>   t/t7102-reset.sh               | 3 +--
>   6 files changed, 2 insertions(+), 12 deletions(-)
>   delete mode 100644 Documentation/config/reset.txt
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f0fb25a371c..43f5e6fd6d3 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -495,8 +495,6 @@ include::config/repack.txt[]
>   
>   include::config/rerere.txt[]
>   
> -include::config/reset.txt[]
> -
>   include::config/sendemail.txt[]
>   
>   include::config/sequencer.txt[]
> diff --git a/Documentation/config/reset.txt b/Documentation/config/reset.txt
> deleted file mode 100644
> index 63b7c45aac2..00000000000
> --- a/Documentation/config/reset.txt
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -reset.quiet::
> -	When set to true, 'git reset' will default to the '--quiet' option.
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index bc1646c3016..f4aca9dd35c 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -105,10 +105,7 @@ OPTIONS
>   
>   -q::
>   --quiet::
> ---no-quiet::
> -	Be quiet, only report errors. The default behavior is set by the
> -	`reset.quiet` config option. `--quiet` and `--no-quiet` will
> -	override the default behavior.
> +	Be quiet, only report errors.
>   
>   --refresh::
>   --no-refresh::
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 7c3828f6fc5..e824aad3604 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -423,7 +423,6 @@ 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,
> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
> index 7db2a97416e..58ca0e56f14 100644
> --- a/contrib/scalar/scalar.c
> +++ b/contrib/scalar/scalar.c
> @@ -152,7 +152,6 @@ static int set_recommended_config(int reconfigure)
>   		{ "pack.useBitmaps", "false", 1 },
>   		{ "pack.useSparse", "true", 1 },
>   		{ "receive.autoGC", "false", 1 },
> -		{ "reset.quiet", "true", 1 },
>   		{ "feature.manyFiles", "false", 1 },
>   		{ "feature.experimental", "false", 1 },
>   		{ "fetch.unpackLimit", "1", 1 },
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 8b62bb39b3d..9e4c4deee35 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -488,8 +488,7 @@ test_expect_success '--mixed refreshes the index' '
>   	# Verify default behavior (without --[no-]refresh or reset.refresh)
>   	test_reset_refreshes_index &&
>   
> -	# With --quiet & reset.quiet
> -	test_reset_refreshes_index "-c reset.quiet=true" &&
> +	# With --quiet
>   	test_reset_refreshes_index "" --quiet
>   '
>   

[PATCH 3/4] reset: deprecate 'reset.refresh' config option

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Remove the 'reset.refresh' option, requiring that users explicitly specify
'--no-refresh' if they want to skip refreshing the index.

The 'reset.refresh' option was introduced in 101cee42dd (reset: introduce
--[no-]refresh option to --mixed, 2022-03-11) as a replacement for the
refresh-skipping behavior originally controlled by 'reset.quiet'.

Although 'reset.refresh=false' functionally served the same purpose as
'reset.quiet=true', it exposed [1] the fact that the existence of a global
"skip refresh" option could potentially cause problems for users. Allowing a
global config option to avoid refreshing the index forces scripts using 'git
reset --mixed' to defensively use '--refresh' if index refresh is expected;
if that option is missing, behavior of a script could vary from user-to-user
without explanation.

Furthermore, globally disabling index refresh in 'reset --mixed' was
initially devised as a passive performance improvement; since the
introduction of the option, other changes have been made to Git (e.g., the
sparse index) with a greater potential performance impact without
sacrificing index correctness. Therefore, we can more aggressively err on
the side of correctness and limit the cases of skipping index refresh to
only when a user specifies the '--no-refresh' option.

[1] https://lore.kernel.org/git/xmqqy2179o3c.fsf@gitster.g/

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

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index f4aca9dd35c..df167eaa766 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -109,9 +109,7 @@ OPTIONS
 
 --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, refresh is enabled.
+	Proactively refresh the index after a mixed reset. Enabled by default.
 
 --pathspec-from-file=<file>::
 	Pathspec is passed in `<file>` instead of commandline args. If
diff --git a/builtin/reset.c b/builtin/reset.c
index e824aad3604..54324217f93 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -423,7 +423,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_reset_config, NULL);
-	git_config_get_bool("reset.refresh", &refresh);
 
 	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
 						PARSE_OPT_KEEP_DASHDASH);
@@ -529,8 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
 				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);
+						 "'--no-refresh' to avoid this."), t_delta_in_ms / 1000.0);
 				}
 			}
 		} else {
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 9e4c4deee35..22477f3a312 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -493,19 +493,9 @@ test_expect_success '--mixed refreshes the index' '
 '
 
 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
+	# Verify that --[no-]refresh controls index refresh
 	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_reset_refreshes_index "" --no-refresh
 '
 
 test_expect_success '--mixed preserves skip-worktree' '
-- 
gitgitgadget

Re: [PATCH 3/4] reset: deprecate 'reset.refresh' config option

[Phillip Wood]

Hi Victoria

On 21/03/2022 20:34, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>

Same concern about the title as the last patch

> Remove the 'reset.refresh' option, requiring that users explicitly specify
> '--no-refresh' if they want to skip refreshing the index.
> 
> The 'reset.refresh' option was introduced in 101cee42dd (reset: introduce
> --[no-]refresh option to --mixed, 2022-03-11) as a replacement for the
> refresh-skipping behavior originally controlled by 'reset.quiet'.
> 
> Although 'reset.refresh=false' functionally served the same purpose as
> 'reset.quiet=true', it exposed [1] the fact that the existence of a global
> "skip refresh" option could potentially cause problems for users. Allowing a
> global config option to avoid refreshing the index forces scripts using 'git
> reset --mixed' to defensively use '--refresh' if index refresh is expected;
> if that option is missing, behavior of a script could vary from user-to-user
> without explanation.
> 
> Furthermore, globally disabling index refresh in 'reset --mixed' was
> initially devised as a passive performance improvement; since the
> introduction of the option, other changes have been made to Git (e.g., the
> sparse index) with a greater potential performance impact without
> sacrificing index correctness. Therefore, we can more aggressively err on
> the side of correctness and limit the cases of skipping index refresh to
> only when a user specifies the '--no-refresh' option.

Thanks for the well explained commit message
> [1] https://lore.kernel.org/git/xmqqy2179o3c.fsf@gitster.g/
> 
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>   Documentation/git-reset.txt |  4 +---
>   builtin/reset.c             |  4 +---
>   t/t7102-reset.sh            | 14 ++------------
>   3 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index f4aca9dd35c..df167eaa766 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -109,9 +109,7 @@ OPTIONS
>   
>   --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, refresh is enabled.
> +	Proactively refresh the index after a mixed reset. Enabled by default.

"Proactively" seems a but superfluous if I'm being picky. There was no 
entry in the config man page for reset.refresh so we don't need to do 
anything there. The code changes below look good

Best Wishes

Phillip


>   --pathspec-from-file=<file>::
>   	Pathspec is passed in `<file>` instead of commandline args. If
> diff --git a/builtin/reset.c b/builtin/reset.c
> index e824aad3604..54324217f93 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -423,7 +423,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>   	};
>   
>   	git_config(git_reset_config, NULL);
> -	git_config_get_bool("reset.refresh", &refresh);
>   
>   	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
>   						PARSE_OPT_KEEP_DASHDASH);
> @@ -529,8 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>   				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
>   				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);
> +						 "'--no-refresh' to avoid this."), t_delta_in_ms / 1000.0);
>   				}
>   			}
>   		} else {
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 9e4c4deee35..22477f3a312 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -493,19 +493,9 @@ test_expect_success '--mixed refreshes the index' '
>   '
>   
>   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
> +	# Verify that --[no-]refresh controls index refresh
>   	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_reset_refreshes_index "" --no-refresh
>   '
>   
>   test_expect_success '--mixed preserves skip-worktree' '

Re: [PATCH 3/4] reset: deprecate 'reset.refresh' config option

[Victoria Dye]

Phillip Wood wrote:
> Hi Victoria
> 
> On 21/03/2022 20:34, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <vdye@github.com>
> 
> Same concern about the title as the last patch
> 

Agreed, I'll change it to "remove" in this and the previous patch.

>> Remove the 'reset.refresh' option, requiring that users explicitly specify
>> '--no-refresh' if they want to skip refreshing the index.
>>
>> The 'reset.refresh' option was introduced in 101cee42dd (reset: introduce
>> --[no-]refresh option to --mixed, 2022-03-11) as a replacement for the
>> refresh-skipping behavior originally controlled by 'reset.quiet'.
>>
>> Although 'reset.refresh=false' functionally served the same purpose as
>> 'reset.quiet=true', it exposed [1] the fact that the existence of a global
>> "skip refresh" option could potentially cause problems for users. Allowing a
>> global config option to avoid refreshing the index forces scripts using 'git
>> reset --mixed' to defensively use '--refresh' if index refresh is expected;
>> if that option is missing, behavior of a script could vary from user-to-user
>> without explanation.
>>
>> Furthermore, globally disabling index refresh in 'reset --mixed' was
>> initially devised as a passive performance improvement; since the
>> introduction of the option, other changes have been made to Git (e.g., the
>> sparse index) with a greater potential performance impact without
>> sacrificing index correctness. Therefore, we can more aggressively err on
>> the side of correctness and limit the cases of skipping index refresh to
>> only when a user specifies the '--no-refresh' option.
> 
> Thanks for the well explained commit message
>> [1] https://lore.kernel.org/git/xmqqy2179o3c.fsf@gitster.g/
>>
>> Signed-off-by: Victoria Dye <vdye@github.com>
>> ---
>>   Documentation/git-reset.txt |  4 +---
>>   builtin/reset.c             |  4 +---
>>   t/t7102-reset.sh            | 14 ++------------
>>   3 files changed, 4 insertions(+), 18 deletions(-)
>>
>> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
>> index f4aca9dd35c..df167eaa766 100644
>> --- a/Documentation/git-reset.txt
>> +++ b/Documentation/git-reset.txt
>> @@ -109,9 +109,7 @@ OPTIONS
>>     --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, refresh is enabled.
>> +    Proactively refresh the index after a mixed reset. Enabled by default.
> 
> "Proactively" seems a but superfluous if I'm being picky. There was no entry in the config man page for reset.refresh so we don't need to do anything there. The code changes below look good
> 

Will update, thanks!

> Best Wishes
> 
> Phillip
> 
> 
>>   --pathspec-from-file=<file>::
>>       Pathspec is passed in `<file>` instead of commandline args. If
>> diff --git a/builtin/reset.c b/builtin/reset.c
>> index e824aad3604..54324217f93 100644
>> --- a/builtin/reset.c
>> +++ b/builtin/reset.c
>> @@ -423,7 +423,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>>       };
>>         git_config(git_reset_config, NULL);
>> -    git_config_get_bool("reset.refresh", &refresh);
>>         argc = parse_options(argc, argv, prefix, options, git_reset_usage,
>>                           PARSE_OPT_KEEP_DASHDASH);
>> @@ -529,8 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>>                   t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
>>                   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);
>> +                         "'--no-refresh' to avoid this."), t_delta_in_ms / 1000.0);
>>                   }
>>               }
>>           } else {
>> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
>> index 9e4c4deee35..22477f3a312 100755
>> --- a/t/t7102-reset.sh
>> +++ b/t/t7102-reset.sh
>> @@ -493,19 +493,9 @@ test_expect_success '--mixed refreshes the index' '
>>   '
>>     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
>> +    # Verify that --[no-]refresh controls index refresh
>>       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_reset_refreshes_index "" --no-refresh
>>   '
>>     test_expect_success '--mixed preserves skip-worktree' '
> 

[PATCH 4/4] reset: deprecate '--refresh', leaving only '--no-refresh'

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

The explicit '--refresh' option was needed in the past when '--quiet',
'reset.quiet', and/or 'reset.refresh' disabled the index refresh in 'reset
--mixed'. Those options have since either been deprecated or made to always
refresh the index by default, leaving only '--[no-]refresh' determining
whether the index is refreshed or not.

Because there is nothing other than '--no-refresh' to disable index refresh,
we do not need a '--refresh' option to counteract some other refresh
disabling.

To ensure users don't use what is effectively a no-op, remove '--refresh'
from the set of 'reset' options, as well as its usage in 'git stash'.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/git-reset.txt | 3 +--
 builtin/reset.c             | 6 +++---
 builtin/stash.c             | 4 ++--
 t/t7102-reset.sh            | 5 ++---
 4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index df167eaa766..ba8dece0c03 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -107,9 +107,8 @@ OPTIONS
 --quiet::
 	Be quiet, only report errors.
 
---refresh::
 --no-refresh::
-	Proactively refresh the index after a mixed reset. Enabled by default.
+	Disable refreshing the index after a mixed reset.
 
 --pathspec-from-file=<file>::
 	Pathspec is passed in `<file>` instead of commandline args. If
diff --git a/builtin/reset.c b/builtin/reset.c
index 54324217f93..d9427abc483 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -392,7 +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 refresh = 1;
 	int patch_mode = 0, pathspec_file_nul = 0, unborn;
 	const char *rev, *pathspec_from_file = NULL;
 	struct object_id oid;
@@ -400,8 +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, "no-refresh", &refresh,
+				N_("skip refreshing the index after reset"), 0),
 		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),
diff --git a/builtin/stash.c b/builtin/stash.c
index 91407d9bbe0..73f2ba88823 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_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL);
+	strvec_pushl(&cp.args, "reset", "--quiet", NULL);
 
 	return run_command(&cp);
 }
@@ -1633,7 +1633,7 @@ 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", "--refresh", "--",
+			strvec_pushl(&cp.args, "reset", "-q", "--",
 				     NULL);
 			add_pathspecs(&cp.args, ps);
 			if (run_command(&cp)) {
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 22477f3a312..7a9b845df8c 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -492,9 +492,8 @@ test_expect_success '--mixed refreshes the index' '
 	test_reset_refreshes_index "" --quiet
 '
 
-test_expect_success '--mixed --[no-]refresh sets refresh behavior' '
-	# Verify that --[no-]refresh controls index refresh
-	test_reset_refreshes_index "" --refresh &&
+test_expect_success '--mixed --no-refresh sets refresh behavior' '
+	# Verify that --no-refresh controls index refresh
 	! test_reset_refreshes_index "" --no-refresh
 '
 
-- 
gitgitgadget

Re: [PATCH 4/4] reset: deprecate '--refresh', leaving only '--no-refresh'

[Phillip Wood]

Hi Victoria

On 21/03/2022 20:34, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
> 
> The explicit '--refresh' option was needed in the past when '--quiet',
> 'reset.quiet', and/or 'reset.refresh' disabled the index refresh in 'reset
> --mixed'. Those options have since either been deprecated or made to always
> refresh the index by default, leaving only '--[no-]refresh' determining
> whether the index is refreshed or not.
> 
> Because there is nothing other than '--no-refresh' to disable index refresh,
> we do not need a '--refresh' option to counteract some other refresh
> disabling.

'--refresh' could be used to countermand a previous '--no-refresh' 
(typically when using an alias that includes '--no-refresh'). Usually we 
have '--foo' even when it is enabled by default e.g. 'commit --verify'. 
I think the code below does actually support '--refresh' as 
parse_options() is smart enough to recognize option names beginning with 
'--no-' and creates an inverse option by removing the prefix rather than 
adding '--no-' as it normally does.

> To ensure users don't use what is effectively a no-op, remove '--refresh'
> from the set of 'reset' options, as well as its usage in 'git stash'.
> 
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>   Documentation/git-reset.txt | 3 +--
>   builtin/reset.c             | 6 +++---
>   builtin/stash.c             | 4 ++--
>   t/t7102-reset.sh            | 5 ++---
>   4 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index df167eaa766..ba8dece0c03 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -107,9 +107,8 @@ OPTIONS
>   --quiet::
>   	Be quiet, only report errors.
>   
> ---refresh::
>   --no-refresh::
> -	Proactively refresh the index after a mixed reset. Enabled by default.
> +	Disable refreshing the index after a mixed reset.
>   
>   --pathspec-from-file=<file>::
>   	Pathspec is passed in `<file>` instead of commandline args. If
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 54324217f93..d9427abc483 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -392,7 +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 refresh = 1;
>   	int patch_mode = 0, pathspec_file_nul = 0, unborn;
>   	const char *rev, *pathspec_from_file = NULL;
>   	struct object_id oid;
> @@ -400,8 +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, "no-refresh", &refresh,
> +				N_("skip refreshing the index after reset"), 0),

I'm confused why we need to change the option type here - am I missing 
something?


Best Wishes

Phillip

Re: [PATCH 4/4] reset: deprecate '--refresh', leaving only '--no-refresh'

[Victoria Dye]

Phillip Wood wrote:
> Hi Victoria
> 
> On 21/03/2022 20:34, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <vdye@github.com>
>>
>> The explicit '--refresh' option was needed in the past when '--quiet',
>> 'reset.quiet', and/or 'reset.refresh' disabled the index refresh in 'reset
>> --mixed'. Those options have since either been deprecated or made to always
>> refresh the index by default, leaving only '--[no-]refresh' determining
>> whether the index is refreshed or not.
>>
>> Because there is nothing other than '--no-refresh' to disable index refresh,
>> we do not need a '--refresh' option to counteract some other refresh
>> disabling.
> 
> '--refresh' could be used to countermand a previous '--no-refresh' (typically when using an alias that includes '--no-refresh'). Usually we have '--foo' even when it is enabled by default e.g. 'commit --verify'. I think the code below does actually support '--refresh' as parse_options() is smart enough to recognize option names beginning with '--no-' and creates an inverse option by removing the prefix rather than adding '--no-' as it normally does.
> 

Makes sense, I didn't think of the alias use-case/generally wanting to
counteract the negative version of the option. Thanks! 

>> To ensure users don't use what is effectively a no-op, remove '--refresh'
>> from the set of 'reset' options, as well as its usage in 'git stash'.
>>
>> Signed-off-by: Victoria Dye <vdye@github.com>
>> ---
>>   Documentation/git-reset.txt | 3 +--
>>   builtin/reset.c             | 6 +++---
>>   builtin/stash.c             | 4 ++--
>>   t/t7102-reset.sh            | 5 ++---
>>   4 files changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
>> index df167eaa766..ba8dece0c03 100644
>> --- a/Documentation/git-reset.txt
>> +++ b/Documentation/git-reset.txt
>> @@ -107,9 +107,8 @@ OPTIONS
>>   --quiet::
>>       Be quiet, only report errors.
>>   ---refresh::
>>   --no-refresh::
>> -    Proactively refresh the index after a mixed reset. Enabled by default.
>> +    Disable refreshing the index after a mixed reset.
>>     --pathspec-from-file=<file>::
>>       Pathspec is passed in `<file>` instead of commandline args. If
>> diff --git a/builtin/reset.c b/builtin/reset.c
>> index 54324217f93..d9427abc483 100644
>> --- a/builtin/reset.c
>> +++ b/builtin/reset.c
>> @@ -392,7 +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 refresh = 1;
>>       int patch_mode = 0, pathspec_file_nul = 0, unborn;
>>       const char *rev, *pathspec_from_file = NULL;
>>       struct object_id oid;
>> @@ -400,8 +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, "no-refresh", &refresh,
>> +                N_("skip refreshing the index after reset"), 0),
> 
> I'm confused why we need to change the option type here - am I missing something?
> 

This was to explicitly prevent a user from specifying "--refresh", only
allowing "--no-refresh". But, based on your earlier comments, there's still
some value in having "--refresh", so I'll drop this patch entirely.

> 
> Best Wishes
> 
> Phillip

[PATCH v2 0/3] reset: make --no-refresh the only way to skip index refresh

[Victoria Dye via GitGitGadget]

Maintainer's note: this is based on vd/stash-silence-reset (specifically,
4b8b0f6fa2 (stash: make internal resets quiet and refresh index,
2022-03-15)).

----------------------------------------------------------------------------

This is a follow-up to the changes in vd/stash-silence-reset [1], in which
index refreshing behavior was decoupled from log silencing in the '--quiet'
option to 'git reset --mixed' by introducing a '--[no-]refresh' option and
'reset.refresh' config setting.

After some discussion [2] on the mailing list, both the
backward-compatibility and use of global options in that series came into
question:

 * '--quiet' still skipped refresh if neither '--[no-]refresh' nor
   'reset.refresh' were specified, meaning that users could still be left
   with an incorrect index state after reset.
 * Having 'reset.quiet' and/or 'reset.refresh' potentially disable index
   refresh by default meant that developers would need to defensively add
   '--refresh' to all internal uses of 'git reset --mixed'. Without that
   option, different config setups could cause variability in index
   correctness from user to user.

In response, this series removes all methods of skipping index refresh in
'git reset --mixed' except for '--no-refresh' itself:

 * Patch [1/3] removes the "fallback" behavior of 'reset.quiet' and
   '--quiet' implying '--no-refresh' if neither '--[no-]refresh' nor
   'config.refresh' were specified. In other words, '--quiet' no longer does
   anything other than log silencing.
 * Patch [2/3] removes 'reset.quiet', since its main use was to disable
   index refresh until that behavior was removed in [1/3].
 * Patch [3/3] removes 'reset.refresh' to avoid users accidentally ending up
   with an incorrect index state after all resets as a result of a global
   setting's passive effects.


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

 * Dropped patch that removed '--refresh', again allowing both
   '--no-refresh' and '--refresh' as valid options.
 * Updated documentation of "--refresh" option to remove unnecessary
   "proactively".
 * Reworded commit titles to change "deprecate" to the more accurate
   "remove".

[1]
https://lore.kernel.org/git/pull.1170.v3.git.1647308982.gitgitgadget@gmail.com/
[2]
https://lore.kernel.org/git/80a2a5a2-256f-6c3b-2430-10bef99ce1e9@github.com/

Thanks! -Victoria

Victoria Dye (3):
  reset: do not make '--quiet' disable index refresh
  reset: remove 'reset.quiet' config option
  reset: remove 'reset.refresh' config option

 Documentation/config.txt       |  2 --
 Documentation/config/reset.txt |  2 --
 Documentation/git-reset.txt    | 12 ++-------
 builtin/reset.c                | 14 ++---------
 contrib/scalar/scalar.c        |  1 -
 t/t7102-reset.sh               | 45 +++++-----------------------------
 6 files changed, 10 insertions(+), 66 deletions(-)
 delete mode 100644 Documentation/config/reset.txt


base-commit: 877d90220e42b40cf5b899dc36a13c348220b54c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1184%2Fvdye%2Freset%2Fclean-up-refresh-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1184/vdye/reset/clean-up-refresh-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1184

Range-diff vs v1:

 1:  f89e9b4ae24 ! 1:  1b607e0610b reset: do not make '--quiet' disable index refresh
     @@ Commit message
          behavior from '--quiet' because it is completely unrelated to the stated
          purpose of the option: "Be quiet, only report errors."
      
     +    Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
          Signed-off-by: Victoria Dye <vdye@github.com>
      
       ## Documentation/git-reset.txt ##
     @@ Documentation/git-reset.txt: OPTIONS
       	Pathspec is passed in `<file>` instead of commandline args. If
      
       ## builtin/reset.c ##
     +@@ builtin/reset.c: 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 refresh = 1;
     + 	int patch_mode = 0, pathspec_file_nul = 0, unborn;
     + 	const char *rev, *pathspec_from_file = NULL;
     + 	struct object_id oid;
      @@ builtin/reset.c: int cmd_reset(int argc, const char **argv, const char *prefix)
       						PARSE_OPT_KEEP_DASHDASH);
       	parse_args(&pathspec, argv, prefix, patch_mode, &rev);
 2:  d9bebd4b4e0 ! 2:  a25aff3ac7c reset: deprecate 'reset.quiet' config option
     @@ Metadata
      Author: Victoria Dye <vdye@github.com>
      
       ## Commit message ##
     -    reset: deprecate 'reset.quiet' config option
     +    reset: remove 'reset.quiet' config option
      
          Remove the 'reset.quiet' config option, remove '--no-quiet' documentation in
          'Documentation/git-reset.txt'. In 4c3abd0551 (reset: add new reset.quiet
 3:  ecd3296fd25 ! 3:  597aa82851c reset: deprecate 'reset.refresh' config option
     @@ Metadata
      Author: Victoria Dye <vdye@github.com>
      
       ## Commit message ##
     -    reset: deprecate 'reset.refresh' config option
     +    reset: remove 'reset.refresh' config option
      
          Remove the 'reset.refresh' option, requiring that users explicitly specify
          '--no-refresh' if they want to skip refreshing the index.
     @@ Documentation/git-reset.txt: OPTIONS
      -	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, refresh is enabled.
     -+	Proactively refresh the index after a mixed reset. Enabled by default.
     ++	Refresh the index after a mixed reset. Enabled by default.
       
       --pathspec-from-file=<file>::
       	Pathspec is passed in `<file>` instead of commandline args. If
 4:  dbb63c4caa8 < -:  ----------- reset: deprecate '--refresh', leaving only '--no-refresh'

-- 
gitgitgadget

[PATCH v2 1/3] reset: do not make '--quiet' disable index refresh

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Update '--quiet' to no longer implicitly skip refreshing the index in a
mixed reset. Users now have the ability to explicitly disable refreshing the
index with the '--no-refresh' option, so they no longer need to use
'--quiet' to do so. Moreover, we explicitly remove the refresh-skipping
behavior from '--quiet' because it is completely unrelated to the stated
purpose of the option: "Be quiet, only report errors."

Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/git-reset.txt |  5 +----
 builtin/reset.c             |  9 +--------
 t/t7102-reset.sh            | 32 +++++---------------------------
 3 files changed, 7 insertions(+), 39 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 89ddc85c2e4..bc1646c3016 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -114,10 +114,7 @@ OPTIONS
 --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.
+	`--[no-]refresh` nor `reset.refresh` are set, refresh is enabled.
 
 --pathspec-from-file=<file>::
 	Pathspec is passed in `<file>` instead of commandline args. If
diff --git a/builtin/reset.c b/builtin/reset.c
index c8a356ec5b0..1804d0eeb84 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -392,7 +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 refresh = 1;
 	int patch_mode = 0, pathspec_file_nul = 0, unborn;
 	const char *rev, *pathspec_from_file = NULL;
 	struct object_id oid;
@@ -430,13 +430,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 						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");
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 1dc3911a060..8b62bb39b3d 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -485,25 +485,12 @@ test_reset_refreshes_index () {
 }
 
 test_expect_success '--mixed refreshes the index' '
-	# 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 &&
+	# Verify default behavior (without --[no-]refresh or reset.refresh)
+	test_reset_refreshes_index &&
 
-	# 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
+	# With --quiet & reset.quiet
+	test_reset_refreshes_index "-c reset.quiet=true" &&
+	test_reset_refreshes_index "" --quiet
 '
 
 test_expect_success '--mixed --[no-]refresh sets refresh behavior' '
@@ -522,15 +509,6 @@ test_expect_success '--mixed --[no-]refresh sets refresh behavior' '
 	! 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' '
 	echo 123 >>file2 &&
 	git add file2 &&
-- 
gitgitgadget

[PATCH v2 2/3] reset: remove 'reset.quiet' config option

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Remove the 'reset.quiet' config option, remove '--no-quiet' documentation in
'Documentation/git-reset.txt'. In 4c3abd0551 (reset: add new reset.quiet
config setting, 2018-10-23), 'reset.quiet' was introduced as a way to
globally change the default behavior of 'git reset --mixed' to skip index
refresh.

However, now that '--quiet' does not affect index refresh, 'reset.quiet'
would only serve to globally silence logging. This was not the original
intention of the config setting, and there's no precedent for such a setting
in other commands with a '--quiet' option, so it appears to be obsolete.

In addition to the options & its documentation, remove 'reset.quiet' from
the recommended config for 'scalar'.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/config.txt       | 2 --
 Documentation/config/reset.txt | 2 --
 Documentation/git-reset.txt    | 5 +----
 builtin/reset.c                | 1 -
 contrib/scalar/scalar.c        | 1 -
 t/t7102-reset.sh               | 3 +--
 6 files changed, 2 insertions(+), 12 deletions(-)
 delete mode 100644 Documentation/config/reset.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f0fb25a371c..43f5e6fd6d3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -495,8 +495,6 @@ include::config/repack.txt[]
 
 include::config/rerere.txt[]
 
-include::config/reset.txt[]
-
 include::config/sendemail.txt[]
 
 include::config/sequencer.txt[]
diff --git a/Documentation/config/reset.txt b/Documentation/config/reset.txt
deleted file mode 100644
index 63b7c45aac2..00000000000
--- a/Documentation/config/reset.txt
+++ /dev/null
@@ -1,2 +0,0 @@
-reset.quiet::
-	When set to true, 'git reset' will default to the '--quiet' option.
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index bc1646c3016..f4aca9dd35c 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -105,10 +105,7 @@ OPTIONS
 
 -q::
 --quiet::
---no-quiet::
-	Be quiet, only report errors. The default behavior is set by the
-	`reset.quiet` config option. `--quiet` and `--no-quiet` will
-	override the default behavior.
+	Be quiet, only report errors.
 
 --refresh::
 --no-refresh::
diff --git a/builtin/reset.c b/builtin/reset.c
index 1804d0eeb84..9ce55afd1be 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -423,7 +423,6 @@ 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,
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 7db2a97416e..58ca0e56f14 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -152,7 +152,6 @@ static int set_recommended_config(int reconfigure)
 		{ "pack.useBitmaps", "false", 1 },
 		{ "pack.useSparse", "true", 1 },
 		{ "receive.autoGC", "false", 1 },
-		{ "reset.quiet", "true", 1 },
 		{ "feature.manyFiles", "false", 1 },
 		{ "feature.experimental", "false", 1 },
 		{ "fetch.unpackLimit", "1", 1 },
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 8b62bb39b3d..9e4c4deee35 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -488,8 +488,7 @@ test_expect_success '--mixed refreshes the index' '
 	# Verify default behavior (without --[no-]refresh or reset.refresh)
 	test_reset_refreshes_index &&
 
-	# With --quiet & reset.quiet
-	test_reset_refreshes_index "-c reset.quiet=true" &&
+	# With --quiet
 	test_reset_refreshes_index "" --quiet
 '
 
-- 
gitgitgadget

[PATCH v2 3/3] reset: remove 'reset.refresh' config option

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Remove the 'reset.refresh' option, requiring that users explicitly specify
'--no-refresh' if they want to skip refreshing the index.

The 'reset.refresh' option was introduced in 101cee42dd (reset: introduce
--[no-]refresh option to --mixed, 2022-03-11) as a replacement for the
refresh-skipping behavior originally controlled by 'reset.quiet'.

Although 'reset.refresh=false' functionally served the same purpose as
'reset.quiet=true', it exposed [1] the fact that the existence of a global
"skip refresh" option could potentially cause problems for users. Allowing a
global config option to avoid refreshing the index forces scripts using 'git
reset --mixed' to defensively use '--refresh' if index refresh is expected;
if that option is missing, behavior of a script could vary from user-to-user
without explanation.

Furthermore, globally disabling index refresh in 'reset --mixed' was
initially devised as a passive performance improvement; since the
introduction of the option, other changes have been made to Git (e.g., the
sparse index) with a greater potential performance impact without
sacrificing index correctness. Therefore, we can more aggressively err on
the side of correctness and limit the cases of skipping index refresh to
only when a user specifies the '--no-refresh' option.

[1] https://lore.kernel.org/git/xmqqy2179o3c.fsf@gitster.g/

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

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index f4aca9dd35c..01cb4c9b9c5 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -109,9 +109,7 @@ OPTIONS
 
 --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, refresh is enabled.
+	Refresh the index after a mixed reset. Enabled by default.
 
 --pathspec-from-file=<file>::
 	Pathspec is passed in `<file>` instead of commandline args. If
diff --git a/builtin/reset.c b/builtin/reset.c
index 9ce55afd1be..1d89faef5ec 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -423,7 +423,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_reset_config, NULL);
-	git_config_get_bool("reset.refresh", &refresh);
 
 	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
 						PARSE_OPT_KEEP_DASHDASH);
@@ -529,8 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
 				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);
+						 "'--no-refresh' to avoid this."), t_delta_in_ms / 1000.0);
 				}
 			}
 		} else {
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 9e4c4deee35..22477f3a312 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -493,19 +493,9 @@ test_expect_success '--mixed refreshes the index' '
 '
 
 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
+	# Verify that --[no-]refresh controls index refresh
 	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_reset_refreshes_index "" --no-refresh
 '
 
 test_expect_success '--mixed preserves skip-worktree' '
-- 
gitgitgadget

Re: [PATCH v2 0/3] reset: make --no-refresh the only way to skip index refresh

[Derrick Stolee]

On 3/23/2022 2:17 PM, Victoria Dye via GitGitGadget wrote:
> This is a follow-up to the changes in vd/stash-silence-reset [1], in which
> index refreshing behavior was decoupled from log silencing in the '--quiet'
> option to 'git reset --mixed' by introducing a '--[no-]refresh' option and
> 'reset.refresh' config setting.

> Changes since V1
> ================
> 
>  * Dropped patch that removed '--refresh', again allowing both
>    '--no-refresh' and '--refresh' as valid options.
>  * Updated documentation of "--refresh" option to remove unnecessary
>    "proactively".
>  * Reworded commit titles to change "deprecate" to the more accurate
>    "remove".

I read through the patches in v2 along with the discussion from v1
and the changes due to that discussion. This version looks good to me.

Thanks,
-Stolee

Re: [PATCH v2 0/3] reset: make --no-refresh the only way to skip index refresh

[Junio C Hamano]

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

> Changes since V1
> ================
>
>  * Dropped patch that removed '--refresh', again allowing both
>    '--no-refresh' and '--refresh' as valid options.

Makes sense.  Thanks for suggesting this, Phillip.

>  * Updated documentation of "--refresh" option to remove unnecessary
>    "proactively".

OK.

>  * Reworded commit titles to change "deprecate" to the more accurate
>    "remove".

OK.

Will queue.  Thanks.

Re: [PATCH v2 0/3] reset: make --no-refresh the only way to skip index refresh

[Phillip Wood]

Hi Victoria

On 23/03/2022 18:17, Victoria Dye via GitGitGadget wrote:

> Changes since V1
> ================
> 
>   * Dropped patch that removed '--refresh', again allowing both
>     '--no-refresh' and '--refresh' as valid options.

I should have been clearer in my comments that I think changing the 
option name no '--no-refresh' whilst retaining '--refresh' is 
worthwhile. '--no-refresh' is the form that users will want most of the 
time and changing the option name means that the useful version will be 
shown by "reset -h".

The range-diff for the other changes looks good

Best Wishes

Phillip

Re: [PATCH v2 0/3] reset: make --no-refresh the only way to skip index refresh

[Junio C Hamano]

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

> I should have been clearer in my comments that I think changing the
> option name no '--no-refresh' whilst retaining '--refresh' is 
> worthwhile. '--no-refresh' is the form that users will want most of
> the time and changing the option name means that the useful version
> will be shown by "reset -h".

I am OK with it shown as "--[no-]refresh" or even "--refresh" alone,
as long as the description describes the "refresh" behaviour and
makes it clear that it is the default, with the expectation that the
users know from other boolean options that "--option" listed in "-h"
would naturally take "--no-option".

But as posted, 

    $ rungit seen reset -h 2>&1 | grep refresh
        --refresh             skip refreshing the index after reset

the explanation given is for "--no-refresh" (which is wrong), so
we'd need some fix in the area.  We could rephrase it to read

        --refresh             refresh the index after reset (default)

but as you suggested, I think mimicking how existing commands with
"--no-<option>" are shown, e.g. builtlin/update-ref.c does
"--no-deref",

    $ git update-ref -h 2>&1 | grep deref
        --no-deref            update <refname> not the one it points to
    $ git grep 'OPT_BOOL.*"no-deref"'
    builtin/update-ref.c:		OPT_BOOL( 0 , "no-deref", &no_deref,

would be a good approach.

> The range-diff for the other changes looks good

Thanks.

#leftoverbit: we may want to discuss if it is a good idea to teach
OPT_BOOL() to list "--[no-]<option>" in "git cmd -h", instead of
just "--<option>".

Re: [PATCH v2 0/3] reset: make --no-refresh the only way to skip index refresh

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ... as you suggested, I think mimicking how existing commands with
> "--no-<option>" are shown, e.g. builtlin/update-ref.c does
> "--no-deref",
>
>     $ git update-ref -h 2>&1 | grep deref
>         --no-deref            update <refname> not the one it points to
>     $ git grep 'OPT_BOOL.*"no-deref"'
>     builtin/update-ref.c:		OPT_BOOL( 0 , "no-deref", &no_deref,
>
> would be a good approach.
>
>> The range-diff for the other changes looks good
>
> Thanks.
>
> #leftoverbit: we may want to discuss if it is a good idea to teach
> OPT_BOOL() to list "--[no-]<option>" in "git cmd -h", instead of
> just "--<option>".


Unfortunately, I merged these already to 'next' before seeing your
comment, so we'd need to go incremental.

How about this?

----- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] reset: show --no-refresh in the short-help

In the short help output from "git reset -h", the recently added
"--[no-]refresh" option is shown like so:

        --refresh             skip refreshing the index after reset

which explains what happens when the option is given in the negative
form, i.e. "--no-refresh".  We could rephrase the explanation to
read "refresh the index after reset (default)" to hint that the user
can say "--no-refresh" to override the default, but listing the
"--no-refresh" form in the list of options would be more helpful.

Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/reset.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git c/builtin/reset.c w/builtin/reset.c
index 1d89faef5e..344fff8f3a 100644
--- c/builtin/reset.c
+++ w/builtin/reset.c
@@ -392,7 +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 no_refresh = 0;
 	int patch_mode = 0, pathspec_file_nul = 0, unborn;
 	const char *rev, *pathspec_from_file = NULL;
 	struct object_id oid;
@@ -400,7 +400,7 @@ 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,
+		OPT_BOOL(0, "no-refresh", &no_refresh,
 				N_("skip refreshing the index after reset")),
 		OPT_SET_INT(0, "mixed", &reset_type,
 						N_("reset HEAD and index"), MIXED),
@@ -519,7 +519,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 (refresh && get_git_work_tree()) {
+			if (!no_refresh && get_git_work_tree()) {
 				uint64_t t_begin, t_delta_in_ms;
 
 				t_begin = getnanotime();

Re: [PATCH v2 0/3] reset: make --no-refresh the only way to skip index refresh

[Victoria Dye]

Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> ... as you suggested, I think mimicking how existing commands with
>> "--no-<option>" are shown, e.g. builtlin/update-ref.c does
>> "--no-deref",
>>
>>     $ git update-ref -h 2>&1 | grep deref
>>         --no-deref            update <refname> not the one it points to
>>     $ git grep 'OPT_BOOL.*"no-deref"'
>>     builtin/update-ref.c:		OPT_BOOL( 0 , "no-deref", &no_deref,
>>
>> would be a good approach.
>>
>>> The range-diff for the other changes looks good
>>
>> Thanks.
>>
>> #leftoverbit: we may want to discuss if it is a good idea to teach
>> OPT_BOOL() to list "--[no-]<option>" in "git cmd -h", instead of
>> just "--<option>".
> 
> 
> Unfortunately, I merged these already to 'next' before seeing your
> comment, so we'd need to go incremental.
> 
> How about this?
> 
> ----- >8 --------- >8 --------- >8 --------- >8 -----
> Subject: [PATCH] reset: show --no-refresh in the short-help
> 
> In the short help output from "git reset -h", the recently added
> "--[no-]refresh" option is shown like so:
> 
>         --refresh             skip refreshing the index after reset
> 
> which explains what happens when the option is given in the negative
> form, i.e. "--no-refresh".  We could rephrase the explanation to
> read "refresh the index after reset (default)" to hint that the user
> can say "--no-refresh" to override the default, but listing the
> "--no-refresh" form in the list of options would be more helpful.
> 
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/reset.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git c/builtin/reset.c w/builtin/reset.c
> index 1d89faef5e..344fff8f3a 100644
> --- c/builtin/reset.c
> +++ w/builtin/reset.c
> @@ -392,7 +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 no_refresh = 0;
>  	int patch_mode = 0, pathspec_file_nul = 0, unborn;
>  	const char *rev, *pathspec_from_file = NULL;
>  	struct object_id oid;
> @@ -400,7 +400,7 @@ 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,
> +		OPT_BOOL(0, "no-refresh", &no_refresh,
>  				N_("skip refreshing the index after reset")),
>  		OPT_SET_INT(0, "mixed", &reset_type,
>  						N_("reset HEAD and index"), MIXED),
> @@ -519,7 +519,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 (refresh && get_git_work_tree()) {
> +			if (!no_refresh && get_git_work_tree()) {
>  				uint64_t t_begin, t_delta_in_ms;
>  
>  				t_begin = getnanotime();

This looks good to me, and it's passing all of the relevant tests. Thank you
both for your help with this!

Re: [PATCH v2 0/3] reset: make --no-refresh the only way to skip index refresh

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

> Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>>> ... as you suggested, I think mimicking how existing commands with
>>> "--no-<option>" are shown, e.g. builtlin/update-ref.c does
>>> "--no-deref",
>>>
>>>     $ git update-ref -h 2>&1 | grep deref
>>>         --no-deref            update <refname> not the one it points to
>>>     $ git grep 'OPT_BOOL.*"no-deref"'
>>>     builtin/update-ref.c:		OPT_BOOL( 0 , "no-deref", &no_deref,
>>>
>>> would be a good approach.
>>>
>>>> The range-diff for the other changes looks good
>>>
>>> Thanks.
>>>
>>> #leftoverbit: we may want to discuss if it is a good idea to teach
>>> OPT_BOOL() to list "--[no-]<option>" in "git cmd -h", instead of
>>> just "--<option>".
>> 
>> 
>> Unfortunately, I merged these already to 'next' before seeing your
>> comment, so we'd need to go incremental.
>> 
>> How about this?
>> 
>> ----- >8 --------- >8 --------- >8 --------- >8 -----
>> Subject: [PATCH] reset: show --no-refresh in the short-help
>> 
>> In the short help output from "git reset -h", the recently added
>> "--[no-]refresh" option is shown like so:
>> 
>>         --refresh             skip refreshing the index after reset
>> 
>> which explains what happens when the option is given in the negative
>> form, i.e. "--no-refresh".  We could rephrase the explanation to
>> read "refresh the index after reset (default)" to hint that the user
>> can say "--no-refresh" to override the default, but listing the
>> "--no-refresh" form in the list of options would be more helpful.
>> 
>> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  builtin/reset.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git c/builtin/reset.c w/builtin/reset.c
>> index 1d89faef5e..344fff8f3a 100644
>> --- c/builtin/reset.c
>> +++ w/builtin/reset.c
>> @@ -392,7 +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 no_refresh = 0;
>>  	int patch_mode = 0, pathspec_file_nul = 0, unborn;
>>  	const char *rev, *pathspec_from_file = NULL;
>>  	struct object_id oid;
>> @@ -400,7 +400,7 @@ 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,
>> +		OPT_BOOL(0, "no-refresh", &no_refresh,
>>  				N_("skip refreshing the index after reset")),
>>  		OPT_SET_INT(0, "mixed", &reset_type,
>>  						N_("reset HEAD and index"), MIXED),
>> @@ -519,7 +519,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 (refresh && get_git_work_tree()) {
>> +			if (!no_refresh && get_git_work_tree()) {
>>  				uint64_t t_begin, t_delta_in_ms;
>>  
>>  				t_begin = getnanotime();
>
> This looks good to me, and it's passing all of the relevant tests. Thank you
> both for your help with this!

OK, will queue this on top.

Thanks.

Re: [PATCH v2 0/3] reset: make --no-refresh the only way to skip index refresh

[Derrick Stolee]

On 3/24/2022 1:33 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> #leftoverbit: we may want to discuss if it is a good idea to teach
>> OPT_BOOL() to list "--[no-]<option>" in "git cmd -h", instead of
>> just "--<option>".

Good idea!

> Unfortunately, I merged these already to 'next' before seeing your
> comment, so we'd need to go incremental.
> 
> How about this?

> -		OPT_BOOL(0, "refresh", &refresh,
> +		OPT_BOOL(0, "no-refresh", &no_refresh,
>  				N_("skip refreshing the index after reset")),

I'm pleasantly surprised that this still allows --refresh (in addition to
--no-no-refresh). So, the only meaningful functional change is indeed the
-h output.

Thanks,
-Stolee

Re: [PATCH v2 0/3] reset: make --no-refresh the only way to skip index refresh

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> On 3/24/2022 1:33 PM, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>> #leftoverbit: we may want to discuss if it is a good idea to teach
>>> OPT_BOOL() to list "--[no-]<option>" in "git cmd -h", instead of
>>> just "--<option>".
>
> Good idea!
>
>> Unfortunately, I merged these already to 'next' before seeing your
>> comment, so we'd need to go incremental.
>> 
>> How about this?
>
>> -		OPT_BOOL(0, "refresh", &refresh,
>> +		OPT_BOOL(0, "no-refresh", &no_refresh,
>>  				N_("skip refreshing the index after reset")),
>
> I'm pleasantly surprised that this still allows --refresh (in addition to
> --no-no-refresh). So, the only meaningful functional change is indeed the
> -h output.

Yeah, it is a pleasant easter egg surprise that --refresh is taken
as the opposite but its cousin that we allow --no-no-refresh is
somehow questionably ugly, albeit it does not hurt anybody, except
for purists who would certainly complain that --no-no-no-refresh is
not understood.