From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Sat, 13 Aug 2011 16:07:51 GMT
Subject: Re: [PATCH 4/6] revert: Allow mixed pick and revert instructions
Message-ID: <CALkWK0=1cVNrHMvMqrEhFYcdV8FkETk8qT1i1oGUJxsWEH9Zww@mail.gmail.com>
In-Reply-To: <20110811201245.GH2277@elie.gateway.2wire.net>

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> 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 :)

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