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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 26, 2023, 13:08 UTC
Message-ID: <a3526864-dd3a-f95c-72e6-44995a9a460f@gmail.com>
In-Reply-To: <kl6lo7k0ym57.fsf@chooglen-macbookpro.roam.corp.google.com>

Hi Glen

On 25/07/2023 17:46, Glen Choo wrote:

Show 31 quoted lines

> 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.

Show 6 quoted lines

> 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