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