Re: [PATCH v2 5/6] rebase: fix rewritten list for failed pick

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Glen

On 25/07/2023 17:46, Glen Choo wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>>>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>>>> index c1fe55dc2c1..a657167befd 100755
>>>> --- a/t/t3404-rebase-interactive.sh
>>>> +++ b/t/t3404-rebase-interactive.sh
>>>> @@ -1289,6 +1289,10 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
>>>>    	test_cmp_rev HEAD F &&
>>>>    	rm file6 &&
>>>>    	test_path_is_missing .git/rebase-merge/author-script &&
>>>> +	test_path_is_missing .git/rebase-merge/patch &&
>>>> +	test_path_is_missing .git/MERGE_MSG &&
>>>> +	test_path_is_missing .git/rebase-merge/message &&
>>>> +	test_path_is_missing .git/rebase-merge/stopped-sha &&
>>>
>>> This also seems to be testing implementation details, and if so, it
>>> would be worth removing them.
>>
>> With the exception of the "patch" file which exists solely for the
>> benefit of the user this is testing an invariant of the implementation
>> which isn't ideal. I'm worried that removing these checks will mask some
>> subtle regression in the future. I think it is unlikely that the names
>> of these files will change in the future as we try to avoid changes that
>> would cause a rebase to fail if git is upgraded while it has stopped for
>> the user to resolve conflicts. I did think about whether we could add
>> some BUG() statements to sequencer.c instead. Unfortunately I don't
>> think it is that easy for the sequencer to know when these files should
>> be missing without relying on the logic that we are tying to test.
> 
> Unfortunately, it's been a while since I reviewed this patch, so forgive
> me if I'm rusty. So you're saying that this test is about checking
> invariants that we want to preserve between Git versions.

Not really. One of the reasons why testing the implementation rather 
than the user observable behavior is a bad idea is that when the 
implementation is changed the test is likely to start failing or keep 
passing without checking anything useful. I was trying to say that in 
this case we're unlikely to change this aspect of the implementation 
because it would be tricky to do so without inconveniencing users who 
upgrade git while rebase is stopped for a conflict resolution and so it 
is unlikely that this test will be affected by future changes to the 
implementation.

> IIRC, there was an earlier patch would be different from an where we
> tested that author-script is missing, but what we really want is for the
> pick to stop. Is the same thing happening here? E.g. is 'testing for
> missing stopped-sha' a stand-in for 'testing that the rewritten list is
> correct'? If so, it would be nice to test that specifically, but if
> that's infeasible, a clarifying comment will probably suffice.

Yes this patch adds a test to t5407-post-rewrite-hook.sh to do that but 
it only checks a failing "pick" command. The reason I think it is useful 
to add these test_path_is_missing checks is that they are checking 
failing "squash" and "merge" commands as well. Maybe I should just bite 
the bullet see how tricky it is to extend the post-rewrite-hook test to 
cover those cases as well.

Best Wishes

Phillip