| Message ID | pull.1184.v2.git.1648059480.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | reset: make --no-refresh the only way to skip index refresh | expand |
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
"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.
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
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>".
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();
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!
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.
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
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.
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'