Re: [PATCH 4/6] revert: Allow mixed pick and revert instructions
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Aug 11, 2011, 20:12 UTC
- Message-ID
- <20110811201245.GH2277@elie.gateway.2wire.net>
- In-Reply-To
- <1313088705-32222-5-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Change the way the instruction parser works, allowing arbitrary > (action, operand) pairs to be parsed.
The first part of this sentence is not very satisfying. Maybe it means something like
Parse the instruction list in .git/sequencer/todo as a list of (action, operand) pairs, instead of assuming all instructions use the same action.
[...]
> This patch lays the foundation for extending the parser to support > more actions so 'git rebase -i' can reuse this machinery in the > future.
Exciting stuff. :)
[...]
> +++ b/builtin/revert.c
[...]
Show 7 quoted lines
> @@ -457,7 +456,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts) > return run_command_v_opt(args, RUN_GIT_CMD); > } > > -static int do_pick_commit(struct commit *commit, struct replay_opts *opts) > +static int do_pick_commit(struct commit *commit, enum replay_action action, > + struct replay_opts *opts)
[...]
Show 7 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.
[...]
> - return NULL;
> + return error(_("Unrecognized action: %s"), start);Probably should be mentioned in the commit message. Doesn't this print the problematic line and all lines after it?
Maybe something like
len = strchrnul(p, '\n') - p;
if (len > 255)
len = 255;
return error(_("Unrecognized action: %.*s"), (int) len, p);would do.
[...]
Show 16 quoted lines
> -static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
> - struct replay_opts *opts)
> +static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
> {
> - struct commit_list **next = todo_list;
> - struct commit *commit;
> + struct replay_insn_list **next = todo_list;
> + struct replay_insn_list item = {0, NULL, NULL};
> char *p = buf;
> int i;
>
> for (i = 1; *p; i++) {
> - commit = parse_insn_line(p, opts);
> - if (!commit)
> + if (parse_insn_line(p, &item) < 0)
> return error(_("Could not parse line %d."), i);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 fatal: unusable instruction sheet ".git/sequencer/todo" hint: to continue after fixing it, use "git cherry-pick --continue" hint: or to bail out, use "git cherry-pick --abort"
Does a "cherry-pick --continue" in this scenario skip the first commit in the todo list? Should it?
Show 7 quoted lines
> --- a/t/t3510-cherry-pick-sequence.sh > +++ b/t/t3510-cherry-pick-sequence.sh > @@ -240,4 +240,62 @@ test_expect_success 'missing commit descriptions in instruction sheet' ' > test_cmp expect actual > ' > > +test_expect_success 'revert --continue continues after cherry-pick' '
Haven't read the tests yet. The general idea of this patch still seems very sane.