Re: [PATCH 08/22] sequencer: remove overzealous assumption

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 31, 2016, 18:36 UTC
Message-ID: <alpine.DEB.2.20.1608311706400.129229@virtualbox>
In-Reply-To: <09fd1436-301a-b0e1-c32a-81a47e4eb351@gmail.com>

Hi Kuba,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

Show 13 quoted lines

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

Show 6 quoted lines

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

Show 16 quoted lines

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