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

[Glen Choo <chooglen@google.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

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

Ah, I see the difference. I think that's it's fair to assume that the
names of the files will be fairly stable, though this series has made it
clear to me that what each file does and when it is written is quite
under-documented, and I wouldn't be surprised to see some of that change
if we start to try to explain the inner workings to ourselves.

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

Yes, that would probably be a good idea. Maybe if we combined them into
a test helper that checks all of "pick", "squash" and "merge", which
also has the added benefit of being able to hide implementation details
in case we decide to change them.