git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 3/4] reset: deprecate 'reset.refresh' config option

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Mar 23, 2022, 16:02 UTC
Message-ID
<4f3cbfd5-65df-f44f-4867-45b1ab69c400@gmail.com>
In-Reply-To
<ecd3296fd25cc936aeb2f8ae160352a2326441c5.1647894889.git.gitgitgadget@gmail.com>
Hi Victoria
On 21/03/2022 20:34, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
Same concern about the title as the last patch
Show 22 quoted lines
> Remove the 'reset.refresh' option, requiring that users explicitly specify
> '--no-refresh' if they want to skip refreshing the index.
> 
> The 'reset.refresh' option was introduced in 101cee42dd (reset: introduce
> --[no-]refresh option to --mixed, 2022-03-11) as a replacement for the
> refresh-skipping behavior originally controlled by 'reset.quiet'.
> 
> Although 'reset.refresh=false' functionally served the same purpose as
> 'reset.quiet=true', it exposed [1] the fact that the existence of a global
> "skip refresh" option could potentially cause problems for users. Allowing a
> global config option to avoid refreshing the index forces scripts using 'git
> reset --mixed' to defensively use '--refresh' if index refresh is expected;
> if that option is missing, behavior of a script could vary from user-to-user
> without explanation.
> 
> Furthermore, globally disabling index refresh in 'reset --mixed' was
> initially devised as a passive performance improvement; since the
> introduction of the option, other changes have been made to Git (e.g., the
> sparse index) with a greater potential performance impact without
> sacrificing index correctness. Therefore, we can more aggressively err on
> the side of correctness and limit the cases of skipping index refresh to
> only when a user specifies the '--no-refresh' option.
Thanks for the well explained commit message
Show 21 quoted lines
> [1] https://lore.kernel.org/git/xmqqy2179o3c.fsf@gitster.g/
> 
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>   Documentation/git-reset.txt |  4 +---
>   builtin/reset.c             |  4 +---
>   t/t7102-reset.sh            | 14 ++------------
>   3 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index f4aca9dd35c..df167eaa766 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -109,9 +109,7 @@ OPTIONS
>   
>   --refresh::
>   --no-refresh::
> -	Proactively refresh the index after a mixed reset. If unspecified, the
> -	behavior falls back on the `reset.refresh` config option. If neither
> -	`--[no-]refresh` nor `reset.refresh` are set, refresh is enabled.
> +	Proactively refresh the index after a mixed reset. Enabled by default.

"Proactively" seems a but superfluous if I'm being picky. There was no entry in the config man page for reset.refresh so we don't need to do anything there. The code changes below look good

Best Wishes
Phillip
Show 50 quoted lines
>   --pathspec-from-file=<file>::
>   	Pathspec is passed in `<file>` instead of commandline args. If
> diff --git a/builtin/reset.c b/builtin/reset.c
> index e824aad3604..54324217f93 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -423,7 +423,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>   	};
>   
>   	git_config(git_reset_config, NULL);
> -	git_config_get_bool("reset.refresh", &refresh);
>   
>   	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
>   						PARSE_OPT_KEEP_DASHDASH);
> @@ -529,8 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>   				t_delta_in_ms = (getnanotime() - t_begin) / 1000000;
>   				if (!quiet && advice_enabled(ADVICE_RESET_NO_REFRESH_WARNING) && t_delta_in_ms > REFRESH_INDEX_DELAY_WARNING_IN_MS) {
>   					advise(_("It took %.2f seconds to refresh the index after reset.  You can use\n"
> -						 "'--no-refresh' to avoid this.  Set the config setting reset.refresh to false\n"
> -						 "to make this the default."), t_delta_in_ms / 1000.0);
> +						 "'--no-refresh' to avoid this."), t_delta_in_ms / 1000.0);
>   				}
>   			}
>   		} else {
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index 9e4c4deee35..22477f3a312 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -493,19 +493,9 @@ test_expect_success '--mixed refreshes the index' '
>   '
>   
>   test_expect_success '--mixed --[no-]refresh sets refresh behavior' '
> -	# Verify that --[no-]refresh and `reset.refresh` control index refresh
> -
> -	# Config setting
> -	test_reset_refreshes_index "-c reset.refresh=true" &&
> -	! test_reset_refreshes_index "-c reset.refresh=false" &&
> -
> -	# Command line option
> +	# Verify that --[no-]refresh controls index refresh
>   	test_reset_refreshes_index "" --refresh &&
> -	! test_reset_refreshes_index "" --no-refresh &&
> -
> -	# Command line option overrides config setting
> -	test_reset_refreshes_index "-c reset.refresh=false" --refresh &&
> -	! test_reset_refreshes_index "-c reset.refresh=true" --no-refresh
> +	! test_reset_refreshes_index "" --no-refresh
>   '
>   
>   test_expect_success '--mixed preserves skip-worktree' '
Previous: Victoria Dye via GitGitGadgetNext: Victoria Dye
Message 8 of 25 in “reset: make --no-refresh the only way to skip index refresh”
  1. 0/4 reset: make --no-refresh the only way to skip index refreshVictoria Dye via GitGitGadget, Mar 21, 2022
  2. 2/4 reset: deprecate 'reset.quiet' config optionVictoria Dye via GitGitGadget, Mar 21, 2022
  3. Phillip WoodMar 23, 2022
  4. 1/4 reset: do not make '--quiet' disable index refreshVictoria Dye via GitGitGadget, Mar 21, 2022
  5. Phillip WoodMar 23, 2022
  6. Victoria DyeMar 23, 2022
  7. 3/4 reset: deprecate 'reset.refresh' config optionVictoria Dye via GitGitGadget, Mar 21, 2022
  8. Phillip WoodMar 23, 2022
  9. Victoria DyeMar 23, 2022
  10. 4/4 reset: deprecate '--refresh', leaving only '--no-refresh'Victoria Dye via GitGitGadget, Mar 21, 2022
  11. Phillip WoodMar 23, 2022
  12. Victoria DyeMar 23, 2022
  13. 0/3 reset: make --no-refresh the only way to skip index refreshVictoria Dye via GitGitGadget, Mar 23, 2022
  14. 1/3 reset: do not make '--quiet' disable index refreshVictoria Dye via GitGitGadget, Mar 23, 2022
  15. 2/3 reset: remove 'reset.quiet' config optionVictoria Dye via GitGitGadget, Mar 23, 2022
  16. 3/3 reset: remove 'reset.refresh' config optionVictoria Dye via GitGitGadget, Mar 23, 2022
  17. Derrick StoleeMar 23, 2022
  18. Junio C HamanoMar 23, 2022
  19. Phillip WoodMar 24, 2022
  20. Junio C HamanoMar 24, 2022
  21. Junio C HamanoMar 24, 2022
  22. Victoria DyeMar 24, 2022
  23. Junio C HamanoMar 24, 2022
  24. Derrick StoleeMar 25, 2022
  25. Junio C HamanoMar 25, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.