From fd56fba97f26bf668749207efd6a45aee2e2f57c Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 15 Mar 2022 01:49:39 +0000 Subject: reset: introduce --[no-]refresh option to --mixed Add a new --[no-]refresh option that is intended to explicitly determine whether a mixed reset should end in an index refresh. Starting at 9ac8125d1a (reset: don't compute unstaged changes after reset when --quiet, 2018-10-23), using the '--quiet' option results in skipping the call to 'refresh_index(...)' at the end of a mixed reset with the goal of improving performance. However, by coupling behavior that modifies the index with the option that silences logs, there is no way for users to have one without the other (i.e., silenced logs with a refreshed index) without incurring the overhead of a separate call to 'git update-index --refresh'. Furthermore, there is minimal user-facing documentation indicating that --quiet skips the index refresh, potentially leading to unexpected issues executing commands after 'git reset --quiet' that do not themselves refresh the index (e.g., internals of 'git stash', 'git read-tree'). To mitigate these issues, '--[no-]refresh' and 'reset.refresh' are introduced to provide a dedicated mechanism for refreshing the index. When either is set, '--quiet' and 'reset.quiet' revert to controlling only whether logs are silenced and do not affect index refresh. Helped-by: Derrick Stolee Helped-by: Junio C Hamano Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- t/t7102-reset.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 66 insertions(+), 7 deletions(-) (limited to 't') diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index d05426062e..1dc3911a06 100755 --- a/t/t7102-reset.sh +++ b/t/t7102-reset.sh @@ -462,14 +462,73 @@ test_expect_success 'resetting an unmodified path is a no-op' ' git diff-index --cached --exit-code HEAD ' +test_reset_refreshes_index () { + + # To test whether the index is refreshed in `git reset --mixed` with + # the given options, create a scenario where we clearly see different + # results depending on whether the refresh occurred or not. + + # Step 0: start with a clean index + git reset --hard HEAD && + + # Step 1: remove file2, but only in the index (no change to worktree) + git rm --cached file2 && + + # Step 2: reset index & leave worktree unchanged from HEAD + git $1 reset $2 --mixed HEAD && + + # Step 3: verify whether the index is refreshed by checking whether + # file2 still has staged changes in the index differing from HEAD (if + # the refresh occurred, there should be no such changes) + git diff-files >output.log && + test_must_be_empty output.log +} + test_expect_success '--mixed refreshes the index' ' - cat >expect <<-\EOF && - Unstaged changes after reset: - M file2 - EOF - echo 123 >>file2 && - git reset --mixed HEAD >output && - test_cmp expect output + # Verify default behavior (with no config settings or command line + # options) + test_reset_refreshes_index +' +test_expect_success '--mixed --[no-]quiet sets default refresh behavior' ' + # Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh) + # determine refresh behavior + + # Config setting + ! test_reset_refreshes_index "-c reset.quiet=true" && + test_reset_refreshes_index "-c reset.quiet=false" && + + # Command line option + ! test_reset_refreshes_index "" --quiet && + test_reset_refreshes_index "" --no-quiet && + + # Command line option overrides config setting + ! test_reset_refreshes_index "-c reset.quiet=false" --quiet && + test_reset_refreshes_index "-c reset.refresh=true" --no-quiet +' + +test_expect_success '--mixed --[no-]refresh sets refresh behavior' ' + # Verify that --[no-]refresh and `reset.refresh` control index refresh + + # Config setting + test_reset_refreshes_index "-c reset.refresh=true" && + ! test_reset_refreshes_index "-c reset.refresh=false" && + + # Command line option + test_reset_refreshes_index "" --refresh && + ! test_reset_refreshes_index "" --no-refresh && + + # Command line option overrides config setting + test_reset_refreshes_index "-c reset.refresh=false" --refresh && + ! test_reset_refreshes_index "-c reset.refresh=true" --no-refresh +' + +test_expect_success '--mixed --refresh overrides --quiet refresh behavior' ' + # Verify that *both* --refresh and `reset.refresh` override the + # default non-refresh behavior of --quiet + test_reset_refreshes_index "" "--quiet --refresh" && + test_reset_refreshes_index "-c reset.quiet=true" --refresh && + test_reset_refreshes_index "-c reset.refresh=true" --quiet && + test_reset_refreshes_index "-c reset.refresh=true -c reset.quiet=true" ' test_expect_success '--mixed preserves skip-worktree' ' -- cgit v1.2.3 From 4b8b0f6fa2778c1f9c373620e3f07787543914c6 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Tue, 15 Mar 2022 01:49:42 +0000 Subject: stash: make internal resets quiet and refresh index Add the options '-q' and '--refresh' to the 'git reset' executed in 'reset_head()', and '--refresh' to the 'git reset -q' executed in 'do_push_stash(...)'. 'stash' is implemented such that git commands invoked as part of it (e.g., 'clean', 'read-tree', 'reset', etc.) have their informational output silenced. However, the 'reset' in 'reset_head()' is *not* called with '-q', leading to the potential for a misleading printout from 'git stash apply --index' if the stash included a removed file: Unstaged changes after reset: D Not only is this confusing in its own right (since, after the reset, 'git stash' execution would stage the deletion in the index), it would be printed even when the stash was applied with the '-q' option. As a result, the messaging is removed entirely by calling 'git status' with '-q'. Additionally, because the default behavior of 'git reset -q' is to skip refreshing the index, but later operations in 'git stash' subcommands expect a non-stale index, enable '--refresh' as well. Helped-by: Junio C Hamano Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- t/t3903-stash.sh | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) (limited to 't') diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index f36e121210..1a5c1bd310 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -261,6 +261,18 @@ test_expect_success 'apply -q is quiet' ' test_must_be_empty output.out ' +test_expect_success 'apply --index -q is quiet' ' + # Added file, deleted file, modified file all staged for commit + echo foo >new-file && + echo test >file && + git add new-file file && + git rm other-file && + + git stash && + git stash apply --index -q >output.out 2>&1 && + test_must_be_empty output.out +' + test_expect_success 'save -q is quiet' ' git stash save --quiet >output.out 2>&1 && test_must_be_empty output.out @@ -291,6 +303,27 @@ test_expect_success 'drop -q is quiet' ' test_must_be_empty output.out ' +test_expect_success 'stash push -q --staged refreshes the index' ' + git reset --hard && + echo test >file && + git add file && + git stash push -q --staged && + git diff-files >output.out && + test_must_be_empty output.out +' + +test_expect_success 'stash apply -q --index refreshes the index' ' + echo test >other-file && + git add other-file && + echo another-change >other-file && + git diff-files >expect && + git stash && + + git stash apply -q --index && + git diff-files >actual && + test_cmp expect actual +' + test_expect_success 'stash -k' ' echo bar3 >file && echo bar4 >file2 && -- cgit v1.2.3 From 45bf76284b40856e47e8809e952167f0316b8a99 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Wed, 23 Mar 2022 18:17:58 +0000 Subject: reset: do not make '--quiet' disable index refresh Update '--quiet' to no longer implicitly skip refreshing the index in a mixed reset. Users now have the ability to explicitly disable refreshing the index with the '--no-refresh' option, so they no longer need to use '--quiet' to do so. Moreover, we explicitly remove the refresh-skipping behavior from '--quiet' because it is completely unrelated to the stated purpose of the option: "Be quiet, only report errors." Helped-by: Phillip Wood Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- t/t7102-reset.sh | 32 +++++--------------------------- 1 file changed, 5 insertions(+), 27 deletions(-) (limited to 't') diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index 1dc3911a06..8b62bb39b3 100755 --- a/t/t7102-reset.sh +++ b/t/t7102-reset.sh @@ -485,25 +485,12 @@ test_reset_refreshes_index () { } test_expect_success '--mixed refreshes the index' ' - # Verify default behavior (with no config settings or command line - # options) - test_reset_refreshes_index -' -test_expect_success '--mixed --[no-]quiet sets default refresh behavior' ' - # Verify that --[no-]quiet and `reset.quiet` (without --[no-]refresh) - # determine refresh behavior - - # Config setting - ! test_reset_refreshes_index "-c reset.quiet=true" && - test_reset_refreshes_index "-c reset.quiet=false" && - - # Command line option - ! test_reset_refreshes_index "" --quiet && - test_reset_refreshes_index "" --no-quiet && + # Verify default behavior (without --[no-]refresh or reset.refresh) + test_reset_refreshes_index && - # Command line option overrides config setting - ! test_reset_refreshes_index "-c reset.quiet=false" --quiet && - test_reset_refreshes_index "-c reset.refresh=true" --no-quiet + # With --quiet & reset.quiet + test_reset_refreshes_index "-c reset.quiet=true" && + test_reset_refreshes_index "" --quiet ' test_expect_success '--mixed --[no-]refresh sets refresh behavior' ' @@ -522,15 +509,6 @@ test_expect_success '--mixed --[no-]refresh sets refresh behavior' ' ! test_reset_refreshes_index "-c reset.refresh=true" --no-refresh ' -test_expect_success '--mixed --refresh overrides --quiet refresh behavior' ' - # Verify that *both* --refresh and `reset.refresh` override the - # default non-refresh behavior of --quiet - test_reset_refreshes_index "" "--quiet --refresh" && - test_reset_refreshes_index "-c reset.quiet=true" --refresh && - test_reset_refreshes_index "-c reset.refresh=true" --quiet && - test_reset_refreshes_index "-c reset.refresh=true -c reset.quiet=true" -' - test_expect_success '--mixed preserves skip-worktree' ' echo 123 >>file2 && git add file2 && -- cgit v1.2.3 From 2efc9b84e5e9ea063ecfb2f813cb56653a03c10a Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Wed, 23 Mar 2022 18:17:59 +0000 Subject: reset: remove 'reset.quiet' config option Remove the 'reset.quiet' config option, remove '--no-quiet' documentation in 'Documentation/git-reset.txt'. In 4c3abd0551 (reset: add new reset.quiet config setting, 2018-10-23), 'reset.quiet' was introduced as a way to globally change the default behavior of 'git reset --mixed' to skip index refresh. However, now that '--quiet' does not affect index refresh, 'reset.quiet' would only serve to globally silence logging. This was not the original intention of the config setting, and there's no precedent for such a setting in other commands with a '--quiet' option, so it appears to be obsolete. In addition to the options & its documentation, remove 'reset.quiet' from the recommended config for 'scalar'. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- t/t7102-reset.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 't') diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index 8b62bb39b3..9e4c4deee3 100755 --- a/t/t7102-reset.sh +++ b/t/t7102-reset.sh @@ -488,8 +488,7 @@ test_expect_success '--mixed refreshes the index' ' # Verify default behavior (without --[no-]refresh or reset.refresh) test_reset_refreshes_index && - # With --quiet & reset.quiet - test_reset_refreshes_index "-c reset.quiet=true" && + # With --quiet test_reset_refreshes_index "" --quiet ' -- cgit v1.2.3 From 7cff6765fe5c1ce97f4afba9432c8aa5c5f877ba Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Wed, 23 Mar 2022 18:18:00 +0000 Subject: reset: remove 'reset.refresh' config option Remove the 'reset.refresh' option, requiring that users explicitly specify '--no-refresh' if they want to skip refreshing the index. The 'reset.refresh' option was introduced in 101cee42dd (reset: introduce --[no-]refresh option to --mixed, 2022-03-11) as a replacement for the refresh-skipping behavior originally controlled by 'reset.quiet'. Although 'reset.refresh=false' functionally served the same purpose as 'reset.quiet=true', it exposed [1] the fact that the existence of a global "skip refresh" option could potentially cause problems for users. Allowing a global config option to avoid refreshing the index forces scripts using 'git reset --mixed' to defensively use '--refresh' if index refresh is expected; if that option is missing, behavior of a script could vary from user-to-user without explanation. Furthermore, globally disabling index refresh in 'reset --mixed' was initially devised as a passive performance improvement; since the introduction of the option, other changes have been made to Git (e.g., the sparse index) with a greater potential performance impact without sacrificing index correctness. Therefore, we can more aggressively err on the side of correctness and limit the cases of skipping index refresh to only when a user specifies the '--no-refresh' option. [1] https://lore.kernel.org/git/xmqqy2179o3c.fsf@gitster.g/ Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- t/t7102-reset.sh | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) (limited to 't') diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index 9e4c4deee3..22477f3a31 100755 --- a/t/t7102-reset.sh +++ b/t/t7102-reset.sh @@ -493,19 +493,9 @@ test_expect_success '--mixed refreshes the index' ' ' test_expect_success '--mixed --[no-]refresh sets refresh behavior' ' - # Verify that --[no-]refresh and `reset.refresh` control index refresh - - # Config setting - test_reset_refreshes_index "-c reset.refresh=true" && - ! test_reset_refreshes_index "-c reset.refresh=false" && - - # Command line option + # Verify that --[no-]refresh controls index refresh test_reset_refreshes_index "" --refresh && - ! test_reset_refreshes_index "" --no-refresh && - - # Command line option overrides config setting - test_reset_refreshes_index "-c reset.refresh=false" --refresh && - ! test_reset_refreshes_index "-c reset.refresh=true" --no-refresh + ! test_reset_refreshes_index "" --no-refresh ' test_expect_success '--mixed preserves skip-worktree' ' -- cgit v1.2.3