Hi Victoria
On 21/03/2022 20:34, Victoria Dye via GitGitGadget wrote:
From: Victoria Dye <vdye@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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' '
