{"thread":{"id":"11837","subject":"[PATCH RESEND] Avoid a useless prefix lookup in strbuf_expand()","startedAt":"2008-02-02T11:09:53Z","lastAt":"2008-02-05T13:17:26Z","messageCount":5,"participants":["Marco Costalba","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"67117","messageId":"1201950593-6119-1-git-send-email-mcostalba@gmail.com","threadId":"11837","inReplyTo":null,"subject":"[PATCH RESEND] Avoid a useless prefix lookup in strbuf_expand()","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2008-02-02T11:09:53Z","receivedAt":"2008-02-02T11:09:53Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"Currently the --prett=format prefix is looked up in a\ntight loop in strbuf_expand(), if prefix is found is then\nused as argument for format_commit_item() that does another\nsearch by a switch statement to select the proper operation.\n\nBecause the switch statement is already able to discard\nunknown matches we don't need the prefix lookup before\nto call format_commit_item()\n\nThis patch removes an useless loop in a very fast path,\nused by, as example, by 'git log' with --pretty=format option\n\nSigned-off-by: Marco Costalba <mcostalba@gmail.com>\n---\n\nThis is the resend of an optimization patch rebased on\ncurrent next.\n\nI resend now that branch is open again.\n\n pretty.c |  202 +++++++++++++++++++++++++++----------------------------------\n strbuf.c |   19 +++----\n strbuf.h |    4 +-\n 3 files changed, 99 insertions(+), 126 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex b987ff2..64ead65 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -282,16 +282,18 @@ static char *logmsg_reencode(const struct commit *commit,\n \treturn out;\n }\n \n-static void format_person_part(struct strbuf *sb, char part,\n+/* returns placeholder length or 0 if placeholder is not known */\n+static size_t format_person_part(struct strbuf *sb, char part,\n                                const char *msg, int len)\n {\n-\tint start, end, tz = 0;\n-\tunsigned long date;\n+\tint start, end, tz = 0, end_of_data;\n+\tunsigned long date = 0;\n \tchar *ep;\n \n-\t/* parse name */\n+\t/* advance 'end' to point to email start delimiter */\n \tfor (end = 0; end < len && msg[end] != '<'; end++)\n \t\t; /* do nothing */\n+\n \t/*\n \t * If it does not even have a '<' and '>', that is\n \t * quite a bogus commit author and we discard it;\n@@ -301,65 +303,72 @@ static void format_person_part(struct strbuf *sb, char part,\n \t * which means start (beginning of email address) must\n \t * be strictly below len.\n \t */\n-\tstart = end + 1;\n-\tif (start >= len - 1)\n-\t\treturn;\n-\twhile (end > 0 && isspace(msg[end - 1]))\n-\t\tend--;\n+\tend_of_data = (end >= len - 1);\n+\n \tif (part == 'n') {\t/* name */\n-\t\tstrbuf_add(sb, msg, end);\n-\t\treturn;\n+\t\tif (!end_of_data) {\n+\t\t\twhile (end > 0 && isspace(msg[end - 1]))\n+\t\t\t\tend--;\n+\t\t\tstrbuf_add(sb, msg, end);\n+\t\t}\n+\t\treturn 2;\n \t}\n+\tstart = ++end; /* save email start position */\n \n-\t/* parse email */\n-\tfor (end = start; end < len && msg[end] != '>'; end++)\n+\t/* advance 'end' to point to email end delimiter */\n+\tfor ( ; end < len && msg[end] != '>'; end++)\n \t\t; /* do nothing */\n \n-\tif (end >= len)\n-\t\treturn;\n+\tend_of_data = (end >= len);\n \n \tif (part == 'e') {\t/* email */\n-\t\tstrbuf_add(sb, msg + start, end - start);\n-\t\treturn;\n+\t\tif (!end_of_data)\n+\t\t\tstrbuf_add(sb, msg + start, end - start);\n+\t\treturn 2;\n \t}\n-\n-\t/* parse date */\n+\t/* advance 'start' to point to date start delimiter */\n \tfor (start = end + 1; start < len && isspace(msg[start]); start++)\n \t\t; /* do nothing */\n-\tif (start >= len)\n-\t\treturn;\n-\tdate = strtoul(msg + start, &ep, 10);\n-\tif (msg + start == ep)\n-\t\treturn;\n+\n+\tend_of_data = (start >= len);\n+\n+\tif (!end_of_data)\n+\t\tdate = strtoul(msg + start, &ep, 10);\n \n \tif (part == 't') {\t/* date, UNIX timestamp */\n-\t\tstrbuf_add(sb, msg + start, ep - (msg + start));\n-\t\treturn;\n+\t\tif (!end_of_data)\n+\t\t\tstrbuf_add(sb, msg + start, ep - (msg + start));\n+\t\treturn 2;\n \t}\n+\tif (!end_of_data) {\t/* parse tz */\n+\t\tfor (start = ep - msg + 1; start < len && isspace(msg[start]); start++)\n+\t\t\t; /* do nothing */\n \n-\t/* parse tz */\n-\tfor (start = ep - msg + 1; start < len && isspace(msg[start]); start++)\n-\t\t; /* do nothing */\n-\tif (start + 1 < len) {\n-\t\ttz = strtoul(msg + start + 1, NULL, 10);\n-\t\tif (msg[start] == '-')\n-\t\t\ttz = -tz;\n+\t\tif (start + 1 < len) {\n+\t\t\ttz = strtoul(msg + start + 1, NULL, 10);\n+\t\t\tif (msg[start] == '-')\n+\t\t\t\ttz = -tz;\n+\t\t}\n \t}\n-\n \tswitch (part) {\n-\tcase 'd':\t/* date */\n-\t\tstrbuf_addstr(sb, show_date(date, tz, DATE_NORMAL));\n-\t\treturn;\n-\tcase 'D':\t/* date, RFC2822 style */\n-\t\tstrbuf_addstr(sb, show_date(date, tz, DATE_RFC2822));\n-\t\treturn;\n-\tcase 'r':\t/* date, relative */\n-\t\tstrbuf_addstr(sb, show_date(date, tz, DATE_RELATIVE));\n-\t\treturn;\n-\tcase 'i':\t/* date, ISO 8601 */\n-\t\tstrbuf_addstr(sb, show_date(date, tz, DATE_ISO8601));\n-\t\treturn;\n-\t}\n+\tcase 'd': /* date */\n+\t\tif (!end_of_data)\n+\t\t\tstrbuf_addstr(sb, show_date(date, tz, DATE_NORMAL));\n+\t\treturn 2;\n+\tcase 'D': /* date, RFC2822 style */\n+\t\tif (!end_of_data)\n+\t\t\tstrbuf_addstr(sb, show_date(date, tz, DATE_RFC2822));\n+\t\treturn 2;\n+\tcase 'r': /* date, relative */\n+\t\tif (!end_of_data)\n+\t\t\tstrbuf_addstr(sb, show_date(date, tz, DATE_RELATIVE));\n+\t\treturn 2;\n+\tcase 'i': /* date, ISO 8601 */\n+\t\tif (!end_of_data)\n+\t\t\tstrbuf_addstr(sb, show_date(date, tz, DATE_ISO8601));\n+\t\treturn 2;\n+\t}\n+\treturn 0; /* unknown placeholder */\n }\n \n struct chunk {\n@@ -440,7 +449,7 @@ static void parse_commit_header(struct format_commit_context *context)\n \tcontext->commit_header_parsed = 1;\n }\n \n-static void format_commit_item(struct strbuf *sb, const char *placeholder,\n+static size_t format_commit_item(struct strbuf *sb, const char *placeholder,\n                                void *context)\n {\n \tstruct format_commit_context *c = context;\n@@ -451,23 +460,23 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,\n \t/* these are independent of the commit */\n \tswitch (placeholder[0]) {\n \tcase 'C':\n-\t\tswitch (placeholder[3]) {\n-\t\tcase 'd':\t/* red */\n+\t\tif (!prefixcmp(placeholder + 1, \"red\")) {\n \t\t\tstrbuf_addstr(sb, \"\\033[31m\");\n-\t\t\treturn;\n-\t\tcase 'e':\t/* green */\n+\t\t\treturn 4;\n+\t\t} else if (!prefixcmp(placeholder + 1, \"green\")) {\n \t\t\tstrbuf_addstr(sb, \"\\033[32m\");\n-\t\t\treturn;\n-\t\tcase 'u':\t/* blue */\n+\t\t\treturn 6;\n+\t\t} else if (!prefixcmp(placeholder + 1, \"blue\")) {\n \t\t\tstrbuf_addstr(sb, \"\\033[34m\");\n-\t\t\treturn;\n-\t\tcase 's':\t/* reset color */\n+\t\t\treturn 5;\n+\t\t} else if (!prefixcmp(placeholder + 1, \"reset\")) {\n \t\t\tstrbuf_addstr(sb, \"\\033[m\");\n-\t\t\treturn;\n-\t\t}\n+\t\t\treturn 6;\n+\t\t} else\n+\t\t\treturn 0;\n \tcase 'n':\t\t/* newline */\n \t\tstrbuf_addch(sb, '\\n');\n-\t\treturn;\n+\t\treturn 1;\n \t}\n \n \t/* these depend on the commit */\n@@ -477,34 +486,34 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,\n \tswitch (placeholder[0]) {\n \tcase 'H':\t\t/* commit hash */\n \t\tstrbuf_addstr(sb, sha1_to_hex(commit->object.sha1));\n-\t\treturn;\n+\t\treturn 1;\n \tcase 'h':\t\t/* abbreviated commit hash */\n \t\tif (add_again(sb, &c->abbrev_commit_hash))\n-\t\t\treturn;\n+\t\t\treturn 1;\n \t\tstrbuf_addstr(sb, find_unique_abbrev(commit->object.sha1,\n \t\t                                     DEFAULT_ABBREV));\n \t\tc->abbrev_commit_hash.len = sb->len - c->abbrev_commit_hash.off;\n-\t\treturn;\n+\t\treturn 1;\n \tcase 'T':\t\t/* tree hash */\n \t\tstrbuf_addstr(sb, sha1_to_hex(commit->tree->object.sha1));\n-\t\treturn;\n+\t\treturn 1;\n \tcase 't':\t\t/* abbreviated tree hash */\n \t\tif (add_again(sb, &c->abbrev_tree_hash))\n-\t\t\treturn;\n+\t\t\treturn 1;\n \t\tstrbuf_addstr(sb, find_unique_abbrev(commit->tree->object.sha1,\n \t\t                                     DEFAULT_ABBREV));\n \t\tc->abbrev_tree_hash.len = sb->len - c->abbrev_tree_hash.off;\n-\t\treturn;\n+\t\treturn 1;\n \tcase 'P':\t\t/* parent hashes */\n \t\tfor (p = commit->parents; p; p = p->next) {\n \t\t\tif (p != commit->parents)\n \t\t\t\tstrbuf_addch(sb, ' ');\n \t\t\tstrbuf_addstr(sb, sha1_to_hex(p->item->object.sha1));\n \t\t}\n-\t\treturn;\n+\t\treturn 1;\n \tcase 'p':\t\t/* abbreviated parent hashes */\n \t\tif (add_again(sb, &c->abbrev_parent_hashes))\n-\t\t\treturn;\n+\t\t\treturn 1;\n \t\tfor (p = commit->parents; p; p = p->next) {\n \t\t\tif (p != commit->parents)\n \t\t\t\tstrbuf_addch(sb, ' ');\n@@ -513,14 +522,14 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,\n \t\t}\n \t\tc->abbrev_parent_hashes.len = sb->len -\n \t\t                              c->abbrev_parent_hashes.off;\n-\t\treturn;\n+\t\treturn 1;\n \tcase 'm':\t\t/* left/right/bottom */\n \t\tstrbuf_addch(sb, (commit->object.flags & BOUNDARY)\n \t\t                 ? '-'\n \t\t                 : (commit->object.flags & SYMMETRIC_LEFT)\n \t\t                 ? '<'\n \t\t                 : '>');\n-\t\treturn;\n+\t\treturn 1;\n \t}\n \n \t/* For the rest we have to parse the commit header. */\n@@ -528,66 +537,33 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,\n \t\tparse_commit_header(c);\n \n \tswitch (placeholder[0]) {\n-\tcase 's':\n+\tcase 's':\t/* subject */\n \t\tstrbuf_add(sb, msg + c->subject.off, c->subject.len);\n-\t\treturn;\n-\tcase 'a':\n-\t\tformat_person_part(sb, placeholder[1],\n+\t\treturn 1;\n+\tcase 'a':\t/* author ... */\n+\t\treturn format_person_part(sb, placeholder[1],\n \t\t                   msg + c->author.off, c->author.len);\n-\t\treturn;\n-\tcase 'c':\n-\t\tformat_person_part(sb, placeholder[1],\n+\tcase 'c':\t/* committer ... */\n+\t\treturn format_person_part(sb, placeholder[1],\n \t\t                   msg + c->committer.off, c->committer.len);\n-\t\treturn;\n-\tcase 'e':\n+\tcase 'e':\t/* encoding */\n \t\tstrbuf_add(sb, msg + c->encoding.off, c->encoding.len);\n-\t\treturn;\n-\tcase 'b':\n+\t\treturn 1;\n+\tcase 'b':\t/* body */\n \t\tstrbuf_addstr(sb, msg + c->body_off);\n-\t\treturn;\n+\t\treturn 1;\n \t}\n+\treturn 0;\t/* unknown placeholder */\n }\n \n void format_commit_message(const struct commit *commit,\n                            const void *format, struct strbuf *sb)\n {\n-\tconst char *placeholders[] = {\n-\t\t\"H\",\t\t/* commit hash */\n-\t\t\"h\",\t\t/* abbreviated commit hash */\n-\t\t\"T\",\t\t/* tree hash */\n-\t\t\"t\",\t\t/* abbreviated tree hash */\n-\t\t\"P\",\t\t/* parent hashes */\n-\t\t\"p\",\t\t/* abbreviated parent hashes */\n-\t\t\"an\",\t\t/* author name */\n-\t\t\"ae\",\t\t/* author email */\n-\t\t\"ad\",\t\t/* author date */\n-\t\t\"aD\",\t\t/* author date, RFC2822 style */\n-\t\t\"ar\",\t\t/* author date, relative */\n-\t\t\"at\",\t\t/* author date, UNIX timestamp */\n-\t\t\"ai\",\t\t/* author date, ISO 8601 */\n-\t\t\"cn\",\t\t/* committer name */\n-\t\t\"ce\",\t\t/* committer email */\n-\t\t\"cd\",\t\t/* committer date */\n-\t\t\"cD\",\t\t/* committer date, RFC2822 style */\n-\t\t\"cr\",\t\t/* committer date, relative */\n-\t\t\"ct\",\t\t/* committer date, UNIX timestamp */\n-\t\t\"ci\",\t\t/* committer date, ISO 8601 */\n-\t\t\"e\",\t\t/* encoding */\n-\t\t\"s\",\t\t/* subject */\n-\t\t\"b\",\t\t/* body */\n-\t\t\"Cred\",\t\t/* red */\n-\t\t\"Cgreen\",\t/* green */\n-\t\t\"Cblue\",\t/* blue */\n-\t\t\"Creset\",\t/* reset color */\n-\t\t\"n\",\t\t/* newline */\n-\t\t\"m\",\t\t/* left/right/bottom */\n-\t\tNULL\n-\t};\n \tstruct format_commit_context context;\n \n \tmemset(&context, 0, sizeof(context));\n \tcontext.commit = commit;\n-\tstrbuf_expand(sb, format, placeholders, format_commit_item, &context);\n+\tstrbuf_expand(sb, format, format_commit_item, &context);\n }\n \n static void pp_header(enum cmit_fmt fmt,\ndiff --git a/strbuf.c b/strbuf.c\nindex 5efcfc8..32ab8e5 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -146,11 +146,12 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)\n \tstrbuf_setlen(sb, sb->len + len);\n }\n \n-void strbuf_expand(struct strbuf *sb, const char *format,\n-                   const char **placeholders, expand_fn_t fn, void *context)\n+void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,\n+                   void *context)\n {\n \tfor (;;) {\n-\t\tconst char *percent, **p;\n+\t\tconst char *percent;\n+\t\tsize_t consumed;\n \n \t\tpercent = strchrnul(format, '%');\n \t\tstrbuf_add(sb, format, percent - format);\n@@ -158,14 +159,10 @@ void strbuf_expand(struct strbuf *sb, const char *format,\n \t\t\tbreak;\n \t\tformat = percent + 1;\n \n-\t\tfor (p = placeholders; *p; p++) {\n-\t\t\tif (!prefixcmp(format, *p))\n-\t\t\t\tbreak;\n-\t\t}\n-\t\tif (*p) {\n-\t\t\tfn(sb, *p, context);\n-\t\t\tformat += strlen(*p);\n-\t\t} else\n+\t\tconsumed = fn(sb, format, context);\n+\t\tif (consumed)\n+\t\t\tformat += consumed;\n+\t\telse\n \t\t\tstrbuf_addch(sb, '%');\n \t}\n }\ndiff --git a/strbuf.h b/strbuf.h\nindex 36d61db..faec229 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -103,8 +103,8 @@ static inline void strbuf_addbuf(struct strbuf *sb, struct strbuf *sb2) {\n }\n extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);\n \n-typedef void (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);\n-extern void strbuf_expand(struct strbuf *sb, const char *format, const char **placeholders, expand_fn_t fn, void *context);\n+typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);\n+extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, void *context);\n \n __attribute__((format(printf,2,3)))\n extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);\n-- \n1.5.4.rc4.39.g524a\n"},{"id":"67266","messageId":"7vsl09u2oz.fsf@gitster.siamese.dyndns.org","threadId":"11837","inReplyTo":"1201950593-6119-1-git-send-email-mcostalba@gmail.com","subject":"Re: [PATCH RESEND] Avoid a useless prefix lookup in strbuf_expand()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-03T21:53:32Z","receivedAt":"2008-02-03T21:53:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marco Costalba <mcostalba@gmail.com> writes:\n\n> diff --git a/pretty.c b/pretty.c\n> index b987ff2..64ead65 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -282,16 +282,18 @@ static char *logmsg_reencode(const struct commit *commit,\n>  \treturn out;\n>  }\n>  \n> -static void format_person_part(struct strbuf *sb, char part,\n> +/* returns placeholder length or 0 if placeholder is not known */\n\nThat \"return placeholder length\" is a bit confusing, and I suspect\nthe reason may be because the interface is misdesigned.\n\nThis function gets only a single character \"part\" and adds the\nmatching information to sb if found, otherwise it doesn't, so\nthe only possible return values are 0 or 2.\n\nWouldn't it be much cleaner if this returned a bool that says \"I\nfound and substituted that 'part' you asked me to handle\"?\n\n> +static size_t format_person_part(struct strbuf *sb, char part,\n>                                 const char *msg, int len)\n>  {\n> -\tint start, end, tz = 0;\n> -\tunsigned long date;\n> +\tint start, end, tz = 0, end_of_data;\n> +\tunsigned long date = 0;\n>  \tchar *ep;\n>  \n> -\t/* parse name */\n> +\t/* advance 'end' to point to email start delimiter */\n>  \tfor (end = 0; end < len && msg[end] != '<'; end++)\n>  \t\t; /* do nothing */\n> +\n>  \t/*\n>  \t * If it does not even have a '<' and '>', that is\n>  \t * quite a bogus commit author and we discard it;\n> @@ -301,65 +303,72 @@ static void format_person_part(struct strbuf *sb, char part,\n>  \t * which means start (beginning of email address) must\n>  \t * be strictly below len.\n>  \t */\n> -\tstart = end + 1;\n> -\tif (start >= len - 1)\n> -\t\treturn;\n> -\twhile (end > 0 && isspace(msg[end - 1]))\n> -\t\tend--;\n\nThe comment you can see in the context seems to refer to the\nlogic implemented by the part you are rewriting.  Don't you need\nto update it?  Also the ealier part of the same comment talks\nabout safety against a malformed input and explains the \"return;\"\nyou are removing here.  It is not clear where that logic has\ngone...\n\n> +\tend_of_data = (end >= len - 1);\n> +\n\nThe variable name \"end_of_data\" is unclear.  What does this\nboolean mean?  The line is without address and timestamp?\nThe item you are parsing is not properly terminated?\n\n>  \tif (part == 'n') {\t/* name */\n> -\t\tstrbuf_add(sb, msg, end);\n> -\t\treturn;\n> +\t\tif (!end_of_data) {\n> +\t\t\twhile (end > 0 && isspace(msg[end - 1]))\n> +\t\t\t\tend--;\n> +\t\t\tstrbuf_add(sb, msg, end);\n> +\t\t}\n> +\t\treturn 2;\n>  \t}\n> +\tstart = ++end; /* save email start position */\n\nWhat happens if end_of_data was already true in this case, I\nhave to wonder...  Language lawyers may point out that the\nresult of ++end would be undefined, which I do not personally\ncare about in this case, but this feels dirty if not wrong.\n\n> @@ -451,23 +460,23 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,\n>  \t/* these are independent of the commit */\n>  \tswitch (placeholder[0]) {\n>  \tcase 'C':\n> -\t\tswitch (placeholder[3]) {\n> -\t\tcase 'd':\t/* red */\n> +\t\tif (!prefixcmp(placeholder + 1, \"red\")) {\n>  \t\t\tstrbuf_addstr(sb, \"\\033[31m\");\n> -\t\t\treturn;\n> -\t\tcase 'e':\t/* green */\n> +\t\t\treturn 4;\n> +\t\t} else if (!prefixcmp(placeholder + 1, \"green\")) {\n>  \t\t\tstrbuf_addstr(sb, \"\\033[32m\");\n> -\t\t\treturn;\n> -\t\tcase 'u':\t/* blue */\n> +\t\t\treturn 6;\n> +\t\t} else if (!prefixcmp(placeholder + 1, \"blue\")) {\n>  \t\t\tstrbuf_addstr(sb, \"\\033[34m\");\n> -\t\t\treturn;\n> -\t\tcase 's':\t/* reset color */\n> +\t\t\treturn 5;\n> +\t\t} else if (!prefixcmp(placeholder + 1, \"reset\")) {\n>  \t\t\tstrbuf_addstr(sb, \"\\033[m\");\n> -\t\t\treturn;\n> -\t\t}\n> +\t\t\treturn 6;\n> +\t\t} else\n> +\t\t\treturn 0;\n\nWhile these look much cleaner than using the magic \"check the\nthird letter that happens to be unique\" hack, the return values\ncan easily go out-of-sync.  I'd suggest to have a static array\nof color names you support and iterate over it.\n\n> @@ -528,66 +537,33 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,\n>  ...\n>  void format_commit_message(const struct commit *commit,\n>                             const void *format, struct strbuf *sb)\n>  {\n> -\tconst char *placeholders[] = {\n> -\t\t\"H\",\t\t/* commit hash */\n> ...\n> -\t\t\"n\",\t\t/* newline */\n> -\t\t\"m\",\t\t/* left/right/bottom */\n> -\t\tNULL\n> -\t};\n>  \tstruct format_commit_context context;\n>  \n>  \tmemset(&context, 0, sizeof(context));\n>  \tcontext.commit = commit;\n> -\tstrbuf_expand(sb, format, placeholders, format_commit_item, &context);\n> +\tstrbuf_expand(sb, format, format_commit_item, &context);\n>  }\n\nThis is much nicer.  We reduced duplicated data from our code.\n"},{"id":"67268","messageId":"e5bfff550802031422q7ac72ee4ra53bb4152e34ea29@mail.gmail.com","threadId":"11837","inReplyTo":"7vsl09u2oz.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH RESEND] Avoid a useless prefix lookup in strbuf_expand()","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2008-02-03T22:22:36Z","receivedAt":"2008-02-03T22:22:36Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On Feb 3, 2008 10:53 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Marco Costalba <mcostalba@gmail.com> writes:\n>\n> >\n> > -static void format_person_part(struct strbuf *sb, char part,\n> > +/* returns placeholder length or 0 if placeholder is not known */\n>\n> That \"return placeholder length\" is a bit confusing, and I suspect\n> the reason may be because the interface is misdesigned.\n>\n> This function gets only a single character \"part\" and adds the\n> matching information to sb if found, otherwise it doesn't, so\n> the only possible return values are 0 or 2.\n>\n> Wouldn't it be much cleaner if this returned a bool that says \"I\n> found and substituted that 'part' you asked me to handle\"?\n>\n\nIt happens that _currently_ placeholders that start with 'a' or 'c'\nhave length of 2 chars, so return value can be only 0 or 2, but this\nis by accident, the return value is really the length of parsed\nplaceholder. Indeed if you see the return value of the caller\nformat_commit_item() is also the length of the parsed placeholder and\nso should be our format_person_part() whom return value is forwarded\nby caller:\n\nfrom format_commit_item()\n\n\tcase 'a':\t/* author ... */\n\t\treturn format_person_part(sb, placeholder[1],\n\t\t                   msg + c->author.off, c->author.len);\n\n\n\n> >       /*\n> >        * If it does not even have a '<' and '>', that is\n> >        * quite a bogus commit author and we discard it;\n> > @@ -301,65 +303,72 @@ static void format_person_part(struct strbuf *sb, char part,\n> >        * which means start (beginning of email address) must\n> >        * be strictly below len.\n> >        */\n> > -     start = end + 1;\n> > -     if (start >= len - 1)\n> > -             return;\n> > -     while (end > 0 && isspace(msg[end - 1]))\n> > -             end--;\n>\n> The comment you can see in the context seems to refer to the\n> logic implemented by the part you are rewriting.  Don't you need\n> to update it?\n\nWhy? if I had written\n\nend_of_data = (end > len - 1);\n\ninstead of\n\nend_of_data = (end >= len - 1);\n\nI agree with you comment would have been obsoleted but is not the case.\n\n>\n> > +     end_of_data = (end >= len - 1);\n> > +\n>\n> The variable name \"end_of_data\" is unclear.  What does this\n> boolean mean?  The line is without address and timestamp?\n> The item you are parsing is not properly terminated?\n>\n\nIt means \"we have nothing more to parse here\" and we _could_ return\nnow but we keep on because we need to know if the part placeholder is\nvalid or is unkown, so we really need to go through following switch\nstatement before to return with a correct return value.\n\n\n> >       if (part == 'n') {      /* name */\n> > -             strbuf_add(sb, msg, end);\n> > -             return;\n> > +             if (!end_of_data) {\n> > +                     while (end > 0 && isspace(msg[end - 1]))\n> > +                             end--;\n> > +                     strbuf_add(sb, msg, end);\n> > +             }\n> > +             return 2;\n> >       }\n> > +     start = ++end; /* save email start position */\n>\n> What happens if end_of_data was already true in this case, I\n> have to wonder...  Language lawyers may point out that the\n> result of ++end would be undefined, which I do not personally\n> care about in this case, but this feels dirty if not wrong.\n>\n\nCould be dirty, but is not wrong because when end_of_data flag as is\nset as is the case, any use of 'end' variable is skipped, only the\nfollowing swicth statement is evaluated with the only purpose to\ncompute a valid return value.\n\n\nThanks for your review\nMarco\n"},{"id":"67486","messageId":"7vtzknga5g.fsf@gitster.siamese.dyndns.org","threadId":"11837","inReplyTo":"1201950593-6119-1-git-send-email-mcostalba@gmail.com","subject":"Re: [PATCH RESEND] Avoid a useless prefix lookup in strbuf_expand()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-05T07:00:43Z","receivedAt":"2008-02-05T07:00:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marco Costalba <mcostalba@gmail.com> writes:\n\n> Currently the --prett=format prefix is looked up in a\n> ...\n> diff --git a/pretty.c b/pretty.c\n> index b987ff2..64ead65 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -282,16 +282,18 @@ static char *logmsg_reencode(const struct commit *commit,\n>  \treturn out;\n>  }\n>  \n> -static void format_person_part(struct strbuf *sb, char part,\n> +/* returns placeholder length or 0 if placeholder is not known */\n> +static size_t format_person_part(struct strbuf *sb, char part,\n>                                 const char *msg, int len)\n>  {\n> -\tint start, end, tz = 0;\n> -\tunsigned long date;\n> +\tint start, end, tz = 0, end_of_data;\n> +\tunsigned long date = 0;\n>  \tchar *ep;\n>  \n> -\t/* parse name */\n> +\t/* advance 'end' to point to email start delimiter */\n>  \tfor (end = 0; end < len && msg[end] != '<'; end++)\n>  \t\t; /* do nothing */\n> +\n\nAnother thing I noticed is that --pretty='format:%an %ae %at'\nwould end up running this function three times.  Perhaps it is\nworth memoizing the result in format_commit_context while you\nare at it?\n"},{"id":"67510","messageId":"e5bfff550802050517o3f4f5f61n8ed6ab2bce57d25f@mail.gmail.com","threadId":"11837","inReplyTo":"7vtzknga5g.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH RESEND] Avoid a useless prefix lookup in strbuf_expand()","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2008-02-05T13:17:26Z","receivedAt":"2008-02-05T13:17:26Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On Feb 5, 2008 8:00 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Another thing I noticed is that --pretty='format:%an %ae %at'\n> would end up running this function three times.  Perhaps it is\n> worth memoizing the result in format_commit_context while you\n> are at it?\n>\n\nYes, I tought about this. But at the end I rejected this idea to keep\nthe code simple and because the optimization was in any case small\n(and also in a special cases only).\n\nI did some profiling with and without my patch and I noticed a nice\nimprovement so that with the patch applied git-rev-list and git-log\n--pretty=format have almost the same execution times.\n\nThis is because the biggest time in both cases is spent in zlib\ndecompress, so also a local further speed up of  format_person_part()\ndoes not gain anything but complicates the code.\n\nMarco\n\nP.S: I will try to adress your concerns on the naming of end_of_data\nflag (hints are welcomed, I'm bad at naming) and on the skipping\nfurther elaborations if end_of_data is true to have cleaner code, I\nwas thinking about using 'goto xxxx' or something like that.\n"}]}