| Message ID | 7f0226bc3e646167808fbd2413dc54e87417230c.1647274230.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash' | expand |
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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. > > 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 <derrickstolee@github.com> > Signed-off-by: Victoria Dye <vdye@github.com> > --- > Documentation/git-reset.txt | 9 +++++ > builtin/reset.c | 13 ++++++- > t/t7102-reset.sh | 77 +++++++++++++++++++++++++++++++++---- > 3 files changed, 91 insertions(+), 8 deletions(-) No complaints, but it is somewhat unsatisfying that we need these two steps that keep --quiet tied to the decision to or not to refresh. In the longer term, it may be cleaner to completely dissociate them, but it probably is not a huge deal. > + /* > + * If refresh is completely unspecified (either by config or by command > + * line option), decide based on 'quiet'. > + */ > + if (refresh < 0) > + refresh = !quiet; OK. > @@ -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(); Quite sensible. > diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh > index d05426062ec..005940778b7 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 Doesn't the verb "refresh" refer to the act of making it "fresh" (again)? i.e. update the cached stat info to up-to-date? "To test whether the index has been refreshed" or "To test whether the cached stat info is up-to-date", perhaps? > + # command will fail if the index is *not* refreshed: > + # 1. update the worktree to match HEAD & remove file2 in the index In other words, file2 tentatively becomes untracked. > + # 2. reset --mixed to unstage the change from step 1 But then, file2 is "added" to the index again, but added from the HEAD. If this did not refresh, then we do not know if the contents of the file in the working tree is the same, and "diff-files" may say "file2 may be modified". If "reset" refreshes, this will take us back to the same state as "reset --hard HEAD", and "diff-files" will not report that "file2" is different. > + # 3. read-tree HEAD~1 (which differs from HEAD in file2) With "-m" option, I presume? Do we want "-u" here, too? > + # 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. This feels a bit brittle. The implementation of "read-tree -m" may choose to refresh beforehand to avoid such a failure. In any case, the name of the helper alone wasn't of any help to realize that this is about checking if "reset" refreshes the index or not. Perhaps call it more like reset_refreshes_index or something? In any case, instead of the big comment block, comments interspersed in the steps may be easier to follow. > + # Step 0: start with a clean index > + git reset --hard HEAD && > + > + # Step 1 # remove file2 from the index > + git rm --cached file2 && > + > + # Step 2 # resurrect file2 to the index from HEAD; if the cached stat # info gets refreshed, this brings us back to the state # after Step 0. If not, "diff-files" would report file2 is # different. > + git $1 reset $2 --mixed HEAD && > + > + # Step 3 > + git read-tree -m HEAD~1 And use "diff-files file2" here? Then you do not even have to rely on HEAD and HEAD~1 being different at file2.
Junio C Hamano wrote: > "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> 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. >> >> 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 <derrickstolee@github.com> >> Signed-off-by: Victoria Dye <vdye@github.com> >> --- >> Documentation/git-reset.txt | 9 +++++ >> builtin/reset.c | 13 ++++++- >> t/t7102-reset.sh | 77 +++++++++++++++++++++++++++++++++---- >> 3 files changed, 91 insertions(+), 8 deletions(-) > > No complaints, but it is somewhat unsatisfying that we need these > two steps that keep --quiet tied to the decision to or not to > refresh. In the longer term, it may be cleaner to completely > dissociate them, but it probably is not a huge deal. > >> + /* >> + * If refresh is completely unspecified (either by config or by command >> + * line option), decide based on 'quiet'. >> + */ >> + if (refresh < 0) >> + refresh = !quiet; > > OK. > >> @@ -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(); > > Quite sensible. > >> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh >> index d05426062ec..005940778b7 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 > > Doesn't the verb "refresh" refer to the act of making it "fresh" > (again)? i.e. update the cached stat info to up-to-date? > > "To test whether the index has been refreshed" or "To test whether > the cached stat info is up-to-date", perhaps? > >> + # command will fail if the index is *not* refreshed: >> + # 1. update the worktree to match HEAD & remove file2 in the index > > In other words, file2 tentatively becomes untracked. > >> + # 2. reset --mixed to unstage the change from step 1 > > But then, file2 is "added" to the index again, but added from the > HEAD. If this did not refresh, then we do not know if the contents > of the file in the working tree is the same, and "diff-files" may > say "file2 may be modified". If "reset" refreshes, this will take > us back to the same state as "reset --hard HEAD", and "diff-files" > will not report that "file2" is different. > >> + # 3. read-tree HEAD~1 (which differs from HEAD in file2) > > With "-m" option, I presume? Do we want "-u" here, too? > >> + # 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. > > This feels a bit brittle. The implementation of "read-tree -m" may > choose to refresh beforehand to avoid such a failure. > > In any case, the name of the helper alone wasn't of any help to > realize that this is about checking if "reset" refreshes the index > or not. Perhaps call it more like > > reset_refreshes_index > > or something? > > In any case, instead of the big comment block, comments interspersed > in the steps may be easier to follow. > >> + # Step 0: start with a clean index >> + git reset --hard HEAD && >> + >> + # Step 1 > # remove file2 from the index >> + git rm --cached file2 && >> + >> + # Step 2 > # resurrect file2 to the index from HEAD; if the cached stat > # info gets refreshed, this brings us back to the state > # after Step 0. If not, "diff-files" would report file2 is > # different. >> + git $1 reset $2 --mixed HEAD && >> + >> + # Step 3 >> + git read-tree -m HEAD~1 > > And use "diff-files file2" here? Then you do not even have to rely > on HEAD and HEAD~1 being different at file2. > These are all helpful suggestions, I'll include them in a re-roll (specifically: rename 'test_index_refreshed' to something mentioning 'reset', move the test comments inline with the steps they execute, and use 'diff-files' rather than 'read-tree'). Thanks!
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..005940778b7 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 $1 reset $2 --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_index_refreshed "-c reset.quiet=true" && + test_index_refreshed "-c reset.quiet=false" && + + # Command line option + ! test_index_refreshed "" --quiet && + test_index_refreshed "" --no-quiet && + + # Command line option overrides config setting + ! 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_index_refreshed "-c reset.refresh=false" && + + # Command line option + test_index_refreshed "" --refresh && + ! test_index_refreshed "" --no-refresh && + + # Command line option overrides config setting + test_index_refreshed "-c reset.refresh=false" --refresh && + ! 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 "" "--quiet --refresh" && + test_index_refreshed "-c reset.quiet=true" --refresh && + 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' '