From e86ec71d20d14861e4a3d047800d3ea3099c946d Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 15 Mar 2022 01:49:38 +0000 Subject: [PATCH 1/9] reset: revise index refresh advice Update the advice describing index refresh from "enumerate unstaged changes" to "refresh the index." Describing 'refresh_index(...)' as "enumerating unstaged changes" is not fully representative of what an index refresh is doing; more generally, it updates the properties of index entries that are affected by outside-of-index state, e.g. CE_UPTODATE, which is affected by the file contents on-disk. This distinction is relevant to operations that read the index but do not refresh first - e.g., 'git read-tree' - where a stale index may cause incorrect behavior. In addition to changing the advice message, use the "advise" function to print advice. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- Documentation/config/advice.txt | 4 ++-- builtin/reset.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index c40eb09cb7..971aad2f23 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -69,8 +69,8 @@ advice.*:: merge to avoid overwriting local changes. resetQuiet:: Advice to consider using the `--quiet` option to linkgit:git-reset[1] - when the command takes more than 2 seconds to enumerate unstaged - changes after reset. + when the command takes more than 2 seconds to refresh the index + after reset. resolveConflict:: Advice shown by various commands when conflicts prevent the operation from being performed. diff --git a/builtin/reset.c b/builtin/reset.c index 6e65e90c5d..a420497a14 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -525,9 +525,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix) _("Unstaged changes after reset:")); t_delta_in_ms = (getnanotime() - t_begin) / 1000000; if (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) { - printf(_("\nIt took %.2f seconds to enumerate unstaged changes after reset. You can\n" + advise(_("It took %.2f seconds to refresh the index after reset. You can\n" "use '--quiet' to avoid this. Set the config setting reset.quiet to true\n" - "to make this the default.\n"), t_delta_in_ms / 1000.0); + "to make this the default."), t_delta_in_ms / 1000.0); } } } else { From fd56fba97f26bf668749207efd6a45aee2e2f57c Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 15 Mar 2022 01:49:39 +0000 Subject: [PATCH 2/9] reset: introduce --[no-]refresh option to --mixed Add a new --[no-]refresh option that is intended to explicitly determine whether a mixed reset should end in an index refresh. Starting at 9ac8125d1a (reset: don't compute unstaged changes after reset when --quiet, 2018-10-23), using the '--quiet' option results in skipping the call to 'refresh_index(...)' at the end of a mixed reset with the goal of improving performance. However, by coupling behavior that modifies the index with the option that silences logs, there is no way for users to have one without the other (i.e., silenced logs with a refreshed index) without incurring the overhead of a separate call to 'git update-index --refresh'. Furthermore, there is minimal user-facing documentation indicating that --quiet skips the index refresh, potentially leading to unexpected issues executing commands after 'git reset --quiet' that do not themselves refresh the index (e.g., internals of 'git stash', 'git read-tree'). To mitigate these issues, '--[no-]refresh' and 'reset.refresh' are introduced to provide a dedicated mechanism for refreshing the index. When either is set, '--quiet' and 'reset.quiet' revert to controlling only whether logs are silenced and do not affect index refresh. Helped-by: Derrick Stolee Helped-by: Junio C Hamano Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- Documentation/git-reset.txt | 9 +++++ builtin/reset.c | 13 ++++++- t/t7102-reset.sh | 73 +++++++++++++++++++++++++++++++++---- 3 files changed, 87 insertions(+), 8 deletions(-) diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt index 6f7685f53d..89ddc85c2e 100644 --- a/Documentation/git-reset.txt +++ b/Documentation/git-reset.txt @@ -110,6 +110,15 @@ OPTIONS `reset.quiet` config option. `--quiet` and `--no-quiet` will override the default behavior. +--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, 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. + --pathspec-from-file=:: Pathspec is passed in `` instead of commandline args. If `` is exactly `-` then standard input is used. Pathspec diff --git a/builtin/reset.c b/builtin/reset.c index a420497a14..7f667e13d7 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -392,6 +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 patch_mode = 0, pathspec_file_nul = 0, unborn; const char *rev, *pathspec_from_file = NULL; struct object_id oid; @@ -399,6 +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, "mixed", &reset_type, N_("reset HEAD and index"), MIXED), OPT_SET_INT(0, "soft", &reset_type, N_("reset only HEAD"), SOFT), @@ -421,11 +424,19 @@ 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, 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"); @@ -517,7 +528,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 (!quiet && get_git_work_tree()) { + if (refresh && get_git_work_tree()) { uint64_t t_begin, t_delta_in_ms; t_begin = getnanotime(); diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index d05426062e..1dc3911a06 100755 --- a/t/t7102-reset.sh +++ b/t/t7102-reset.sh @@ -462,14 +462,73 @@ test_expect_success 'resetting an unmodified path is a no-op' ' git diff-index --cached --exit-code HEAD ' +test_reset_refreshes_index () { + + # To test whether the index is refreshed in `git reset --mixed` with + # the given options, create a scenario where we clearly see different + # results depending on whether the refresh occurred or not. + + # Step 0: start with a clean index + git reset --hard HEAD && + + # Step 1: remove file2, but only in the index (no change to worktree) + git rm --cached file2 && + + # Step 2: reset index & leave worktree unchanged from HEAD + git $1 reset $2 --mixed HEAD && + + # Step 3: verify whether the index is refreshed by checking whether + # file2 still has staged changes in the index differing from HEAD (if + # the refresh occurred, there should be no such changes) + git diff-files >output.log && + test_must_be_empty output.log +} + test_expect_success '--mixed refreshes the index' ' - cat >expect <<-\EOF && - Unstaged changes after reset: - M file2 - EOF - echo 123 >>file2 && - git reset --mixed HEAD >output && - test_cmp expect output + # 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 && + + # 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 +' + +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 + 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_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' ' From 9396251b371b9475b60461ddb27bd22282c86d79 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 15 Mar 2022 01:49:40 +0000 Subject: [PATCH 3/9] reset: replace '--quiet' with '--no-refresh' in performance advice Replace references to '--quiet' with '--no-refresh' in the advice on how to skip refreshing the index. When the advice was introduced, '--quiet' was the only way to avoid the expensive 'refresh_index(...)' at the end of a mixed reset. After introducing '--no-refresh', however, '--quiet' became only a fallback option for determining refresh behavior, overridden by '--[no-]refresh' or 'reset.refresh' if either is set. To ensure users are advised to use the most reliable option for avoiding 'refresh_index(...)', replace recommendation of '--quiet' with '--[no-]refresh'. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- Documentation/config/advice.txt | 8 ++++---- advice.c | 2 +- advice.h | 2 +- builtin/reset.c | 8 ++++---- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index 971aad2f23..83c2a95661 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -67,10 +67,10 @@ advice.*:: commitBeforeMerge:: Advice shown when linkgit:git-merge[1] refuses to merge to avoid overwriting local changes. - resetQuiet:: - Advice to consider using the `--quiet` option to linkgit:git-reset[1] - when the command takes more than 2 seconds to refresh the index - after reset. + resetNoRefresh:: + Advice to consider using the `--no-refresh` option to + linkgit:git-reset[1] when the command takes more than 2 seconds + to refresh the index after reset. resolveConflict:: Advice shown by various commands when conflicts prevent the operation from being performed. diff --git a/advice.c b/advice.c index 2e1fd48304..cb755c551a 100644 --- a/advice.c +++ b/advice.c @@ -61,7 +61,7 @@ static struct { [ADVICE_PUSH_NON_FF_MATCHING] = { "pushNonFFMatching", 1 }, [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName", 1 }, [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected", 1 }, - [ADVICE_RESET_QUIET_WARNING] = { "resetQuiet", 1 }, + [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh", 1 }, [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict", 1 }, [ADVICE_RM_HINTS] = { "rmHints", 1 }, [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse", 1 }, diff --git a/advice.h b/advice.h index a3957123a1..f95af70ced 100644 --- a/advice.h +++ b/advice.h @@ -36,7 +36,7 @@ struct string_list; ADVICE_PUSH_UPDATE_REJECTED_ALIAS, ADVICE_PUSH_UPDATE_REJECTED, ADVICE_PUSH_REF_NEEDS_UPDATE, - ADVICE_RESET_QUIET_WARNING, + ADVICE_RESET_NO_REFRESH_WARNING, ADVICE_RESOLVE_CONFLICT, ADVICE_RM_HINTS, ADVICE_SEQUENCER_IN_USE, diff --git a/builtin/reset.c b/builtin/reset.c index 7f667e13d7..feab85e03d 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -535,10 +535,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix) refresh_index(&the_index, flags, NULL, NULL, _("Unstaged changes after reset:")); t_delta_in_ms = (getnanotime() - t_begin) / 1000000; - if (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) { - advise(_("It took %.2f seconds to refresh the index after reset. You can\n" - "use '--quiet' to avoid this. Set the config setting reset.quiet to true\n" - "to make this the default."), t_delta_in_ms / 1000.0); + if (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); } } } else { From d492abb0ae4a00c5647189b22f7c130fb364e700 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 15 Mar 2022 01:49:41 +0000 Subject: [PATCH 4/9] reset: suppress '--no-refresh' advice if logging is silenced If using '--quiet' or 'reset.quiet=true', do not print the 'resetnoRefresh' advice string. For applications that rely on '--quiet' disabling all non-error logs, the advice message should be suppressed accordingly. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- builtin/reset.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/reset.c b/builtin/reset.c index feab85e03d..c8a356ec5b 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -535,7 +535,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) refresh_index(&the_index, flags, NULL, NULL, _("Unstaged changes after reset:")); t_delta_in_ms = (getnanotime() - t_begin) / 1000000; - if (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) { + 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); From 4b8b0f6fa2778c1f9c373620e3f07787543914c6 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 15 Mar 2022 01:49:42 +0000 Subject: [PATCH 5/9] stash: make internal resets quiet and refresh index Add the options '-q' and '--refresh' to the 'git reset' executed in 'reset_head()', and '--refresh' to the 'git reset -q' executed in 'do_push_stash(...)'. 'stash' is implemented such that git commands invoked as part of it (e.g., 'clean', 'read-tree', 'reset', etc.) have their informational output silenced. However, the 'reset' in 'reset_head()' is *not* called with '-q', leading to the potential for a misleading printout from 'git stash apply --index' if the stash included a removed file: Unstaged changes after reset: D Not only is this confusing in its own right (since, after the reset, 'git stash' execution would stage the deletion in the index), it would be printed even when the stash was applied with the '-q' option. As a result, the messaging is removed entirely by calling 'git status' with '-q'. Additionally, because the default behavior of 'git reset -q' is to skip refreshing the index, but later operations in 'git stash' subcommands expect a non-stale index, enable '--refresh' as well. Helped-by: Junio C Hamano Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- builtin/stash.c | 5 +++-- t/t3903-stash.sh | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index 3e8af210fd..91407d9bbe 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_push(&cp.args, "reset"); + strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL); return run_command(&cp); } @@ -1633,7 +1633,8 @@ 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", "--", NULL); + strvec_pushl(&cp.args, "reset", "-q", "--refresh", "--", + NULL); add_pathspecs(&cp.args, ps); if (run_command(&cp)) { ret = -1; diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index f36e121210..1a5c1bd310 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -261,6 +261,18 @@ test_expect_success 'apply -q is quiet' ' test_must_be_empty output.out ' +test_expect_success 'apply --index -q is quiet' ' + # Added file, deleted file, modified file all staged for commit + echo foo >new-file && + echo test >file && + git add new-file file && + git rm other-file && + + git stash && + git stash apply --index -q >output.out 2>&1 && + test_must_be_empty output.out +' + test_expect_success 'save -q is quiet' ' git stash save --quiet >output.out 2>&1 && test_must_be_empty output.out @@ -291,6 +303,27 @@ test_expect_success 'drop -q is quiet' ' test_must_be_empty output.out ' +test_expect_success 'stash push -q --staged refreshes the index' ' + git reset --hard && + echo test >file && + git add file && + git stash push -q --staged && + git diff-files >output.out && + test_must_be_empty output.out +' + +test_expect_success 'stash apply -q --index refreshes the index' ' + echo test >other-file && + git add other-file && + echo another-change >other-file && + git diff-files >expect && + git stash && + + git stash apply -q --index && + git diff-files >actual && + test_cmp expect actual +' + test_expect_success 'stash -k' ' echo bar3 >file && echo bar4 >file2 && From 45bf76284b40856e47e8809e952167f0316b8a99 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Wed, 23 Mar 2022 18:17:58 +0000 Subject: [PATCH 6/9] reset: do not make '--quiet' disable index refresh 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 Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- 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 89ddc85c2e..bc1646c301 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=:: Pathspec is passed in `` instead of commandline args. If diff --git a/builtin/reset.c b/builtin/reset.c index c8a356ec5b..1804d0eeb8 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 1dc3911a06..8b62bb39b3 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 + # Verify default behavior (without --[no-]refresh or reset.refresh) + test_reset_refreshes_index && - # 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 && - - # 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 && From 2efc9b84e5e9ea063ecfb2f813cb56653a03c10a Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Wed, 23 Mar 2022 18:17:59 +0000 Subject: [PATCH 7/9] 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 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 Signed-off-by: Junio C Hamano --- 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 f0fb25a371..43f5e6fd6d 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 63b7c45aac..0000000000 --- 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 bc1646c301..f4aca9dd35 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 1804d0eeb8..9ce55afd1b 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 7db2a97416..58ca0e56f1 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 8b62bb39b3..9e4c4deee3 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 ' From 7cff6765fe5c1ce97f4afba9432c8aa5c5f877ba Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Wed, 23 Mar 2022 18:18:00 +0000 Subject: [PATCH 8/9] 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. 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 Signed-off-by: Junio C Hamano --- 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 f4aca9dd35..01cb4c9b9c 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=:: Pathspec is passed in `` instead of commandline args. If diff --git a/builtin/reset.c b/builtin/reset.c index 9ce55afd1b..1d89faef5e 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 9e4c4deee3..22477f3a31 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' ' From 5891c76cd012536a5bb833e512f7262a4b4358c1 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 24 Mar 2022 10:33:10 -0700 Subject: [PATCH 9/9] 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 Acked-by: Victoria Dye Signed-off-by: Junio C Hamano --- builtin/reset.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 1d89faef5e..344fff8f3a 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 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();