Re: [PATCH 5/6] revert: report fine-grained error messages from insn parser

Hi,

Jonathan Nieder wrote:

Ramkumar Ramachandra wrote:

quoted

Three kinds of errors can arise from parsing '.git/sequencer/todo':
1. Unrecognized action
2. Malformed object name
3. Object name does not refer to a valid commit

Since we would like to make the instruction sheet user-editable in the
future (much like the 'rebase -i' sheet), report more fine-grained
parse errors prefixed with the filename and line number.

Seems like a sensible idea.  In other words,
[...]

Thanks for the new wording.

[...]

quoted

--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -714,26 +714,29 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)

      return 0;
 }

-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
+static int parse_insn_line(char *bol, char *eol,
+                     struct replay_insn_list *item, int lineno)
 {
+     const char *todo_file = git_path(SEQ_TODO_FILE);

This idiom is _still_ scary.  I don't want to have to shout about
this, but why didn't the commit message at least acknowledge it to put
the reader's mind at ease when this has come up again and again?

Carried over from the previous re-roll; sorry I didn't pay enough attention.

[...]

quoted

+             bol += strlen("revert ");
+     } else {
+             error_len = eol - bol > 255 ? 255 : eol - bol;
+             return error(_("%s:%d: Unrecognized action: %.*s"),
+                     todo_file, lineno, (int)error_len, bol);

Ah, so my example above was wrong: the actual message is

       error: .git/sequencer/todo:5: Unrecognized action: the quick bro
       wn fox jumps over the lazy dog

I guess that's fine.  Is it intended?

quoted

+     }

      /* Eat up extra spaces/ tabs before object name */
-     padding = strspn(bol, " \t");
-     if (!padding)
-             return -1;
-     bol += padding;
+     bol += strspn(bol, " \t");

What does this have to do with the stated goal of the patch?  It seems
to me that an unrelated and unadvertised bugfix snuck in.

Not a bugfix: since I have to report sensible error messages now, I
changed the "pick" and "revert" checks to "pick " || "pick\t" and
"revert " || "revert\t" -- then, I can report "invalid action" if it
doesn't match instead of a useless "missing space after action".

[...]
By the way, this is gross.  Probably get_sha1 should grow a variant
that takes a buffer and a length.

Yes; will do.

[...]

quoted

      item->operand = lookup_commit_reference(commit_sha1);
-     if (!item->operand)
-             return -1;
+     if (!item->operand) {
+             error_len = eol - bol > 255 ? 255 : eol - bol;
+             return error(_("%s:%d: Not a valid commit: %.*s"),
+                     todo_file, lineno, (int)error_len, bol);
+     }

Hmm, this one can be emitted even when there was no corruption or
internal error, because the user removed a commit she was
cherry-picking and the gc-ed before a "git cherry-pick --continue".
Alternatively, it can happen because the repository has grown very
crowded and what used to be an unambiguous commit name no longer is
one (not enough digits).  Will the error message be intuitive in these
situations?

Something like "Unable to look up commit: %s" perhaps?

[...]

quoted

@@ -761,8 +770,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)

      for (i = 1; *p; i++) {
              char *eol = strchrnul(p, '\n');
-             if (parse_insn_line(p, eol, &item) < 0)
-                     return error(_("Could not parse line %d."), i);
+             if (parse_insn_line(p, eol, &item, i) < 0)
+                     return -1;

Not related to this patch, but why the "< 0" test?  It makes this
reader worry that there is some unusual return value convention that
he should be taking into account.

Right.  Will fix.

Thanks.

-- Ram