Re: [PATCH 11/17] revert: Save data for continuing after conflict resolution

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> fatal: Malformed instruction sheet: .git/sequencer/todo
>> Technically speaking, this is correct.  However, this may not be ideal
>> from an end-user's perspective.  Anyway, this is going to change soon
>> -- do you think this is worth correcting here and now?
>
> Yes, thanks.  (A bird in the hand and all that.)

Okay, I thought about this for a while.  Either we can:
1. Assume that there'll never be a stray "revert" somewhere in the
middle of the instruction sheet when we invoke a "cherry-pick
--continue" and viceversa (ie. the instruction sheet is not corrupt/
hand-edited).  We can return a special exit status from the parser the
moment we encounter a "revert" during a "cherry-pick" assuming that
the entire sheet is filled with "revert" instructions, and die in
process_subcommand (now changed to pick_revisions) with a beautiful
message like "You're trying to resume a previous revert using
cherry-pick --continue, and we don't currently support this".
2. We can modify the API of the parser heavily to parse action +
commit instead of just commits.  Then, we can iterate through the list
and find/ report inconsistencies in process_subcommand.  In this case,
we can say "You're trying to resume a cherry-pick, but your
instruction sheet contains some revert instructions" or "You're trying
to resume a previous revert using cherry-pick --continue, and we don't
currently support this" as appropriate (after counting).  Although
this has a very beautiful end-user effect, I don't like the idea of
returning action + commit and counting.
3. Instead of just saying "fatal: Malformed instruction sheet" like we
do now, we can put in something a little more helpful like "error:
Cannot cherry-pick during a revert" before this message.  So:

$ git cherry-pick moo
... conflict ...
# resolve conflict
$ git revert --continue
error: Cannot cherry-pick during a revert
fatal: Malformed instruction sheet

This makes it clear to the user that the instruction sheet is to
blame, and in what way.  This is simple, and makes no assumptions
about whether or not the instruction sheet was hand-edited by the
user, so I like this.

Thoughts?

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
diff --git a/builtin/revert.c b/builtin/revert.c
index 0df724f..fcbf2a1 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -667,8 +667,13 @@ static struct commit *parse_insn_line(char
*start, struct replay_opts *opts)
 	 * Verify that the action matches up with the one in
 	 * opts; we don't support arbitrary instructions
 	 */
-	if (action != opts->action)
+	if (action != opts->action) {
+		const char *action_str;
+		action_str = action == REVERT ? "revert" : "cherry-pick";
+		error(_("Cannot %s during a %s"), action_str,
+			action_name(opts));
 		return NULL;
+	}

 	if ((get_sha1(sha1_abbrev, commit_sha1) < 0)
 		|| !(commit = lookup_commit_reference(commit_sha1)))