Re: [PATCH 4/6] revert: Allow mixed pick and revert instructions
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:51:47
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. :) [...]
quoted hunk ↗ jump to hunk
+++ b/builtin/revert.c
[...]
quoted hunk ↗ jump to hunk
@@ -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)
[...]
quoted hunk ↗ jump to hunk
@@ -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.
[...]quoted hunk ↗ jump to hunk
-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?
quoted hunk ↗ jump to hunk
--- 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.