{"thread":{"id":"57545","subject":"[PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash'","startedAt":"2022-03-12T00:08:56Z","lastAt":"2022-03-18T19:20:30Z","messageCount":39,"participants":["Victoria Dye via GitGitGadget","Victoria Dye","Junio C Hamano","Derrick Stolee","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"451188","messageId":"pull.1170.git.1647043729.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":null,"subject":"[PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-12T00:08:44Z","receivedAt":"2022-03-12T00:08:56Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"In the process of working on tests for 'git stash' sparse index integration,\nI found that the '--quiet' option in 'git stash' does not suppress all\nnon-error output when used with '--index'. Specifically, this comes from an\ninvocation of 'git reset' without the '--quiet' flag in 'reset_head()'. Upon\nenabling that flag, though, I discovered that 1) 'reset' does not refresh\nthe index if '--quiet' is specified (as of [1]) and 2) 'git stash' needs the\nindex to be refreshed after the reset.\n\nThis series aims to decouple the \"suppress logging\" and \"skip index refresh\"\nbehaviors in 'git reset --mixed', then allow 'stash' to internally use reset\nwith logs suppressed but index refresh enabled. This is accomplished by\nintroducing the '--[no-]refresh' option and 'reset.refresh' config setting\nto 'git reset'. Additionally, in the spirit of backward-compatibility,\n'--quiet' and/or 'reset.quiet=true' without any specified \"refresh\"\noption/config will continue to skip 'refresh_index(...)'.\n\nThere are also some minor updates to the advice that suggests skipping the\nindex refresh:\n\n * replace recommendation to use \"--quiet\" with \"--no-refresh\"\n * use 'advise()' rather than 'printf()'\n * rename the advice config setting from 'advice.resetQuiet' to to\n 'advice.resetNoRefresh'\n * suppress advice if '--quiet' is specified in 'reset'\n\nFinally, tests are added to 't7102-reset.sh' verifying whether index refresh\nhappens when expected and to 't3903-stash.sh' verifying that 'apply --quiet'\nno longer prints extraneous logs.\n\n[1] https://lore.kernel.org/git/20181023190423.5772-2-peartben@gmail.com/\n\nThanks! -Victoria\n\nVictoria Dye (5):\n reset: revise index refresh advice\n reset: introduce --[no-]refresh option to --mixed\n reset: replace '--quiet' with '--no-refresh' in performance advice\n reset: suppress '--no-refresh' advice if logging is silenced\n stash: make internal resets quiet and refresh index\n\n Documentation/config/advice.txt | 8 +--\n Documentation/git-reset.txt | 9 ++++\n advice.c | 2 +-\n advice.h | 2 +-\n builtin/reset.c | 21 ++++++--\n builtin/stash.c | 5 +-\n t/t3903-stash.sh | 12 +++++\n t/t7102-reset.sh | 95 ++++++++++++++++++++++++++++++---\n 8 files changed, 134 insertions(+), 20 deletions(-)\n\n\nbase-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1170%2Fvdye%2Fstash%2Fmake-reset-quiet-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1170/vdye/stash/make-reset-quiet-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1170\n-- \ngitgitgadget\n"},{"id":"451189","messageId":"7206ef8dd8ab088feab841c5574d486b6f19af5e.1647043729.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.git.1647043729.gitgitgadget@gmail.com","subject":"[PATCH 1/5] reset: revise index refresh advice","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-12T00:08:45Z","receivedAt":"2022-03-12T00:08:58Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nUpdate the advice describing index refresh from \"enumerate unstaged changes\"\nto \"refresh the index.\" Describing 'refresh_index(...)' as \"enumerating\nunstaged changes\" is not fully representative of what an index refresh is\ndoing; more generally, it updates the properties of index entries that are\naffected by outside-of-index state, e.g. CE_UPTODATE, which is affected by\nthe file contents on-disk. This distinction is relevant to operations that\nread the index but do not refresh first - e.g., 'git read-tree' - where a\nstale index may cause incorrect behavior.\n\nIn addition to changing the advice message, use the \"advise\" function to\nprint advice.\n\nSigned-off-by: Victoria Dye \n---\n Documentation/config/advice.txt | 4 ++--\n builtin/reset.c | 4 ++--\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex c40eb09cb7e..971aad2f237 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -69,8 +69,8 @@ advice.*::\n \t\tmerge to avoid overwriting local changes.\n \tresetQuiet::\n \t\tAdvice to consider using the `--quiet` option to linkgit:git-reset[1]\n-\t\twhen the command takes more than 2 seconds to enumerate unstaged\n-\t\tchanges after reset.\n+\t\twhen the command takes more than 2 seconds to refresh the index\n+\t\tafter reset.\n \tresolveConflict::\n \t\tAdvice shown by various commands when conflicts\n \t\tprevent the operation from being performed.\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 6e65e90c5db..a420497a14f 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -525,9 +525,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t _(\"Unstaged changes after reset:\"));\n \t\t\t\tt_delta_in_ms = (getnanotime() - t_begin) / 1000000;\n \t\t\t\tif (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n-\t\t\t\t\tprintf(_(\"\\nIt took %.2f seconds to enumerate unstaged changes after reset. You can\\n\"\n+\t\t\t\t\tadvise(_(\"It took %.2f seconds to refresh the index after reset. You can\\n\"\n \t\t\t\t\t\t\"use '--quiet' to avoid this. Set the config setting reset.quiet to true\\n\"\n-\t\t\t\t\t\t\"to make this the default.\\n\"), t_delta_in_ms / 1000.0);\n+\t\t\t\t\t\t\"to make this the default.\"), t_delta_in_ms / 1000.0);\n \t\t\t\t}\n \t\t\t}\n \t\t} else {\n-- \ngitgitgadget\n\n"},{"id":"451190","messageId":"bda93703013e3576101079d6aa4b821ae4c73fb7.1647043729.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.git.1647043729.gitgitgadget@gmail.com","subject":"[PATCH 2/5] reset: introduce --[no-]refresh option to --mixed","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-12T00:08:46Z","receivedAt":"2022-03-12T00:09:01Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd a new --[no-]refresh option that is intended to explicitly determine\nwhether a mixed reset should end in an index refresh.\n\nA few years ago, [1] introduced behavior to the '--quiet' option to skip the\ncall to 'refresh_index(...)' at the end of a mixed reset with the goal of\nimproving performance. However, by coupling behavior that modifies the index\nwith the option that silences logs, there is no way for users to have one\nwithout the other (i.e., silenced logs with a refreshed index) without\nincurring the overhead of a separate call to 'git update-index --refresh'.\nFurthermore, there is minimal user-facing documentation indicating that\n--quiet skips the index refresh, potentially leading to unexpected issues\nexecuting commands after 'git reset --quiet' that do not themselves refresh\nthe index (e.g., internals of 'git stash', 'git read-tree').\n\nTo mitigate these issues, '--[no-]refresh' and 'reset.refresh' are\nintroduced to provide a dedicated mechanism for refreshing the index. When\neither is set, '--quiet' and 'reset.quiet' revert to controlling only\nwhether logs are silenced and do not affect index refresh.\n\n[1] 9ac8125d1a (reset: don't compute unstaged changes after reset when\n --quiet, 2018-10-23)\n\nSigned-off-by: Victoria Dye \n---\n Documentation/git-reset.txt | 9 +++++\n builtin/reset.c | 13 ++++++-\n t/t7102-reset.sh | 77 +++++++++++++++++++++++++++++++++----\n 3 files changed, 91 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt\nindex 6f7685f53d5..89ddc85c2e4 100644\n--- a/Documentation/git-reset.txt\n+++ b/Documentation/git-reset.txt\n@@ -110,6 +110,15 @@ OPTIONS\n \t`reset.quiet` config option. `--quiet` and `--no-quiet` will\n \toverride the default behavior.\n \n+--refresh::\n+--no-refresh::\n+\tProactively refresh the index after a mixed reset. If unspecified, the\n+\tbehavior falls back on the `reset.refresh` config option. If neither\n+\t`--[no-]refresh` nor `reset.refresh` are set, the default behavior is\n+\tdecided by the `--[no-]quiet` option and/or `reset.quiet` config.\n+\tIf `--quiet` is specified or `reset.quiet` is set with no command-line\n+\t\"quiet\" setting, refresh is disabled. Otherwise, refresh is enabled.\n+\n --pathspec-from-file=::\n \tPathspec is passed in `` instead of commandline args. If\n \t`` is exactly `-` then standard input is used. Pathspec\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex a420497a14f..7f667e13d71 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -392,6 +392,7 @@ static int git_reset_config(const char *var, const char *value, void *cb)\n int cmd_reset(int argc, const char **argv, const char *prefix)\n {\n \tint reset_type = NONE, update_ref_status = 0, quiet = 0;\n+\tint refresh = -1;\n \tint patch_mode = 0, pathspec_file_nul = 0, unborn;\n \tconst char *rev, *pathspec_from_file = NULL;\n \tstruct object_id oid;\n@@ -399,6 +400,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tint intent_to_add = 0;\n \tconst struct option options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n+\t\tOPT_BOOL(0, \"refresh\", &refresh,\n+\t\t\t\tN_(\"skip refreshing the index after reset\")),\n \t\tOPT_SET_INT(0, \"mixed\", &reset_type,\n \t\t\t\t\t\tN_(\"reset HEAD and index\"), MIXED),\n \t\tOPT_SET_INT(0, \"soft\", &reset_type, N_(\"reset only HEAD\"), SOFT),\n@@ -421,11 +424,19 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_reset_config, NULL);\n \tgit_config_get_bool(\"reset.quiet\", &quiet);\n+\tgit_config_get_bool(\"reset.refresh\", &refresh);\n \n \targc = parse_options(argc, argv, prefix, options, git_reset_usage,\n \t\t\t\t\t\tPARSE_OPT_KEEP_DASHDASH);\n \tparse_args(&pathspec, argv, prefix, patch_mode, &rev);\n \n+\t/*\n+\t * If refresh is completely unspecified (either by config or by command\n+\t * line option), decide based on 'quiet'.\n+\t */\n+\tif (refresh < 0)\n+\t\trefresh = !quiet;\n+\n \tif (pathspec_from_file) {\n \t\tif (patch_mode)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--pathspec-from-file\", \"--patch\");\n@@ -517,7 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\tif (read_from_tree(&pathspec, &oid, intent_to_add))\n \t\t\t\treturn 1;\n \t\t\tthe_index.updated_skipworktree = 1;\n-\t\t\tif (!quiet && get_git_work_tree()) {\n+\t\t\tif (refresh && get_git_work_tree()) {\n \t\t\t\tuint64_t t_begin, t_delta_in_ms;\n \n \t\t\t\tt_begin = getnanotime();\ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex d05426062ec..5e68180f3b2 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -462,14 +462,77 @@ test_expect_success 'resetting an unmodified path is a no-op' '\n \tgit diff-index --cached --exit-code HEAD\n '\n \n+test_index_refreshed () {\n+\n+\t# To test whether the index is refresh, create a scenario where a\n+\t# command will fail if the index is *not* refreshed:\n+\t# 1. update the worktree to match HEAD & remove file2 in the index\n+\t# 2. reset --mixed to unstage the change from step 1\n+\t# 3. read-tree HEAD~1 (which differs from HEAD in file2)\n+\t# If the index is refreshed in step 2, then file2 in the index will be\n+\t# up-to-date with HEAD and read-tree will succeed (thus failing the\n+\t# test). If the index is *not* refreshed, however, the staged deletion\n+\t# of file2 from step 1 will conflict with the changes from the tree read\n+\t# in step 3, resulting in a failure.\n+\n+\t# Step 0: start with a clean index\n+\tgit reset --hard HEAD &&\n+\n+\t# Step 1\n+\tgit rm --cached file2 &&\n+\n+\t# Step 2\n+\tgit reset $1 --mixed HEAD &&\n+\n+\t# Step 3\n+\tgit read-tree -m HEAD~1\n+}\n+\n test_expect_success '--mixed refreshes the index' '\n-\tcat >expect <<-\\EOF &&\n-\tUnstaged changes after reset:\n-\tM\tfile2\n-\tEOF\n-\techo 123 >>file2 &&\n-\tgit reset --mixed HEAD >output &&\n-\ttest_cmp expect output\n+\t# Verify default behavior (with no config settings or command line\n+\t# options)\n+\ttest_index_refreshed &&\n+'\n+test_expect_success '--mixed --[no-]quiet sets default refresh behavior' '\n+\t# Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh)\n+\t# determine refresh behavior\n+\n+\t# Config setting\n+\ttest_must_fail test_index_refreshed -c reset.quiet=true &&\n+\ttest_index_refreshed -c reset.quiet=true &&\n+\n+\t# Command line option\n+\ttest_must_fail test_index_refreshed --quiet &&\n+\ttest_index_refreshed --no-quiet &&\n+\n+\t# Command line option overrides config setting\n+\ttest_must_fail test_index_refreshed -c reset.quiet=false --quiet &&\n+\ttest_index_refreshed -c reset.refresh=true --no-quiet\n+'\n+\n+test_expect_success '--mixed --[no-]refresh sets refresh behavior' '\n+\t# Verify that --[no-]refresh and `reset.refresh` control index refresh\n+\n+\t# Config setting\n+\ttest_index_refreshed -c reset.refresh=true &&\n+\ttest_must_fail test_index_refreshed -c reset.refresh=false &&\n+\n+\t# Command line option\n+\ttest_index_refreshed --refresh &&\n+\ttest_must_fail test_index_refreshed --no-refresh &&\n+\n+\t# Command line option overrides config setting\n+\ttest_index_refreshed -c reset.refresh=false --refresh &&\n+\ttest_must_fail test_index_refreshed -c reset.refresh=true --no-refresh\n+'\n+\n+test_expect_success '--mixed --refresh overrides --quiet refresh behavior' '\n+\t# Verify that *both* --refresh and `reset.refresh` override the\n+\t# default non-refresh behavior of --quiet\n+\ttest_index_refreshed --refresh --quiet &&\n+\ttest_index_refreshed --refresh -c reset.quiet=true &&\n+\ttest_index_refreshed -c reset.refresh=true --quiet &&\n+\ttest_index_refreshed -c reset.refresh=true -c reset.quiet=true\n '\n \n test_expect_success '--mixed preserves skip-worktree' '\n-- \ngitgitgadget\n\n"},{"id":"451191","messageId":"63c5ee36feb38ed192f4c60bb1c1c46a9a7a6b89.1647043729.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.git.1647043729.gitgitgadget@gmail.com","subject":"[PATCH 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-12T00:08:47Z","receivedAt":"2022-03-12T00:09:02Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nReplace references to '--quiet' with '--no-refresh' in the advice on how to\nskip refreshing the index. When the advice was introduced, '--quiet' was the\nonly way to avoid the expensive 'refresh_index(...)' at the end of a mixed\nreset. After introducing '--no-refresh', however, '--quiet' became only a\nfallback option for determining refresh behavior, overridden by\n'--[no-]refresh' or 'reset.refresh' if either is set. To ensure users are\nadvised to use the most reliable option for avoiding 'refresh_index(...)',\nreplace recommendation of '--quiet' with '--[no-]refresh'.\n\nSigned-off-by: Victoria Dye \n---\n Documentation/config/advice.txt | 8 ++++----\n advice.c | 2 +-\n advice.h | 2 +-\n builtin/reset.c | 8 ++++----\n 4 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex 971aad2f237..83c2a956611 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -67,10 +67,10 @@ advice.*::\n \tcommitBeforeMerge::\n \t\tAdvice shown when linkgit:git-merge[1] refuses to\n \t\tmerge to avoid overwriting local changes.\n-\tresetQuiet::\n-\t\tAdvice to consider using the `--quiet` option to linkgit:git-reset[1]\n-\t\twhen the command takes more than 2 seconds to refresh the index\n-\t\tafter reset.\n+\tresetNoRefresh::\n+\t\tAdvice to consider using the `--no-refresh` option to\n+\t\tlinkgit:git-reset[1] when the command takes more than 2 seconds\n+\t\tto refresh the index after reset.\n \tresolveConflict::\n \t\tAdvice shown by various commands when conflicts\n \t\tprevent the operation from being performed.\ndiff --git a/advice.c b/advice.c\nindex 2e1fd483040..cb755c551a2 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -61,7 +61,7 @@ static struct {\n \t[ADVICE_PUSH_NON_FF_MATCHING]\t\t\t= { \"pushNonFFMatching\", 1 },\n \t[ADVICE_PUSH_UNQUALIFIED_REF_NAME]\t\t= { \"pushUnqualifiedRefName\", 1 },\n \t[ADVICE_PUSH_UPDATE_REJECTED]\t\t\t= { \"pushUpdateRejected\", 1 },\n-\t[ADVICE_RESET_QUIET_WARNING]\t\t\t= { \"resetQuiet\", 1 },\n+\t[ADVICE_RESET_NO_REFRESH_WARNING]\t\t= { \"resetNoRefresh\", 1 },\n \t[ADVICE_RESOLVE_CONFLICT]\t\t\t= { \"resolveConflict\", 1 },\n \t[ADVICE_RM_HINTS]\t\t\t\t= { \"rmHints\", 1 },\n \t[ADVICE_SEQUENCER_IN_USE]\t\t\t= { \"sequencerInUse\", 1 },\ndiff --git a/advice.h b/advice.h\nindex a3957123a16..f95af70ced4 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -36,7 +36,7 @@ struct string_list;\n \tADVICE_PUSH_UPDATE_REJECTED_ALIAS,\n \tADVICE_PUSH_UPDATE_REJECTED,\n \tADVICE_PUSH_REF_NEEDS_UPDATE,\n-\tADVICE_RESET_QUIET_WARNING,\n+\tADVICE_RESET_NO_REFRESH_WARNING,\n \tADVICE_RESOLVE_CONFLICT,\n \tADVICE_RM_HINTS,\n \tADVICE_SEQUENCER_IN_USE,\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 7f667e13d71..feab85e03de 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -535,10 +535,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\trefresh_index(&the_index, flags, NULL, NULL,\n \t\t\t\t\t _(\"Unstaged changes after reset:\"));\n \t\t\t\tt_delta_in_ms = (getnanotime() - t_begin) / 1000000;\n-\t\t\t\tif (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n-\t\t\t\t\tadvise(_(\"It took %.2f seconds to refresh the index after reset. You can\\n\"\n-\t\t\t\t\t\t\"use '--quiet' to avoid this. Set the config setting reset.quiet to true\\n\"\n-\t\t\t\t\t\t\"to make this the default.\"), t_delta_in_ms / 1000.0);\n+\t\t\t\tif (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n+\t\t\t\t\tadvise(_(\"It took %.2f seconds to refresh the index after reset. You can use\\n\"\n+\t\t\t\t\t\t \"'--no-refresh' to avoid this. Set the config setting reset.refresh to false\\n\"\n+\t\t\t\t\t\t \"to make this the default.\"), t_delta_in_ms / 1000.0);\n \t\t\t\t}\n \t\t\t}\n \t\t} else {\n-- \ngitgitgadget\n\n"},{"id":"451192","messageId":"3c65a9f19936548883522a4795048a7cf756c6bd.1647043729.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.git.1647043729.gitgitgadget@gmail.com","subject":"[PATCH 4/5] reset: suppress '--no-refresh' advice if logging is silenced","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-12T00:08:48Z","receivedAt":"2022-03-12T00:09:04Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nIf using '--quiet' or 'reset.quiet=true', do not print the 'resetnoRefresh'\nadvice string. For applications that rely on '--quiet' disabling all\nnon-error logs, the advice message should be suppressed accordingly.\n\nSigned-off-by: Victoria Dye \n---\n builtin/reset.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex feab85e03de..c8a356ec5b0 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -535,7 +535,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\trefresh_index(&the_index, flags, NULL, NULL,\n \t\t\t\t\t _(\"Unstaged changes after reset:\"));\n \t\t\t\tt_delta_in_ms = (getnanotime() - t_begin) / 1000000;\n-\t\t\t\tif (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n+\t\t\t\tif (!quiet && advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n \t\t\t\t\tadvise(_(\"It took %.2f seconds to refresh the index after reset. You can use\\n\"\n \t\t\t\t\t\t \"'--no-refresh' to avoid this. Set the config setting reset.refresh to false\\n\"\n \t\t\t\t\t\t \"to make this the default.\"), t_delta_in_ms / 1000.0);\n-- \ngitgitgadget\n\n"},{"id":"451193","messageId":"052499bbc93cb6a60a3b7f7861de78dd4075fd7f.1647043729.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.git.1647043729.gitgitgadget@gmail.com","subject":"[PATCH 5/5] stash: make internal resets quiet and refresh index","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-12T00:08:49Z","receivedAt":"2022-03-12T00:09:05Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd the options '-q' and '--refresh' to the 'git reset' executed in\n'reset_head()', and '--refresh' to the 'git reset -q' executed in\n'do_push_stash(...)'.\n\n'stash' is implemented such that git commands invoked as part of it (e.g.,\n'clean', 'read-tree', 'reset', etc.) have their informational output\nsilenced. However, the 'reset' in 'reset_head()' is *not* called with '-q',\nleading to the potential for a misleading printout from 'git stash apply\n--index' if the stash included a removed file:\n\nUnstaged changes after reset: D \n\nNot only is this confusing in its own right (since, after the reset, 'git\nstash' execution would stage the deletion in the index), it would be printed\neven when the stash was applied with the '-q' option. As a result, the\nmessaging is removed entirely by calling 'git status' with '-q'.\n\nAdditionally, because the default behavior of 'git reset -q' is to skip\nrefreshing the index, but later operations in 'git stash' subcommands expect\na non-stale index, enable '--refresh' as well.\n\nSigned-off-by: Victoria Dye \n---\n builtin/stash.c | 5 ++--\n t/t3903-stash.sh | 12 ++++++++++\n t/t7102-reset.sh | 60 +++++++++++++++++++++++++++++++-----------------\n 3 files changed, 54 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 3e8af210fde..91407d9bbe0 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -310,7 +310,7 @@ static int reset_head(void)\n \t * API for resetting.\n \t */\n \tcp.git_cmd = 1;\n-\tstrvec_push(&cp.args, \"reset\");\n+\tstrvec_pushl(&cp.args, \"reset\", \"--quiet\", \"--refresh\", NULL);\n \n \treturn run_command(&cp);\n }\n@@ -1633,7 +1633,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \n \t\t\tcp.git_cmd = 1;\n-\t\t\tstrvec_pushl(&cp.args, \"reset\", \"-q\", \"--\", NULL);\n+\t\t\tstrvec_pushl(&cp.args, \"reset\", \"-q\", \"--refresh\", \"--\",\n+\t\t\t\t NULL);\n \t\t\tadd_pathspecs(&cp.args, ps);\n \t\t\tif (run_command(&cp)) {\n \t\t\t\tret = -1;\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex f36e121210e..17f2ad2344c 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -261,6 +261,18 @@ test_expect_success 'apply -q is quiet' '\n \ttest_must_be_empty output.out\n '\n \n+test_expect_success 'apply --index -q is quiet' '\n+\t# Added file, deleted file, modified file all staged for commit\n+\techo foo >new-file &&\n+\techo test >file &&\n+\tgit add new-file file &&\n+\tgit rm other-file &&\n+\n+\tgit stash &&\n+\tgit stash apply --index -q >output.out 2>&1 &&\n+\ttest_must_be_empty output.out\n+'\n+\n test_expect_success 'save -q is quiet' '\n \tgit stash save --quiet >output.out 2>&1 &&\n \ttest_must_be_empty output.out\ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex 5e68180f3b2..f2076545133 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -482,7 +482,7 @@ test_index_refreshed () {\n \tgit rm --cached file2 &&\n \n \t# Step 2\n-\tgit reset $1 --mixed HEAD &&\n+\tgit reset $@ --mixed HEAD &&\n \n \t# Step 3\n \tgit read-tree -m HEAD~1\n@@ -491,48 +491,66 @@ test_index_refreshed () {\n test_expect_success '--mixed refreshes the index' '\n \t# Verify default behavior (with no config settings or command line\n \t# options)\n-\ttest_index_refreshed &&\n+\ttest_index_refreshed\n '\n test_expect_success '--mixed --[no-]quiet sets default refresh behavior' '\n \t# Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh)\n \t# determine refresh behavior\n \n-\t# Config setting\n-\ttest_must_fail test_index_refreshed -c reset.quiet=true &&\n-\ttest_index_refreshed -c reset.quiet=true &&\n-\n \t# Command line option\n-\ttest_must_fail test_index_refreshed --quiet &&\n+\t! test_index_refreshed --quiet &&\n \ttest_index_refreshed --no-quiet &&\n \n-\t# Command line option overrides config setting\n-\ttest_must_fail test_index_refreshed -c reset.quiet=false --quiet &&\n-\ttest_index_refreshed -c reset.refresh=true --no-quiet\n+\t# Config: reset.quiet=false\n+\ttest_config reset.quiet false &&\n+\t(\n+\t\ttest_index_refreshed &&\n+\t\t! test_index_refreshed --quiet\n+\t) &&\n+\n+\t# Config: reset.quiet=true\n+\ttest_config reset.quiet true &&\n+\t(\n+\t\t! test_index_refreshed &&\n+\t\ttest_index_refreshed --no-quiet\n+\t)\n '\n \n test_expect_success '--mixed --[no-]refresh sets refresh behavior' '\n \t# Verify that --[no-]refresh and `reset.refresh` control index refresh\n \n-\t# Config setting\n-\ttest_index_refreshed -c reset.refresh=true &&\n-\ttest_must_fail test_index_refreshed -c reset.refresh=false &&\n-\n \t# Command line option\n \ttest_index_refreshed --refresh &&\n-\ttest_must_fail test_index_refreshed --no-refresh &&\n+\t! test_index_refreshed --no-refresh &&\n+\n+\t# Config: reset.refresh=false\n+\ttest_config reset.refresh false &&\n+\t(\n+\t\t! test_index_refreshed &&\n+\t\ttest_index_refreshed --refresh\n+\t) &&\n \n-\t# Command line option overrides config setting\n-\ttest_index_refreshed -c reset.refresh=false --refresh &&\n-\ttest_must_fail test_index_refreshed -c reset.refresh=true --no-refresh\n+\t# Config: reset.refresh=true\n+\ttest_config reset.refresh true &&\n+\t(\n+\t\ttest_index_refreshed &&\n+\t\t! test_index_refreshed --no-refresh\n+\t)\n '\n \n test_expect_success '--mixed --refresh overrides --quiet refresh behavior' '\n \t# Verify that *both* --refresh and `reset.refresh` override the\n \t# default non-refresh behavior of --quiet\n+\n \ttest_index_refreshed --refresh --quiet &&\n-\ttest_index_refreshed --refresh -c reset.quiet=true &&\n-\ttest_index_refreshed -c reset.refresh=true --quiet &&\n-\ttest_index_refreshed -c reset.refresh=true -c reset.quiet=true\n+\n+\t# Config: reset.quiet=true\n+\ttest_config reset.quiet true &&\n+\ttest_index_refreshed --refresh &&\n+\n+\t# Config: reset.quiet=true, reset.refresh=true\n+\ttest_config reset.refresh true &&\n+\ttest_index_refreshed\n '\n \n test_expect_success '--mixed preserves skip-worktree' '\n-- \ngitgitgadget\n"},{"id":"451223","messageId":"3a4594d2-e59d-28dd-290f-adb6fd1740a9@github.com","threadId":"57545","inReplyTo":"pull.1170.git.1647043729.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash'","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-03-12T17:12:03Z","receivedAt":"2022-03-12T17:12:09Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Victoria Dye via GitGitGadget wrote:\n> In the process of working on tests for 'git stash' sparse index integration,\n> I found that the '--quiet' option in 'git stash' does not suppress all\n> non-error output when used with '--index'. Specifically, this comes from an\n> invocation of 'git reset' without the '--quiet' flag in 'reset_head()'. Upon\n> enabling that flag, though, I discovered that 1) 'reset' does not refresh\n> the index if '--quiet' is specified (as of [1]) and 2) 'git stash' needs the\n> index to be refreshed after the reset.\n> \n> This series aims to decouple the \"suppress logging\" and \"skip index refresh\"\n> behaviors in 'git reset --mixed', then allow 'stash' to internally use reset\n> with logs suppressed but index refresh enabled. This is accomplished by\n> introducing the '--[no-]refresh' option and 'reset.refresh' config setting\n> to 'git reset'. Additionally, in the spirit of backward-compatibility,\n> '--quiet' and/or 'reset.quiet=true' without any specified \"refresh\"\n> option/config will continue to skip 'refresh_index(...)'.\n> \n> There are also some minor updates to the advice that suggests skipping the\n> index refresh:\n> \n> * replace recommendation to use \"--quiet\" with \"--no-refresh\"\n> * use 'advise()' rather than 'printf()'\n> * rename the advice config setting from 'advice.resetQuiet' to to\n> 'advice.resetNoRefresh'\n> * suppress advice if '--quiet' is specified in 'reset'\n> \n> Finally, tests are added to 't7102-reset.sh' verifying whether index refresh\n> happens when expected and to 't3903-stash.sh' verifying that 'apply --quiet'\n> no longer prints extraneous logs.\n> \n> [1] https://lore.kernel.org/git/20181023190423.5772-2-peartben@gmail.com/\n> \n> Thanks! -Victoria\n> \n> Victoria Dye (5):\n> reset: revise index refresh advice\n> reset: introduce --[no-]refresh option to --mixed\n> reset: replace '--quiet' with '--no-refresh' in performance advice\n> reset: suppress '--no-refresh' advice if logging is silenced\n> stash: make internal resets quiet and refresh index\n> \n> Documentation/config/advice.txt | 8 +--\n> Documentation/git-reset.txt | 9 ++++\n> advice.c | 2 +-\n> advice.h | 2 +-\n> builtin/reset.c | 21 ++++++--\n> builtin/stash.c | 5 +-\n> t/t3903-stash.sh | 12 +++++\n> t/t7102-reset.sh | 95 ++++++++++++++++++++++++++++++---\n> 8 files changed, 134 insertions(+), 20 deletions(-)\n> \n> \n> base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1170%2Fvdye%2Fstash%2Fmake-reset-quiet-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1170/vdye/stash/make-reset-quiet-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1170\n\nIt looks like I submitted without updating the cover letter title from a\nprevious iteration (should be \"Allow 'reset --quiet' to refresh the index,\nuse 'reset --quiet' in 'stash' \"). I'll update it in the next version along\nwith any code changes.\n\nSorry about that!\n\n\n"},{"id":"451284","messageId":"xmqqwngxrr2v.fsf@gitster.g","threadId":"57545","inReplyTo":"pull.1170.git.1647043729.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-14T06:22:32Z","receivedAt":"2022-03-14T06:22:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> ... 1) 'reset' does not refresh\n> the index if '--quiet' is specified (as of [1]) and 2) 'git stash' needs the\n> index to be refreshed after the reset.\n\nThe latter is not surprising, but I have to agree with the series\nthat the former is unbelievably bad design taste, and I am happy to\nsee that this series separates out refreshing decision from the\nverbosity.\n\n"},{"id":"451313","messageId":"fac1f57c-1bb8-2458-91ce-db5cb55f2e7d@github.com","threadId":"57545","inReplyTo":"bda93703013e3576101079d6aa4b821ae4c73fb7.1647043729.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/5] reset: introduce --[no-]refresh option to --mixed","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-14T15:05:55Z","receivedAt":"2022-03-14T15:06:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye \n> \n> Add a new --[no-]refresh option that is intended to explicitly determine\n> whether a mixed reset should end in an index refresh.\n> \n> A few years ago, [1] introduced behavior to the '--quiet' option to skip the\n...\n> [1] 9ac8125d1a (reset: don't compute unstaged changes after reset when\n> --quiet, 2018-10-23)\n\nI believe convention would normally have this listing of the commit in-line\nwith your discussion of it. The \"[1]\" probably works, too, but I can't say\nthat I've seen that used except for URLs. Something like:\n\n Starting at , the '--quiet' option skips refresh_index()...\n\n> call to 'refresh_index(...)' at the end of a mixed reset with the goal of\n> improving performance. However, by coupling behavior that modifies the index\n> with the option that silences logs, there is no way for users to have one\n> without the other (i.e., silenced logs with a refreshed index) without\n> incurring the overhead of a separate call to 'git update-index --refresh'.\n> Furthermore, there is minimal user-facing documentation indicating that\n> --quiet skips the index refresh, potentially leading to unexpected issues\n> executing commands after 'git reset --quiet' that do not themselves refresh\n> the index (e.g., internals of 'git stash', 'git read-tree').\n> \n> To mitigate these issues, '--[no-]refresh' and 'reset.refresh' are\n> introduced to provide a dedicated mechanism for refreshing the index. When\n> either is set, '--quiet' and 'reset.quiet' revert to controlling only\n> whether logs are silenced and do not affect index refresh.\n\nWell motivated change.\n\n> +test_index_refreshed () {\n> +\n> +\t# To test whether the index is refresh, create a scenario where a\n> +\t# command will fail if the index is *not* refreshed:\n> +\t# 1. update the worktree to match HEAD & remove file2 in the index\n> +\t# 2. reset --mixed to unstage the change from step 1\n> +\t# 3. read-tree HEAD~1 (which differs from HEAD in file2)\n> +\t# If the index is refreshed in step 2, then file2 in the index will be\n> +\t# up-to-date with HEAD and read-tree will succeed (thus failing the\n> +\t# test). If the index is *not* refreshed, however, the staged deletion\n> +\t# of file2 from step 1 will conflict with the changes from the tree read\n> +\t# in step 3, resulting in a failure.\n> +\n> +\t# Step 0: start with a clean index\n> +\tgit reset --hard HEAD &&\n> +\n> +\t# Step 1\n> +\tgit rm --cached file2 &&\n> +\n> +\t# Step 2\n> +\tgit reset $1 --mixed HEAD &&\n> +\n> +\t# Step 3\n> +\tgit read-tree -m HEAD~1\n> +}\n> +\n> test_expect_success '--mixed refreshes the index' '\n> -\tcat >expect <<-\\EOF &&\n> -\tUnstaged changes after reset:\n> -\tM\tfile2\n> -\tEOF\n> -\techo 123 >>file2 &&\n> -\tgit reset --mixed HEAD >output &&\n> -\ttest_cmp expect output\n> +\t# Verify default behavior (with no config settings or command line\n> +\t# options)\n> +\ttest_index_refreshed &&\n> +'\n\nIt looks like this test ends with an &&. There's also a missing newline\nafter the test.\n\n> +test_expect_success '--mixed --[no-]quiet sets default refresh behavior' '\n> +\t# Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh)\n> +\t# determine refresh behavior\n> +\n> +\t# Config setting\n> +\ttest_must_fail test_index_refreshed -c reset.quiet=true &&\n\nThis is failing, but not for the reason you want: It is running\n\n\tgit reset -c --mixed HEAD\n\nand failing to parse the \"-c\", I bet.\n\nPerhaps you want to have two arguments: one for config settings\nand another for arguments, meaning your call in test_index_refreshed\nshould be\n\n\tgit $1 reset $2 --mixed HEAD\n\nand calls like this should be\n\n\ttest_index_refreshed \"-c reset.quiet=true\" \"\" &&\n\n> +\ttest_index_refreshed -c reset.quiet=true &&\n> +\n> +\t# Command line option\n> +\ttest_must_fail test_index_refreshed --quiet &&\n> +\ttest_index_refreshed --no-quiet &&\n\nIf you take a change like I recommend above, these would be\n\n\ttest_index_refreshed \"\" --no-quiet &&\n\nThanks,\n-Stolee\n"},{"id":"451314","messageId":"41db12c3-029f-2e44-703c-90cb45ed9875@github.com","threadId":"57545","inReplyTo":"052499bbc93cb6a60a3b7f7861de78dd4075fd7f.1647043729.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/5] stash: make internal resets quiet and refresh index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-14T15:10:00Z","receivedAt":"2022-03-14T15:10:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye \n> \n> Add the options '-q' and '--refresh' to the 'git reset' executed in\n> 'reset_head()', and '--refresh' to the 'git reset -q' executed in\n> 'do_push_stash(...)'.\n\n> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\n> index 5e68180f3b2..f2076545133 100755\n> --- a/t/t7102-reset.sh\n> +++ b/t/t7102-reset.sh\n> @@ -482,7 +482,7 @@ test_index_refreshed () {\n> \tgit rm --cached file2 &&\n> \n> \t# Step 2\n> -\tgit reset $1 --mixed HEAD &&\n> +\tgit reset $@ --mixed HEAD &&\n\nI see you change this from \"$1\" to \"$@\", which won't help with\nthe \"-c key=value\" issues from earlier.\n\n> \n> \t# Step 3\n> \tgit read-tree -m HEAD~1\n> @@ -491,48 +491,66 @@ test_index_refreshed () {\n> test_expect_success '--mixed refreshes the index' '\n> \t# Verify default behavior (with no config settings or command line\n> \t# options)\n> -\ttest_index_refreshed &&\n> +\ttest_index_refreshed\n\nAh, I see you fixed this here, probably just a rebase issue, then.\n\n> test_expect_success '--mixed --[no-]quiet sets default refresh behavior' '\n> \t# Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh)\n> \t# determine refresh behavior\n> \n> -\t# Config setting\n> -\ttest_must_fail test_index_refreshed -c reset.quiet=true &&\n> -\ttest_index_refreshed -c reset.quiet=true &&\n> -\n\nAh, and the -c changes are removed here. You could still test them\nusing the trick I mention in reply to patch 2.\n\n> \t# Command line option\n> -\ttest_must_fail test_index_refreshed --quiet &&\n> +\t! test_index_refreshed --quiet &&\n> \ttest_index_refreshed --no-quiet &&\n> \n> -\t# Command line option overrides config setting\n> -\ttest_must_fail test_index_refreshed -c reset.quiet=false --quiet &&\n> -\ttest_index_refreshed -c reset.refresh=true --no-quiet\n> +\t# Config: reset.quiet=false\n> +\ttest_config reset.quiet false &&\n> +\t(\n> +\t\ttest_index_refreshed &&\n> +\t\t! test_index_refreshed --quiet\n> +\t) &&\n> +\n> +\t# Config: reset.quiet=true\n> +\ttest_config reset.quiet true &&\n> +\t(\n> +\t\t! test_index_refreshed &&\n> +\t\ttest_index_refreshed --no-quiet\n> +\t)\n\nI'm not sure why you need sub-shells here. The test_config is\nnot scoped to the shell. These lines could be avoided with the\n-c trick, which should make it a bit simpler to show what you\nintend to be testing here.\n\nThanks,\n-Stolee\n"},{"id":"451315","messageId":"b9ba7ad3-c361-abcd-4735-b011d502dcad@github.com","threadId":"57545","inReplyTo":"fac1f57c-1bb8-2458-91ce-db5cb55f2e7d@github.com","subject":"Re: [PATCH 2/5] reset: introduce --[no-]refresh option to --mixed","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-14T15:13:17Z","receivedAt":"2022-03-14T15:13:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/14/2022 11:05 AM, Derrick Stolee wrote:\n> On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:\n>> From: Victoria Dye \n\n>> +test_index_refreshed () {\n>> +\n>> +\t# To test whether the index is refresh, create a scenario where a\n>> +\t# command will fail if the index is *not* refreshed:\n>> +\t# 1. update the worktree to match HEAD & remove file2 in the index\n>> +\t# 2. reset --mixed to unstage the change from step 1\n>> +\t# 3. read-tree HEAD~1 (which differs from HEAD in file2)\n>> +\t# If the index is refreshed in step 2, then file2 in the index will be\n>> +\t# up-to-date with HEAD and read-tree will succeed (thus failing the\n>> +\t# test). If the index is *not* refreshed, however, the staged deletion\n>> +\t# of file2 from step 1 will conflict with the changes from the tree read\n>> +\t# in step 3, resulting in a failure.\n>> +\n>> +\t# Step 0: start with a clean index\n>> +\tgit reset --hard HEAD &&\n>> +\n>> +\t# Step 1\n>> +\tgit rm --cached file2 &&\n>> +\n>> +\t# Step 2\n>> +\tgit reset $1 --mixed HEAD &&\n\n> Perhaps you want to have two arguments: one for config settings\n> and another for arguments, meaning your call in test_index_refreshed\n> should be\n> \n> \tgit $1 reset $2 --mixed HEAD\n> \n> and calls like this should be\n> \n> \ttest_index_refreshed \"-c reset.quiet=true\" \"\" &&\n\nAfter looking at your other examples, I thought of another example\nthat you might want to consider.\n\nIn the helper, use:\n\n\tgit $@ --mixed HEAD &&\n\nand then for the callers, use\n\n\ttest_index_refreshed refresh &&\n\nor\n\n\ttest_index_refreshed -c reset.quiet=true refresh &&\n\nor\n\n\ttest_index_refreshed refresh --quiet &&\n\nand similarly through the other tests.\n\nThanks,\n-Stolee\n"},{"id":"451316","messageId":"09550c4c-10ce-5439-98ac-ca71b9309c69@github.com","threadId":"57545","inReplyTo":"pull.1170.git.1647043729.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-14T15:13:33Z","receivedAt":"2022-03-14T15:14:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:\n> In the process of working on tests for 'git stash' sparse index integration,\n> I found that the '--quiet' option in 'git stash' does not suppress all\n> non-error output when used with '--index'. Specifically, this comes from an\n> invocation of 'git reset' without the '--quiet' flag in 'reset_head()'. Upon\n> enabling that flag, though, I discovered that 1) 'reset' does not refresh\n> the index if '--quiet' is specified (as of [1]) and 2) 'git stash' needs the\n> index to be refreshed after the reset.\n> \n> This series aims to decouple the \"suppress logging\" and \"skip index refresh\"\n> behaviors in 'git reset --mixed', then allow 'stash' to internally use reset\n> with logs suppressed but index refresh enabled. This is accomplished by\n> introducing the '--[no-]refresh' option and 'reset.refresh' config setting\n> to 'git reset'. Additionally, in the spirit of backward-compatibility,\n> '--quiet' and/or 'reset.quiet=true' without any specified \"refresh\"\n> option/config will continue to skip 'refresh_index(...)'.\n> \n> There are also some minor updates to the advice that suggests skipping the\n> index refresh:\n> \n> * replace recommendation to use \"--quiet\" with \"--no-refresh\"\n> * use 'advise()' rather than 'printf()'\n> * rename the advice config setting from 'advice.resetQuiet' to to\n> 'advice.resetNoRefresh'\n> * suppress advice if '--quiet' is specified in 'reset'\n> \n> Finally, tests are added to 't7102-reset.sh' verifying whether index refresh\n> happens when expected and to 't3903-stash.sh' verifying that 'apply --quiet'\n> no longer prints extraneous logs.\n\nGood find! Code looks great.\n\nThere are some test issues in Patch 2 that you correct in patch 5, so\nmaybe we just need to squash the changes in the right order.\n\nHowever, I recommended two alternate ways to design your helper so you\ncan test config and command-line options more easily.\n\nThanks,\n-Stolee\n"},{"id":"451319","messageId":"5742cf75-4a5c-ffc1-7791-891dd47dc956@github.com","threadId":"57545","inReplyTo":"fac1f57c-1bb8-2458-91ce-db5cb55f2e7d@github.com","subject":"Re: [PATCH 2/5] reset: introduce --[no-]refresh option to --mixed","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-03-14T15:55:32Z","receivedAt":"2022-03-14T15:55:36Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Derrick Stolee wrote:\n> On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:\n>> From: Victoria Dye \n>>\n>> Add a new --[no-]refresh option that is intended to explicitly determine\n>> whether a mixed reset should end in an index refresh.\n>>\n>> A few years ago, [1] introduced behavior to the '--quiet' option to skip the\n> ...\n>> [1] 9ac8125d1a (reset: don't compute unstaged changes after reset when\n>> --quiet, 2018-10-23)\n> \n> I believe convention would normally have this listing of the commit in-line\n> with your discussion of it. The \"[1]\" probably works, too, but I can't say\n> that I've seen that used except for URLs. Something like:\n> \n> Starting at , the '--quiet' option skips refresh_index()...\n> \n>> call to 'refresh_index(...)' at the end of a mixed reset with the goal of\n>> improving performance. However, by coupling behavior that modifies the index\n>> with the option that silences logs, there is no way for users to have one\n>> without the other (i.e., silenced logs with a refreshed index) without\n>> incurring the overhead of a separate call to 'git update-index --refresh'.\n>> Furthermore, there is minimal user-facing documentation indicating that\n>> --quiet skips the index refresh, potentially leading to unexpected issues\n>> executing commands after 'git reset --quiet' that do not themselves refresh\n>> the index (e.g., internals of 'git stash', 'git read-tree').\n>>\n>> To mitigate these issues, '--[no-]refresh' and 'reset.refresh' are\n>> introduced to provide a dedicated mechanism for refreshing the index. When\n>> either is set, '--quiet' and 'reset.quiet' revert to controlling only\n>> whether logs are silenced and do not affect index refresh.\n> \n> Well motivated change.\n> \n>> +test_index_refreshed () {\n>> +\n>> +\t# To test whether the index is refresh, create a scenario where a\n>> +\t# command will fail if the index is *not* refreshed:\n>> +\t# 1. update the worktree to match HEAD & remove file2 in the index\n>> +\t# 2. reset --mixed to unstage the change from step 1\n>> +\t# 3. read-tree HEAD~1 (which differs from HEAD in file2)\n>> +\t# If the index is refreshed in step 2, then file2 in the index will be\n>> +\t# up-to-date with HEAD and read-tree will succeed (thus failing the\n>> +\t# test). If the index is *not* refreshed, however, the staged deletion\n>> +\t# of file2 from step 1 will conflict with the changes from the tree read\n>> +\t# in step 3, resulting in a failure.\n>> +\n>> +\t# Step 0: start with a clean index\n>> +\tgit reset --hard HEAD &&\n>> +\n>> +\t# Step 1\n>> +\tgit rm --cached file2 &&\n>> +\n>> +\t# Step 2\n>> +\tgit reset $1 --mixed HEAD &&\n>> +\n>> +\t# Step 3\n>> +\tgit read-tree -m HEAD~1\n>> +}\n>> +\n>> test_expect_success '--mixed refreshes the index' '\n>> -\tcat >expect <<-\\EOF &&\n>> -\tUnstaged changes after reset:\n>> -\tM\tfile2\n>> -\tEOF\n>> -\techo 123 >>file2 &&\n>> -\tgit reset --mixed HEAD >output &&\n>> -\ttest_cmp expect output\n>> +\t# Verify default behavior (with no config settings or command line\n>> +\t# options)\n>> +\ttest_index_refreshed &&\n>> +'\n> \n> It looks like this test ends with an &&. There's also a missing newline\n> after the test.\n> \n>> +test_expect_success '--mixed --[no-]quiet sets default refresh behavior' '\n>> +\t# Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh)\n>> +\t# determine refresh behavior\n>> +\n>> +\t# Config setting\n>> +\ttest_must_fail test_index_refreshed -c reset.quiet=true &&\n> \n> This is failing, but not for the reason you want: It is running\n> \n> \tgit reset -c --mixed HEAD\n> \n> and failing to parse the \"-c\", I bet.\n> \n> Perhaps you want to have two arguments: one for config settings\n> and another for arguments, meaning your call in test_index_refreshed\n> should be\n> \n> \tgit $1 reset $2 --mixed HEAD\n> \n> and calls like this should be\n> \n> \ttest_index_refreshed \"-c reset.quiet=true\" \"\" &&\n> \n\nAs you noted in patch 5, I switched to using `test_config` mostly because I\ncouldn't figure out how to get this syntax right (although, now that you\npoint it out, I *definitely* should have seen that). I prefer using the\ninline config like this over `test_config`, so I'll update to do what you\nsuggest here.\n\n>> +\ttest_index_refreshed -c reset.quiet=true &&\n>> +\n>> +\t# Command line option\n>> +\ttest_must_fail test_index_refreshed --quiet &&\n>> +\ttest_index_refreshed --no-quiet &&\n> \n> If you take a change like I recommend above, these would be\n> \n> \ttest_index_refreshed \"\" --no-quiet &&\n> \n> Thanks,\n> -Stolee\n\n"},{"id":"451320","messageId":"53967755-a8c4-d9d7-dd7b-d08782ab88ee@github.com","threadId":"57545","inReplyTo":"41db12c3-029f-2e44-703c-90cb45ed9875@github.com","subject":"Re: [PATCH 5/5] stash: make internal resets quiet and refresh index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-03-14T15:56:43Z","receivedAt":"2022-03-14T15:57:09Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Derrick Stolee wrote:\n> On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:\n>> From: Victoria Dye \n>>\n>> Add the options '-q' and '--refresh' to the 'git reset' executed in\n>> 'reset_head()', and '--refresh' to the 'git reset -q' executed in\n>> 'do_push_stash(...)'.\n> \n>> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\n>> index 5e68180f3b2..f2076545133 100755\n>> --- a/t/t7102-reset.sh\n>> +++ b/t/t7102-reset.sh\n>> @@ -482,7 +482,7 @@ test_index_refreshed () {\n>> \tgit rm --cached file2 &&\n>> \n>> \t# Step 2\n>> -\tgit reset $1 --mixed HEAD &&\n>> +\tgit reset $@ --mixed HEAD &&\n> \n> I see you change this from \"$1\" to \"$@\", which won't help with\n> the \"-c key=value\" issues from earlier.\n> \n>> \n>> \t# Step 3\n>> \tgit read-tree -m HEAD~1\n>> @@ -491,48 +491,66 @@ test_index_refreshed () {\n>> test_expect_success '--mixed refreshes the index' '\n>> \t# Verify default behavior (with no config settings or command line\n>> \t# options)\n>> -\ttest_index_refreshed &&\n>> +\ttest_index_refreshed\n> \n> Ah, I see you fixed this here, probably just a rebase issue, then.\n> \n\nThat's exactly what happened, and it will be fixed (and triple-checked) in\nV2. :)\n\n>> test_expect_success '--mixed --[no-]quiet sets default refresh behavior' '\n>> \t# Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh)\n>> \t# determine refresh behavior\n>> \n>> -\t# Config setting\n>> -\ttest_must_fail test_index_refreshed -c reset.quiet=true &&\n>> -\ttest_index_refreshed -c reset.quiet=true &&\n>> -\n> \n> Ah, and the -c changes are removed here. You could still test them\n> using the trick I mention in reply to patch 2.\n> \n>> \t# Command line option\n>> -\ttest_must_fail test_index_refreshed --quiet &&\n>> +\t! test_index_refreshed --quiet &&\n>> \ttest_index_refreshed --no-quiet &&\n>> \n>> -\t# Command line option overrides config setting\n>> -\ttest_must_fail test_index_refreshed -c reset.quiet=false --quiet &&\n>> -\ttest_index_refreshed -c reset.refresh=true --no-quiet\n>> +\t# Config: reset.quiet=false\n>> +\ttest_config reset.quiet false &&\n>> +\t(\n>> +\t\ttest_index_refreshed &&\n>> +\t\t! test_index_refreshed --quiet\n>> +\t) &&\n>> +\n>> +\t# Config: reset.quiet=true\n>> +\ttest_config reset.quiet true &&\n>> +\t(\n>> +\t\t! test_index_refreshed &&\n>> +\t\ttest_index_refreshed --no-quiet\n>> +\t)\n> \n> I'm not sure why you need sub-shells here. The test_config is\n> not scoped to the shell. These lines could be avoided with the\n> -c trick, which should make it a bit simpler to show what you\n> intend to be testing here.\n> \n\nThis was mostly organizational, but I'll remove them entirely in favor of\nthe line config option in V2.\n\n> Thanks,\n> -Stolee\n\n"},{"id":"451321","messageId":"pull.1170.v2.git.1647274230.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.git.1647043729.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-14T16:10:25Z","receivedAt":"2022-03-14T16:10:37Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"In the process of working on tests for 'git stash' sparse index integration,\nI found that the '--quiet' option in 'git stash' does not suppress all\nnon-error output when used with '--index'. Specifically, this comes from an\ninvocation of 'git reset' without the '--quiet' flag in 'reset_head()'. Upon\nenabling that flag, though, I discovered that 1) 'reset' does not refresh\nthe index if '--quiet' is specified (as of [1]) and 2) 'git stash' needs the\nindex to be refreshed after the reset.\n\nThis series aims to decouple the \"suppress logging\" and \"skip index refresh\"\nbehaviors in 'git reset --mixed', then allow 'stash' to internally use reset\nwith logs suppressed but index refresh enabled. This is accomplished by\nintroducing the '--[no-]refresh' option and 'reset.refresh' config setting\nto 'git reset'. Additionally, in the spirit of backward-compatibility,\n'--quiet' and/or 'reset.quiet=true' without any specified \"refresh\"\noption/config will continue to skip 'refresh_index(...)'.\n\nThere are also some minor updates to the advice that suggests skipping the\nindex refresh:\n\n * replace recommendation to use \"--quiet\" with \"--no-refresh\"\n * use 'advise()' rather than 'printf()'\n * rename the advice config setting from 'advice.resetQuiet' to to\n 'advice.resetNoRefresh'\n * suppress advice if '--quiet' is specified in 'reset'\n\nFinally, tests are added to 't7102-reset.sh' verifying whether index refresh\nhappens when expected and to 't3903-stash.sh' verifying that 'apply --quiet'\nno longer prints extraneous logs.\n\n\nChanges since V1\n================\n\n * Update cover letter title\n * Squash 't7102' test fixes from patch 5 into patch 2\n * Refactor 't7102' tests to properly use inline config settings\n\n[1] https://lore.kernel.org/git/20181023190423.5772-2-peartben@gmail.com/\n\nThanks! -Victoria\n\nVictoria Dye (5):\n reset: revise index refresh advice\n reset: introduce --[no-]refresh option to --mixed\n reset: replace '--quiet' with '--no-refresh' in performance advice\n reset: suppress '--no-refresh' advice if logging is silenced\n stash: make internal resets quiet and refresh index\n\n Documentation/config/advice.txt | 8 ++--\n Documentation/git-reset.txt | 9 ++++\n advice.c | 2 +-\n advice.h | 2 +-\n builtin/reset.c | 21 ++++++---\n builtin/stash.c | 5 ++-\n t/t3903-stash.sh | 12 +++++\n t/t7102-reset.sh | 77 ++++++++++++++++++++++++++++++---\n 8 files changed, 116 insertions(+), 20 deletions(-)\n\n\nbase-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1170%2Fvdye%2Fstash%2Fmake-reset-quiet-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1170/vdye/stash/make-reset-quiet-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1170\n\nRange-diff vs v1:\n\n 1: 7206ef8dd8a = 1: 7206ef8dd8a reset: revise index refresh advice\n 2: bda93703013 ! 2: 7f0226bc3e6 reset: introduce --[no-]refresh option to --mixed\n @@ Commit message\n Add a new --[no-]refresh option that is intended to explicitly determine\n whether a mixed reset should end in an index refresh.\n \n - A few years ago, [1] introduced behavior to the '--quiet' option to skip the\n - call to 'refresh_index(...)' at the end of a mixed reset with the goal of\n - improving performance. However, by coupling behavior that modifies the index\n - with the option that silences logs, there is no way for users to have one\n - without the other (i.e., silenced logs with a refreshed index) without\n + Starting at 9ac8125d1a (reset: don't compute unstaged changes after reset\n + when --quiet, 2018-10-23), using the '--quiet' option results in skipping\n + the call to 'refresh_index(...)' at the end of a mixed reset with the goal\n + of improving performance. However, by coupling behavior that modifies the\n + index with the option that silences logs, there is no way for users to have\n + one without the other (i.e., silenced logs with a refreshed index) without\n incurring the overhead of a separate call to 'git update-index --refresh'.\n Furthermore, there is minimal user-facing documentation indicating that\n --quiet skips the index refresh, potentially leading to unexpected issues\n @@ Commit message\n either is set, '--quiet' and 'reset.quiet' revert to controlling only\n whether logs are silenced and do not affect index refresh.\n \n - [1] 9ac8125d1a (reset: don't compute unstaged changes after reset when\n - --quiet, 2018-10-23)\n -\n + Helped-by: Derrick Stolee \n Signed-off-by: Victoria Dye \n \n ## Documentation/git-reset.txt ##\n @@ t/t7102-reset.sh: test_expect_success 'resetting an unmodified path is a no-op'\n +\tgit rm --cached file2 &&\n +\n +\t# Step 2\n -+\tgit reset $1 --mixed HEAD &&\n ++\tgit $1 reset $2 --mixed HEAD &&\n +\n +\t# Step 3\n +\tgit read-tree -m HEAD~1\n @@ t/t7102-reset.sh: test_expect_success 'resetting an unmodified path is a no-op'\n -\ttest_cmp expect output\n +\t# Verify default behavior (with no config settings or command line\n +\t# options)\n -+\ttest_index_refreshed &&\n ++\ttest_index_refreshed\n +'\n +test_expect_success '--mixed --[no-]quiet sets default refresh behavior' '\n +\t# Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh)\n +\t# determine refresh behavior\n +\n +\t# Config setting\n -+\ttest_must_fail test_index_refreshed -c reset.quiet=true &&\n -+\ttest_index_refreshed -c reset.quiet=true &&\n ++\t! test_index_refreshed \"-c reset.quiet=true\" &&\n ++\ttest_index_refreshed \"-c reset.quiet=false\" &&\n +\n +\t# Command line option\n -+\ttest_must_fail test_index_refreshed --quiet &&\n -+\ttest_index_refreshed --no-quiet &&\n ++\t! test_index_refreshed \"\" --quiet &&\n ++\ttest_index_refreshed \"\" --no-quiet &&\n +\n +\t# Command line option overrides config setting\n -+\ttest_must_fail test_index_refreshed -c reset.quiet=false --quiet &&\n -+\ttest_index_refreshed -c reset.refresh=true --no-quiet\n ++\t! test_index_refreshed \"-c reset.quiet=false\" --quiet &&\n ++\ttest_index_refreshed \"-c reset.refresh=true\" --no-quiet\n +'\n +\n +test_expect_success '--mixed --[no-]refresh sets refresh behavior' '\n +\t# Verify that --[no-]refresh and `reset.refresh` control index refresh\n +\n +\t# Config setting\n -+\ttest_index_refreshed -c reset.refresh=true &&\n -+\ttest_must_fail test_index_refreshed -c reset.refresh=false &&\n ++\ttest_index_refreshed \"-c reset.refresh=true\" &&\n ++\t! test_index_refreshed \"-c reset.refresh=false\" &&\n +\n +\t# Command line option\n -+\ttest_index_refreshed --refresh &&\n -+\ttest_must_fail test_index_refreshed --no-refresh &&\n ++\ttest_index_refreshed \"\" --refresh &&\n ++\t! test_index_refreshed \"\" --no-refresh &&\n +\n +\t# Command line option overrides config setting\n -+\ttest_index_refreshed -c reset.refresh=false --refresh &&\n -+\ttest_must_fail test_index_refreshed -c reset.refresh=true --no-refresh\n ++\ttest_index_refreshed \"-c reset.refresh=false\" --refresh &&\n ++\t! test_index_refreshed \"-c reset.refresh=true\" --no-refresh\n +'\n +\n +test_expect_success '--mixed --refresh overrides --quiet refresh behavior' '\n +\t# Verify that *both* --refresh and `reset.refresh` override the\n +\t# default non-refresh behavior of --quiet\n -+\ttest_index_refreshed --refresh --quiet &&\n -+\ttest_index_refreshed --refresh -c reset.quiet=true &&\n -+\ttest_index_refreshed -c reset.refresh=true --quiet &&\n -+\ttest_index_refreshed -c reset.refresh=true -c reset.quiet=true\n ++\ttest_index_refreshed \"\" \"--quiet --refresh\" &&\n ++\ttest_index_refreshed \"-c reset.quiet=true\" --refresh &&\n ++\ttest_index_refreshed \"-c reset.refresh=true\" --quiet &&\n ++\ttest_index_refreshed \"-c reset.refresh=true -c reset.quiet=true\"\n '\n \n test_expect_success '--mixed preserves skip-worktree' '\n 3: 63c5ee36feb = 3: f869723d64f reset: replace '--quiet' with '--no-refresh' in performance advice\n 4: 3c65a9f1993 = 4: cffca0ea5c6 reset: suppress '--no-refresh' advice if logging is silenced\n 5: 052499bbc93 ! 5: 3334d4cb6f3 stash: make internal resets quiet and refresh index\n @@ t/t3903-stash.sh: test_expect_success 'apply -q is quiet' '\n test_expect_success 'save -q is quiet' '\n \tgit stash save --quiet >output.out 2>&1 &&\n \ttest_must_be_empty output.out\n -\n - ## t/t7102-reset.sh ##\n -@@ t/t7102-reset.sh: test_index_refreshed () {\n - \tgit rm --cached file2 &&\n - \n - \t# Step 2\n --\tgit reset $1 --mixed HEAD &&\n -+\tgit reset $@ --mixed HEAD &&\n - \n - \t# Step 3\n - \tgit read-tree -m HEAD~1\n -@@ t/t7102-reset.sh: test_index_refreshed () {\n - test_expect_success '--mixed refreshes the index' '\n - \t# Verify default behavior (with no config settings or command line\n - \t# options)\n --\ttest_index_refreshed &&\n -+\ttest_index_refreshed\n - '\n - test_expect_success '--mixed --[no-]quiet sets default refresh behavior' '\n - \t# Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh)\n - \t# determine refresh behavior\n - \n --\t# Config setting\n --\ttest_must_fail test_index_refreshed -c reset.quiet=true &&\n --\ttest_index_refreshed -c reset.quiet=true &&\n --\n - \t# Command line option\n --\ttest_must_fail test_index_refreshed --quiet &&\n -+\t! test_index_refreshed --quiet &&\n - \ttest_index_refreshed --no-quiet &&\n - \n --\t# Command line option overrides config setting\n --\ttest_must_fail test_index_refreshed -c reset.quiet=false --quiet &&\n --\ttest_index_refreshed -c reset.refresh=true --no-quiet\n -+\t# Config: reset.quiet=false\n -+\ttest_config reset.quiet false &&\n -+\t(\n -+\t\ttest_index_refreshed &&\n -+\t\t! test_index_refreshed --quiet\n -+\t) &&\n -+\n -+\t# Config: reset.quiet=true\n -+\ttest_config reset.quiet true &&\n -+\t(\n -+\t\t! test_index_refreshed &&\n -+\t\ttest_index_refreshed --no-quiet\n -+\t)\n - '\n - \n - test_expect_success '--mixed --[no-]refresh sets refresh behavior' '\n - \t# Verify that --[no-]refresh and `reset.refresh` control index refresh\n - \n --\t# Config setting\n --\ttest_index_refreshed -c reset.refresh=true &&\n --\ttest_must_fail test_index_refreshed -c reset.refresh=false &&\n --\n - \t# Command line option\n - \ttest_index_refreshed --refresh &&\n --\ttest_must_fail test_index_refreshed --no-refresh &&\n -+\t! test_index_refreshed --no-refresh &&\n -+\n -+\t# Config: reset.refresh=false\n -+\ttest_config reset.refresh false &&\n -+\t(\n -+\t\t! test_index_refreshed &&\n -+\t\ttest_index_refreshed --refresh\n -+\t) &&\n - \n --\t# Command line option overrides config setting\n --\ttest_index_refreshed -c reset.refresh=false --refresh &&\n --\ttest_must_fail test_index_refreshed -c reset.refresh=true --no-refresh\n -+\t# Config: reset.refresh=true\n -+\ttest_config reset.refresh true &&\n -+\t(\n -+\t\ttest_index_refreshed &&\n -+\t\t! test_index_refreshed --no-refresh\n -+\t)\n - '\n - \n - test_expect_success '--mixed --refresh overrides --quiet refresh behavior' '\n - \t# Verify that *both* --refresh and `reset.refresh` override the\n - \t# default non-refresh behavior of --quiet\n -+\n - \ttest_index_refreshed --refresh --quiet &&\n --\ttest_index_refreshed --refresh -c reset.quiet=true &&\n --\ttest_index_refreshed -c reset.refresh=true --quiet &&\n --\ttest_index_refreshed -c reset.refresh=true -c reset.quiet=true\n -+\n -+\t# Config: reset.quiet=true\n -+\ttest_config reset.quiet true &&\n -+\ttest_index_refreshed --refresh &&\n -+\n -+\t# Config: reset.quiet=true, reset.refresh=true\n -+\ttest_config reset.refresh true &&\n -+\ttest_index_refreshed\n - '\n - \n - test_expect_success '--mixed preserves skip-worktree' '\n\n-- \ngitgitgadget\n"},{"id":"451322","messageId":"7f0226bc3e646167808fbd2413dc54e87417230c.1647274230.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.v2.git.1647274230.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] reset: introduce --[no-]refresh option to --mixed","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-14T16:10:27Z","receivedAt":"2022-03-14T16:10:40Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd a new --[no-]refresh option that is intended to explicitly determine\nwhether a mixed reset should end in an index refresh.\n\nStarting at 9ac8125d1a (reset: don't compute unstaged changes after reset\nwhen --quiet, 2018-10-23), using the '--quiet' option results in skipping\nthe call to 'refresh_index(...)' at the end of a mixed reset with the goal\nof improving performance. However, by coupling behavior that modifies the\nindex with the option that silences logs, there is no way for users to have\none without the other (i.e., silenced logs with a refreshed index) without\nincurring the overhead of a separate call to 'git update-index --refresh'.\nFurthermore, there is minimal user-facing documentation indicating that\n--quiet skips the index refresh, potentially leading to unexpected issues\nexecuting commands after 'git reset --quiet' that do not themselves refresh\nthe index (e.g., internals of 'git stash', 'git read-tree').\n\nTo mitigate these issues, '--[no-]refresh' and 'reset.refresh' are\nintroduced to provide a dedicated mechanism for refreshing the index. When\neither is set, '--quiet' and 'reset.quiet' revert to controlling only\nwhether logs are silenced and do not affect index refresh.\n\nHelped-by: Derrick Stolee \nSigned-off-by: Victoria Dye \n---\n Documentation/git-reset.txt | 9 +++++\n builtin/reset.c | 13 ++++++-\n t/t7102-reset.sh | 77 +++++++++++++++++++++++++++++++++----\n 3 files changed, 91 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt\nindex 6f7685f53d5..89ddc85c2e4 100644\n--- a/Documentation/git-reset.txt\n+++ b/Documentation/git-reset.txt\n@@ -110,6 +110,15 @@ OPTIONS\n \t`reset.quiet` config option. `--quiet` and `--no-quiet` will\n \toverride the default behavior.\n \n+--refresh::\n+--no-refresh::\n+\tProactively refresh the index after a mixed reset. If unspecified, the\n+\tbehavior falls back on the `reset.refresh` config option. If neither\n+\t`--[no-]refresh` nor `reset.refresh` are set, the default behavior is\n+\tdecided by the `--[no-]quiet` option and/or `reset.quiet` config.\n+\tIf `--quiet` is specified or `reset.quiet` is set with no command-line\n+\t\"quiet\" setting, refresh is disabled. Otherwise, refresh is enabled.\n+\n --pathspec-from-file=::\n \tPathspec is passed in `` instead of commandline args. If\n \t`` is exactly `-` then standard input is used. Pathspec\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex a420497a14f..7f667e13d71 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -392,6 +392,7 @@ static int git_reset_config(const char *var, const char *value, void *cb)\n int cmd_reset(int argc, const char **argv, const char *prefix)\n {\n \tint reset_type = NONE, update_ref_status = 0, quiet = 0;\n+\tint refresh = -1;\n \tint patch_mode = 0, pathspec_file_nul = 0, unborn;\n \tconst char *rev, *pathspec_from_file = NULL;\n \tstruct object_id oid;\n@@ -399,6 +400,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tint intent_to_add = 0;\n \tconst struct option options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n+\t\tOPT_BOOL(0, \"refresh\", &refresh,\n+\t\t\t\tN_(\"skip refreshing the index after reset\")),\n \t\tOPT_SET_INT(0, \"mixed\", &reset_type,\n \t\t\t\t\t\tN_(\"reset HEAD and index\"), MIXED),\n \t\tOPT_SET_INT(0, \"soft\", &reset_type, N_(\"reset only HEAD\"), SOFT),\n@@ -421,11 +424,19 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_reset_config, NULL);\n \tgit_config_get_bool(\"reset.quiet\", &quiet);\n+\tgit_config_get_bool(\"reset.refresh\", &refresh);\n \n \targc = parse_options(argc, argv, prefix, options, git_reset_usage,\n \t\t\t\t\t\tPARSE_OPT_KEEP_DASHDASH);\n \tparse_args(&pathspec, argv, prefix, patch_mode, &rev);\n \n+\t/*\n+\t * If refresh is completely unspecified (either by config or by command\n+\t * line option), decide based on 'quiet'.\n+\t */\n+\tif (refresh < 0)\n+\t\trefresh = !quiet;\n+\n \tif (pathspec_from_file) {\n \t\tif (patch_mode)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--pathspec-from-file\", \"--patch\");\n@@ -517,7 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\tif (read_from_tree(&pathspec, &oid, intent_to_add))\n \t\t\t\treturn 1;\n \t\t\tthe_index.updated_skipworktree = 1;\n-\t\t\tif (!quiet && get_git_work_tree()) {\n+\t\t\tif (refresh && get_git_work_tree()) {\n \t\t\t\tuint64_t t_begin, t_delta_in_ms;\n \n \t\t\t\tt_begin = getnanotime();\ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex d05426062ec..005940778b7 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -462,14 +462,77 @@ test_expect_success 'resetting an unmodified path is a no-op' '\n \tgit diff-index --cached --exit-code HEAD\n '\n \n+test_index_refreshed () {\n+\n+\t# To test whether the index is refresh, create a scenario where a\n+\t# command will fail if the index is *not* refreshed:\n+\t# 1. update the worktree to match HEAD & remove file2 in the index\n+\t# 2. reset --mixed to unstage the change from step 1\n+\t# 3. read-tree HEAD~1 (which differs from HEAD in file2)\n+\t# If the index is refreshed in step 2, then file2 in the index will be\n+\t# up-to-date with HEAD and read-tree will succeed (thus failing the\n+\t# test). If the index is *not* refreshed, however, the staged deletion\n+\t# of file2 from step 1 will conflict with the changes from the tree read\n+\t# in step 3, resulting in a failure.\n+\n+\t# Step 0: start with a clean index\n+\tgit reset --hard HEAD &&\n+\n+\t# Step 1\n+\tgit rm --cached file2 &&\n+\n+\t# Step 2\n+\tgit $1 reset $2 --mixed HEAD &&\n+\n+\t# Step 3\n+\tgit read-tree -m HEAD~1\n+}\n+\n test_expect_success '--mixed refreshes the index' '\n-\tcat >expect <<-\\EOF &&\n-\tUnstaged changes after reset:\n-\tM\tfile2\n-\tEOF\n-\techo 123 >>file2 &&\n-\tgit reset --mixed HEAD >output &&\n-\ttest_cmp expect output\n+\t# Verify default behavior (with no config settings or command line\n+\t# options)\n+\ttest_index_refreshed\n+'\n+test_expect_success '--mixed --[no-]quiet sets default refresh behavior' '\n+\t# Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh)\n+\t# determine refresh behavior\n+\n+\t# Config setting\n+\t! test_index_refreshed \"-c reset.quiet=true\" &&\n+\ttest_index_refreshed \"-c reset.quiet=false\" &&\n+\n+\t# Command line option\n+\t! test_index_refreshed \"\" --quiet &&\n+\ttest_index_refreshed \"\" --no-quiet &&\n+\n+\t# Command line option overrides config setting\n+\t! test_index_refreshed \"-c reset.quiet=false\" --quiet &&\n+\ttest_index_refreshed \"-c reset.refresh=true\" --no-quiet\n+'\n+\n+test_expect_success '--mixed --[no-]refresh sets refresh behavior' '\n+\t# Verify that --[no-]refresh and `reset.refresh` control index refresh\n+\n+\t# Config setting\n+\ttest_index_refreshed \"-c reset.refresh=true\" &&\n+\t! test_index_refreshed \"-c reset.refresh=false\" &&\n+\n+\t# Command line option\n+\ttest_index_refreshed \"\" --refresh &&\n+\t! test_index_refreshed \"\" --no-refresh &&\n+\n+\t# Command line option overrides config setting\n+\ttest_index_refreshed \"-c reset.refresh=false\" --refresh &&\n+\t! test_index_refreshed \"-c reset.refresh=true\" --no-refresh\n+'\n+\n+test_expect_success '--mixed --refresh overrides --quiet refresh behavior' '\n+\t# Verify that *both* --refresh and `reset.refresh` override the\n+\t# default non-refresh behavior of --quiet\n+\ttest_index_refreshed \"\" \"--quiet --refresh\" &&\n+\ttest_index_refreshed \"-c reset.quiet=true\" --refresh &&\n+\ttest_index_refreshed \"-c reset.refresh=true\" --quiet &&\n+\ttest_index_refreshed \"-c reset.refresh=true -c reset.quiet=true\"\n '\n \n test_expect_success '--mixed preserves skip-worktree' '\n-- \ngitgitgadget\n\n"},{"id":"451323","messageId":"7206ef8dd8ab088feab841c5574d486b6f19af5e.1647274230.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.v2.git.1647274230.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] reset: revise index refresh advice","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-14T16:10:26Z","receivedAt":"2022-03-14T16:10:43Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nUpdate the advice describing index refresh from \"enumerate unstaged changes\"\nto \"refresh the index.\" Describing 'refresh_index(...)' as \"enumerating\nunstaged changes\" is not fully representative of what an index refresh is\ndoing; more generally, it updates the properties of index entries that are\naffected by outside-of-index state, e.g. CE_UPTODATE, which is affected by\nthe file contents on-disk. This distinction is relevant to operations that\nread the index but do not refresh first - e.g., 'git read-tree' - where a\nstale index may cause incorrect behavior.\n\nIn addition to changing the advice message, use the \"advise\" function to\nprint advice.\n\nSigned-off-by: Victoria Dye \n---\n Documentation/config/advice.txt | 4 ++--\n builtin/reset.c | 4 ++--\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex c40eb09cb7e..971aad2f237 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -69,8 +69,8 @@ advice.*::\n \t\tmerge to avoid overwriting local changes.\n \tresetQuiet::\n \t\tAdvice to consider using the `--quiet` option to linkgit:git-reset[1]\n-\t\twhen the command takes more than 2 seconds to enumerate unstaged\n-\t\tchanges after reset.\n+\t\twhen the command takes more than 2 seconds to refresh the index\n+\t\tafter reset.\n \tresolveConflict::\n \t\tAdvice shown by various commands when conflicts\n \t\tprevent the operation from being performed.\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 6e65e90c5db..a420497a14f 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -525,9 +525,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t _(\"Unstaged changes after reset:\"));\n \t\t\t\tt_delta_in_ms = (getnanotime() - t_begin) / 1000000;\n \t\t\t\tif (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n-\t\t\t\t\tprintf(_(\"\\nIt took %.2f seconds to enumerate unstaged changes after reset. You can\\n\"\n+\t\t\t\t\tadvise(_(\"It took %.2f seconds to refresh the index after reset. You can\\n\"\n \t\t\t\t\t\t\"use '--quiet' to avoid this. Set the config setting reset.quiet to true\\n\"\n-\t\t\t\t\t\t\"to make this the default.\\n\"), t_delta_in_ms / 1000.0);\n+\t\t\t\t\t\t\"to make this the default.\"), t_delta_in_ms / 1000.0);\n \t\t\t\t}\n \t\t\t}\n \t\t} else {\n-- \ngitgitgadget\n\n"},{"id":"451324","messageId":"cffca0ea5c6b09fa1a0725f81440b6358f79d303.1647274230.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.v2.git.1647274230.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] reset: suppress '--no-refresh' advice if logging is silenced","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-14T16:10:29Z","receivedAt":"2022-03-14T16:10:49Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nIf using '--quiet' or 'reset.quiet=true', do not print the 'resetnoRefresh'\nadvice string. For applications that rely on '--quiet' disabling all\nnon-error logs, the advice message should be suppressed accordingly.\n\nSigned-off-by: Victoria Dye \n---\n builtin/reset.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex feab85e03de..c8a356ec5b0 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -535,7 +535,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\trefresh_index(&the_index, flags, NULL, NULL,\n \t\t\t\t\t _(\"Unstaged changes after reset:\"));\n \t\t\t\tt_delta_in_ms = (getnanotime() - t_begin) / 1000000;\n-\t\t\t\tif (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n+\t\t\t\tif (!quiet && advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n \t\t\t\t\tadvise(_(\"It took %.2f seconds to refresh the index after reset. You can use\\n\"\n \t\t\t\t\t\t \"'--no-refresh' to avoid this. Set the config setting reset.refresh to false\\n\"\n \t\t\t\t\t\t \"to make this the default.\"), t_delta_in_ms / 1000.0);\n-- \ngitgitgadget\n\n"},{"id":"451325","messageId":"f869723d64f929736fc604830de1eeaf822a6f37.1647274230.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.v2.git.1647274230.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-14T16:10:28Z","receivedAt":"2022-03-14T16:10:52Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nReplace references to '--quiet' with '--no-refresh' in the advice on how to\nskip refreshing the index. When the advice was introduced, '--quiet' was the\nonly way to avoid the expensive 'refresh_index(...)' at the end of a mixed\nreset. After introducing '--no-refresh', however, '--quiet' became only a\nfallback option for determining refresh behavior, overridden by\n'--[no-]refresh' or 'reset.refresh' if either is set. To ensure users are\nadvised to use the most reliable option for avoiding 'refresh_index(...)',\nreplace recommendation of '--quiet' with '--[no-]refresh'.\n\nSigned-off-by: Victoria Dye \n---\n Documentation/config/advice.txt | 8 ++++----\n advice.c | 2 +-\n advice.h | 2 +-\n builtin/reset.c | 8 ++++----\n 4 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex 971aad2f237..83c2a956611 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -67,10 +67,10 @@ advice.*::\n \tcommitBeforeMerge::\n \t\tAdvice shown when linkgit:git-merge[1] refuses to\n \t\tmerge to avoid overwriting local changes.\n-\tresetQuiet::\n-\t\tAdvice to consider using the `--quiet` option to linkgit:git-reset[1]\n-\t\twhen the command takes more than 2 seconds to refresh the index\n-\t\tafter reset.\n+\tresetNoRefresh::\n+\t\tAdvice to consider using the `--no-refresh` option to\n+\t\tlinkgit:git-reset[1] when the command takes more than 2 seconds\n+\t\tto refresh the index after reset.\n \tresolveConflict::\n \t\tAdvice shown by various commands when conflicts\n \t\tprevent the operation from being performed.\ndiff --git a/advice.c b/advice.c\nindex 2e1fd483040..cb755c551a2 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -61,7 +61,7 @@ static struct {\n \t[ADVICE_PUSH_NON_FF_MATCHING]\t\t\t= { \"pushNonFFMatching\", 1 },\n \t[ADVICE_PUSH_UNQUALIFIED_REF_NAME]\t\t= { \"pushUnqualifiedRefName\", 1 },\n \t[ADVICE_PUSH_UPDATE_REJECTED]\t\t\t= { \"pushUpdateRejected\", 1 },\n-\t[ADVICE_RESET_QUIET_WARNING]\t\t\t= { \"resetQuiet\", 1 },\n+\t[ADVICE_RESET_NO_REFRESH_WARNING]\t\t= { \"resetNoRefresh\", 1 },\n \t[ADVICE_RESOLVE_CONFLICT]\t\t\t= { \"resolveConflict\", 1 },\n \t[ADVICE_RM_HINTS]\t\t\t\t= { \"rmHints\", 1 },\n \t[ADVICE_SEQUENCER_IN_USE]\t\t\t= { \"sequencerInUse\", 1 },\ndiff --git a/advice.h b/advice.h\nindex a3957123a16..f95af70ced4 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -36,7 +36,7 @@ struct string_list;\n \tADVICE_PUSH_UPDATE_REJECTED_ALIAS,\n \tADVICE_PUSH_UPDATE_REJECTED,\n \tADVICE_PUSH_REF_NEEDS_UPDATE,\n-\tADVICE_RESET_QUIET_WARNING,\n+\tADVICE_RESET_NO_REFRESH_WARNING,\n \tADVICE_RESOLVE_CONFLICT,\n \tADVICE_RM_HINTS,\n \tADVICE_SEQUENCER_IN_USE,\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 7f667e13d71..feab85e03de 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -535,10 +535,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\trefresh_index(&the_index, flags, NULL, NULL,\n \t\t\t\t\t _(\"Unstaged changes after reset:\"));\n \t\t\t\tt_delta_in_ms = (getnanotime() - t_begin) / 1000000;\n-\t\t\t\tif (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n-\t\t\t\t\tadvise(_(\"It took %.2f seconds to refresh the index after reset. You can\\n\"\n-\t\t\t\t\t\t\"use '--quiet' to avoid this. Set the config setting reset.quiet to true\\n\"\n-\t\t\t\t\t\t\"to make this the default.\"), t_delta_in_ms / 1000.0);\n+\t\t\t\tif (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n+\t\t\t\t\tadvise(_(\"It took %.2f seconds to refresh the index after reset. You can use\\n\"\n+\t\t\t\t\t\t \"'--no-refresh' to avoid this. Set the config setting reset.refresh to false\\n\"\n+\t\t\t\t\t\t \"to make this the default.\"), t_delta_in_ms / 1000.0);\n \t\t\t\t}\n \t\t\t}\n \t\t} else {\n-- \ngitgitgadget\n\n"},{"id":"451326","messageId":"3334d4cb6f302a35986d94ea8ffcd1ee9c6aae5d.1647274230.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.v2.git.1647274230.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] stash: make internal resets quiet and refresh index","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-14T16:10:30Z","receivedAt":"2022-03-14T16:10:54Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd the options '-q' and '--refresh' to the 'git reset' executed in\n'reset_head()', and '--refresh' to the 'git reset -q' executed in\n'do_push_stash(...)'.\n\n'stash' is implemented such that git commands invoked as part of it (e.g.,\n'clean', 'read-tree', 'reset', etc.) have their informational output\nsilenced. However, the 'reset' in 'reset_head()' is *not* called with '-q',\nleading to the potential for a misleading printout from 'git stash apply\n--index' if the stash included a removed file:\n\nUnstaged changes after reset: D \n\nNot only is this confusing in its own right (since, after the reset, 'git\nstash' execution would stage the deletion in the index), it would be printed\neven when the stash was applied with the '-q' option. As a result, the\nmessaging is removed entirely by calling 'git status' with '-q'.\n\nAdditionally, because the default behavior of 'git reset -q' is to skip\nrefreshing the index, but later operations in 'git stash' subcommands expect\na non-stale index, enable '--refresh' as well.\n\nSigned-off-by: Victoria Dye \n---\n builtin/stash.c | 5 +++--\n t/t3903-stash.sh | 12 ++++++++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 3e8af210fde..91407d9bbe0 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -310,7 +310,7 @@ static int reset_head(void)\n \t * API for resetting.\n \t */\n \tcp.git_cmd = 1;\n-\tstrvec_push(&cp.args, \"reset\");\n+\tstrvec_pushl(&cp.args, \"reset\", \"--quiet\", \"--refresh\", NULL);\n \n \treturn run_command(&cp);\n }\n@@ -1633,7 +1633,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \n \t\t\tcp.git_cmd = 1;\n-\t\t\tstrvec_pushl(&cp.args, \"reset\", \"-q\", \"--\", NULL);\n+\t\t\tstrvec_pushl(&cp.args, \"reset\", \"-q\", \"--refresh\", \"--\",\n+\t\t\t\t NULL);\n \t\t\tadd_pathspecs(&cp.args, ps);\n \t\t\tif (run_command(&cp)) {\n \t\t\t\tret = -1;\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex f36e121210e..17f2ad2344c 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -261,6 +261,18 @@ test_expect_success 'apply -q is quiet' '\n \ttest_must_be_empty output.out\n '\n \n+test_expect_success 'apply --index -q is quiet' '\n+\t# Added file, deleted file, modified file all staged for commit\n+\techo foo >new-file &&\n+\techo test >file &&\n+\tgit add new-file file &&\n+\tgit rm other-file &&\n+\n+\tgit stash &&\n+\tgit stash apply --index -q >output.out 2>&1 &&\n+\ttest_must_be_empty output.out\n+'\n+\n test_expect_success 'save -q is quiet' '\n \tgit stash save --quiet >output.out 2>&1 &&\n \ttest_must_be_empty output.out\n-- \ngitgitgadget\n"},{"id":"451330","messageId":"c041cf9d-729d-73bd-1ca8-e15c570eb031@github.com","threadId":"57545","inReplyTo":"pull.1170.v2.git.1647274230.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-14T16:30:16Z","receivedAt":"2022-03-14T16:30:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/14/2022 12:10 PM, Victoria Dye via GitGitGadget wrote:\n \n> Changes since V1\n> ================\n> \n> * Update cover letter title\n> * Squash 't7102' test fixes from patch 5 into patch 2\n> * Refactor 't7102' tests to properly use inline config settings\n\nThanks. This version answers all of my concerns and looks ready\nto go from my perspective.\n\nThanks,\n-Stolee\n"},{"id":"451334","messageId":"xmqq7d8ws5bo.fsf@gitster.g","threadId":"57545","inReplyTo":"7f0226bc3e646167808fbd2413dc54e87417230c.1647274230.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/5] reset: introduce --[no-]refresh option to --mixed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-14T19:27:07Z","receivedAt":"2022-03-14T19:27:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> From: Victoria Dye \n>\n> Add a new --[no-]refresh option that is intended to explicitly determine\n> whether a mixed reset should end in an index refresh.\n>\n> Starting at 9ac8125d1a (reset: don't compute unstaged changes after reset\n> when --quiet, 2018-10-23), using the '--quiet' option results in skipping\n> the call to 'refresh_index(...)' at the end of a mixed reset with the goal\n> of improving performance. However, by coupling behavior that modifies the\n> index with the option that silences logs, there is no way for users to have\n> one without the other (i.e., silenced logs with a refreshed index) without\n> incurring the overhead of a separate call to 'git update-index --refresh'.\n> Furthermore, there is minimal user-facing documentation indicating that\n> --quiet skips the index refresh, potentially leading to unexpected issues\n> executing commands after 'git reset --quiet' that do not themselves refresh\n> the index (e.g., internals of 'git stash', 'git read-tree').\n>\n> To mitigate these issues, '--[no-]refresh' and 'reset.refresh' are\n> introduced to provide a dedicated mechanism for refreshing the index. When\n> either is set, '--quiet' and 'reset.quiet' revert to controlling only\n> whether logs are silenced and do not affect index refresh.\n>\n> Helped-by: Derrick Stolee \n> Signed-off-by: Victoria Dye \n> ---\n> Documentation/git-reset.txt | 9 +++++\n> builtin/reset.c | 13 ++++++-\n> t/t7102-reset.sh | 77 +++++++++++++++++++++++++++++++++----\n> 3 files changed, 91 insertions(+), 8 deletions(-)\n\nNo complaints, but it is somewhat unsatisfying that we need these\ntwo steps that keep --quiet tied to the decision to or not to\nrefresh. In the longer term, it may be cleaner to completely\ndissociate them, but it probably is not a huge deal.\n\n> +\t/*\n> +\t * If refresh is completely unspecified (either by config or by command\n> +\t * line option), decide based on 'quiet'.\n> +\t */\n> +\tif (refresh < 0)\n> +\t\trefresh = !quiet;\n\nOK.\n\n> @@ -517,7 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n> \t\t\tif (read_from_tree(&pathspec, &oid, intent_to_add))\n> \t\t\t\treturn 1;\n> \t\t\tthe_index.updated_skipworktree = 1;\n> -\t\t\tif (!quiet && get_git_work_tree()) {\n> +\t\t\tif (refresh && get_git_work_tree()) {\n> \t\t\t\tuint64_t t_begin, t_delta_in_ms;\n> \n> \t\t\t\tt_begin = getnanotime();\n\nQuite sensible.\n\n> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\n> index d05426062ec..005940778b7 100755\n> --- a/t/t7102-reset.sh\n> +++ b/t/t7102-reset.sh\n> @@ -462,14 +462,77 @@ test_expect_success 'resetting an unmodified path is a no-op' '\n> \tgit diff-index --cached --exit-code HEAD\n> '\n> \n> +test_index_refreshed () {\n> +\n> +\t# To test whether the index is refresh, create a scenario where a\n\nDoesn't the verb \"refresh\" refer to the act of making it \"fresh\"\n(again)? i.e. update the cached stat info to up-to-date?\n\n\"To test whether the index has been refreshed\" or \"To test whether\nthe cached stat info is up-to-date\", perhaps?\n\n> +\t# command will fail if the index is *not* refreshed:\n> +\t# 1. update the worktree to match HEAD & remove file2 in the index\n\nIn other words, file2 tentatively becomes untracked.\n\n> +\t# 2. reset --mixed to unstage the change from step 1\n\nBut then, file2 is \"added\" to the index again, but added from the\nHEAD. If this did not refresh, then we do not know if the contents\nof the file in the working tree is the same, and \"diff-files\" may\nsay \"file2 may be modified\". If \"reset\" refreshes, this will take\nus back to the same state as \"reset --hard HEAD\", and \"diff-files\"\nwill not report that \"file2\" is different.\n\n> +\t# 3. read-tree HEAD~1 (which differs from HEAD in file2)\n\nWith \"-m\" option, I presume? Do we want \"-u\" here, too?\n\n> +\t# If the index is refreshed in step 2, then file2 in the index will be\n> +\t# up-to-date with HEAD and read-tree will succeed (thus failing the\n> +\t# test). If the index is *not* refreshed, however, the staged deletion\n> +\t# of file2 from step 1 will conflict with the changes from the tree read\n> +\t# in step 3, resulting in a failure.\n\nThis feels a bit brittle. The implementation of \"read-tree -m\" may\nchoose to refresh beforehand to avoid such a failure.\n\nIn any case, the name of the helper alone wasn't of any help to\nrealize that this is about checking if \"reset\" refreshes the index\nor not. Perhaps call it more like\n\n\treset_refreshes_index\n\nor something?\n\nIn any case, instead of the big comment block, comments interspersed\nin the steps may be easier to follow. \n\n> +\t# Step 0: start with a clean index\n> +\tgit reset --hard HEAD &&\n> +\n> +\t# Step 1\n\t# remove file2 from the index\n> +\tgit rm --cached file2 &&\n> +\n> +\t# Step 2\n\t# resurrect file2 to the index from HEAD; if the cached stat\n\t# info gets refreshed, this brings us back to the state\n # after Step 0. If not, \"diff-files\" would report file2 is\n\t# different.\n> +\tgit $1 reset $2 --mixed HEAD &&\n> +\n> +\t# Step 3\n> +\tgit read-tree -m HEAD~1\n\nAnd use \"diff-files file2\" here? Then you do not even have to rely\non HEAD and HEAD~1 being different at file2.\n\n"},{"id":"451335","messageId":"xmqqy21cqq8c.fsf@gitster.g","threadId":"57545","inReplyTo":"cffca0ea5c6b09fa1a0725f81440b6358f79d303.1647274230.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/5] reset: suppress '--no-refresh' advice if logging is silenced","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-14T19:38:27Z","receivedAt":"2022-03-14T19:38:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> From: Victoria Dye \n>\n> If using '--quiet' or 'reset.quiet=true', do not print the 'resetnoRefresh'\n> advice string. For applications that rely on '--quiet' disabling all\n> non-error logs, the advice message should be suppressed accordingly.\n\nFunny, but it is true that we would want to squelch advice messages\nunder 'quiet' mode.\n\nBut why did we even spend 2 seconds to refresh the index if --quiet\nis given in the first place? Isn't it because an explicit --refresh\nor reset.refresh=yes was in effect? IOW, the user wanted the\ncommand to be quiet but still wanted it to refresh the index.\n\nSo it would be yet another reason why we do not want to show the\nmessage: the user knows what they are doing and explicitly asked\nus to spend cycles to refresh.\n\n\n> Signed-off-by: Victoria Dye \n> ---\n> builtin/reset.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index feab85e03de..c8a356ec5b0 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -535,7 +535,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n> \t\t\t\trefresh_index(&the_index, flags, NULL, NULL,\n> \t\t\t\t\t _(\"Unstaged changes after reset:\"));\n> \t\t\t\tt_delta_in_ms = (getnanotime() - t_begin) / 1000000;\n> -\t\t\t\tif (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n> +\t\t\t\tif (!quiet && advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n> \t\t\t\t\tadvise(_(\"It took %.2f seconds to refresh the index after reset. You can use\\n\"\n> \t\t\t\t\t\t \"'--no-refresh' to avoid this. Set the config setting reset.refresh to false\\n\"\n> \t\t\t\t\t\t \"to make this the default.\"), t_delta_in_ms / 1000.0);\n"},{"id":"451336","messageId":"xmqqtuc0qq0w.fsf@gitster.g","threadId":"57545","inReplyTo":"3334d4cb6f302a35986d94ea8ffcd1ee9c6aae5d.1647274230.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/5] stash: make internal resets quiet and refresh index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-14T19:42:55Z","receivedAt":"2022-03-14T19:43:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> From: Victoria Dye \n>\n> Add the options '-q' and '--refresh' to the 'git reset' executed in\n> 'reset_head()', and '--refresh' to the 'git reset -q' executed in\n> 'do_push_stash(...)'.\n>\n> 'stash' is implemented such that git commands invoked as part of it (e.g.,\n> 'clean', 'read-tree', 'reset', etc.) have their informational output\n> silenced. However, the 'reset' in 'reset_head()' is *not* called with '-q',\n> leading to the potential for a misleading printout from 'git stash apply\n> --index' if the stash included a removed file:\n>\n> Unstaged changes after reset: D \n>\n> Not only is this confusing in its own right (since, after the reset, 'git\n> stash' execution would stage the deletion in the index), it would be printed\n> even when the stash was applied with the '-q' option. As a result, the\n> messaging is removed entirely by calling 'git status' with '-q'.\n>\n> Additionally, because the default behavior of 'git reset -q' is to skip\n> refreshing the index, but later operations in 'git stash' subcommands expect\n> a non-stale index, enable '--refresh' as well.\n>\n> Signed-off-by: Victoria Dye \n> ---\n> builtin/stash.c | 5 +++--\n> t/t3903-stash.sh | 12 ++++++++++++\n> 2 files changed, 15 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 3e8af210fde..91407d9bbe0 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -310,7 +310,7 @@ static int reset_head(void)\n> \t * API for resetting.\n> \t */\n> \tcp.git_cmd = 1;\n> -\tstrvec_push(&cp.args, \"reset\");\n> +\tstrvec_pushl(&cp.args, \"reset\", \"--quiet\", \"--refresh\", NULL);\n> \n> \treturn run_command(&cp);\n> }\n> @@ -1633,7 +1633,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n> \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> \n> \t\t\tcp.git_cmd = 1;\n> -\t\t\tstrvec_pushl(&cp.args, \"reset\", \"-q\", \"--\", NULL);\n> +\t\t\tstrvec_pushl(&cp.args, \"reset\", \"-q\", \"--refresh\", \"--\",\n> +\t\t\t\t NULL);\n> \t\t\tadd_pathspecs(&cp.args, ps);\n> \t\t\tif (run_command(&cp)) {\n> \t\t\t\tret = -1;\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index f36e121210e..17f2ad2344c 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -261,6 +261,18 @@ test_expect_success 'apply -q is quiet' '\n> \ttest_must_be_empty output.out\n> '\n> \n> +test_expect_success 'apply --index -q is quiet' '\n> +\t# Added file, deleted file, modified file all staged for commit\n> +\techo foo >new-file &&\n> +\techo test >file &&\n> +\tgit add new-file file &&\n> +\tgit rm other-file &&\n> +\n> +\tgit stash &&\n\nHpmh. The hunk that updates reset_head() does get exercised by\n\"apply --index\", so testing that is OK, but we are also patching\n\"do_push_stash()\" to be quiet, so don't we care the chattyness of\nthis step, too?\n\nIn these steps, we also want the same \"did the command refresh the\nindex?\" tests, no?\n\n> +\tgit stash apply --index -q >output.out 2>&1 &&\n> +\ttest_must_be_empty output.out\n> +'\n> +\n> test_expect_success 'save -q is quiet' '\n> \tgit stash save --quiet >output.out 2>&1 &&\n> \ttest_must_be_empty output.out\n\nOther than these nits I noticed, the overall idea of the topic is\nwell presented. Thanks for working on this.\n\n"},{"id":"451357","messageId":"xmqqlexcjf8f.fsf@gitster.g","threadId":"57545","inReplyTo":"pull.1170.v2.git.1647274230.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-14T23:17:52Z","receivedAt":"2022-03-14T23:18:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> In the process of working on tests for 'git stash' sparse index integration,\n> I found that the '--quiet' option in 'git stash' does not suppress all\n> non-error output when used with '--index'. Specifically, this comes from an\n> invocation of 'git reset' without the '--quiet' flag in 'reset_head()'. Upon\n> enabling that flag, though, I discovered that 1) 'reset' does not refresh\n> the index if '--quiet' is specified (as of [1]) and 2) 'git stash' needs the\n> index to be refreshed after the reset.\n>\n> This series aims to decouple the \"suppress logging\" and \"skip index refresh\"\n> behaviors in 'git reset --mixed', then allow 'stash' to internally use reset\n> with logs suppressed but index refresh enabled. This is accomplished by\n> introducing the '--[no-]refresh' option and 'reset.refresh' config setting\n> to 'git reset'. Additionally, in the spirit of backward-compatibility,\n> '--quiet' and/or 'reset.quiet=true' without any specified \"refresh\"\n> option/config will continue to skip 'refresh_index(...)'.\n>\n> There are also some minor updates to the advice that suggests skipping the\n> index refresh:\n>\n> * replace recommendation to use \"--quiet\" with \"--no-refresh\"\n> * use 'advise()' rather than 'printf()'\n> * rename the advice config setting from 'advice.resetQuiet' to to\n> 'advice.resetNoRefresh'\n> * suppress advice if '--quiet' is specified in 'reset'\n>\n> Finally, tests are added to 't7102-reset.sh' verifying whether index refresh\n> happens when expected and to 't3903-stash.sh' verifying that 'apply --quiet'\n> no longer prints extraneous logs.\n\nI've read this iteration, too, and except for \"we should test not\njust 'apply -index' but 'push'\" in [5/5], everything looked good.\n\nThanks.\n"},{"id":"451359","messageId":"ce5904f1-ee15-1be8-4019-d1cd10ac83f7@github.com","threadId":"57545","inReplyTo":"xmqq7d8ws5bo.fsf@gitster.g","subject":"Re: [PATCH v2 2/5] reset: introduce --[no-]refresh option to --mixed","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-03-14T23:48:54Z","receivedAt":"2022-03-14T23:49:00Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> \"Victoria Dye via GitGitGadget\" writes:\n> \n>> From: Victoria Dye \n>>\n>> Add a new --[no-]refresh option that is intended to explicitly determine\n>> whether a mixed reset should end in an index refresh.\n>>\n>> Starting at 9ac8125d1a (reset: don't compute unstaged changes after reset\n>> when --quiet, 2018-10-23), using the '--quiet' option results in skipping\n>> the call to 'refresh_index(...)' at the end of a mixed reset with the goal\n>> of improving performance. However, by coupling behavior that modifies the\n>> index with the option that silences logs, there is no way for users to have\n>> one without the other (i.e., silenced logs with a refreshed index) without\n>> incurring the overhead of a separate call to 'git update-index --refresh'.\n>> Furthermore, there is minimal user-facing documentation indicating that\n>> --quiet skips the index refresh, potentially leading to unexpected issues\n>> executing commands after 'git reset --quiet' that do not themselves refresh\n>> the index (e.g., internals of 'git stash', 'git read-tree').\n>>\n>> To mitigate these issues, '--[no-]refresh' and 'reset.refresh' are\n>> introduced to provide a dedicated mechanism for refreshing the index. When\n>> either is set, '--quiet' and 'reset.quiet' revert to controlling only\n>> whether logs are silenced and do not affect index refresh.\n>>\n>> Helped-by: Derrick Stolee \n>> Signed-off-by: Victoria Dye \n>> ---\n>> Documentation/git-reset.txt | 9 +++++\n>> builtin/reset.c | 13 ++++++-\n>> t/t7102-reset.sh | 77 +++++++++++++++++++++++++++++++++----\n>> 3 files changed, 91 insertions(+), 8 deletions(-)\n> \n> No complaints, but it is somewhat unsatisfying that we need these\n> two steps that keep --quiet tied to the decision to or not to\n> refresh. In the longer term, it may be cleaner to completely\n> dissociate them, but it probably is not a huge deal.\n> \n>> +\t/*\n>> +\t * If refresh is completely unspecified (either by config or by command\n>> +\t * line option), decide based on 'quiet'.\n>> +\t */\n>> +\tif (refresh < 0)\n>> +\t\trefresh = !quiet;\n> \n> OK.\n> \n>> @@ -517,7 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n>> \t\t\tif (read_from_tree(&pathspec, &oid, intent_to_add))\n>> \t\t\t\treturn 1;\n>> \t\t\tthe_index.updated_skipworktree = 1;\n>> -\t\t\tif (!quiet && get_git_work_tree()) {\n>> +\t\t\tif (refresh && get_git_work_tree()) {\n>> \t\t\t\tuint64_t t_begin, t_delta_in_ms;\n>> \n>> \t\t\t\tt_begin = getnanotime();\n> \n> Quite sensible.\n> \n>> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\n>> index d05426062ec..005940778b7 100755\n>> --- a/t/t7102-reset.sh\n>> +++ b/t/t7102-reset.sh\n>> @@ -462,14 +462,77 @@ test_expect_success 'resetting an unmodified path is a no-op' '\n>> \tgit diff-index --cached --exit-code HEAD\n>> '\n>> \n>> +test_index_refreshed () {\n>> +\n>> +\t# To test whether the index is refresh, create a scenario where a\n> \n> Doesn't the verb \"refresh\" refer to the act of making it \"fresh\"\n> (again)? i.e. update the cached stat info to up-to-date?\n> \n> \"To test whether the index has been refreshed\" or \"To test whether\n> the cached stat info is up-to-date\", perhaps?\n> \n>> +\t# command will fail if the index is *not* refreshed:\n>> +\t# 1. update the worktree to match HEAD & remove file2 in the index\n> \n> In other words, file2 tentatively becomes untracked.\n> \n>> +\t# 2. reset --mixed to unstage the change from step 1\n> \n> But then, file2 is \"added\" to the index again, but added from the\n> HEAD. If this did not refresh, then we do not know if the contents\n> of the file in the working tree is the same, and \"diff-files\" may\n> say \"file2 may be modified\". If \"reset\" refreshes, this will take\n> us back to the same state as \"reset --hard HEAD\", and \"diff-files\"\n> will not report that \"file2\" is different.\n> \n>> +\t# 3. read-tree HEAD~1 (which differs from HEAD in file2)\n> \n> With \"-m\" option, I presume? Do we want \"-u\" here, too?\n> \n>> +\t# If the index is refreshed in step 2, then file2 in the index will be\n>> +\t# up-to-date with HEAD and read-tree will succeed (thus failing the\n>> +\t# test). If the index is *not* refreshed, however, the staged deletion\n>> +\t# of file2 from step 1 will conflict with the changes from the tree read\n>> +\t# in step 3, resulting in a failure.\n> \n> This feels a bit brittle. The implementation of \"read-tree -m\" may\n> choose to refresh beforehand to avoid such a failure.\n> \n> In any case, the name of the helper alone wasn't of any help to\n> realize that this is about checking if \"reset\" refreshes the index\n> or not. Perhaps call it more like\n> \n> \treset_refreshes_index\n> \n> or something?\n> \n> In any case, instead of the big comment block, comments interspersed\n> in the steps may be easier to follow. \n> \n>> +\t# Step 0: start with a clean index\n>> +\tgit reset --hard HEAD &&\n>> +\n>> +\t# Step 1\n> \t# remove file2 from the index\n>> +\tgit rm --cached file2 &&\n>> +\n>> +\t# Step 2\n> \t# resurrect file2 to the index from HEAD; if the cached stat\n> \t# info gets refreshed, this brings us back to the state\n> # after Step 0. If not, \"diff-files\" would report file2 is\n> \t# different.\n>> +\tgit $1 reset $2 --mixed HEAD &&\n>> +\n>> +\t# Step 3\n>> +\tgit read-tree -m HEAD~1\n> \n> And use \"diff-files file2\" here? Then you do not even have to rely\n> on HEAD and HEAD~1 being different at file2.\n> \n\nThese are all helpful suggestions, I'll include them in a re-roll\n(specifically: rename 'test_index_refreshed' to something mentioning\n'reset', move the test comments inline with the steps they execute, and use\n'diff-files' rather than 'read-tree'). \n\nThanks!\n"},{"id":"451360","messageId":"cccf5552-a753-b472-0e22-d7f45542d424@github.com","threadId":"57545","inReplyTo":"xmqqtuc0qq0w.fsf@gitster.g","subject":"Re: [PATCH v2 5/5] stash: make internal resets quiet and refresh index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-03-14T23:54:12Z","receivedAt":"2022-03-14T23:54:17Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> \"Victoria Dye via GitGitGadget\" writes:\n> \n>> From: Victoria Dye \n>>\n>> Add the options '-q' and '--refresh' to the 'git reset' executed in\n>> 'reset_head()', and '--refresh' to the 'git reset -q' executed in\n>> 'do_push_stash(...)'.\n>>\n>> 'stash' is implemented such that git commands invoked as part of it (e.g.,\n>> 'clean', 'read-tree', 'reset', etc.) have their informational output\n>> silenced. However, the 'reset' in 'reset_head()' is *not* called with '-q',\n>> leading to the potential for a misleading printout from 'git stash apply\n>> --index' if the stash included a removed file:\n>>\n>> Unstaged changes after reset: D \n>>\n>> Not only is this confusing in its own right (since, after the reset, 'git\n>> stash' execution would stage the deletion in the index), it would be printed\n>> even when the stash was applied with the '-q' option. As a result, the\n>> messaging is removed entirely by calling 'git status' with '-q'.\n>>\n>> Additionally, because the default behavior of 'git reset -q' is to skip\n>> refreshing the index, but later operations in 'git stash' subcommands expect\n>> a non-stale index, enable '--refresh' as well.\n>>\n>> Signed-off-by: Victoria Dye \n>> ---\n>> builtin/stash.c | 5 +++--\n>> t/t3903-stash.sh | 12 ++++++++++++\n>> 2 files changed, 15 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/builtin/stash.c b/builtin/stash.c\n>> index 3e8af210fde..91407d9bbe0 100644\n>> --- a/builtin/stash.c\n>> +++ b/builtin/stash.c\n>> @@ -310,7 +310,7 @@ static int reset_head(void)\n>> \t * API for resetting.\n>> \t */\n>> \tcp.git_cmd = 1;\n>> -\tstrvec_push(&cp.args, \"reset\");\n>> +\tstrvec_pushl(&cp.args, \"reset\", \"--quiet\", \"--refresh\", NULL);\n>> \n>> \treturn run_command(&cp);\n>> }\n>> @@ -1633,7 +1633,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n>> \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n>> \n>> \t\t\tcp.git_cmd = 1;\n>> -\t\t\tstrvec_pushl(&cp.args, \"reset\", \"-q\", \"--\", NULL);\n>> +\t\t\tstrvec_pushl(&cp.args, \"reset\", \"-q\", \"--refresh\", \"--\",\n>> +\t\t\t\t NULL);\n>> \t\t\tadd_pathspecs(&cp.args, ps);\n>> \t\t\tif (run_command(&cp)) {\n>> \t\t\t\tret = -1;\n>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n>> index f36e121210e..17f2ad2344c 100755\n>> --- a/t/t3903-stash.sh\n>> +++ b/t/t3903-stash.sh\n>> @@ -261,6 +261,18 @@ test_expect_success 'apply -q is quiet' '\n>> \ttest_must_be_empty output.out\n>> '\n>> \n>> +test_expect_success 'apply --index -q is quiet' '\n>> +\t# Added file, deleted file, modified file all staged for commit\n>> +\techo foo >new-file &&\n>> +\techo test >file &&\n>> +\tgit add new-file file &&\n>> +\tgit rm other-file &&\n>> +\n>> +\tgit stash &&\n> \n> Hpmh. The hunk that updates reset_head() does get exercised by\n> \"apply --index\", so testing that is OK, but we are also patching\n> \"do_push_stash()\" to be quiet, so don't we care the chattyness of\n> this step, too?\n> \n\nThe '-q' option was already present in the reset in 'do_push_stash()', but I\ndid add the '--refresh' option to ensure that (for example) 'git stash push\n--staged' refreshes the index. With that in mind...\n\n> In these steps, we also want the same \"did the command refresh the\n> index?\" tests, no?\n> \n\nyes, both cases where '--refresh' was added should be tested (since both\nwill fail if they don't include that option). I'll add them in the next\niteration.\n\n>> +\tgit stash apply --index -q >output.out 2>&1 &&\n>> +\ttest_must_be_empty output.out\n>> +'\n>> +\n>> test_expect_success 'save -q is quiet' '\n>> \tgit stash save --quiet >output.out 2>&1 &&\n>> \ttest_must_be_empty output.out\n> \n> Other than these nits I noticed, the overall idea of the topic is\n> well presented. Thanks for working on this.\n> \n\n"},{"id":"451366","messageId":"pull.1170.v3.git.1647308982.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.v2.git.1647274230.gitgitgadget@gmail.com","subject":"[PATCH v3 0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-15T01:49:37Z","receivedAt":"2022-03-15T01:49:50Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"In the process of working on tests for 'git stash' sparse index integration,\nI found that the '--quiet' option in 'git stash' does not suppress all\nnon-error output when used with '--index'. Specifically, this comes from an\ninvocation of 'git reset' without the '--quiet' flag in 'reset_head()'. Upon\nenabling that flag, though, I discovered that 1) 'reset' does not refresh\nthe index if '--quiet' is specified (as of [1]) and 2) 'git stash' needs the\nindex to be refreshed after the reset.\n\nThis series aims to decouple the \"suppress logging\" and \"skip index refresh\"\nbehaviors in 'git reset --mixed', then allow 'stash' to internally use reset\nwith logs suppressed but index refresh enabled. This is accomplished by\nintroducing the '--[no-]refresh' option and 'reset.refresh' config setting\nto 'git reset'. Additionally, in the spirit of backward-compatibility,\n'--quiet' and/or 'reset.quiet=true' without any specified \"refresh\"\noption/config will continue to skip 'refresh_index(...)'.\n\nThere are also some minor updates to the advice that suggests skipping the\nindex refresh:\n\n * replace recommendation to use \"--quiet\" with \"--no-refresh\"\n * use 'advise()' rather than 'printf()'\n * rename the advice config setting from 'advice.resetQuiet' to to\n 'advice.resetNoRefresh'\n * suppress advice if '--quiet' is specified in 'reset'\n\nFinally, tests are added to 't7102-reset.sh' verifying whether index refresh\nhappens when expected and to 't3903-stash.sh' verifying that 'apply --quiet'\nno longer prints extraneous logs.\n\n\nChanges since V2\n================\n\n * Rename 'test_index_refreshed' to 'test_reset_refreshes_index'\n * Move comments explaining the \"reset refreshes index\" test logic inline\n with execution\n * Replace 'git read-tree' with 'git diff-files' in\n 'test_reset_refreshes_index' for a (hopefully) more future-proofed\n command verifying for index refresh\n * Add tests for ensuring the index is refreshed after 'git stash push\n --staged' and 'git stash apply --index'\n\n\nChanges since V1\n================\n\n * Update cover letter title\n * Squash 't7102' test fixes from patch 5 into patch 2\n * Refactor 't7102' tests to properly use inline config settings\n\n[1] https://lore.kernel.org/git/20181023190423.5772-2-peartben@gmail.com/\n\nThanks! -Victoria\n\nVictoria Dye (5):\n reset: revise index refresh advice\n reset: introduce --[no-]refresh option to --mixed\n reset: replace '--quiet' with '--no-refresh' in performance advice\n reset: suppress '--no-refresh' advice if logging is silenced\n stash: make internal resets quiet and refresh index\n\n Documentation/config/advice.txt | 8 ++--\n Documentation/git-reset.txt | 9 ++++\n advice.c | 2 +-\n advice.h | 2 +-\n builtin/reset.c | 21 +++++++---\n builtin/stash.c | 5 ++-\n t/t3903-stash.sh | 33 +++++++++++++++\n t/t7102-reset.sh | 73 +++++++++++++++++++++++++++++----\n 8 files changed, 133 insertions(+), 20 deletions(-)\n\n\nbase-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1170%2Fvdye%2Fstash%2Fmake-reset-quiet-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1170/vdye/stash/make-reset-quiet-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1170\n\nRange-diff vs v2:\n\n 1: 7206ef8dd8a = 1: 7206ef8dd8a reset: revise index refresh advice\n 2: 7f0226bc3e6 ! 2: 101cee42dd6 reset: introduce --[no-]refresh option to --mixed\n @@ Commit message\n whether logs are silenced and do not affect index refresh.\n \n Helped-by: Derrick Stolee \n + Helped-by: Junio C Hamano \n Signed-off-by: Victoria Dye \n \n ## Documentation/git-reset.txt ##\n @@ t/t7102-reset.sh: test_expect_success 'resetting an unmodified path is a no-op'\n \tgit diff-index --cached --exit-code HEAD\n '\n \n -+test_index_refreshed () {\n -+\n -+\t# To test whether the index is refresh, create a scenario where a\n -+\t# command will fail if the index is *not* refreshed:\n -+\t# 1. update the worktree to match HEAD & remove file2 in the index\n -+\t# 2. reset --mixed to unstage the change from step 1\n -+\t# 3. read-tree HEAD~1 (which differs from HEAD in file2)\n -+\t# If the index is refreshed in step 2, then file2 in the index will be\n -+\t# up-to-date with HEAD and read-tree will succeed (thus failing the\n -+\t# test). If the index is *not* refreshed, however, the staged deletion\n -+\t# of file2 from step 1 will conflict with the changes from the tree read\n -+\t# in step 3, resulting in a failure.\n ++test_reset_refreshes_index () {\n ++\n ++\t# To test whether the index is refreshed in `git reset --mixed` with\n ++\t# the given options, create a scenario where we clearly see different\n ++\t# results depending on whether the refresh occurred or not.\n +\n +\t# Step 0: start with a clean index\n +\tgit reset --hard HEAD &&\n +\n -+\t# Step 1\n ++\t# Step 1: remove file2, but only in the index (no change to worktree)\n +\tgit rm --cached file2 &&\n +\n -+\t# Step 2\n ++\t# Step 2: reset index & leave worktree unchanged from HEAD\n +\tgit $1 reset $2 --mixed HEAD &&\n +\n -+\t# Step 3\n -+\tgit read-tree -m HEAD~1\n ++\t# Step 3: verify whether the index is refreshed by checking whether\n ++\t# file2 still has staged changes in the index differing from HEAD (if\n ++\t# the refresh occurred, there should be no such changes)\n ++\tgit diff-files >output.log &&\n ++\ttest_must_be_empty output.log\n +}\n +\n test_expect_success '--mixed refreshes the index' '\n @@ t/t7102-reset.sh: test_expect_success 'resetting an unmodified path is a no-op'\n -\ttest_cmp expect output\n +\t# Verify default behavior (with no config settings or command line\n +\t# options)\n -+\ttest_index_refreshed\n ++\ttest_reset_refreshes_index\n +'\n +test_expect_success '--mixed --[no-]quiet sets default refresh behavior' '\n +\t# Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh)\n +\t# determine refresh behavior\n +\n +\t# Config setting\n -+\t! test_index_refreshed \"-c reset.quiet=true\" &&\n -+\ttest_index_refreshed \"-c reset.quiet=false\" &&\n ++\t! test_reset_refreshes_index \"-c reset.quiet=true\" &&\n ++\ttest_reset_refreshes_index \"-c reset.quiet=false\" &&\n +\n +\t# Command line option\n -+\t! test_index_refreshed \"\" --quiet &&\n -+\ttest_index_refreshed \"\" --no-quiet &&\n ++\t! test_reset_refreshes_index \"\" --quiet &&\n ++\ttest_reset_refreshes_index \"\" --no-quiet &&\n +\n +\t# Command line option overrides config setting\n -+\t! test_index_refreshed \"-c reset.quiet=false\" --quiet &&\n -+\ttest_index_refreshed \"-c reset.refresh=true\" --no-quiet\n ++\t! test_reset_refreshes_index \"-c reset.quiet=false\" --quiet &&\n ++\ttest_reset_refreshes_index \"-c reset.refresh=true\" --no-quiet\n +'\n +\n +test_expect_success '--mixed --[no-]refresh sets refresh behavior' '\n +\t# Verify that --[no-]refresh and `reset.refresh` control index refresh\n +\n +\t# Config setting\n -+\ttest_index_refreshed \"-c reset.refresh=true\" &&\n -+\t! test_index_refreshed \"-c reset.refresh=false\" &&\n ++\ttest_reset_refreshes_index \"-c reset.refresh=true\" &&\n ++\t! test_reset_refreshes_index \"-c reset.refresh=false\" &&\n +\n +\t# Command line option\n -+\ttest_index_refreshed \"\" --refresh &&\n -+\t! test_index_refreshed \"\" --no-refresh &&\n ++\ttest_reset_refreshes_index \"\" --refresh &&\n ++\t! test_reset_refreshes_index \"\" --no-refresh &&\n +\n +\t# Command line option overrides config setting\n -+\ttest_index_refreshed \"-c reset.refresh=false\" --refresh &&\n -+\t! test_index_refreshed \"-c reset.refresh=true\" --no-refresh\n ++\ttest_reset_refreshes_index \"-c reset.refresh=false\" --refresh &&\n ++\t! test_reset_refreshes_index \"-c reset.refresh=true\" --no-refresh\n +'\n +\n +test_expect_success '--mixed --refresh overrides --quiet refresh behavior' '\n +\t# Verify that *both* --refresh and `reset.refresh` override the\n +\t# default non-refresh behavior of --quiet\n -+\ttest_index_refreshed \"\" \"--quiet --refresh\" &&\n -+\ttest_index_refreshed \"-c reset.quiet=true\" --refresh &&\n -+\ttest_index_refreshed \"-c reset.refresh=true\" --quiet &&\n -+\ttest_index_refreshed \"-c reset.refresh=true -c reset.quiet=true\"\n ++\ttest_reset_refreshes_index \"\" \"--quiet --refresh\" &&\n ++\ttest_reset_refreshes_index \"-c reset.quiet=true\" --refresh &&\n ++\ttest_reset_refreshes_index \"-c reset.refresh=true\" --quiet &&\n ++\ttest_reset_refreshes_index \"-c reset.refresh=true -c reset.quiet=true\"\n '\n \n test_expect_success '--mixed preserves skip-worktree' '\n 3: f869723d64f = 3: eb5958194ee reset: replace '--quiet' with '--no-refresh' in performance advice\n 4: cffca0ea5c6 = 4: 548c9303c44 reset: suppress '--no-refresh' advice if logging is silenced\n 5: 3334d4cb6f3 ! 5: 4c45351a0c4 stash: make internal resets quiet and refresh index\n @@ Commit message\n refreshing the index, but later operations in 'git stash' subcommands expect\n a non-stale index, enable '--refresh' as well.\n \n + Helped-by: Junio C Hamano \n Signed-off-by: Victoria Dye \n \n ## builtin/stash.c ##\n @@ t/t3903-stash.sh: test_expect_success 'apply -q is quiet' '\n test_expect_success 'save -q is quiet' '\n \tgit stash save --quiet >output.out 2>&1 &&\n \ttest_must_be_empty output.out\n +@@ t/t3903-stash.sh: test_expect_success 'drop -q is quiet' '\n + \ttest_must_be_empty output.out\n + '\n + \n ++test_expect_success 'stash push -q --staged refreshes the index' '\n ++\tgit reset --hard &&\n ++\techo test >file &&\n ++\tgit add file &&\n ++\tgit stash push -q --staged &&\n ++\tgit diff-files >output.out &&\n ++\ttest_must_be_empty output.out\n ++'\n ++\n ++test_expect_success 'stash apply -q --index refreshes the index' '\n ++\techo test >other-file &&\n ++\tgit add other-file &&\n ++\techo another-change >other-file &&\n ++\tgit diff-files >expect &&\n ++\tgit stash &&\n ++\n ++\tgit stash apply -q --index &&\n ++\tgit diff-files >actual &&\n ++\ttest_cmp expect actual\n ++'\n ++\n + test_expect_success 'stash -k' '\n + \techo bar3 >file &&\n + \techo bar4 >file2 &&\n\n-- \ngitgitgadget\n"},{"id":"451367","messageId":"7206ef8dd8ab088feab841c5574d486b6f19af5e.1647308982.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.v3.git.1647308982.gitgitgadget@gmail.com","subject":"[PATCH v3 1/5] reset: revise index refresh advice","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-15T01:49:38Z","receivedAt":"2022-03-15T01:49:53Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nUpdate the advice describing index refresh from \"enumerate unstaged changes\"\nto \"refresh the index.\" Describing 'refresh_index(...)' as \"enumerating\nunstaged changes\" is not fully representative of what an index refresh is\ndoing; more generally, it updates the properties of index entries that are\naffected by outside-of-index state, e.g. CE_UPTODATE, which is affected by\nthe file contents on-disk. This distinction is relevant to operations that\nread the index but do not refresh first - e.g., 'git read-tree' - where a\nstale index may cause incorrect behavior.\n\nIn addition to changing the advice message, use the \"advise\" function to\nprint advice.\n\nSigned-off-by: Victoria Dye \n---\n Documentation/config/advice.txt | 4 ++--\n builtin/reset.c | 4 ++--\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex c40eb09cb7e..971aad2f237 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -69,8 +69,8 @@ advice.*::\n \t\tmerge to avoid overwriting local changes.\n \tresetQuiet::\n \t\tAdvice to consider using the `--quiet` option to linkgit:git-reset[1]\n-\t\twhen the command takes more than 2 seconds to enumerate unstaged\n-\t\tchanges after reset.\n+\t\twhen the command takes more than 2 seconds to refresh the index\n+\t\tafter reset.\n \tresolveConflict::\n \t\tAdvice shown by various commands when conflicts\n \t\tprevent the operation from being performed.\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 6e65e90c5db..a420497a14f 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -525,9 +525,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t _(\"Unstaged changes after reset:\"));\n \t\t\t\tt_delta_in_ms = (getnanotime() - t_begin) / 1000000;\n \t\t\t\tif (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n-\t\t\t\t\tprintf(_(\"\\nIt took %.2f seconds to enumerate unstaged changes after reset. You can\\n\"\n+\t\t\t\t\tadvise(_(\"It took %.2f seconds to refresh the index after reset. You can\\n\"\n \t\t\t\t\t\t\"use '--quiet' to avoid this. Set the config setting reset.quiet to true\\n\"\n-\t\t\t\t\t\t\"to make this the default.\\n\"), t_delta_in_ms / 1000.0);\n+\t\t\t\t\t\t\"to make this the default.\"), t_delta_in_ms / 1000.0);\n \t\t\t\t}\n \t\t\t}\n \t\t} else {\n-- \ngitgitgadget\n\n"},{"id":"451368","messageId":"eb5958194ee127db78e5028482c82dd8e5384f52.1647308982.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.v3.git.1647308982.gitgitgadget@gmail.com","subject":"[PATCH v3 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-15T01:49:40Z","receivedAt":"2022-03-15T01:49:54Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nReplace references to '--quiet' with '--no-refresh' in the advice on how to\nskip refreshing the index. When the advice was introduced, '--quiet' was the\nonly way to avoid the expensive 'refresh_index(...)' at the end of a mixed\nreset. After introducing '--no-refresh', however, '--quiet' became only a\nfallback option for determining refresh behavior, overridden by\n'--[no-]refresh' or 'reset.refresh' if either is set. To ensure users are\nadvised to use the most reliable option for avoiding 'refresh_index(...)',\nreplace recommendation of '--quiet' with '--[no-]refresh'.\n\nSigned-off-by: Victoria Dye \n---\n Documentation/config/advice.txt | 8 ++++----\n advice.c | 2 +-\n advice.h | 2 +-\n builtin/reset.c | 8 ++++----\n 4 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex 971aad2f237..83c2a956611 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -67,10 +67,10 @@ advice.*::\n \tcommitBeforeMerge::\n \t\tAdvice shown when linkgit:git-merge[1] refuses to\n \t\tmerge to avoid overwriting local changes.\n-\tresetQuiet::\n-\t\tAdvice to consider using the `--quiet` option to linkgit:git-reset[1]\n-\t\twhen the command takes more than 2 seconds to refresh the index\n-\t\tafter reset.\n+\tresetNoRefresh::\n+\t\tAdvice to consider using the `--no-refresh` option to\n+\t\tlinkgit:git-reset[1] when the command takes more than 2 seconds\n+\t\tto refresh the index after reset.\n \tresolveConflict::\n \t\tAdvice shown by various commands when conflicts\n \t\tprevent the operation from being performed.\ndiff --git a/advice.c b/advice.c\nindex 2e1fd483040..cb755c551a2 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -61,7 +61,7 @@ static struct {\n \t[ADVICE_PUSH_NON_FF_MATCHING]\t\t\t= { \"pushNonFFMatching\", 1 },\n \t[ADVICE_PUSH_UNQUALIFIED_REF_NAME]\t\t= { \"pushUnqualifiedRefName\", 1 },\n \t[ADVICE_PUSH_UPDATE_REJECTED]\t\t\t= { \"pushUpdateRejected\", 1 },\n-\t[ADVICE_RESET_QUIET_WARNING]\t\t\t= { \"resetQuiet\", 1 },\n+\t[ADVICE_RESET_NO_REFRESH_WARNING]\t\t= { \"resetNoRefresh\", 1 },\n \t[ADVICE_RESOLVE_CONFLICT]\t\t\t= { \"resolveConflict\", 1 },\n \t[ADVICE_RM_HINTS]\t\t\t\t= { \"rmHints\", 1 },\n \t[ADVICE_SEQUENCER_IN_USE]\t\t\t= { \"sequencerInUse\", 1 },\ndiff --git a/advice.h b/advice.h\nindex a3957123a16..f95af70ced4 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -36,7 +36,7 @@ struct string_list;\n \tADVICE_PUSH_UPDATE_REJECTED_ALIAS,\n \tADVICE_PUSH_UPDATE_REJECTED,\n \tADVICE_PUSH_REF_NEEDS_UPDATE,\n-\tADVICE_RESET_QUIET_WARNING,\n+\tADVICE_RESET_NO_REFRESH_WARNING,\n \tADVICE_RESOLVE_CONFLICT,\n \tADVICE_RM_HINTS,\n \tADVICE_SEQUENCER_IN_USE,\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 7f667e13d71..feab85e03de 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -535,10 +535,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\trefresh_index(&the_index, flags, NULL, NULL,\n \t\t\t\t\t _(\"Unstaged changes after reset:\"));\n \t\t\t\tt_delta_in_ms = (getnanotime() - t_begin) / 1000000;\n-\t\t\t\tif (advice_enabled(ADVICE_RESET_QUIET_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n-\t\t\t\t\tadvise(_(\"It took %.2f seconds to refresh the index after reset. You can\\n\"\n-\t\t\t\t\t\t\"use '--quiet' to avoid this. Set the config setting reset.quiet to true\\n\"\n-\t\t\t\t\t\t\"to make this the default.\"), t_delta_in_ms / 1000.0);\n+\t\t\t\tif (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n+\t\t\t\t\tadvise(_(\"It took %.2f seconds to refresh the index after reset. You can use\\n\"\n+\t\t\t\t\t\t \"'--no-refresh' to avoid this. Set the config setting reset.refresh to false\\n\"\n+\t\t\t\t\t\t \"to make this the default.\"), t_delta_in_ms / 1000.0);\n \t\t\t\t}\n \t\t\t}\n \t\t} else {\n-- \ngitgitgadget\n\n"},{"id":"451369","messageId":"101cee42dd6d5a4e5f12da647eafab9334d69a2d.1647308982.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.v3.git.1647308982.gitgitgadget@gmail.com","subject":"[PATCH v3 2/5] reset: introduce --[no-]refresh option to --mixed","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-15T01:49:39Z","receivedAt":"2022-03-15T01:49:54Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd a new --[no-]refresh option that is intended to explicitly determine\nwhether a mixed reset should end in an index refresh.\n\nStarting at 9ac8125d1a (reset: don't compute unstaged changes after reset\nwhen --quiet, 2018-10-23), using the '--quiet' option results in skipping\nthe call to 'refresh_index(...)' at the end of a mixed reset with the goal\nof improving performance. However, by coupling behavior that modifies the\nindex with the option that silences logs, there is no way for users to have\none without the other (i.e., silenced logs with a refreshed index) without\nincurring the overhead of a separate call to 'git update-index --refresh'.\nFurthermore, there is minimal user-facing documentation indicating that\n--quiet skips the index refresh, potentially leading to unexpected issues\nexecuting commands after 'git reset --quiet' that do not themselves refresh\nthe index (e.g., internals of 'git stash', 'git read-tree').\n\nTo mitigate these issues, '--[no-]refresh' and 'reset.refresh' are\nintroduced to provide a dedicated mechanism for refreshing the index. When\neither is set, '--quiet' and 'reset.quiet' revert to controlling only\nwhether logs are silenced and do not affect index refresh.\n\nHelped-by: Derrick Stolee \nHelped-by: Junio C Hamano \nSigned-off-by: Victoria Dye \n---\n Documentation/git-reset.txt | 9 +++++\n builtin/reset.c | 13 ++++++-\n t/t7102-reset.sh | 73 +++++++++++++++++++++++++++++++++----\n 3 files changed, 87 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt\nindex 6f7685f53d5..89ddc85c2e4 100644\n--- a/Documentation/git-reset.txt\n+++ b/Documentation/git-reset.txt\n@@ -110,6 +110,15 @@ OPTIONS\n \t`reset.quiet` config option. `--quiet` and `--no-quiet` will\n \toverride the default behavior.\n \n+--refresh::\n+--no-refresh::\n+\tProactively refresh the index after a mixed reset. If unspecified, the\n+\tbehavior falls back on the `reset.refresh` config option. If neither\n+\t`--[no-]refresh` nor `reset.refresh` are set, the default behavior is\n+\tdecided by the `--[no-]quiet` option and/or `reset.quiet` config.\n+\tIf `--quiet` is specified or `reset.quiet` is set with no command-line\n+\t\"quiet\" setting, refresh is disabled. Otherwise, refresh is enabled.\n+\n --pathspec-from-file=::\n \tPathspec is passed in `` instead of commandline args. If\n \t`` is exactly `-` then standard input is used. Pathspec\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex a420497a14f..7f667e13d71 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -392,6 +392,7 @@ static int git_reset_config(const char *var, const char *value, void *cb)\n int cmd_reset(int argc, const char **argv, const char *prefix)\n {\n \tint reset_type = NONE, update_ref_status = 0, quiet = 0;\n+\tint refresh = -1;\n \tint patch_mode = 0, pathspec_file_nul = 0, unborn;\n \tconst char *rev, *pathspec_from_file = NULL;\n \tstruct object_id oid;\n@@ -399,6 +400,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tint intent_to_add = 0;\n \tconst struct option options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"be quiet, only report errors\")),\n+\t\tOPT_BOOL(0, \"refresh\", &refresh,\n+\t\t\t\tN_(\"skip refreshing the index after reset\")),\n \t\tOPT_SET_INT(0, \"mixed\", &reset_type,\n \t\t\t\t\t\tN_(\"reset HEAD and index\"), MIXED),\n \t\tOPT_SET_INT(0, \"soft\", &reset_type, N_(\"reset only HEAD\"), SOFT),\n@@ -421,11 +424,19 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_reset_config, NULL);\n \tgit_config_get_bool(\"reset.quiet\", &quiet);\n+\tgit_config_get_bool(\"reset.refresh\", &refresh);\n \n \targc = parse_options(argc, argv, prefix, options, git_reset_usage,\n \t\t\t\t\t\tPARSE_OPT_KEEP_DASHDASH);\n \tparse_args(&pathspec, argv, prefix, patch_mode, &rev);\n \n+\t/*\n+\t * If refresh is completely unspecified (either by config or by command\n+\t * line option), decide based on 'quiet'.\n+\t */\n+\tif (refresh < 0)\n+\t\trefresh = !quiet;\n+\n \tif (pathspec_from_file) {\n \t\tif (patch_mode)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--pathspec-from-file\", \"--patch\");\n@@ -517,7 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\tif (read_from_tree(&pathspec, &oid, intent_to_add))\n \t\t\t\treturn 1;\n \t\t\tthe_index.updated_skipworktree = 1;\n-\t\t\tif (!quiet && get_git_work_tree()) {\n+\t\t\tif (refresh && get_git_work_tree()) {\n \t\t\t\tuint64_t t_begin, t_delta_in_ms;\n \n \t\t\t\tt_begin = getnanotime();\ndiff --git a/t/t7102-reset.sh b/t/t7102-reset.sh\nindex d05426062ec..1dc3911a060 100755\n--- a/t/t7102-reset.sh\n+++ b/t/t7102-reset.sh\n@@ -462,14 +462,73 @@ test_expect_success 'resetting an unmodified path is a no-op' '\n \tgit diff-index --cached --exit-code HEAD\n '\n \n+test_reset_refreshes_index () {\n+\n+\t# To test whether the index is refreshed in `git reset --mixed` with\n+\t# the given options, create a scenario where we clearly see different\n+\t# results depending on whether the refresh occurred or not.\n+\n+\t# Step 0: start with a clean index\n+\tgit reset --hard HEAD &&\n+\n+\t# Step 1: remove file2, but only in the index (no change to worktree)\n+\tgit rm --cached file2 &&\n+\n+\t# Step 2: reset index & leave worktree unchanged from HEAD\n+\tgit $1 reset $2 --mixed HEAD &&\n+\n+\t# Step 3: verify whether the index is refreshed by checking whether\n+\t# file2 still has staged changes in the index differing from HEAD (if\n+\t# the refresh occurred, there should be no such changes)\n+\tgit diff-files >output.log &&\n+\ttest_must_be_empty output.log\n+}\n+\n test_expect_success '--mixed refreshes the index' '\n-\tcat >expect <<-\\EOF &&\n-\tUnstaged changes after reset:\n-\tM\tfile2\n-\tEOF\n-\techo 123 >>file2 &&\n-\tgit reset --mixed HEAD >output &&\n-\ttest_cmp expect output\n+\t# Verify default behavior (with no config settings or command line\n+\t# options)\n+\ttest_reset_refreshes_index\n+'\n+test_expect_success '--mixed --[no-]quiet sets default refresh behavior' '\n+\t# Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh)\n+\t# determine refresh behavior\n+\n+\t# Config setting\n+\t! test_reset_refreshes_index \"-c reset.quiet=true\" &&\n+\ttest_reset_refreshes_index \"-c reset.quiet=false\" &&\n+\n+\t# Command line option\n+\t! test_reset_refreshes_index \"\" --quiet &&\n+\ttest_reset_refreshes_index \"\" --no-quiet &&\n+\n+\t# Command line option overrides config setting\n+\t! test_reset_refreshes_index \"-c reset.quiet=false\" --quiet &&\n+\ttest_reset_refreshes_index \"-c reset.refresh=true\" --no-quiet\n+'\n+\n+test_expect_success '--mixed --[no-]refresh sets refresh behavior' '\n+\t# Verify that --[no-]refresh and `reset.refresh` control index refresh\n+\n+\t# Config setting\n+\ttest_reset_refreshes_index \"-c reset.refresh=true\" &&\n+\t! test_reset_refreshes_index \"-c reset.refresh=false\" &&\n+\n+\t# Command line option\n+\ttest_reset_refreshes_index \"\" --refresh &&\n+\t! test_reset_refreshes_index \"\" --no-refresh &&\n+\n+\t# Command line option overrides config setting\n+\ttest_reset_refreshes_index \"-c reset.refresh=false\" --refresh &&\n+\t! test_reset_refreshes_index \"-c reset.refresh=true\" --no-refresh\n+'\n+\n+test_expect_success '--mixed --refresh overrides --quiet refresh behavior' '\n+\t# Verify that *both* --refresh and `reset.refresh` override the\n+\t# default non-refresh behavior of --quiet\n+\ttest_reset_refreshes_index \"\" \"--quiet --refresh\" &&\n+\ttest_reset_refreshes_index \"-c reset.quiet=true\" --refresh &&\n+\ttest_reset_refreshes_index \"-c reset.refresh=true\" --quiet &&\n+\ttest_reset_refreshes_index \"-c reset.refresh=true -c reset.quiet=true\"\n '\n \n test_expect_success '--mixed preserves skip-worktree' '\n-- \ngitgitgadget\n\n"},{"id":"451370","messageId":"548c9303c448f85b4c1c5db44886b8b9b7b44db5.1647308982.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.v3.git.1647308982.gitgitgadget@gmail.com","subject":"[PATCH v3 4/5] reset: suppress '--no-refresh' advice if logging is silenced","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-15T01:49:41Z","receivedAt":"2022-03-15T01:49:57Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nIf using '--quiet' or 'reset.quiet=true', do not print the 'resetnoRefresh'\nadvice string. For applications that rely on '--quiet' disabling all\nnon-error logs, the advice message should be suppressed accordingly.\n\nSigned-off-by: Victoria Dye \n---\n builtin/reset.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex feab85e03de..c8a356ec5b0 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -535,7 +535,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\trefresh_index(&the_index, flags, NULL, NULL,\n \t\t\t\t\t _(\"Unstaged changes after reset:\"));\n \t\t\t\tt_delta_in_ms = (getnanotime() - t_begin) / 1000000;\n-\t\t\t\tif (advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n+\t\t\t\tif (!quiet && advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {\n \t\t\t\t\tadvise(_(\"It took %.2f seconds to refresh the index after reset. You can use\\n\"\n \t\t\t\t\t\t \"'--no-refresh' to avoid this. Set the config setting reset.refresh to false\\n\"\n \t\t\t\t\t\t \"to make this the default.\"), t_delta_in_ms / 1000.0);\n-- \ngitgitgadget\n\n"},{"id":"451371","messageId":"4c45351a0c4b8b955a9101ed9445a763750ca3c3.1647308982.git.gitgitgadget@gmail.com","threadId":"57545","inReplyTo":"pull.1170.v3.git.1647308982.gitgitgadget@gmail.com","subject":"[PATCH v3 5/5] stash: make internal resets quiet and refresh index","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-15T01:49:42Z","receivedAt":"2022-03-15T01:49:57Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nAdd the options '-q' and '--refresh' to the 'git reset' executed in\n'reset_head()', and '--refresh' to the 'git reset -q' executed in\n'do_push_stash(...)'.\n\n'stash' is implemented such that git commands invoked as part of it (e.g.,\n'clean', 'read-tree', 'reset', etc.) have their informational output\nsilenced. However, the 'reset' in 'reset_head()' is *not* called with '-q',\nleading to the potential for a misleading printout from 'git stash apply\n--index' if the stash included a removed file:\n\nUnstaged changes after reset: D \n\nNot only is this confusing in its own right (since, after the reset, 'git\nstash' execution would stage the deletion in the index), it would be printed\neven when the stash was applied with the '-q' option. As a result, the\nmessaging is removed entirely by calling 'git status' with '-q'.\n\nAdditionally, because the default behavior of 'git reset -q' is to skip\nrefreshing the index, but later operations in 'git stash' subcommands expect\na non-stale index, enable '--refresh' as well.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Victoria Dye \n---\n builtin/stash.c | 5 +++--\n t/t3903-stash.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 3e8af210fde..91407d9bbe0 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -310,7 +310,7 @@ static int reset_head(void)\n \t * API for resetting.\n \t */\n \tcp.git_cmd = 1;\n-\tstrvec_push(&cp.args, \"reset\");\n+\tstrvec_pushl(&cp.args, \"reset\", \"--quiet\", \"--refresh\", NULL);\n \n \treturn run_command(&cp);\n }\n@@ -1633,7 +1633,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \n \t\t\tcp.git_cmd = 1;\n-\t\t\tstrvec_pushl(&cp.args, \"reset\", \"-q\", \"--\", NULL);\n+\t\t\tstrvec_pushl(&cp.args, \"reset\", \"-q\", \"--refresh\", \"--\",\n+\t\t\t\t NULL);\n \t\t\tadd_pathspecs(&cp.args, ps);\n \t\t\tif (run_command(&cp)) {\n \t\t\t\tret = -1;\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex f36e121210e..1a5c1bd3109 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -261,6 +261,18 @@ test_expect_success 'apply -q is quiet' '\n \ttest_must_be_empty output.out\n '\n \n+test_expect_success 'apply --index -q is quiet' '\n+\t# Added file, deleted file, modified file all staged for commit\n+\techo foo >new-file &&\n+\techo test >file &&\n+\tgit add new-file file &&\n+\tgit rm other-file &&\n+\n+\tgit stash &&\n+\tgit stash apply --index -q >output.out 2>&1 &&\n+\ttest_must_be_empty output.out\n+'\n+\n test_expect_success 'save -q is quiet' '\n \tgit stash save --quiet >output.out 2>&1 &&\n \ttest_must_be_empty output.out\n@@ -291,6 +303,27 @@ test_expect_success 'drop -q is quiet' '\n \ttest_must_be_empty output.out\n '\n \n+test_expect_success 'stash push -q --staged refreshes the index' '\n+\tgit reset --hard &&\n+\techo test >file &&\n+\tgit add file &&\n+\tgit stash push -q --staged &&\n+\tgit diff-files >output.out &&\n+\ttest_must_be_empty output.out\n+'\n+\n+test_expect_success 'stash apply -q --index refreshes the index' '\n+\techo test >other-file &&\n+\tgit add other-file &&\n+\techo another-change >other-file &&\n+\tgit diff-files >expect &&\n+\tgit stash &&\n+\n+\tgit stash apply -q --index &&\n+\tgit diff-files >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'stash -k' '\n \techo bar3 >file &&\n \techo bar4 >file2 &&\n-- \ngitgitgadget\n"},{"id":"451382","messageId":"xmqqee33h5ty.fsf@gitster.g","threadId":"57545","inReplyTo":"4c45351a0c4b8b955a9101ed9445a763750ca3c3.1647308982.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 5/5] stash: make internal resets quiet and refresh index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-15T10:23:53Z","receivedAt":"2022-03-15T10:23:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> +test_expect_success 'apply --index -q is quiet' '\n\nHmph. So being quiet and refreshing index are tested separately in\ndifferent tests, and this one is only about being quiet?\n\nI wonder if a single test that checks chattiness and refreshing of\n\"git stash -q\" and \"git apply --index -q\" (that's 2x2 which is 4)\nwould be sufficient?\n\n> +\t# Added file, deleted file, modified file all staged for commit\n> +\techo foo >new-file &&\n> +\techo test >file &&\n> +\tgit add new-file file &&\n> +\tgit rm other-file &&\n> +\n> +\tgit stash &&\n\nAs this is only about chattiness about \"apply --index -q\", this\ncommand goes unchecked (4 - 2 = 2).\n\n> +\tgit stash apply --index -q >output.out 2>&1 &&\n\nAnd this is only about chattiness so we do not test if the index\ngets refreshed after this operation (2 - 1 = 1).\n\n> +\ttest_must_be_empty output.out\n\nThis ensures that \"git stash apply --index -q\" is silent, as that is\nthe main objective of this step: make sure reset won't chatter,\nespecially when stash is told to be --quiet, which is good.\n\nBut with a few more lines, this set-up can also test the other three\nwith minimum additional effort, no?\n\n> +'\n> +\n> test_expect_success 'save -q is quiet' '\n> \tgit stash save --quiet >output.out 2>&1 &&\n> \ttest_must_be_empty output.out\n> @@ -291,6 +303,27 @@ test_expect_success 'drop -q is quiet' '\n> \ttest_must_be_empty output.out\n> '\n> \n> +test_expect_success 'stash push -q --staged refreshes the index' '\n> +\tgit reset --hard &&\n> +\techo test >file &&\n> +\tgit add file &&\n> +\tgit stash push -q --staged &&\n\n\"git stash\" and \"git stash push -q --staged\" may do different\nthings, so leaving the plain \"git stash\" untested for refreshing in\nan earlier test, and \"git stash\" with different options being tested\nfor refreshing here, makes me wonder about a gap in test coverage.\n\nThe overall theme of the whole topic was that chatty output from\n\"git reset\" run as an implementation detail seeps through from \"git\nstash\", IIUC. So, making sure that our index is refreshed after the\noperation is good, but at the same time, wouldn't we want to see\nwhat the output of this command says (or be silent)?\n"},{"id":"451509","messageId":"a4a7c3ea-3a40-1110-1c1e-a089daf01281@github.com","threadId":"57545","inReplyTo":"xmqqee33h5ty.fsf@gitster.g","subject":"Re: [PATCH v3 5/5] stash: make internal resets quiet and refresh index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-03-16T20:07:09Z","receivedAt":"2022-03-16T20:07:15Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> \"Victoria Dye via GitGitGadget\" writes:\n> \n>> +test_expect_success 'apply --index -q is quiet' '\n> \n> Hmph. So being quiet and refreshing index are tested separately in\n> different tests, and this one is only about being quiet?\n> \n\nYes, because the changes in this series only correct the \"quietness\" of 'git\napply --index -q'. Similarly, there are two instances where '--refresh' was\nadded to 'reset's in 'stash', and two corresponding tests added here.\n\n> I wonder if a single test that checks chattiness and refreshing of\n> \"git stash -q\" and \"git apply --index -q\" (that's 2x2 which is 4)\n> would be sufficient?\n> \n>> +\t# Added file, deleted file, modified file all staged for commit\n>> +\techo foo >new-file &&\n>> +\techo test >file &&\n>> +\tgit add new-file file &&\n>> +\tgit rm other-file &&\n>> +\n>> +\tgit stash &&\n> \n> As this is only about chattiness about \"apply --index -q\", this\n> command goes unchecked (4 - 2 = 2).\n> \n>> +\tgit stash apply --index -q >output.out 2>&1 &&\n> \n> And this is only about chattiness so we do not test if the index\n> gets refreshed after this operation (2 - 1 = 1).\n> \n>> +\ttest_must_be_empty output.out\n> \n> This ensures that \"git stash apply --index -q\" is silent, as that is\n> the main objective of this step: make sure reset won't chatter,\n> especially when stash is told to be --quiet, which is good.\n> \n> But with a few more lines, this set-up can also test the other three\n> with minimum additional effort, no?\n> \n\nI like having the \"quietness\" and \"refresh\" checks separate because they're\nnot inextricably linked. One could fail while the other doesn't (or both\ncould independently fail), where two separate results would be slightly more\nhelpful information for a developer debugging the tests. The same reasoning\napplies to 'stash push' with 'stash apply' (they can fail independently, so\nthey're tested separately). \n\nI'm not normally super adherent to a \"test each mode of failure separately\"\nparadigm (evidenced by some of the more complicated tests in\n't1092-sparse-checkout-compatibility'), but 't3903-stash.sh' already has\ndedicated'-q' tests separate from other functionality tests, so I followed\nthe precedent.\n\n>> +'\n>> +\n>> test_expect_success 'save -q is quiet' '\n>> \tgit stash save --quiet >output.out 2>&1 &&\n>> \ttest_must_be_empty output.out\n>> @@ -291,6 +303,27 @@ test_expect_success 'drop -q is quiet' '\n>> \ttest_must_be_empty output.out\n>> '\n>> \n>> +test_expect_success 'stash push -q --staged refreshes the index' '\n>> +\tgit reset --hard &&\n>> +\techo test >file &&\n>> +\tgit add file &&\n>> +\tgit stash push -q --staged &&\n> \n> \"git stash\" and \"git stash push -q --staged\" may do different\n> things, so leaving the plain \"git stash\" untested for refreshing in\n> an earlier test, and \"git stash\" with different options being tested\n> for refreshing here, makes me wonder about a gap in test coverage.\n> \n\nYes and no, I think. There is a gap in that stash is not otherwise tested for index refresh, but in the context of this series, that's primarily a concern where 'git reset [--mixed] -q' is used internally - if the refresh didn't happen, the command could (incorrectly) fail altogether. The new tests cover usage of 'stash' wherever a unique 'git reset [--mixed]' exists, and those are theoretically the main sources of a failure-inducing non-refreshed index.\n\n> The overall theme of the whole topic was that chatty output from\n> \"git reset\" run as an implementation detail seeps through from \"git\n> stash\", IIUC. So, making sure that our index is refreshed after the\n> operation is good, but at the same time, wouldn't we want to see\n> what the output of this command says (or be silent)?\n\nThe goal of this series was to fix 'git stash apply --index -q' specifically\n- it's a somewhat narrow scope, to the point where I would have included it\nin another series I'm working on if it weren't for the other changes needed\nin 'reset'. In line with that, I tried to keep the tests here tied\none-to-one with the changes made in 'stash':\n\n* add '-q' to 'reset_head()' -> 'apply --index -q is quiet'\n* add '--refresh' to 'reset_head()' -> 'stash apply -q --index refreshes the\n index'\n* add '--refresh' to 'do_push_stash()' -> 'stash push -q --staged refreshes\n the index'\n\nSo while I do think 'stash' testing could be expanded, my intention with\nthis particular series was to unblock changes relying on 'git stash apply\n--index -q' to work properly. If I'm making this too narrowly-scoped,\nthough, I can submit a patch on top of this one that more broadly expands\ntesting in 'stash' for '-q'. Let me know what you think!\n"},{"id":"451519","messageId":"xmqq1qz1lirs.fsf@gitster.g","threadId":"57545","inReplyTo":"a4a7c3ea-3a40-1110-1c1e-a089daf01281@github.com","subject":"Re: [PATCH v3 5/5] stash: make internal resets quiet and refresh index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-16T20:55:19Z","receivedAt":"2022-03-16T20:55:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye writes:\n\n> I like having the \"quietness\" and \"refresh\" checks separate because they're\n> not inextricably linked. One could fail while the other doesn't (or both\n> could independently fail), where two separate results would be slightly more\n> helpful information for a developer debugging the tests. The same reasoning\n> applies to 'stash push' with 'stash apply' (they can fail independently, so\n> they're tested separately). \n\nOK.\n\n"},{"id":"451618","messageId":"80a5f411-2ddb-177f-4139-1dfff436deda@gmail.com","threadId":"57545","inReplyTo":"101cee42dd6d5a4e5f12da647eafab9334d69a2d.1647308982.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/5] reset: introduce --[no-]refresh option to --mixed","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-18T11:08:31Z","receivedAt":"2022-03-18T11:08:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Victoria\n\n[replying to v3 rather than v4 as gmail still hasn't delivered that version]\n\nOn 15/03/2022 01:49, Victoria Dye via GitGitGadget wrote:\n> +\t/*\n> +\t * If refresh is completely unspecified (either by config or by command\n> +\t * line option), decide based on 'quiet'.\n> +\t */\n> +\tif (refresh < 0)\n> +\t\trefresh = !quiet;\n\nThis keeps the current behavior of not refreshing when --quiet is given. \nI wonder how disruptive it would be to take the opportunity to get rid \nof that hack and go back the the original behavior of refreshing when \n--quiet is given. There are a couple of assumptions that make me think \nit might be acceptable\n\n1 - anyone using a sparse index wont notice as refreshing the index\n should be fast for them\n\n2 - the large repositories that are affected exist in managed\n environments where an admin who reads the release notes could set\n reset.refresh in a central config so individual users are not\n inconvenienced.\n\nBest Wishes\n\nPhillip\n"},{"id":"451636","messageId":"xmqqy2179o3c.fsf@gitster.g","threadId":"57545","inReplyTo":"80a5f411-2ddb-177f-4139-1dfff436deda@gmail.com","subject":"Re: [PATCH v3 2/5] reset: introduce --[no-]refresh option to --mixed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-18T17:17:59Z","receivedAt":"2022-03-18T17:18:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> This keeps the current behavior of not refreshing when --quiet is\n> given. I wonder how disruptive it would be to take the opportunity to\n> get rid of that hack and go back the the original behavior of\n> refreshing when --quiet is given. There are a couple of assumptions\n> that make me think it might be acceptable\n>\n> 1 - anyone using a sparse index wont notice as refreshing the index\n> should be fast for them\n>\n> 2 - the large repositories that are affected exist in managed\n> environments where an admin who reads the release notes could set\n> reset.refresh in a central config so individual users are not\n> inconvenienced.\n\nI would very much prefer to see \"--quiet\" not making contribution to\nthe decision to refresh or not in the longer term. Many plumbing\ncommands expect that the calling scripts refresh the index with an\nexplicit use of \"update-index --refresh\" and leave the index not\nrefreshed, but working on unrefreshed index is a trade-off between\nperformance and correctness.\n\n * Turning \"--quiet\" not to refresh may incur performance regression\n for shorter term. It will not hurt correctness.\n\n * Introducing \"--no-refresh\" to mark \"reset --quiet\" invocations,\n where the freshness of the index does not matter for correctness,\n would help regain performance without breaking scripts. All\n \"reset --quiet\" invocations in scripts written before this series\n are supposed to be safe (as they lived with their \"reset --quiet\"\n that does not refresh), but newly written scripts may start\n expecting that \"reset --quiet\" would refresh for correctness.\n\n * If we allow reset.refresh to be set to \"no\", however, that would\n affect _all_ uses of \"reset --quiet\", including the ones in newly\n written scripts that expect \"reset --quiet\" to refresh. They\n would be forced to say \"reset --quiet --refresh\", just in case\n the user has such a configuration; otherwise these scripts will\n be declared \"buggy\" for not explicitly saying \"--refresh\".\n\nI do not think reset.refresh is a good idea, but I very much like\nthe idea to making \"reset\" (regardless of \"--quiet\") to refresh by\ndefault.\n\nThanks.\n\n\n"},{"id":"451641","messageId":"80a2a5a2-256f-6c3b-2430-10bef99ce1e9@github.com","threadId":"57545","inReplyTo":"xmqqy2179o3c.fsf@gitster.g","subject":"Re: [PATCH v3 2/5] reset: introduce --[no-]refresh option to --mixed","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-03-18T19:19:08Z","receivedAt":"2022-03-18T19:20:30Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> This keeps the current behavior of not refreshing when --quiet is\n>> given. I wonder how disruptive it would be to take the opportunity to\n>> get rid of that hack and go back the the original behavior of\n>> refreshing when --quiet is given. There are a couple of assumptions\n>> that make me think it might be acceptable\n>>\n>> 1 - anyone using a sparse index wont notice as refreshing the index\n>> should be fast for them\n>>\n>> 2 - the large repositories that are affected exist in managed\n>> environments where an admin who reads the release notes could set\n>> reset.refresh in a central config so individual users are not\n>> inconvenienced.\n> \n> I would very much prefer to see \"--quiet\" not making contribution to\n> the decision to refresh or not in the longer term. Many plumbing\n> commands expect that the calling scripts refresh the index with an\n> explicit use of \"update-index --refresh\" and leave the index not\n> refreshed, but working on unrefreshed index is a trade-off between\n> performance and correctness.\n> \n> * Turning \"--quiet\" not to refresh may incur performance regression\n> for shorter term. It will not hurt correctness.\n> \n\nI tend to agree with you and Phillip on this. I took a more conservative\napproach with the intention of preserving as much backward compatibility as\npossible, but having '--quiet' disable refresh (to me) actively hurts its\ncorrectness. If backwards-compatibility isn't a huge concern, I'll gladly\nmake that change.\n\n> * Introducing \"--no-refresh\" to mark \"reset --quiet\" invocations,\n> where the freshness of the index does not matter for correctness,\n> would help regain performance without breaking scripts. All\n> \"reset --quiet\" invocations in scripts written before this series\n> are supposed to be safe (as they lived with their \"reset --quiet\"\n> that does not refresh), but newly written scripts may start\n> expecting that \"reset --quiet\" would refresh for correctness.\n> \n> * If we allow reset.refresh to be set to \"no\", however, that would\n> affect _all_ uses of \"reset --quiet\", including the ones in newly\n> written scripts that expect \"reset --quiet\" to refresh. They\n> would be forced to say \"reset --quiet --refresh\", just in case\n> the user has such a configuration; otherwise these scripts will\n> be declared \"buggy\" for not explicitly saying \"--refresh\".\n> \n\nI added the option as a \"replacement\" for 'reset.quiet' (specifically, its\nability to summarily disable refresh), but I can definitely see how it would\nlead to issues in the future. I'm happy to remove it, but should\n'reset.quiet' be removed as well? No other commands have a config option for\n'quiet', and it presents a similar issue of potentially suppressing a\nhelpful feature (in this case, informational printouts) across all\ninvocations unless otherwise specified.\n\n> I do not think reset.refresh is a good idea, but I very much like\n> the idea to making \"reset\" (regardless of \"--quiet\") to refresh by\n> default.\n> \n\nI was hesitant to go this far because it would force people that were\ncomfortably relying on 'reset.quiet' to need to always use '--no-refresh' to\nget the same behavior. But, to Phillip's point earlier, there are other\noptions now (like sparse index) that could provide a just-as-substantial (if\nnot greater) performance boost without sacrificing the refresh.\n\n> Thanks.\n> \n> \n\nSince this is already in 'next' (and, in its current state, I don't think it\ndoes any more damage than the pre-series state), I'll send a new series on\ntop of this that deprecates 'reset.refresh' and 'reset.quiet', and makes\n'--refresh' the default for all of 'reset'.\n\nThanks, both!\n"}]}