Derrick Stolee wrote:
> On 3/11/2022 7:08 PM, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <vdye@xxxxxxxxxx>
>>
>> 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
