Message ID | bda93703013e3576101079d6aa4b821ae4c73fb7.1647043729.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash' | expand |
On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote: > From: Victoria Dye <vdye@github.com> > > Add a new --[no-]refresh option that is intended to explicitly determine > whether a mixed reset should end in an index refresh. > > A few years ago, [1] introduced behavior to the '--quiet' option to skip the ... > [1] 9ac8125d1a (reset: don't compute unstaged changes after reset when > --quiet, 2018-10-23) I believe convention would normally have this listing of the commit in-line with your discussion of it. The "[1]" probably works, too, but I can't say that I've seen that used except for URLs. Something like: Starting at <commit>, the '--quiet' option skips refresh_index()... > 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. Well motivated change. > +test_index_refreshed () { > + > + # To test whether the index is refresh, create a scenario where a > + # command will fail if the index is *not* refreshed: > + # 1. update the worktree to match HEAD & remove file2 in the index > + # 2. reset --mixed to unstage the change from step 1 > + # 3. read-tree HEAD~1 (which differs from HEAD in file2) > + # If the index is refreshed in step 2, then file2 in the index will be > + # up-to-date with HEAD and read-tree will succeed (thus failing the > + # test). If the index is *not* refreshed, however, the staged deletion > + # of file2 from step 1 will conflict with the changes from the tree read > + # in step 3, resulting in a failure. > + > + # Step 0: start with a clean index > + git reset --hard HEAD && > + > + # Step 1 > + git rm --cached file2 && > + > + # Step 2 > + git reset $1 --mixed HEAD && > + > + # Step 3 > + git read-tree -m HEAD~1 > +} > + > 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_index_refreshed && > +' It looks like this test ends with an &&. There's also a missing newline after the test. > +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_must_fail test_index_refreshed -c reset.quiet=true && This is failing, but not for the reason you want: It is running git reset -c --mixed HEAD and failing to parse the "-c", I bet. Perhaps you want to have two arguments: one for config settings and another for arguments, meaning your call in test_index_refreshed should be git $1 reset $2 --mixed HEAD and calls like this should be test_index_refreshed "-c reset.quiet=true" "" && > + test_index_refreshed -c reset.quiet=true && > + > + # Command line option > + test_must_fail test_index_refreshed --quiet && > + test_index_refreshed --no-quiet && If you take a change like I recommend above, these would be test_index_refreshed "" --no-quiet && Thanks, -Stolee
On 3/14/2022 11:05 AM, Derrick Stolee wrote: > On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote: >> From: Victoria Dye <vdye@github.com> >> +test_index_refreshed () { >> + >> + # To test whether the index is refresh, create a scenario where a >> + # command will fail if the index is *not* refreshed: >> + # 1. update the worktree to match HEAD & remove file2 in the index >> + # 2. reset --mixed to unstage the change from step 1 >> + # 3. read-tree HEAD~1 (which differs from HEAD in file2) >> + # If the index is refreshed in step 2, then file2 in the index will be >> + # up-to-date with HEAD and read-tree will succeed (thus failing the >> + # test). If the index is *not* refreshed, however, the staged deletion >> + # of file2 from step 1 will conflict with the changes from the tree read >> + # in step 3, resulting in a failure. >> + >> + # Step 0: start with a clean index >> + git reset --hard HEAD && >> + >> + # Step 1 >> + git rm --cached file2 && >> + >> + # Step 2 >> + git reset $1 --mixed HEAD && > Perhaps you want to have two arguments: one for config settings > and another for arguments, meaning your call in test_index_refreshed > should be > > git $1 reset $2 --mixed HEAD > > and calls like this should be > > test_index_refreshed "-c reset.quiet=true" "" && After looking at your other examples, I thought of another example that you might want to consider. In the helper, use: git $@ --mixed HEAD && and then for the callers, use test_index_refreshed refresh && or test_index_refreshed -c reset.quiet=true refresh && or test_index_refreshed refresh --quiet && and similarly through the other tests. Thanks, -Stolee
Derrick Stolee wrote: > On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote: >> From: Victoria Dye <vdye@github.com> >> >> Add a new --[no-]refresh option that is intended to explicitly determine >> whether a mixed reset should end in an index refresh. >> >> A few years ago, [1] introduced behavior to the '--quiet' option to skip the > ... >> [1] 9ac8125d1a (reset: don't compute unstaged changes after reset when >> --quiet, 2018-10-23) > > I believe convention would normally have this listing of the commit in-line > with your discussion of it. The "[1]" probably works, too, but I can't say > that I've seen that used except for URLs. Something like: > > Starting at <commit>, the '--quiet' option skips refresh_index()... > >> 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. > > Well motivated change. > >> +test_index_refreshed () { >> + >> + # To test whether the index is refresh, create a scenario where a >> + # command will fail if the index is *not* refreshed: >> + # 1. update the worktree to match HEAD & remove file2 in the index >> + # 2. reset --mixed to unstage the change from step 1 >> + # 3. read-tree HEAD~1 (which differs from HEAD in file2) >> + # If the index is refreshed in step 2, then file2 in the index will be >> + # up-to-date with HEAD and read-tree will succeed (thus failing the >> + # test). If the index is *not* refreshed, however, the staged deletion >> + # of file2 from step 1 will conflict with the changes from the tree read >> + # in step 3, resulting in a failure. >> + >> + # Step 0: start with a clean index >> + git reset --hard HEAD && >> + >> + # Step 1 >> + git rm --cached file2 && >> + >> + # Step 2 >> + git reset $1 --mixed HEAD && >> + >> + # Step 3 >> + git read-tree -m HEAD~1 >> +} >> + >> 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_index_refreshed && >> +' > > It looks like this test ends with an &&. There's also a missing newline > after the test. > >> +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_must_fail test_index_refreshed -c reset.quiet=true && > > This is failing, but not for the reason you want: It is running > > git reset -c --mixed HEAD > > and failing to parse the "-c", I bet. > > Perhaps you want to have two arguments: one for config settings > and another for arguments, meaning your call in test_index_refreshed > should be > > git $1 reset $2 --mixed HEAD > > and calls like this should be > > test_index_refreshed "-c reset.quiet=true" "" && > As you noted in patch 5, I switched to using `test_config` mostly because I couldn't figure out how to get this syntax right (although, now that you point it out, I *definitely* should have seen that). I prefer using the inline config like this over `test_config`, so I'll update to do what you suggest here. >> + test_index_refreshed -c reset.quiet=true && >> + >> + # Command line option >> + test_must_fail test_index_refreshed --quiet && >> + test_index_refreshed --no-quiet && > > If you take a change like I recommend above, these would be > > test_index_refreshed "" --no-quiet && > > Thanks, > -Stolee
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt index 6f7685f53d5..89ddc85c2e4 100644 --- a/Documentation/git-reset.txt +++ b/Documentation/git-reset.txt @@ -110,6 +110,15 @@ OPTIONS `reset.quiet` config option. `--quiet` and `--no-quiet` will override the default behavior. +--refresh:: +--no-refresh:: + Proactively refresh the index after a mixed reset. If unspecified, the + behavior falls back on the `reset.refresh` config option. If neither + `--[no-]refresh` nor `reset.refresh` are set, the default behavior is + decided by the `--[no-]quiet` option and/or `reset.quiet` config. + If `--quiet` is specified or `reset.quiet` is set with no command-line + "quiet" setting, refresh is disabled. Otherwise, refresh is enabled. + --pathspec-from-file=<file>:: Pathspec is passed in `<file>` instead of commandline args. If `<file>` is exactly `-` then standard input is used. Pathspec diff --git a/builtin/reset.c b/builtin/reset.c index a420497a14f..7f667e13d71 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -392,6 +392,7 @@ static int git_reset_config(const char *var, const char *value, void *cb) int cmd_reset(int argc, const char **argv, const char *prefix) { int reset_type = NONE, update_ref_status = 0, quiet = 0; + int refresh = -1; int patch_mode = 0, pathspec_file_nul = 0, unborn; const char *rev, *pathspec_from_file = NULL; struct object_id oid; @@ -399,6 +400,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix) int intent_to_add = 0; const struct option options[] = { OPT__QUIET(&quiet, N_("be quiet, only report errors")), + OPT_BOOL(0, "refresh", &refresh, + N_("skip refreshing the index after reset")), OPT_SET_INT(0, "mixed", &reset_type, N_("reset HEAD and index"), MIXED), OPT_SET_INT(0, "soft", &reset_type, N_("reset only HEAD"), SOFT), @@ -421,11 +424,19 @@ int cmd_reset(int argc, const char **argv, const char *prefix) git_config(git_reset_config, NULL); git_config_get_bool("reset.quiet", &quiet); + git_config_get_bool("reset.refresh", &refresh); argc = parse_options(argc, argv, prefix, options, git_reset_usage, PARSE_OPT_KEEP_DASHDASH); parse_args(&pathspec, argv, prefix, patch_mode, &rev); + /* + * If refresh is completely unspecified (either by config or by command + * line option), decide based on 'quiet'. + */ + if (refresh < 0) + refresh = !quiet; + if (pathspec_from_file) { if (patch_mode) die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--patch"); @@ -517,7 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (read_from_tree(&pathspec, &oid, intent_to_add)) return 1; the_index.updated_skipworktree = 1; - if (!quiet && get_git_work_tree()) { + if (refresh && get_git_work_tree()) { uint64_t t_begin, t_delta_in_ms; t_begin = getnanotime(); diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh index d05426062ec..5e68180f3b2 100755 --- a/t/t7102-reset.sh +++ b/t/t7102-reset.sh @@ -462,14 +462,77 @@ test_expect_success 'resetting an unmodified path is a no-op' ' git diff-index --cached --exit-code HEAD ' +test_index_refreshed () { + + # To test whether the index is refresh, create a scenario where a + # command will fail if the index is *not* refreshed: + # 1. update the worktree to match HEAD & remove file2 in the index + # 2. reset --mixed to unstage the change from step 1 + # 3. read-tree HEAD~1 (which differs from HEAD in file2) + # If the index is refreshed in step 2, then file2 in the index will be + # up-to-date with HEAD and read-tree will succeed (thus failing the + # test). If the index is *not* refreshed, however, the staged deletion + # of file2 from step 1 will conflict with the changes from the tree read + # in step 3, resulting in a failure. + + # Step 0: start with a clean index + git reset --hard HEAD && + + # Step 1 + git rm --cached file2 && + + # Step 2 + git reset $1 --mixed HEAD && + + # Step 3 + git read-tree -m HEAD~1 +} + 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_index_refreshed && +' +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_must_fail test_index_refreshed -c reset.quiet=true && + test_index_refreshed -c reset.quiet=true && + + # Command line option + test_must_fail test_index_refreshed --quiet && + test_index_refreshed --no-quiet && + + # Command line option overrides config setting + test_must_fail test_index_refreshed -c reset.quiet=false --quiet && + test_index_refreshed -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_index_refreshed -c reset.refresh=true && + test_must_fail test_index_refreshed -c reset.refresh=false && + + # Command line option + test_index_refreshed --refresh && + test_must_fail test_index_refreshed --no-refresh && + + # Command line option overrides config setting + test_index_refreshed -c reset.refresh=false --refresh && + test_must_fail test_index_refreshed -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_index_refreshed --refresh --quiet && + test_index_refreshed --refresh -c reset.quiet=true && + test_index_refreshed -c reset.refresh=true --quiet && + test_index_refreshed -c reset.refresh=true -c reset.quiet=true ' test_expect_success '--mixed preserves skip-worktree' '