Re: [PATCH 4/6] revert: Allow mixed pick and revert instructions

Hi,

Jonathan Nieder writes:

Ramkumar Ramachandra wrote:

quoted

Change the way the instruction parser works, allowing arbitrary
(action, operand) pairs to be parsed.

       Parse the instruction list in .git/sequencer/todo as a list
       of (action, operand) pairs, instead of assuming all instructions
       use the same action.

Fixed.  Thanks :)

quoted

@@ -517,7 +517,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)

              /* TRANSLATORS: The first %s will be "revert" or
                 "cherry-pick", the second %s a SHA1 */
              return error(_("%s: cannot parse parent commit %s"),
-                     action_name(opts), sha1_to_hex(parent->object.sha1));
+                     action == REPLAY_REVERT ? "revert" : "cherry-pick",
+                     sha1_to_hex(parent->object.sha1));

My first thought was "why stop using the helper function action_name"?
But now I see that it previously came from "opts" (i.e., the command
line) and now comes from the todo file.

The command name there was never really important except when
cherry-pick or revert is being called by a script, and the message
indicates which command was having trouble parsing the commit.  If I
am using "git cherry-pick --continue" to continue after a failed
revert, I suspect action_name(opts) ["cherry-pick: "] would actually be
more sensible than the command name corresponding to the particular
pick/revert line.

You're right.  Removed this hunk.

Maybe something like

       len = strchrnul(p, '\n') - p;
       if (len > 255)
               len = 255;
       return error(_("Unrecognized action: %.*s"), (int) len, p);

would do.

Excellent idea!  I fixed the buffer overflow message to do this too.
By the way, shouldn't error() do this?

Could we can make this error message more clearly suggest that it's
giving context to the error above it?  For example, something vaguely
like

       error: unrecognized action: reset c78a78c9 Going back
       error: on line 7

Good suggestion.  Fixed.

Does a "cherry-pick --continue" in this scenario skip the first commit
in the todo list?  Should it?

It shouldn't and it doesn't.  See how read_populate_opts and
read_populate_todo are called before the segment that drops the first
commit.

-- Ram