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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 28, 2023, 13:19 UTC
Message-ID: <fd7ffcfd-288c-7f17-565d-7633ab507d51@gmail.com>
In-Reply-To: <kl6lfs5azhqd.fsf@chooglen-macbookpro.roam.corp.google.com>

Hi Glen

On 26/07/2023 18:48, Glen Choo wrote:

Show 20 quoted lines

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

That is certainly true

> and I wouldn't be surprised to see some of that change
> if we start to try to explain the inner workings to ourselves.

I agree. In the end I've removed the state file checks from the tests in favor of adding explicit checks that we refuse to commit staged changes and adding more cases to the test for the "post-rewrite" hook. I think it would probably be useful to add some assertions to the sequencer in a future series. We can assert things like "if this state file exists then so should these other ones" and "if this state file does not exist then these others should not" without relying on the logic in the sequencer. The sequencer assertions wouldn't know if the message file should exist but know what other files should exist if it does and the tests for committing staged changes then effectively check if the message file should exist.

Thanks for your comments on this series, I'll send a re-roll next week

Best Wishes

Phillip

Show 11 quoted lines

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