Re: [PATCH 4/6] revert: Allow mixed pick and revert instructions
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- Aug 13, 2011, 16:07 UTC
- Message-ID
- <CALkWK0=1cVNrHMvMqrEhFYcdV8FkETk8qT1i1oGUJxsWEH9Zww@mail.gmail.com>
- In-Reply-To
- <20110811201245.GH2277@elie.gateway.2wire.net>
Hi,
Jonathan Nieder writes:
Show 7 quoted lines
> 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 :)
Show 19 quoted lines
>> @@ -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.
Show 8 quoted lines
> 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?
Show 6 quoted lines
> 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