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
[...]
> @@ -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)
[...]
> @@ -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.
[...]
> -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?
> --- 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html