git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 5/8] revert: report fine-grained errors from insn parser

From
Ramkumar Ramachandra <artagnon@gmail.com>
Date
Jan 10, 2012, 16:13 UTC
Message-ID
<1326212039-13806-6-git-send-email-artagnon@gmail.com>
In-Reply-To
<1326212039-13806-1-git-send-email-artagnon@gmail.com>

The infrastructure that parses '.git/sequencer/todo' is meant to handle arbitrary user input some day, so it can be used as the implementation of 'git rebase --interactive' and 'git sequence --edit'. It is currently sub-optimal for that purpose because the parse error messages just say:

  error: Could not parse line 5.

This patch shifts responsibility to parse_insn_line(), which can come up with a more detailed message like:

  error: .git/sequencer/todo:5: unrecognized action: frobnicate

Once the operator is allowed to edit the sequence, the message might be adjusted to something like:

  error: <sequence you just gave me>:5: unrecognized action: frobnicate

instead of exposing an implementation detail. Some day "git sequence --edit" could even re-launch the editor with an error message in a comment before the problematic line and the cursor pointing there. For now, pointing to the explicit filename is useful since this should only happen if there was filesystem corruption, tampering, or a git bug.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   30 ++++++++++++++++++++++++------
 1 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 9a09471..0954d22 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -732,7 +732,22 @@ 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_error(const char *message, const char *file,
+		int lineno, char *error_line)
+{
+	const char *suffix = "";
+	int error_len = strcspn(error_line, " \t\n");
+
+	if (error_len > 20) {
+		error_len = 20;
+		suffix = "...";
+	}
+	return error(_("%s:%d: %s: %.*s%s"), file, lineno, message,
+		error_len, error_line, suffix);
+}
+
+static int parse_insn_line(char *bol, char *eol,
+			struct replay_insn_list *item, int lineno)
 {
 	unsigned char commit_sha1[20];
 	char *end_of_object_name;
@@ -745,7 +760,8 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
 		item->action = REPLAY_REVERT;
 		bol += strlen("revert ");
 	} else
-		return -1;
+		return parse_error(_("unrecognized action"),
+				git_path(SEQ_TODO_FILE), lineno, bol);
 
 	/* Eat up extra spaces/ tabs before object name */
 	bol += strspn(bol, " \t");
@@ -757,11 +773,13 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
 	*end_of_object_name = saved;
 
 	if (status < 0)
-		return -1;
+		return parse_error(_("malformed object name"),
+				git_path(SEQ_TODO_FILE), lineno, bol);
 
 	item->operand = lookup_commit_reference(commit_sha1);
 	if (!item->operand)
-		return -1;
+		return parse_error(_("not a valid commit"),
+				git_path(SEQ_TODO_FILE), lineno, bol);
 
 	item->next = NULL;
 	return 0;
@@ -776,8 +794,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))
-			return error(_("Could not parse line %d."), i);
+		if (parse_insn_line(p, eol, &item, i))
+			return -1;
 		next = replay_insn_list_append(item.operand, item.action, next);
 		p = *eol ? eol + 1 : eol;
 	}

-- 
1.7.8.2
Previous: Ramkumar RamachandraNext: Jonathan Nieder
Message 60 of 64 in “The move to sequencer.c”
  1. 0/6 The move to sequencer.cRamkumar Ramachandra, Jan 8, 2012
  2. 1/6 revert: move replay_action, replay_subcommand to headerRamkumar Ramachandra, Jan 8, 2012
  3. Jonathan NiederJan 8, 2012
  4. 2/6 revert: decouple sequencer actions from builtin commandsRamkumar Ramachandra, Jan 8, 2012
  5. Jonathan NiederJan 8, 2012
  6. Ramkumar RamachandraJan 8, 2012
  7. Jonathan NiederJan 8, 2012
  8. Ramkumar RamachandraJan 8, 2012
  9. Jonathan NiederJan 8, 2012
  10. 3/6 revert: don't let revert continue a cherry-pickRamkumar Ramachandra, Jan 8, 2012
  11. Jonathan NiederJan 8, 2012
  12. Ramkumar RamachandraJan 8, 2012
  13. Jonathan NiederJan 8, 2012
  14. Ramkumar RamachandraJan 8, 2012
  15. Jonathan NiederJan 8, 2012
  16. 4/6 revert: allow mixing "pick" and "revert" actionsRamkumar Ramachandra, Jan 8, 2012
  17. Jonathan NiederJan 8, 2012
  18. Ramkumar RamachandraJan 8, 2012
  19. Jonathan NiederJan 8, 2012
  20. Jonathan NiederJan 8, 2012
  21. Ramkumar RamachandraJan 10, 2012
  22. 5/6 revert: report fine-grained error messages from insn parserRamkumar Ramachandra, Jan 8, 2012
  23. Jonathan NiederJan 8, 2012
  24. Ramkumar RamachandraJan 8, 2012
  25. Jonathan NiederJan 8, 2012
  26. Ramkumar RamachandraJan 10, 2012
  27. 6/6 sequencer: factor code out of revert builtinRamkumar Ramachandra, Jan 8, 2012
  28. Jonathan NiederJan 8, 2012
  29. Ramkumar RamachandraJan 10, 2012
  30. Jonathan NiederJan 8, 2012
  31. Ramkumar RamachandraJan 8, 2012
  32. Jonathan NiederJan 8, 2012
  33. 0/8 The move to sequencer.cRamkumar Ramachandra, Jan 10, 2012
  34. 1/8 revert: prepare to move replay_action to headerRamkumar Ramachandra, Jan 10, 2012
  35. Jonathan NiederJan 10, 2012
  36. 2/8 revert: decouple sequencer actions from builtin commandsRamkumar Ramachandra, Jan 10, 2012
  37. Jonathan NiederJan 10, 2012
  38. Ramkumar RamachandraJan 11, 2012
  39. Ramkumar RamachandraJan 11, 2012
  40. Jonathan NiederJan 11, 2012
  41. Ramkumar RamachandraJan 11, 2012
  42. Ramkumar RamachandraJan 11, 2012
  43. Jonathan NiederJan 11, 2012
  44. Ramkumar RamachandraJan 11, 2012
  45. Jonathan NiederJan 11, 2012
  46. Ramkumar RamachandraJan 11, 2012
  47. Jonathan NiederJan 11, 2012
  48. Jonathan NiederJan 11, 2012
  49. Ramkumar RamachandraJan 11, 2012
  50. Jonathan NiederJan 11, 2012
  51. Ramkumar RamachandraJan 11, 2012
  52. 0/2 The move to sequencer.cRamkumar Ramachandra, Jan 11, 2012
  53. 1/2 revert: prepare to move replay_action to headerRamkumar Ramachandra, Jan 11, 2012
  54. 2/2 sequencer: factor code out of revert builtinRamkumar Ramachandra, Jan 11, 2012
  55. Jonathan NiederJan 11, 2012
  56. 3/8 revert: allow mixing "pick" and "revert" actionsRamkumar Ramachandra, Jan 10, 2012
  57. 4/8 revert: separate out parse errors logicallyRamkumar Ramachandra, Jan 10, 2012
  58. Jonathan NiederJan 10, 2012
  59. Ramkumar RamachandraJan 11, 2012
  60. 5/8 revert: report fine-grained errors from insn parserRamkumar Ramachandra, Jan 10, 2012
  61. Jonathan NiederJan 11, 2012
  62. 6/8 sha1_name: introduce getn_sha1() to take lengthRamkumar Ramachandra, Jan 10, 2012
  63. 7/8 revert: use getn_sha1() to simplify insn parsingRamkumar Ramachandra, Jan 10, 2012
  64. 8/8 sequencer: factor code out of revert builtinRamkumar Ramachandra, Jan 10, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.