Re: [PATCH 2/5] reset: introduce --[no-]refresh option to --mixed
- From
Derrick Stolee <derrickstolee@github.com>
- Date
- Mar 14, 2022, 15:05 UTC
- Message-ID
- <fac1f57c-1bb8-2458-91ce-db5cb55f2e7d@github.com>
- In-Reply-To
- <bda93703013e3576101079d6aa4b821ae4c73fb7.1647043729.git.gitgitgadget@gmail.com>
On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:
Show 6 quoted lines
> 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()...
Show 14 quoted lines
> 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.
Show 38 quoted lines
> +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.
Show 6 quoted lines
> +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" "" &&
Show 5 quoted lines
> + 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