reset: make --no-refresh the only way to skip index refresh

From: Victoria Dye <[email protected]>

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

---
  Documentation/git-reset.txt |  5 +----
  builtin/reset.c             |  7 -------
  t/t7102-reset.sh            | 32 +++++---------------------------
  3 files changed, 6 insertions(+), 38 deletions(-)

Documentation/git-reset.txt +1 -4

index 89ddc85c2e4..bc1646c3016 100644
@@ -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

builtin/reset.c +0 -0

index c8a356ec5b0..7c3828f6fc5 100644

From: Victoria Dye <[email protected]>

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

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

Documentation/config.txt +0 -2

index f0fb25a371c..43f5e6fd6d3 100644
@@ -495,8 +495,6 @@ include::config/repack.txt[]
 
  include::config/rerere.txt[]
 
-include::config/reset.txt[]
-
  include::config/sendemail.txt[]
 
  include::config/sequencer.txt[]

Documentation/config/reset.txt deleted +0 -2

deleted file mode 100644
index 63b7c45aac2..00000000000
@@ -1,2 +0,0 @@
-reset.quiet::
-	When set to true, 'git reset' will default to the '--quiet' option.

Documentation/git-reset.txt +1 -4

index bc1646c3016..f4aca9dd35c 100644
@@ -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::

builtin/reset.c +0 -1

index 7c3828f6fc5..e824aad3604 100644
@@ -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,

contrib/scalar/scalar.c +0 -1

index 7db2a97416e..58ca0e56f14 100644
@@ -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 },

t/t7102-reset.sh +1 -2

index 8b62bb39b3d..9e4c4deee35 100755
@@ -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
  '
 

From: Victoria Dye <[email protected]>

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

Signed-off-by Victoria Dye <[email protected]>

---
  Documentation/git-reset.txt |  4 +---
  builtin/reset.c             |  4 +---
  t/t7102-reset.sh            | 14 ++------------
  3 files changed, 4 insertions(+), 18 deletions(-)

Documentation/git-reset.txt +0 -0

index f4aca9dd35c..df167eaa766 100644

Documentation/git-reset.txt +0 -0

  --pathspec-from-file=<file>::
  	Pathspec is passed in `<file>` instead of commandline args. If

builtin/reset.c +1 -3

index e824aad3604..54324217f93 100644
@@ -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 {

t/t7102-reset.sh +2 -12

index 9e4c4deee35..22477f3a312 100755
@@ -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' '

From: Victoria Dye <[email protected]>

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

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

Documentation/git-reset.txt +1 -2

index df167eaa766..ba8dece0c03 100644
@@ -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

builtin/reset.c +1 -1

index 54324217f93..d9427abc483 100644
@@ -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;

Hi Victoria

Quoted 38 lines · Victoria Dye via GitGitGadget

From: Victoria Dye <[email protected]>

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

---
  Documentation/git-reset.txt |  5 +----
  builtin/reset.c             |  7 ------- t/t7102-reset.sh            | 32 +++++---------------------------

  3 files changed, 6 insertions(+), 38 deletions(-)

Documentation/git-reset.txt +0 -0

index 89ddc85c2e4..bc1646c3016 100644
@@ -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

builtin/reset.c +0 -0

index c8a356ec5b0..7c3828f6fc5 100644
@@ -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);

Quoted 6 lines

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

Hi Victoria

Quoted 8 lines · Victoria Dye via GitGitGadget

From: Victoria Dye <[email protected]>

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.

Quoted 3 lines

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.

Quoted 39 lines

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

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

Documentation/git-reset.txt +0 -0

index df167eaa766..ba8dece0c03 100644
@@ -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

builtin/reset.c +1 -1

index 54324217f93..d9427abc483 100644
@@ -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;

builtin/reset.c int cmd_reset(int argc, const char **argv, const char *prefix) · line 404

Quoted 6 lines

builtin/reset.c +2 -2

       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

Hi Victoria

Quoted 1 line · Victoria Dye via GitGitGadget

From: Victoria Dye <[email protected]>

Same concern about the title as the last patch

Quoted 15 lines

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.

Quoted 7 lines

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

Quoted 9 lines

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

Signed-off-by Victoria Dye <[email protected]>

---
  Documentation/git-reset.txt |  4 +---
  builtin/reset.c             |  4 +---
  t/t7102-reset.sh            | 14 ++------------

  3 files changed, 4 insertions(+), 18 deletions(-)

Quoted 11 lines

Documentation/git-reset.txt +0 -0

index f4aca9dd35c..df167eaa766 100644
@@ -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

Quoted 46 lines

  --pathspec-from-file=<file>::
      Pathspec is passed in <file> instead of commandline args. If

builtin/reset.c +0 -1

index e824aad3604..54324217f93 100644
@@ -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 {

t/t7102-reset.sh +0 -0

index 9e4c4deee35..22477f3a312 100755
@@ -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

t/t7102-reset.sh +1 -6

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

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

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

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

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

Quoted 1 line

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

Quoted 11 lines · Junio C Hamano

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

Quoted 7 lines

Quoted 1 line

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

Signed-off-by Junio C Hamano <[email protected]>

---
 builtin/reset.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

builtin/reset.c +2 -2

index 1d89faef5e..344fff8f3a 100644
@@ -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),

Quoted 77 lines · Junio C Hamano

Quoted 11 lines · Junio C Hamano

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

Quoted 7 lines

Quoted 1 line

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

Signed-off-by Junio C Hamano <[email protected]>

---
 builtin/reset.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

builtin/reset.c +3 -3

index 1d89faef5e..344fff8f3a 100644
@@ -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;
 

builtin/reset.c int cmd_reset(int argc, const char **argv, const char *prefix) · line 525

Quoted 1 line

builtin/reset.c +0 -0

 				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!

Quoted 3 lines · Junio C Hamano

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

Quoted 4 lines · Junio C Hamano

Quoted 3 lines · Junio C Hamano

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

Quoted 4 lines

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

How about this?

builtin/reset.c int cmd_reset(int argc, const char **argv, const char *prefix) · line 404

Quoted 3 lines

builtin/reset.c +1 -1

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