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

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

From
Ramkumar Ramachandra <artagnon@gmail.com>
Date
Dec 6, 2011, 20:30 UTC
Message-ID
<CALkWK0nNtvrLHxQv17jfrFQ=BcwLfgh7eE9X-nDCCYY0nsOskg@mail.gmail.com>
In-Reply-To
<7vliqph8a6.fsf@alter.siamese.dyndns.org>
Hi Junio,
Junio C Hamano wrote:
Show 18 quoted lines
> A few comments and thoughts, all minor.
>
>  * On "revert: simplify getting commit subject in format_todo()"
>
>   The old code used to use get_message() on cur->item, checked its return
>   value (if cur->item is not parsed, or has already been used and its buffer
>   cleared, cur->item->buffer would be NULL and get_message() returns -1) and
>   issued an error. The new code uses find_commit_subject without first
>   checking if cur->item->buffer is NULL.
>
>   Does this mean the old code was overly defensive, or is the new code too
>   lax?
>
>   I understand that parse_insn_line() uses lookup_commit_reference() which
>   calls parse_object() on the object name (and if it is a tag, deref_tag()
>   will parse the tagged object until we see something that is not a tag), so
>   we know cur->item is parsed before we see it, so I suspect you merely were
>   being overly defensive, but I may be missing something.

Right. Actually, I was being overly defensive because I was being lazy about having to deal the an empty-commit-subject case in the parser. With "revert: make commit subjects in insn sheet optional", that's no more a concern- I'll reorder these two patches, and explain this detail in the commit message in the re-roll.

Show 7 quoted lines
>  * On "revert: make commit subjects in insn sheet optional"
>
>   After finding the verb and advancing the pointer "bol" at the beginning of
>   the object name, end_of_object_name variable points at the first SP or LF
>   using strcspn(bol, " \n"), but I wonder why we are not grabbing a run of
>   hexdigits instead, i.e. strspn(bol, "0123456789abcdef"). Is this so that
>   we can allow something like "pick rr/revert-cherry-pick~3"?
Yes, it is exactly for that reason :)
>   I also wonder if this should be (sorry for pcre) "(pick|revert)\s+(\S+)\s"
>   instead, i.e. allow users with fat fingers to use one or more SP or even HT
>   to separate the verb and the operand.

Hm, I'm not too enthusiastic about this change, because we don't advertise hand-editing the instruction sheet yet: it's just some extra parsing cruft along with guardian buffer-overflow code that buys us no immediate benefits.

>   The last test you added to 3510 in this patch runs test_line_count
>   unconditionally, by the way.

Good catch. Missing the chaining '&&' seems to be quite a common error: I vaguely remember seeing a patch to detect this sometime ago. Jonathan?

Show 5 quoted lines
>  * On "revert: allow mixed pick and revert instructions"
>
>   Reporting what we did not understand from parse_insn_line() is a good
>   idea, but I think the line number should be reported at the beginning
>   of the same line.
Makes sense.  Do you like this?
diff --git a/builtin/revert.c b/builtin/revert.c
index e447449..cfa770e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -715,7 +715,8 @@ 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)
 {
 	unsigned char commit_sha1[20];
 	char *end_of_object_name;
@@ -731,7 +732,8 @@ static int parse_insn_line(char *bol, char *eol,
struct replay_insn_list *item)
 		size_t len = strchrnul(bol, '\n') - bol;
 		if (len > 255)
 			len = 255;
-		return error(_("Unrecognized action: %.*s"), (int)len, bol);
+		return error(_("Line %d: Unrecognized action: %.*s"),
+			lineno, (int)len, bol);
 	}

 	end_of_object_name = bol + strcspn(bol, " \n");
@@ -741,11 +743,11 @@ static int parse_insn_line(char *bol, char *eol,
struct replay_insn_list *item)
 	*end_of_object_name = saved;

 	if (status < 0)
-		return error(_("Malformed object name: %s"), bol);
+		return error(_("Line %d: Malformed object name: %s"), lineno, bol);

 	item->operand = lookup_commit_reference(commit_sha1);
 	if (!item->operand)
-		return error(_("Not a valid commit: %s"), bol);
+		return error(_("Line %d: Not a valid commit: %s"), lineno, bol);

 	item->next = NULL;
 	return 0;
@@ -760,8 +762,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(_("on line %d."), i);
+		if (parse_insn_line(p, eol, &item, lineno) < 0)
+			return -1;
 		next = replay_insn_list_append(item.action, item.operand, next);
 		p = *eol ? eol + 1 : eol;
 	}
--

> I'd say that I'd prefer queuing re-rolled patches posted on the list; I'll
> discard the rr/revert-cherry-pick (6a156fd) from my tree.

Thanks for letting me know -- will post shortly.

-- Ram
Previous: Junio C HamanoNext: Jonathan Nieder
Message 4 of 45 in “What's cooking in git.git (Dec 2011, #02; Mon, 5)”
  1. Junio C HamanoDec 6, 2011
  2. Ramkumar RamachandraDec 6, 2011
  3. Junio C HamanoDec 6, 2011
  4. Ramkumar RamachandraDec 6, 2011
  5. &&-chaining testerJonathan Nieder, Dec 7, 2011
  6. Ramkumar RamachandraDec 7, 2011
  7. 01/15 t1013 (loose-object-format): fix && chainingRamkumar Ramachandra, Dec 7, 2011
  8. 02/15 t1300 (repo-config): fix && chainingRamkumar Ramachandra, Dec 7, 2011
  9. 03/15 t1412 (reflog-loop): fix && chainingRamkumar Ramachandra, Dec 7, 2011
  10. 04/15 t1007 (hash-object): fix && chainingRamkumar Ramachandra, Dec 7, 2011
  11. Jonathan NiederDec 7, 2011
  12. Ramkumar RamachandraDec 8, 2011
  13. 05/15 t1510 (repo-setup): fix && chainingRamkumar Ramachandra, Dec 7, 2011
  14. 06/15 t1511 (rev-parse-caret): fix && chainingRamkumar Ramachandra, Dec 7, 2011
  15. 07/15 t1510 (worktree): fix && chainingRamkumar Ramachandra, Dec 7, 2011
  16. Jonathan NiederDec 7, 2011
  17. Ramkumar RamachandraDec 8, 2011
  18. 08/15 t3200 (branch): fix && chainingRamkumar Ramachandra, Dec 7, 2011
  19. Jonathan NiederDec 7, 2011
  20. Ramkumar RamachandraDec 8, 2011
  21. 09/15 t3418 (rebase-continue): fix && chainingRamkumar Ramachandra, Dec 7, 2011
  22. 10/15 t3400 (rebase): fix && chainingRamkumar Ramachandra, Dec 7, 2011
  23. 11/15 t3310 (notes-merge-manual-resolve): fix && chainingRamkumar Ramachandra, Dec 7, 2011
  24. 12/15 t3419 (rebase-patch-id): fix && chainingRamkumar Ramachandra, Dec 7, 2011
  25. 13/15 t3030 (merge-recursive): use test_expect_codeRamkumar Ramachandra, Dec 7, 2011
  26. Jonathan NiederDec 7, 2011
  27. 14/15 t1006 (cat-file): use test_cmpRamkumar Ramachandra, Dec 7, 2011
  28. Jonathan NiederDec 7, 2011
  29. 15/15 t3040 (subprojects-basic): modernize styleRamkumar Ramachandra, Dec 7, 2011
  30. Jonathan NiederDec 7, 2011
  31. Ramkumar RamachandraDec 8, 2011
  32. Matthieu MoyDec 8, 2011
  33. Junio C HamanoDec 8, 2011
  34. Jeff KingDec 6, 2011
  35. Erik Faye-LundDec 6, 2011
  36. Jeff KingDec 6, 2011
  37. Erik Faye-LundDec 8, 2011
  38. Junio C HamanoDec 8, 2011
  39. Junio C HamanoDec 6, 2011
  40. Jeff KingDec 6, 2011
  41. Nguyen Thai Ngoc DuyDec 6, 2011
  42. Junio C HamanoDec 6, 2011
  43. Nguyen Thai Ngoc DuyDec 6, 2011
  44. Junio C HamanoDec 6, 2011
  45. Luke DiamandDec 6, 2011

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.