Hi Glen
On 26/07/2023 18:48, Glen Choo wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> 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
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.
