| Message ID | ecd3296fd25cc936aeb2f8ae160352a2326441c5.1647894889.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reset: make --no-refresh the only way to skip index refresh | expand |
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' '
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' ' >
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' '