Re: [PATCH 4/6] revert: Allow mixed pick and revert instructions

[Jonathan Nieder <jrnieder@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
[...]
> @@ -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.