Hi Kuba, On Wed, 31 Aug 2016, Jakub Narębski wrote: > W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze: > > The sequencer was introduced to make the cherry-pick and revert > > functionality available as library function, with the original idea > > being to extend the sequencer to also implement the rebase -i > > functionality. > > > > The test to ensure that all of the commands in the script are identical > > to the overall operation does not mesh well with that. > > Actually the question is what does the test that got removed in this > commit actually check. Is it high-level sanity check that todo list > for git-cherry-pick contains only 'pick', and for git-revert contains > only 'revert'? It might have been that at some stage. But should we really check that? Or should we check the *effects*? I am of the opinion that overzealous checking of certain implementation details is something to be avoided. > > Therefore let's just get rid of the test that wants to verify that this > > limitation is still in place, in preparation for the upcoming work to > > teach the sequencer to do rebase -i's work. > > Is it "upcoming work" as in one of the patches in this series? > If so, which patch? I left this a little vague, didn't I? ;-) The problem is that the `git-rebase-todo` most definitely does *not* want to be restricted to a single command. So if you must have a patch that disagrees with this overzealous check, the "revamp todo parsing" one is probably the first. But it is better to think of this at a higher level than just patches: it is wrong to limit the todo script to contain only identical commands. > > diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh > > index 7b7a89d..6465edf 100755 > > --- a/t/t3510-cherry-pick-sequence.sh > > +++ b/t/t3510-cherry-pick-sequence.sh > > @@ -459,17 +459,6 @@ test_expect_success 'malformed instruction sheet 1' ' > > test_expect_code 128 git cherry-pick --continue > > ' > > > > -test_expect_success 'malformed instruction sheet 2' ' > > Hmmm... the description is somewhat lacking (especially compared to > the rest of test), anyway. > > BTW. we should probably rename 'malformed instruction sheet 2' > to 'malformed instruction sheet' if there are no further such > tests after this removal, isn't it? No, we cannot rename it after this patch because the patch removes it ;-) (It is not a file name but really a label for a test case.) Thanks for the review, Johannes