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

From: Junio C Hamano <[email-masked]>
Date: Wed, 31 Aug 2016 12:01:31 -0700

Johannes Schindelin <[email-masked]> writes:

> The problem is that the `git-rebase-todo` most definitely does *not* want
> to be restricted to a single command.

Looking at the test in question, it is about what happens when "git
cherry-pick A..B" runs, and the test assumes it gets a sequence of
"pick".  It would be a bug to see any "revert" in there if you are
doing a multi cherry-pick.

And what is checked is if "cherry-pick --continue" notices a
malformed instruction that has something other than "pick".

If the final invocation were "sequencer --continue", then I
perfectly well understand why it is not a good idea to limit the set
of instructions only to "pick" (or "revert").  But I do not think
the test performed here is for that general case.  The user knows
Git stopped in the middle of cherry-pick and expects cherry-pick to
continue.

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

So if you think of this at even higher level, the check done in
parse_insn_line() that _assumes_ that opts->action must match the
actions on each line is _WRONG_, but what this test expects to see
is perfectly reasonable, I would think.

It is a different matter if it makes sense to _verify_ that the user
didn't make nonsensical change to the generated insn and error out
when s/he did.  I tend to think it is pointless, and I wouldn't object
if this test is removed due to that reason.