{"thread":{"id":"10745","subject":"[PATCH 2/2] --pretty=format: on-demand format expansion","startedAt":"2007-11-09T00:49:42Z","lastAt":"2007-11-11T10:29:12Z","messageCount":24,"participants":["René Scharfe","Johannes Schindelin","Jeff King","Junio C Hamano","Paul Mackerras"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"58961","messageId":"4733AEA6.1040802@lsrfire.ath.cx","threadId":"10745","inReplyTo":null,"subject":"[PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-09T00:49:42Z","receivedAt":"2007-11-09T00:49:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Some of the --pretty=format placeholders expansions are expensive to\ncalculate.  This is made worse by the current code's use of\ninterpolate(), which requires _all_ placeholders are to be prepared\nup front.\n\nOne way to speed this up is to check which placeholders are present\nin the format string and to prepare only the expansions that are\nneeded.  That still leaves the allocation overhead of interpolate().\n\nAnother way is to use a callback based approach together with the\nstrbuf library to keep allocations to a minimum and avoid string\ncopies.  That's what this patch does.  It introduces a new strbuf\nfunction, strbuf_expand().\n\nThe function takes a format string, list of placeholder strings,\na user supplied function 'fn', and an opaque pointer 'context'\nto tell 'fn' what thingy to operate on.\n\nThe function 'fn' is expected to accept a strbuf, a parsed\nplaceholder string and the 'context' pointer, and append the\ninterpolated value for the 'context' thingy, according to the\nformat specified by the placeholder.\n\nThanks to Pierre Habouzit for his suggestion to use strchrnul() and\nthe code surrounding its callsite.  And thanks to Junio for most of\nthis commit message. :)\n\nHere my measurements of most of Paul Mackerras' test cases that\nhighlighted the performance problem (best of three runs):\n    \n(master)\n$ time git log --pretty=oneline >/dev/null\n\nreal    0m0.390s\nuser    0m0.340s\nsys     0m0.040s\n\n(master)\n$ time git log --pretty=raw >/dev/null\n\nreal    0m0.434s\nuser    0m0.408s\nsys     0m0.016s\n\n(master)\n$ time git log --pretty=\"format:%H {%P} %ct\" >/dev/null\n\nreal    0m1.347s\nuser    0m0.080s\nsys     0m1.256s\n\n(interp_find_active -- Dscho)\n$ time ./git log --pretty=\"format:%H {%P} %ct\" >/dev/null\n\nreal    0m0.694s\nuser    0m0.020s\nsys     0m0.672s\n\n(strbuf_expand -- this patch)\n$ time ./git log --pretty=\"format:%H {%P} %ct\" >/dev/null\n\nreal    0m0.395s\nuser    0m0.352s\nsys     0m0.028s\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\nThis version fixes a bug in the first one, which would eat\npercent signs that were not followed by a placeholder from the\nlist.  That may be a valid thing to do for a brand-new parser,\nbut it regressed on what interpolate() did.\n\n strbuf.c |   24 ++++++\n strbuf.h |    3 +\n pretty.c |  276 ++++++++++++++++++++++++++++++++++----------------------------\n 3 files changed, 180 insertions(+), 123 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex f4201e1..536b432 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -129,6 +129,30 @@ 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+{\n+\tfor (;;) {\n+\t\tconst char *percent, **p;\n+\n+\t\tpercent = strchrnul(format, '%');\n+\t\tstrbuf_add(sb, format, percent - format);\n+\t\tif (!*percent)\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\t\tstrbuf_addch(sb, '%');\n+\t}\n+}\n+\n size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)\n {\n \tsize_t res;\ndiff --git a/strbuf.h b/strbuf.h\nindex cd7f295..21d8944 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -102,6 +102,9 @@ static inline void strbuf_addbuf(struct strbuf *sb, struct strbuf *sb2) {\n \tstrbuf_add(sb, sb2->buf, sb2->len);\n }\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+\n __attribute__((format(printf,2,3)))\n extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);\n \ndiff --git a/pretty.c b/pretty.c\nindex 490cede..9fbd73f 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1,6 +1,5 @@\n #include \"cache.h\"\n #include \"commit.h\"\n-#include \"interpolate.h\"\n #include \"utf8.h\"\n #include \"diff.h\"\n #include \"revision.h\"\n@@ -283,7 +282,8 @@ static char *logmsg_reencode(const struct commit *commit,\n \treturn out;\n }\n \n-static void fill_person(struct interp *table, const char *msg, int len)\n+static void 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@@ -295,7 +295,10 @@ static void fill_person(struct interp *table, const char *msg, int len)\n \tstart = end + 1;\n \twhile (end > 0 && isspace(msg[end - 1]))\n \t\tend--;\n-\ttable[0].value = xmemdupz(msg, end);\n+\tif (part == 'n') {\t/* name */\n+\t\tstrbuf_add(sb, msg, end);\n+\t\treturn;\n+\t}\n \n \tif (start >= len)\n \t\treturn;\n@@ -307,7 +310,10 @@ static void fill_person(struct interp *table, const char *msg, int len)\n \tif (end >= len)\n \t\treturn;\n \n-\ttable[1].value = xmemdupz(msg + start, end - start);\n+\tif (part == 'e') {\t/* email */\n+\t\tstrbuf_add(sb, msg + start, end - start);\n+\t\treturn;\n+\t}\n \n \t/* parse date */\n \tfor (start = end + 1; start < len && isspace(msg[start]); start++)\n@@ -318,7 +324,10 @@ static void fill_person(struct interp *table, const char *msg, int len)\n \tif (msg + start == ep)\n \t\treturn;\n \n-\ttable[5].value = xmemdupz(msg + start, ep - (msg + start));\n+\tif (part == 't') {\t/* date, UNIX timestamp */\n+\t\tstrbuf_add(sb, msg + start, ep - (msg + start));\n+\t\treturn;\n+\t}\n \n \t/* parse tz */\n \tfor (start = ep - msg + 1; start < len && isspace(msg[start]); start++)\n@@ -329,123 +338,107 @@ static void fill_person(struct interp *table, const char *msg, int len)\n \t\t\ttz = -tz;\n \t}\n \n-\tinterp_set_entry(table, 2, show_date(date, tz, DATE_NORMAL));\n-\tinterp_set_entry(table, 3, show_date(date, tz, DATE_RFC2822));\n-\tinterp_set_entry(table, 4, show_date(date, tz, DATE_RELATIVE));\n-\tinterp_set_entry(table, 6, show_date(date, tz, DATE_ISO8601));\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 }\n \n-void format_commit_message(const struct commit *commit,\n-                           const void *format, struct strbuf *sb)\n+static void format_commit_item(struct strbuf *sb, const char *placeholder,\n+                               void *context)\n {\n-\tstruct interp table[] = {\n-\t\t{ \"%H\" },\t/* commit hash */\n-\t\t{ \"%h\" },\t/* abbreviated commit hash */\n-\t\t{ \"%T\" },\t/* tree hash */\n-\t\t{ \"%t\" },\t/* abbreviated tree hash */\n-\t\t{ \"%P\" },\t/* parent hashes */\n-\t\t{ \"%p\" },\t/* abbreviated parent hashes */\n-\t\t{ \"%an\" },\t/* author name */\n-\t\t{ \"%ae\" },\t/* author email */\n-\t\t{ \"%ad\" },\t/* author date */\n-\t\t{ \"%aD\" },\t/* author date, RFC2822 style */\n-\t\t{ \"%ar\" },\t/* author date, relative */\n-\t\t{ \"%at\" },\t/* author date, UNIX timestamp */\n-\t\t{ \"%ai\" },\t/* author date, ISO 8601 */\n-\t\t{ \"%cn\" },\t/* committer name */\n-\t\t{ \"%ce\" },\t/* committer email */\n-\t\t{ \"%cd\" },\t/* committer date */\n-\t\t{ \"%cD\" },\t/* committer date, RFC2822 style */\n-\t\t{ \"%cr\" },\t/* committer date, relative */\n-\t\t{ \"%ct\" },\t/* committer date, UNIX timestamp */\n-\t\t{ \"%ci\" },\t/* committer date, ISO 8601 */\n-\t\t{ \"%e\" },\t/* encoding */\n-\t\t{ \"%s\" },\t/* subject */\n-\t\t{ \"%b\" },\t/* body */\n-\t\t{ \"%Cred\" },\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/* newline */\n-\t\t{ \"%m\" },\t/* left/right/bottom */\n-\t};\n-\tenum interp_index {\n-\t\tIHASH = 0, IHASH_ABBREV,\n-\t\tITREE, ITREE_ABBREV,\n-\t\tIPARENTS, IPARENTS_ABBREV,\n-\t\tIAUTHOR_NAME, IAUTHOR_EMAIL,\n-\t\tIAUTHOR_DATE, IAUTHOR_DATE_RFC2822, IAUTHOR_DATE_RELATIVE,\n-\t\tIAUTHOR_TIMESTAMP, IAUTHOR_ISO8601,\n-\t\tICOMMITTER_NAME, ICOMMITTER_EMAIL,\n-\t\tICOMMITTER_DATE, ICOMMITTER_DATE_RFC2822,\n-\t\tICOMMITTER_DATE_RELATIVE, ICOMMITTER_TIMESTAMP,\n-\t\tICOMMITTER_ISO8601,\n-\t\tIENCODING,\n-\t\tISUBJECT,\n-\t\tIBODY,\n-\t\tIRED, IGREEN, IBLUE, IRESET_COLOR,\n-\t\tINEWLINE,\n-\t\tILEFT_RIGHT,\n-\t};\n+\tconst struct commit *commit = context;\n \tstruct commit_list *p;\n-\tchar parents[1024];\n-\tunsigned long len;\n \tint i;\n \tenum { HEADER, SUBJECT, BODY } state;\n \tconst char *msg = commit->buffer;\n \n-\tif (ILEFT_RIGHT + 1 != ARRAY_SIZE(table))\n-\t\tdie(\"invalid interp table!\");\n-\n \t/* these are independent of the commit */\n-\tinterp_set_entry(table, IRED, \"\\033[31m\");\n-\tinterp_set_entry(table, IGREEN, \"\\033[32m\");\n-\tinterp_set_entry(table, IBLUE, \"\\033[34m\");\n-\tinterp_set_entry(table, IRESET_COLOR, \"\\033[m\");\n-\tinterp_set_entry(table, INEWLINE, \"\\n\");\n+\tswitch (placeholder[0]) {\n+\tcase 'C':\n+\t\tswitch (placeholder[3]) {\n+\t\tcase 'd':\t/* red */\n+\t\t\tstrbuf_addstr(sb, \"\\033[31m\");\n+\t\t\treturn;\n+\t\tcase 'e':\t/* green */\n+\t\t\tstrbuf_addstr(sb, \"\\033[32m\");\n+\t\t\treturn;\n+\t\tcase 'u':\t/* blue */\n+\t\t\tstrbuf_addstr(sb, \"\\033[34m\");\n+\t\t\treturn;\n+\t\tcase 's':\t/* reset color */\n+\t\t\tstrbuf_addstr(sb, \"\\033[m\");\n+\t\t\treturn;\n+\t\t}\n+\tcase 'n':\t\t/* newline */\n+\t\tstrbuf_addch(sb, '\\n');\n+\t\treturn;\n+\t}\n \n \t/* these depend on the commit */\n \tif (!commit->object.parsed)\n \t\tparse_object(commit->object.sha1);\n-\tinterp_set_entry(table, IHASH, sha1_to_hex(commit->object.sha1));\n-\tinterp_set_entry(table, IHASH_ABBREV,\n-\t\t\tfind_unique_abbrev(commit->object.sha1,\n-\t\t\t\tDEFAULT_ABBREV));\n-\tinterp_set_entry(table, ITREE, sha1_to_hex(commit->tree->object.sha1));\n-\tinterp_set_entry(table, ITREE_ABBREV,\n-\t\t\tfind_unique_abbrev(commit->tree->object.sha1,\n-\t\t\t\tDEFAULT_ABBREV));\n-\tinterp_set_entry(table, ILEFT_RIGHT,\n-\t\t\t (commit->object.flags & BOUNDARY)\n-\t\t\t ? \"-\"\n-\t\t\t : (commit->object.flags & SYMMETRIC_LEFT)\n-\t\t\t ? \"<\"\n-\t\t\t : \">\");\n-\n-\tparents[1] = 0;\n-\tfor (i = 0, p = commit->parents;\n-\t\t\tp && i < sizeof(parents) - 1;\n-\t\t\tp = p->next)\n-\t\ti += snprintf(parents + i, sizeof(parents) - i - 1, \" %s\",\n-\t\t\tsha1_to_hex(p->item->object.sha1));\n-\tinterp_set_entry(table, IPARENTS, parents + 1);\n-\n-\tparents[1] = 0;\n-\tfor (i = 0, p = commit->parents;\n-\t\t\tp && i < sizeof(parents) - 1;\n-\t\t\tp = p->next)\n-\t\ti += snprintf(parents + i, sizeof(parents) - i - 1, \" %s\",\n-\t\t\tfind_unique_abbrev(p->item->object.sha1,\n-\t\t\t\tDEFAULT_ABBREV));\n-\tinterp_set_entry(table, IPARENTS_ABBREV, parents + 1);\n \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+\tcase 'h':\t\t/* abbreviated commit hash */\n+\t\tstrbuf_addstr(sb, find_unique_abbrev(commit->object.sha1,\n+\t\t                                     DEFAULT_ABBREV));\n+\t\treturn;\n+\tcase 'T':\t\t/* tree hash */\n+\t\tstrbuf_addstr(sb, sha1_to_hex(commit->tree->object.sha1));\n+\t\treturn;\n+\tcase 't':\t\t/* abbreviated tree hash */\n+\t\tstrbuf_addstr(sb, find_unique_abbrev(commit->tree->object.sha1,\n+\t\t                                     DEFAULT_ABBREV));\n+\t\treturn;\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+\tcase 'p':\t\t/* abbreviated 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, find_unique_abbrev(\n+\t\t\t\t\tp->item->object.sha1, DEFAULT_ABBREV));\n+\t\t}\n+\t\treturn;\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}\n+\n+\t/* For the rest we have to parse the commit header. */\n \tfor (i = 0, state = HEADER; msg[i] && state < BODY; i++) {\n \t\tint eol;\n \t\tfor (eol = i; msg[eol] && msg[eol] != '\\n'; eol++)\n \t\t\t; /* do nothing */\n \n \t\tif (state == SUBJECT) {\n-\t\t\ttable[ISUBJECT].value = xmemdupz(msg + i, eol - i);\n+\t\t\tif (placeholder[0] == 's') {\n+\t\t\t\tstrbuf_add(sb, msg + i, eol - i);\n+\t\t\t\treturn;\n+\t\t\t}\n \t\t\ti = eol;\n \t\t}\n \t\tif (i == eol) {\n@@ -453,29 +446,66 @@ void format_commit_message(const struct commit *commit,\n \t\t\t/* strip empty lines */\n \t\t\twhile (msg[eol + 1] == '\\n')\n \t\t\t\teol++;\n-\t\t} else if (!prefixcmp(msg + i, \"author \"))\n-\t\t\tfill_person(table + IAUTHOR_NAME,\n-\t\t\t\t\tmsg + i + 7, eol - i - 7);\n-\t\telse if (!prefixcmp(msg + i, \"committer \"))\n-\t\t\tfill_person(table + ICOMMITTER_NAME,\n-\t\t\t\t\tmsg + i + 10, eol - i - 10);\n-\t\telse if (!prefixcmp(msg + i, \"encoding \"))\n-\t\t\ttable[IENCODING].value =\n-\t\t\t\txmemdupz(msg + i + 9, eol - i - 9);\n+\t\t} else if (!prefixcmp(msg + i, \"author \")) {\n+\t\t\tif (placeholder[0] == 'a') {\n+\t\t\t\tformat_person_part(sb, placeholder[1],\n+\t\t\t\t                   msg + i + 7, eol - i - 7);\n+\t\t\t\treturn;\n+\t\t\t}\n+\t\t} else if (!prefixcmp(msg + i, \"committer \")) {\n+\t\t\tif (placeholder[0] == 'c') {\n+\t\t\t\tformat_person_part(sb, placeholder[1],\n+\t\t\t\t                   msg + i + 10, eol - i - 10);\n+\t\t\t\treturn;\n+\t\t\t}\n+\t\t} else if (!prefixcmp(msg + i, \"encoding \")) {\n+\t\t\tif (placeholder[0] == 'e') {\n+\t\t\t\tstrbuf_add(sb, msg + i + 9, eol - i - 9);\n+\t\t\t\treturn;\n+\t\t\t}\n+\t\t}\n \t\ti = eol;\n \t}\n-\tif (msg[i])\n-\t\ttable[IBODY].value = xstrdup(msg + i);\n-\n-\tlen = interpolate(sb->buf + sb->len, strbuf_avail(sb),\n-\t\t\t\tformat, table, ARRAY_SIZE(table));\n-\tif (len > strbuf_avail(sb)) {\n-\t\tstrbuf_grow(sb, len);\n-\t\tinterpolate(sb->buf + sb->len, strbuf_avail(sb) + 1,\n-\t\t\t\t\tformat, table, ARRAY_SIZE(table));\n-\t}\n-\tstrbuf_setlen(sb, sb->len + len);\n-\tinterp_clear_table(table, ARRAY_SIZE(table));\n+\tif (msg[i] && placeholder[0] == 'b')\t/* body */\n+\t\tstrbuf_addstr(sb, msg + i);\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+\tstrbuf_expand(sb, format, placeholders, format_commit_item, (void *)commit);\n }\n \n static void pp_header(enum cmit_fmt fmt,\n \n"},{"id":"58970","messageId":"Pine.LNX.4.64.0711090122470.4362@racer.site","threadId":"10745","inReplyTo":"4733AEA6.1040802@lsrfire.ath.cx","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-09T01:24:00Z","receivedAt":"2007-11-09T01:24:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 9 Nov 2007, Ren? Scharfe wrote:\n\n>  strbuf.c |   24 ++++++\n>  strbuf.h |    3 +\n>  pretty.c |  276 ++++++++++++++++++++++++++++++++++----------------------------\n\nI would be so grateful if you could (trivially) split up this patch into \nthe addition of strbuf_expend() (with a small example in the commit \nmessage), and a patch that uses it in pretty.c.\n\nThanks,\nDscho\n"},{"id":"58991","messageId":"20071109045040.GC31760@sigill.intra.peff.net","threadId":"10745","inReplyTo":"4733AEA6.1040802@lsrfire.ath.cx","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-09T04:50:40Z","receivedAt":"2007-11-09T04:50:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 09, 2007 at 01:49:42AM +0100, René Scharfe wrote:\n\n> Another way is to use a callback based approach together with the\n> strbuf library to keep allocations to a minimum and avoid string\n> copies.  That's what this patch does.  It introduces a new strbuf\n> function, strbuf_expand().\n\nI think this is definitely the right approach, but there are some format\nstrings where the performance will be worse. Specifically:\n\n  - formatting expensive items multiple times will incur work\n    proportional to the number of times the item is used (in the old\n    code, it was calculated just once).  e.g., \"%h%h%h%h\"\n\n  - formatting some items goes to some work that can be re-used by other\n    items (e.g., %ad and %ar both need to parse the author date)\n\nAnd we could obviously overcome both by caching the results of expensive\noperations.  I'm not sure if these will be a problem in practice. For\nthe first one, the new code is so much faster that I needed to do\n\n  git-log --pretty=format:%h%h%h%h%h%h%h%h\n\nto get a performance regression from the old code, which seems rather\nunlikely. For the second, it is easy to imagine multiple \"person\" items\nbeing used together, although their cost to produce is not all that\nhigh. It looks like about .05 seconds to parse a date (over all commits\nin git.git):\n\n$ time ./git-log --pretty='format:' >/dev/null\nreal    0m0.441s\nuser    0m0.424s\nsys     0m0.004s\n\n$ time ./git-log --pretty='format:%ad' >/dev/null\nreal    0m0.477s\nuser    0m0.472s\nsys     0m0.000s\n\n$ time ./git-log --pretty='format:%ad %aD' >/dev/null\nreal    0m0.527s\nuser    0m0.520s\nsys     0m0.004s\n\nwhere the last two could probably end up costing about the same if we cached\nthe author parsing (but the caching will have a cost, too, so it might not end\nup being a big win).\n\nSo it might make sense to cache some items as we figure them out. This\nshould be done by the calling code and not by strbuf_expand (since it\ndoesn't know which things are worth caching (and for fast things,\nallocating memory for a cache entry is likely to be slower), or how the\nexpansions relate to each other).\n\nA partial patch on top of yours is below (it caches commit and tree\nabbreviations; parent abbreviations and person-parsing are probably\nworth doing). Some timings:\n\nNull format (these are average-looking runs; the differences got lost\nin the noise):\n\n# your patch\n$ time git-log --pretty=format: >/dev/null\nreal    0m0.409s\nuser    0m0.384s\nsys     0m0.012s\n\n# with my patch\n$ time ./git-log --pretty=format: >/dev/null\nreal    0m0.413s\nuser    0m0.404s\nsys     0m0.004s\n\nSingle abbrev lookup (mine should be slightly slower because of\nmalloc/free of cache):\n\n# your patch\n$ time git-log --pretty=format:%h >/dev/null\nreal    0m0.536s\nuser    0m0.456s\nsys     0m0.080s\n\n# with my patch\n$ time ./git-log --pretty=format:%h >/dev/null\nreal    0m0.553s\nuser    0m0.464s\nsys     0m0.088s\n\nTwo abbrev lookups (I win by a little bit, but definitely not lost in\nthe noise):\n\n# your patch\n$ time git-log --pretty=format:%h%h >/dev/null\nreal    0m0.671s\nuser    0m0.496s\nsys     0m0.144s\n\n# my patch\n$ time ./git-log  --pretty=format:%h%h >/dev/null\nreal    0m0.567s\nuser    0m0.480s\nsys     0m0.080s\n\nAnd of course I can make pathological cases where mine wins hands down\n(on \"%h%h%h%h%h%h%h%h\", mine stays the same but yours bumps to 1.2s).\n\nSo I think this is probably worth doing. Even if doubled work isn't the\ncommon case,\n  1. It doesn't hurt the common case much at all (I think on average it\n     is slower, but the timings were totally lost in the noise)\n  2. It has a measurable impact on reasonable cases (like just using an\n     expensive substitution twice)\n  3. It has a huge impact on pathological cases (though I'm not sure we\n     care about those that much)\n  4. It's very little extra code, and it should be obvious to read. It\n     also documents the technique for other users of strbuf_expand,\n     where the \"doubled\" cases may be more common.\n\n-Peff\n\n---\n pretty.c |   47 ++++++++++++++++++++++++++++++++++++++++-------\n 1 files changed, 40 insertions(+), 7 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 9fbd73f..8ae6fdd 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -282,6 +282,27 @@ static char *logmsg_reencode(const struct commit *commit,\n \treturn out;\n }\n \n+struct pretty_context {\n+\tconst struct commit *commit;\n+\tchar *abbrev_commit;\n+\tchar *abbrev_tree;\n+};\n+\n+static void pretty_context_init(struct pretty_context *p, const struct commit *c)\n+{\n+\tp->commit = c;\n+\tp->abbrev_commit = NULL;\n+\tp->abbrev_tree = NULL;\n+}\n+\n+static void pretty_context_free(struct pretty_context *p)\n+{\n+\tif (p->abbrev_commit)\n+\t\tfree(p->abbrev_commit);\n+\tif (p->abbrev_tree)\n+\t\tfree(p->abbrev_tree);\n+}\n+\n static void format_person_part(struct strbuf *sb, char part,\n                                const char *msg, int len)\n {\n@@ -355,9 +376,10 @@ static void format_person_part(struct strbuf *sb, char part,\n }\n \n static void format_commit_item(struct strbuf *sb, const char *placeholder,\n-                               void *context)\n+                               void *vcontext)\n {\n-\tconst struct commit *commit = context;\n+\tstruct pretty_context *context = vcontext;\n+\tconst struct commit *commit = context->commit;\n \tstruct commit_list *p;\n \tint i;\n \tenum { HEADER, SUBJECT, BODY } state;\n@@ -394,15 +416,23 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,\n \t\tstrbuf_addstr(sb, sha1_to_hex(commit->object.sha1));\n \t\treturn;\n \tcase 'h':\t\t/* abbreviated commit hash */\n-\t\tstrbuf_addstr(sb, find_unique_abbrev(commit->object.sha1,\n-\t\t                                     DEFAULT_ABBREV));\n+\t\tif (!context->abbrev_commit)\n+\t\t\tcontext->abbrev_commit = xstrdup(\n+\t\t\t\t\tfind_unique_abbrev(\n+\t\t\t\t\t\tcommit->object.sha1,\n+\t\t\t\t\t\tDEFAULT_ABBREV));\n+\t\tstrbuf_addstr(sb, context->abbrev_commit);\n \t\treturn;\n \tcase 'T':\t\t/* tree hash */\n \t\tstrbuf_addstr(sb, sha1_to_hex(commit->tree->object.sha1));\n \t\treturn;\n \tcase 't':\t\t/* abbreviated tree hash */\n-\t\tstrbuf_addstr(sb, find_unique_abbrev(commit->tree->object.sha1,\n-\t\t                                     DEFAULT_ABBREV));\n+\t\tif (!context->abbrev_tree)\n+\t\t\tcontext->abbrev_tree = xstrdup(\n+\t\t\t\t\tfind_unique_abbrev(\n+\t\t\t\t\tcommit->tree->object.sha1,\n+\t\t\t\t\tDEFAULT_ABBREV));\n+\t\tstrbuf_addstr(sb, context->abbrev_tree);\n \t\treturn;\n \tcase 'P':\t\t/* parent hashes */\n \t\tfor (p = commit->parents; p; p = p->next) {\n@@ -505,7 +535,10 @@ void format_commit_message(const struct commit *commit,\n \t\t\"m\",\t\t/* left/right/bottom */\n \t\tNULL\n \t};\n-\tstrbuf_expand(sb, format, placeholders, format_commit_item, (void *)commit);\n+\tstruct pretty_context context;\n+\tpretty_context_init(&context, commit);\n+\tstrbuf_expand(sb, format, placeholders, format_commit_item, &context);\n+\tpretty_context_free(&context);\n }\n \n static void pp_header(enum cmit_fmt fmt,\n"},{"id":"58992","messageId":"20071109045256.GD31760@sigill.intra.peff.net","threadId":"10745","inReplyTo":"4733AEA6.1040802@lsrfire.ath.cx","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-09T04:52:56Z","receivedAt":"2007-11-09T04:52:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 09, 2007 at 01:49:42AM +0100, René Scharfe wrote:\n\n> +\tstrbuf_expand(sb, format, placeholders, format_commit_item, (void *)commit);\n\nThis void cast is pointless, since all pointers types convert implicitly\nto void pointers anyway. At best, it does nothing, and at worst, it\nhides an actual type error if the function signature or the type of\n'commit' change.\n\n(In the patch I just sent out, I had to change this line anyway, and\nremoved the cast).\n\n-Peff\n"},{"id":"59124","messageId":"4734CD78.4000704@lsrfire.ath.cx","threadId":"10745","inReplyTo":"Pine.LNX.4.64.0711090122470.4362@racer.site","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-09T21:13:28Z","receivedAt":"2007-11-09T21:13:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Johannes Schindelin schrieb:\n> Hi,\n> \n> On Fri, 9 Nov 2007, Ren? Scharfe wrote:\n> \n>>  strbuf.c |   24 ++++++\n>>  strbuf.h |    3 +\n>>  pretty.c |  276 ++++++++++++++++++++++++++++++++++----------------------------\n> \n> I would be so grateful if you could (trivially) split up this patch into \n> the addition of strbuf_expend() (with a small example in the commit \n> message), and a patch that uses it in pretty.c.\n\nMakes sense.  Will do next time.\n\nThanks,\nRené\n"},{"id":"59128","messageId":"Pine.LNX.4.64.0711092218150.4362@racer.site","threadId":"10745","inReplyTo":"4734CD78.4000704@lsrfire.ath.cx","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-09T22:18:54Z","receivedAt":"2007-11-09T22:18:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 9 Nov 2007, Ren? Scharfe wrote:\n\n> Johannes Schindelin schrieb:\n> > Hi,\n> > \n> > On Fri, 9 Nov 2007, Ren? Scharfe wrote:\n> > \n> >>  strbuf.c |   24 ++++++\n> >>  strbuf.h |    3 +\n> >>  pretty.c |  276 ++++++++++++++++++++++++++++++++++----------------------------\n> > \n> > I would be so grateful if you could (trivially) split up this patch into \n> > the addition of strbuf_expend() (with a small example in the commit \n> > message), and a patch that uses it in pretty.c.\n> \n> Makes sense.  Will do next time.\n\nYou mean next time you write strbuf_expand()?\n\n;-)  I saw that Junio already applied your patch as is.  Hmm.\n\nCiao,\nDscho\n"},{"id":"59132","messageId":"7vr6izqcg8.fsf@gitster.siamese.dyndns.org","threadId":"10745","inReplyTo":"Pine.LNX.4.64.0711092218150.4362@racer.site","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-09T22:30:15Z","receivedAt":"2007-11-09T22:30:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> You mean next time you write strbuf_expand()?\n>\n> ;-)  I saw that Junio already applied your patch as is.  Hmm.\n\nWhich only means we expect incremental improvements on top of it\nfrom now on, it does not mean \"it is perfect and cast in stone\".\n"},{"id":"59136","messageId":"4734EA4E.8070405@lsrfire.ath.cx","threadId":"10745","inReplyTo":"20071109045040.GC31760@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-09T23:16:30Z","receivedAt":"2007-11-09T23:16:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Jeff King schrieb:\n> - formatting expensive items multiple times will incur work \n> proportional to the number of times the item is used (in the old \n> code, it was calculated just once).  e.g., \"%h%h%h%h\"\n> \n> - formatting some items goes to some work that can be re-used by\n> other items (e.g., %ad and %ar both need to parse the author date)\n> \n> And we could obviously overcome both by caching the results of\n> expensive operations.  I'm not sure if these will be a problem in\n> practice. For the first one, the new code is so much faster that I\n> needed to do\n> \n> git-log --pretty=format:%h%h%h%h%h%h%h%h\n> \n> to get a performance regression from the old code, which seems rather\n>  unlikely. For the second, it is easy to imagine multiple \"person\"\n> items being used together, although their cost to produce is not all\n> that high. It looks like about .05 seconds to parse a date (over all\n> commits in git.git):\n> \n> $ time ./git-log --pretty='format:' >/dev/null real    0m0.441s user\n> 0m0.424s sys     0m0.004s\n> \n> $ time ./git-log --pretty='format:%ad' >/dev/null real    0m0.477s \n> user    0m0.472s sys     0m0.000s\n> \n> $ time ./git-log --pretty='format:%ad %aD' >/dev/null real\n> 0m0.527s user    0m0.520s sys     0m0.004s\n> \n> where the last two could probably end up costing about the same if we\n> cached the author parsing (but the caching will have a cost, too, so\n> it might not end up being a big win).\n> \n> So it might make sense to cache some items as we figure them out.\n> This should be done by the calling code and not by strbuf_expand\n> (since it doesn't know which things are worth caching (and for fast\n> things, allocating memory for a cache entry is likely to be slower),\n> or how the expansions relate to each other).\n\nI fully agree with the above.\n\n> A partial patch on top of yours is below (it caches commit and tree \n> abbreviations; parent abbreviations and person-parsing are probably \n> worth doing). Some timings:\n\n... but I object to the choice of items to cache.  Are there real-world\nformats containing the same placeholder twice or even more often?\n\nThere is probably more to gain from the interdependencies of different\nplaceholders.  The patch below attempts to avoid parsing the commit\ntwice, by saving pointers to the different parts.\n\n(next)\n$ time git log --pretty=format:\"* %cd %cn%n%n%s%n%b\" >/dev/null\n\nreal    0m0.631s\nuser    0m0.584s\nsys     0m0.040s\n\n(next+patch)\n$ time git log --pretty=format:\"* %cd %cn%n%n%s%n%b\" >/dev/null\n\nreal    0m0.570s\nuser    0m0.512s\nsys     0m0.044s\n\nThe format is from http://svn.tue.mpg.de/tentakel/trunk/tentakel/Makefile\nwhich I found while googling for real-world use cases.\n\nWhile the code size is increased by the patch, most of the code gets\nsimplified.\n\n\n pretty.c |  128 ++++++++++++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 86 insertions(+), 42 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 9fbd73f..b2f10a3 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -354,14 +354,69 @@ static void format_person_part(struct strbuf *sb, char part,\n \t}\n }\n \n+struct parsed_commit_header {\n+\tint parsed;\n+\tconst char *subject_start;\n+\tconst char *author_start;\n+\tconst char *committer_start;\n+\tconst char *encoding_start;\n+\tconst char *body_start;\n+\tsize_t subject_len;\n+\tsize_t author_len;\n+\tsize_t committer_len;\n+\tsize_t encoding_len;\n+};\n+\n+struct format_commit_context {\n+\tconst struct commit *commit;\n+\tstruct parsed_commit_header *parsed_commit_header;\n+};\n+\n+static void parse_commit_header(struct parsed_commit_header *pch,\n+                                const struct commit *commit)\n+{\n+\tconst char *msg = commit->buffer;\n+\tint i;\n+\tenum { HEADER, SUBJECT, BODY } state;\n+\n+\tfor (i = 0, state = HEADER; msg[i] && state < BODY; i++) {\n+\t\tint eol;\n+\t\tfor (eol = i; msg[eol] && msg[eol] != '\\n'; eol++)\n+\t\t\t; /* do nothing */\n+\n+\t\tif (state == SUBJECT) {\n+\t\t\tpch->subject_start = msg + i;\n+\t\t\tpch->subject_len = eol - i;\n+\t\t\ti = eol;\n+\t\t}\n+\t\tif (i == eol) {\n+\t\t\tstate++;\n+\t\t\t/* strip empty lines */\n+\t\t\twhile (msg[eol + 1] == '\\n')\n+\t\t\t\teol++;\n+\t\t} else if (!prefixcmp(msg + i, \"author \")) {\n+\t\t\tpch->author_start = msg + i + 7;\n+\t\t\tpch->author_len = eol - i - 7;\n+\t\t} else if (!prefixcmp(msg + i, \"committer \")) {\n+\t\t\tpch->committer_start = msg + i + 10;\n+\t\t\tpch->committer_len = eol - i - 10;\n+\t\t} else if (!prefixcmp(msg + i, \"encoding \")) {\n+\t\t\tpch->encoding_start = msg + i + 9;\n+\t\t\tpch->encoding_len = eol - i - 9;\n+\t\t}\n+\t\ti = eol;\n+\t}\n+\tpch->body_start = msg + i;\n+\tpch->parsed = 1;\n+}\n+\n static void format_commit_item(struct strbuf *sb, const char *placeholder,\n                                void *context)\n {\n-\tconst struct commit *commit = context;\n+\tstruct format_commit_context *c = context;\n+\tconst struct commit *commit = c->commit;\n+\tstruct parsed_commit_header *pch = c->parsed_commit_header;\n \tstruct commit_list *p;\n-\tint i;\n-\tenum { HEADER, SUBJECT, BODY } state;\n-\tconst char *msg = commit->buffer;\n \n \t/* these are independent of the commit */\n \tswitch (placeholder[0]) {\n@@ -429,45 +484,28 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,\n \t}\n \n \t/* For the rest we have to parse the commit header. */\n-\tfor (i = 0, state = HEADER; msg[i] && state < BODY; i++) {\n-\t\tint eol;\n-\t\tfor (eol = i; msg[eol] && msg[eol] != '\\n'; eol++)\n-\t\t\t; /* do nothing */\n+\tif (!pch->parsed)\n+\t\tparse_commit_header(pch, commit);\n \n-\t\tif (state == SUBJECT) {\n-\t\t\tif (placeholder[0] == 's') {\n-\t\t\t\tstrbuf_add(sb, msg + i, eol - i);\n-\t\t\t\treturn;\n-\t\t\t}\n-\t\t\ti = eol;\n-\t\t}\n-\t\tif (i == eol) {\n-\t\t\tstate++;\n-\t\t\t/* strip empty lines */\n-\t\t\twhile (msg[eol + 1] == '\\n')\n-\t\t\t\teol++;\n-\t\t} else if (!prefixcmp(msg + i, \"author \")) {\n-\t\t\tif (placeholder[0] == 'a') {\n-\t\t\t\tformat_person_part(sb, placeholder[1],\n-\t\t\t\t                   msg + i + 7, eol - i - 7);\n-\t\t\t\treturn;\n-\t\t\t}\n-\t\t} else if (!prefixcmp(msg + i, \"committer \")) {\n-\t\t\tif (placeholder[0] == 'c') {\n-\t\t\t\tformat_person_part(sb, placeholder[1],\n-\t\t\t\t                   msg + i + 10, eol - i - 10);\n-\t\t\t\treturn;\n-\t\t\t}\n-\t\t} else if (!prefixcmp(msg + i, \"encoding \")) {\n-\t\t\tif (placeholder[0] == 'e') {\n-\t\t\t\tstrbuf_add(sb, msg + i + 9, eol - i - 9);\n-\t\t\t\treturn;\n-\t\t\t}\n-\t\t}\n-\t\ti = eol;\n+\tswitch (placeholder[0]) {\n+\tcase 's':\n+\t\tstrbuf_add(sb, pch->subject_start, pch->subject_len);\n+\t\treturn;\n+\tcase 'a':\n+\t\tformat_person_part(sb, placeholder[1],\n+\t\t                   pch->author_start, pch->author_len);\n+\t\treturn;\n+\tcase 'c':\n+\t\tformat_person_part(sb, placeholder[1],\n+\t\t                   pch->committer_start, pch->committer_len);\n+\t\treturn;\n+\tcase 'e':\n+\t\tstrbuf_add(sb, pch->encoding_start, pch->encoding_len);\n+\t\treturn;\n+\tcase 'b':\n+\t\tstrbuf_addstr(sb, pch->body_start);\n+\t\treturn;\n \t}\n-\tif (msg[i] && placeholder[0] == 'b')\t/* body */\n-\t\tstrbuf_addstr(sb, msg + i);\n }\n \n void format_commit_message(const struct commit *commit,\n@@ -505,7 +543,13 @@ void format_commit_message(const struct commit *commit,\n \t\t\"m\",\t\t/* left/right/bottom */\n \t\tNULL\n \t};\n-\tstrbuf_expand(sb, format, placeholders, format_commit_item, (void *)commit);\n+\tstruct parsed_commit_header pch;\n+\tstruct format_commit_context context;\n+\n+\tmemset(&pch, 0, sizeof(pch));\n+\tcontext.commit = commit;\n+\tcontext.parsed_commit_header = &pch;\n+\tstrbuf_expand(sb, format, placeholders, format_commit_item, &context);\n }\n \n static void pp_header(enum cmit_fmt fmt,\n"},{"id":"59138","messageId":"4734EB36.2030408@lsrfire.ath.cx","threadId":"10745","inReplyTo":"20071109045256.GD31760@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-09T23:20:22Z","receivedAt":"2007-11-09T23:20:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Jeff King schrieb:\n> On Fri, Nov 09, 2007 at 01:49:42AM +0100, René Scharfe wrote:\n> \n>> +\tstrbuf_expand(sb, format, placeholders, format_commit_item, (void *)commit);\n> \n> This void cast is pointless, since all pointers types convert implicitly\n> to void pointers anyway. At best, it does nothing, and at worst, it\n> hides an actual type error if the function signature or the type of\n> 'commit' change.\n\nWhen commit (of type const struct commit*) is implicitly converted to\nvoid *, gcc complains because the \"const\" qualifier is silently dropped.\n"},{"id":"59147","messageId":"18228.61334.145989.77872@cargo.ozlabs.ibm.com","threadId":"10745","inReplyTo":"4733AEA6.1040802@lsrfire.ath.cx","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2007-11-09T23:39:02Z","receivedAt":"2007-11-09T23:39:02Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"Thanks for doing this.\n\nCould I ask for a couple more things?  I would find it useful to have\nformat codes for the number of characters in the commit body.  Having\na code for the number of bytes in the commit body would also be\nuseful.  When reading commits in with Tcl, the number of characters is\nmore useful, but I can use the number of bytes (and I imagine others\nwould find that useful).\n\nPaul.\n"},{"id":"59150","messageId":"Pine.LNX.4.64.0711100030090.4362@racer.site","threadId":"10745","inReplyTo":"4734EA4E.8070405@lsrfire.ath.cx","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-10T00:31:41Z","receivedAt":"2007-11-10T00:31:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 10 Nov 2007, Ren? Scharfe wrote:\n\n> (next)\n> $ time git log --pretty=format:\"* %cd %cn%n%n%s%n%b\" >/dev/null\n> \n> real    0m0.631s\n> user    0m0.584s\n> sys     0m0.040s\n> \n> (next+patch)\n> $ time git log --pretty=format:\"* %cd %cn%n%n%s%n%b\" >/dev/null\n> \n> real    0m0.570s\n> user    0m0.512s\n> sys     0m0.044s\n\nWow.\n\nIf you keep going like that, \"git log\" will be slower than \"git log \n--pretty=format:bla\" soon.\n\nCiao,\nDscho\n"},{"id":"59153","messageId":"20071110004635.GA14992@sigill.intra.peff.net","threadId":"10745","inReplyTo":"4734EA4E.8070405@lsrfire.ath.cx","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-10T00:46:37Z","receivedAt":"2007-11-10T00:46:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 10, 2007 at 12:16:30AM +0100, René Scharfe wrote:\n\n> > A partial patch on top of yours is below (it caches commit and tree \n> > abbreviations; parent abbreviations and person-parsing are probably \n> > worth doing). Some timings:\n> \n> ... but I object to the choice of items to cache.  Are there real-world\n> formats containing the same placeholder twice or even more often?\n\nMy choice of items was more \"here is what I am talking about\" and not\n\"this is the best set of items.\"\n\nAs for what real-world workloads are like, part of the _point_ of\n--pretty=format: is for one-off formats that users use in their\nworkflow. So yes, I have used formats that repeat specifiers, but they\nare probably not common.\n\nThe point of my timings was to show not only that we sped up that\nuncommon case, but that there was negligible cost to the common case.\nAnd since we don't know what formats users will provide, it makes sense\nnot to have lousy performance on the uncommon.\n\n> There is probably more to gain from the interdependencies of different\n> placeholders.  The patch below attempts to avoid parsing the commit\n> twice, by saving pointers to the different parts.\n\nLooks sane, although I don't see any reason this couldn't just go on top\nof my patch, and then we can speed up both cases.\n\n> (next)\n> $ time git log --pretty=format:\"* %cd %cn%n%n%s%n%b\" >/dev/null\n> real    0m0.631s\n[...]\n> (next+patch)\n> $ time git log --pretty=format:\"* %cd %cn%n%n%s%n%b\" >/dev/null\n> real    0m0.570s\n\nGreat, you have sped up the uncommon case. But what is the cost to \"git\nlog --pretty=format:\" and \"git log --pretty=format:%cd\"?\n\n-Peff\n"},{"id":"59154","messageId":"20071110004932.GB14992@sigill.intra.peff.net","threadId":"10745","inReplyTo":"Pine.LNX.4.64.0711100030090.4362@racer.site","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-10T00:49:33Z","receivedAt":"2007-11-10T00:49:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 10, 2007 at 12:31:41AM +0000, Johannes Schindelin wrote:\n\n> If you keep going like that, \"git log\" will be slower than \"git log \n> --pretty=format:bla\" soon.\n\nMaybe I am dreaming, but the code might be much simpler and more\nreadable if --pretty=* were implemented in terms of --pretty=format,\nassuming the speeds are equivalent.\n\n-Peff\n"},{"id":"59155","messageId":"20071110005145.GC14992@sigill.intra.peff.net","threadId":"10745","inReplyTo":"4734EB36.2030408@lsrfire.ath.cx","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-10T00:51:45Z","receivedAt":"2007-11-10T00:51:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 10, 2007 at 12:20:22AM +0100, René Scharfe wrote:\n\n> > This void cast is pointless, since all pointers types convert implicitly\n> > to void pointers anyway. At best, it does nothing, and at worst, it\n> > hides an actual type error if the function signature or the type of\n> > 'commit' change.\n> \n> When commit (of type const struct commit*) is implicitly converted to\n> void *, gcc complains because the \"const\" qualifier is silently dropped.\n\nBleh. You're right, of course. I don't like casting away constness, but\nit is stupid for strbuf_expand to take a \"const void *\" since it has no\nidea how it will be used (although I note your caching patch also neatly\ndoes away with this, anyway).\n\n-Peff\n"},{"id":"59192","messageId":"47359221.7090707@lsrfire.ath.cx","threadId":"10745","inReplyTo":"20071110004635.GA14992@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-10T11:12:33Z","receivedAt":"2007-11-10T11:12:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Jeff King schrieb:\n> On Sat, Nov 10, 2007 at 12:16:30AM +0100, René Scharfe wrote:\n> \n>>> A partial patch on top of yours is below (it caches commit and tree \n>>> abbreviations; parent abbreviations and person-parsing are probably \n>>> worth doing). Some timings:\n>> ... but I object to the choice of items to cache.  Are there real-world\n>> formats containing the same placeholder twice or even more often?\n> \n> My choice of items was more \"here is what I am talking about\" and not\n> \"this is the best set of items.\"\n> \n> As for what real-world workloads are like, part of the _point_ of\n> --pretty=format: is for one-off formats that users use in their\n> workflow. So yes, I have used formats that repeat specifiers, but they\n> are probably not common.\n\nHmm, OK.\n\n> The point of my timings was to show not only that we sped up that\n> uncommon case, but that there was negligible cost to the common case.\n> And since we don't know what formats users will provide, it makes sense\n> not to have lousy performance on the uncommon.\n\nMy suspicion was that it's more like \"never used\" rather than\n\"uncommon\".  Since you're a duplicate placeholder user, it's not \"never\"\nany more. :)\n\n>> There is probably more to gain from the interdependencies of different\n>> placeholders.  The patch below attempts to avoid parsing the commit\n>> twice, by saving pointers to the different parts.\n> \n> Looks sane, although I don't see any reason this couldn't just go on top\n> of my patch, and then we can speed up both cases.\n\nYes, the two are independent.  I don't like the malloc()'s in your\npatch, though, and have cooked up a different one on top of a cleaned up\nversion of mine.  It plays the dirty trick of reading expansions of\nrepeated placeholders from the strbuf..\n\nRené\n"},{"id":"59193","messageId":"4735928C.3040009@lsrfire.ath.cx","threadId":"10745","inReplyTo":"20071110004635.GA14992@sigill.intra.peff.net","subject":"[PATCH 1/3] --pretty=format: parse commit message only once","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-10T11:14:20Z","receivedAt":"2007-11-10T11:14:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"As Jeff King pointed out, some placeholder expansions are related to\neach other: the steps to calculate one go most of the way towards\ncalculating the other, too.\n\nThis patch makes format_commit_message() parse the commit message\nonly once, remembering the position of each item.  This speeds up\nhandling of format strings containing multiple placeholders from the\nset %s, %a*, %c*, %e, %b.\n\nHere are the timings for the git version in next.  The first one is\nto estimate the overhead of the caching, the second one is taken\nfrom http://svn.tue.mpg.de/tentakel/trunk/tentakel/Makefile as an\nexample of a format string found in the wild.  The times are the\nfastest of three consecutive runs in each case:\n\n$ time git log --pretty=format:%e >/dev/null\n\nreal    0m0.381s\nuser    0m0.340s\nsys     0m0.024s\n\n$ time git log --pretty=format:\"* %cd %cn%n%n%s%n%b\" >/dev/null\n\nreal    0m0.623s\nuser    0m0.556s\nsys     0m0.052s\n\nAnd here the times with this patch:\n\n$ time git log --pretty=format:%e >/dev/null\n\nreal    0m0.385s\nuser    0m0.332s\nsys     0m0.040s\n\n$ time git log --pretty=format:\"* %cd %cn%n%n%s%n%b\" >/dev/null\n\nreal    0m0.563s\nuser    0m0.504s\nsys     0m0.048s\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n pretty.c |  124 +++++++++++++++++++++++++++++++++++++++++---------------------\n 1 files changed, 82 insertions(+), 42 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 9fbd73f..0c2f83b 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -354,14 +354,67 @@ static void format_person_part(struct strbuf *sb, char part,\n \t}\n }\n \n-static void format_commit_item(struct strbuf *sb, const char *placeholder,\n-                               void *context)\n+struct chunk {\n+\tsize_t off;\n+\tsize_t len;\n+};\n+\n+struct format_commit_context {\n+\tconst struct commit *commit;\n+\n+\t/* These offsets are relative to the start of the commit message. */\n+\tint commit_header_parsed;\n+\tstruct chunk subject;\n+\tstruct chunk author;\n+\tstruct chunk committer;\n+\tstruct chunk encoding;\n+\tsize_t body_off;\n+};\n+\n+static void parse_commit_header(struct format_commit_context *context)\n {\n-\tconst struct commit *commit = context;\n-\tstruct commit_list *p;\n+\tconst char *msg = context->commit->buffer;\n \tint i;\n \tenum { HEADER, SUBJECT, BODY } state;\n+\n+\tfor (i = 0, state = HEADER; msg[i] && state < BODY; i++) {\n+\t\tint eol;\n+\t\tfor (eol = i; msg[eol] && msg[eol] != '\\n'; eol++)\n+\t\t\t; /* do nothing */\n+\n+\t\tif (state == SUBJECT) {\n+\t\t\tcontext->subject.off = i;\n+\t\t\tcontext->subject.len = eol - i;\n+\t\t\ti = eol;\n+\t\t}\n+\t\tif (i == eol) {\n+\t\t\tstate++;\n+\t\t\t/* strip empty lines */\n+\t\t\twhile (msg[eol + 1] == '\\n')\n+\t\t\t\teol++;\n+\t\t} else if (!prefixcmp(msg + i, \"author \")) {\n+\t\t\tcontext->author.off = i + 7;\n+\t\t\tcontext->author.len = eol - i - 7;\n+\t\t} else if (!prefixcmp(msg + i, \"committer \")) {\n+\t\t\tcontext->committer.off = i + 10;\n+\t\t\tcontext->committer.len = eol - i - 10;\n+\t\t} else if (!prefixcmp(msg + i, \"encoding \")) {\n+\t\t\tcontext->encoding.off = i + 9;\n+\t\t\tcontext->encoding.len = eol - i - 9;\n+\t\t}\n+\t\ti = eol;\n+\t}\n+\tcontext->body_off = i;\n+\tcontext->commit_header_parsed = 1;\n+}\n+\n+static void format_commit_item(struct strbuf *sb, const char *placeholder,\n+                               void *context)\n+{\n+\tstruct format_commit_context *c = context;\n+\tconst struct commit *commit = c->commit;\n \tconst char *msg = commit->buffer;\n+\tstruct commit_list *p;\n \n \t/* these are independent of the commit */\n \tswitch (placeholder[0]) {\n@@ -429,45 +482,28 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,\n \t}\n \n \t/* For the rest we have to parse the commit header. */\n-\tfor (i = 0, state = HEADER; msg[i] && state < BODY; i++) {\n-\t\tint eol;\n-\t\tfor (eol = i; msg[eol] && msg[eol] != '\\n'; eol++)\n-\t\t\t; /* do nothing */\n+\tif (!c->commit_header_parsed)\n+\t\tparse_commit_header(c);\n \n-\t\tif (state == SUBJECT) {\n-\t\t\tif (placeholder[0] == 's') {\n-\t\t\t\tstrbuf_add(sb, msg + i, eol - i);\n-\t\t\t\treturn;\n-\t\t\t}\n-\t\t\ti = eol;\n-\t\t}\n-\t\tif (i == eol) {\n-\t\t\tstate++;\n-\t\t\t/* strip empty lines */\n-\t\t\twhile (msg[eol + 1] == '\\n')\n-\t\t\t\teol++;\n-\t\t} else if (!prefixcmp(msg + i, \"author \")) {\n-\t\t\tif (placeholder[0] == 'a') {\n-\t\t\t\tformat_person_part(sb, placeholder[1],\n-\t\t\t\t                   msg + i + 7, eol - i - 7);\n-\t\t\t\treturn;\n-\t\t\t}\n-\t\t} else if (!prefixcmp(msg + i, \"committer \")) {\n-\t\t\tif (placeholder[0] == 'c') {\n-\t\t\t\tformat_person_part(sb, placeholder[1],\n-\t\t\t\t                   msg + i + 10, eol - i - 10);\n-\t\t\t\treturn;\n-\t\t\t}\n-\t\t} else if (!prefixcmp(msg + i, \"encoding \")) {\n-\t\t\tif (placeholder[0] == 'e') {\n-\t\t\t\tstrbuf_add(sb, msg + i + 9, eol - i - 9);\n-\t\t\t\treturn;\n-\t\t\t}\n-\t\t}\n-\t\ti = eol;\n+\tswitch (placeholder[0]) {\n+\tcase 's':\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\t                   msg + c->author.off, c->author.len);\n+\t\treturn;\n+\tcase 'c':\n+\t\tformat_person_part(sb, placeholder[1],\n+\t\t                   msg + c->committer.off, c->committer.len);\n+\t\treturn;\n+\tcase 'e':\n+\t\tstrbuf_add(sb, msg + c->encoding.off, c->encoding.len);\n+\t\treturn;\n+\tcase 'b':\n+\t\tstrbuf_addstr(sb, msg + c->body_off);\n+\t\treturn;\n \t}\n-\tif (msg[i] && placeholder[0] == 'b')\t/* body */\n-\t\tstrbuf_addstr(sb, msg + i);\n }\n \n void format_commit_message(const struct commit *commit,\n@@ -505,7 +541,11 @@ void format_commit_message(const struct commit *commit,\n \t\t\"m\",\t\t/* left/right/bottom */\n \t\tNULL\n \t};\n-\tstrbuf_expand(sb, format, placeholders, format_commit_item, (void *)commit);\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 }\n \n static void pp_header(enum cmit_fmt fmt,\n-- \n1.5.3.5.1651.g30bf\n"},{"id":"59195","messageId":"473592F5.2070407@lsrfire.ath.cx","threadId":"10745","inReplyTo":"20071110004635.GA14992@sigill.intra.peff.net","subject":"[PATCH 2/3] add strbuf_adddup()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-10T11:16:05Z","receivedAt":"2007-11-10T11:16:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a new function, strbuf_adddup(), that appends a duplicate of a\npart of a struct strbuf to end of the latter.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n strbuf.c |    7 +++++++\n strbuf.h |    1 +\n 2 files changed, 8 insertions(+), 0 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 536b432..dbd8c4b 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -106,6 +106,13 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len)\n \tstrbuf_setlen(sb, sb->len + len);\n }\n \n+void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len)\n+{\n+\tstrbuf_grow(sb, len);\n+\tmemcpy(sb->buf + sb->len, sb->buf + pos, len);\n+\tstrbuf_setlen(sb, sb->len + len);\n+}\n+\n void strbuf_addf(struct strbuf *sb, const char *fmt, ...)\n {\n \tint len;\ndiff --git a/strbuf.h b/strbuf.h\nindex 21d8944..1391912 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -101,6 +101,7 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s) {\n static inline void strbuf_addbuf(struct strbuf *sb, struct strbuf *sb2) {\n \tstrbuf_add(sb, sb2->buf, sb2->len);\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-- \n1.5.3.5.1651.g30bf\n"},{"id":"59197","messageId":"47359382.1010600@lsrfire.ath.cx","threadId":"10745","inReplyTo":"20071110004635.GA14992@sigill.intra.peff.net","subject":"[PATCH 3/3] --format=pretty: avoid calculating expensive expansions twice","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-10T11:18:26Z","receivedAt":"2007-11-10T11:18:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"As Jeff King remarked, format strings with duplicate placeholders can\nbe slow to expand, because each instance is calculated anew.\n\nThis patch makes use of the fact that format_commit_message() and its\nhelper functions only ever add stuff to the end of the strbuf.  For\ncertain expensive placeholders, store the offset and length of their\nexpansion with the strbuf at the first occurrence.  Later they\nexpansion result can simply be copied from there -- no malloc() or\nstrdup() required.\n\nThese certain placeholders are the abbreviated commit, tree and\nparent hashes, as the search for a unique abbreviated hash is quite\ncostly.  Here are the times for next (best of three runs):\n\n$ time git log --pretty=format:%h >/dev/null\n\nreal    0m0.611s\nuser    0m0.404s\nsys     0m0.204s\n\n$ time git log --pretty=format:%h%h%h%h >/dev/null\n\nreal    0m1.206s\nuser    0m0.744s\nsys     0m0.452s\n\nAnd here those with this patch (and the previous two); the speedup\nof the single placeholder case is just noise:\n\n$ time git log --pretty=format:%h >/dev/null\n\nreal    0m0.608s\nuser    0m0.416s\nsys     0m0.192s\n\n$ time git log --pretty=format:%h%h%h%h >/dev/null\n\nreal    0m0.639s\nuser    0m0.488s\nsys     0m0.140s\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n pretty.c |   32 ++++++++++++++++++++++++++++++++\n 1 files changed, 32 insertions(+), 0 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 0c2f83b..ab142b8 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -369,8 +369,30 @@ struct format_commit_context {\n \tstruct chunk committer;\n \tstruct chunk encoding;\n \tsize_t body_off;\n+\n+\t/* The following ones are relative to the result struct strbuf. */\n+\tstruct chunk abbrev_commit_hash;\n+\tstruct chunk abbrev_tree_hash;\n+\tstruct chunk abbrev_parent_hashes;\n };\n \n+static int add_again(struct strbuf *sb, struct chunk *chunk)\n+{\n+\tif (chunk->len) {\n+\t\tstrbuf_adddup(sb, chunk->off, chunk->len);\n+\t\treturn 1;\n+\t}\n+\n+\t/*\n+\t * We haven't seen this chunk before.  Our caller is surely\n+\t * going to add it the hard way now.  Remember the most likely\n+\t * start of the to-be-added chunk: the current end of the\n+\t * struct strbuf.\n+\t */\n+\tchunk->off = sb->len;\n+\treturn 0;\n+}\n+\n static void parse_commit_header(struct format_commit_context *context)\n {\n \tconst char *msg = context->commit->buffer;\n@@ -447,15 +469,21 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,\n \t\tstrbuf_addstr(sb, sha1_to_hex(commit->object.sha1));\n \t\treturn;\n \tcase 'h':\t\t/* abbreviated commit hash */\n+\t\tif (add_again(sb, &c->abbrev_commit_hash))\n+\t\t\treturn;\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 \tcase 'T':\t\t/* tree hash */\n \t\tstrbuf_addstr(sb, sha1_to_hex(commit->tree->object.sha1));\n \t\treturn;\n \tcase 't':\t\t/* abbreviated tree hash */\n+\t\tif (add_again(sb, &c->abbrev_tree_hash))\n+\t\t\treturn;\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 \tcase 'P':\t\t/* parent hashes */\n \t\tfor (p = commit->parents; p; p = p->next) {\n@@ -465,12 +493,16 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,\n \t\t}\n \t\treturn;\n \tcase 'p':\t\t/* abbreviated parent hashes */\n+\t\tif (add_again(sb, &c->abbrev_parent_hashes))\n+\t\t\treturn;\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, find_unique_abbrev(\n \t\t\t\t\tp->item->object.sha1, DEFAULT_ABBREV));\n \t\t}\n+\t\tc->abbrev_parent_hashes.len = sb->len -\n+\t\t                              c->abbrev_parent_hashes.off;\n \t\treturn;\n \tcase 'm':\t\t/* left/right/bottom */\n \t\tstrbuf_addch(sb, (commit->object.flags & BOUNDARY)\n-- \n1.5.3.5.1651.g30bf\n"},{"id":"59220","messageId":"Pine.LNX.4.64.0711101605560.4362@racer.site","threadId":"10745","inReplyTo":"47359221.7090707@lsrfire.ath.cx","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-10T16:07:10Z","receivedAt":"2007-11-10T16:07:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 10 Nov 2007, Ren? Scharfe wrote:\n\n> [...] have cooked up a different one on top of a cleaned up version of \n> mine.  It plays the dirty trick of reading expansions of repeated \n> placeholders from the strbuf..\n\n... which would not work (likely even segfault) if you work with the same \nprivate data on different strbufs.\n\nBut I guess it will not matter much in practice.\n\nCiao,\nDscho\n"},{"id":"59221","messageId":"4735DB3A.2020905@lsrfire.ath.cx","threadId":"10745","inReplyTo":"Pine.LNX.4.64.0711101605560.4362@racer.site","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-10T16:24:26Z","receivedAt":"2007-11-10T16:24:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Johannes Schindelin schrieb:\n> Hi,\n> \n> On Sat, 10 Nov 2007, René Scharfe wrote:\n> \n>> [...] have cooked up a different one on top of a cleaned up version of \n>> mine.  It plays the dirty trick of reading expansions of repeated \n>> placeholders from the strbuf..\n> \n> ... which would not work (likely even segfault) if you work with the same \n> private data on different strbufs.\n> \n> But I guess it will not matter much in practice.\n\nOnly a single strbuf is used, and the function that copies the data\naround, strbuf_adddup(), operates on a single strbuf, only.  Copying\ndata between two strbufs using strbuf_add() etc. would be safe.\n\nWhat one should *not* do is this:\n\n\tstrbuf_add(sb, sb->buf + offset, length);\n\nThis leads to problems when the buffer is realloc()ated by strbuf_add().\n\nWhat other things can go wrong?  A segfault would definitely matter..\n\nThanks,\nRené\n"},{"id":"59240","messageId":"20071110203435.GB20592@sigill.intra.peff.net","threadId":"10745","inReplyTo":"47359221.7090707@lsrfire.ath.cx","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-10T20:34:35Z","receivedAt":"2007-11-10T20:34:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 10, 2007 at 12:12:33PM +0100, René Scharfe wrote:\n\n> Yes, the two are independent.  I don't like the malloc()'s in your\n> patch, though, and have cooked up a different one on top of a cleaned up\n> version of mine.  It plays the dirty trick of reading expansions of\n> repeated placeholders from the strbuf..\n\nYes, the mallocs would obviously not be necessary for the commit\nparsing, since we already have a buffer, but I assumed they would be for\nthe abbreviations.\n\nYour solution leaves me partly disgusted at the hackishness, and partly\ndelighted at the cleverness. I think the way you have coded it is quite\nclear, though, so I don't think anyone is likely to get confused reading\nit. I haven't read through it carefully yet, but a tentative Ack from\nme.\n\n-Peff\n"},{"id":"59241","messageId":"20071110203616.GA21098@sigill.intra.peff.net","threadId":"10745","inReplyTo":"Pine.LNX.4.64.0711101605560.4362@racer.site","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-10T20:36:17Z","receivedAt":"2007-11-10T20:36:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 10, 2007 at 04:07:10PM +0000, Johannes Schindelin wrote:\n\n> > [...] have cooked up a different one on top of a cleaned up version of \n> > mine.  It plays the dirty trick of reading expansions of repeated \n> > placeholders from the strbuf..\n> \n> ... which would not work (likely even segfault) if you work with the same \n> private data on different strbufs.\n\nThe nice thing is that the cache validity only lasts for the call to\nstrbuf_expand (and its callbacks), so there is really very little chance\nof somebody mis-using this.\n\n-Peff\n"},{"id":"59287","messageId":"20071111081305.GA27798@sigill.intra.peff.net","threadId":"10745","inReplyTo":"20071110203435.GB20592@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] --pretty=format: on-demand format expansion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-11T08:13:05Z","receivedAt":"2007-11-11T08:13:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 10, 2007 at 03:34:35PM -0500, Jeff King wrote:\n\n> Your solution leaves me partly disgusted at the hackishness, and partly\n> delighted at the cleverness. I think the way you have coded it is quite\n> clear, though, so I don't think anyone is likely to get confused reading\n> it. I haven't read through it carefully yet, but a tentative Ack from\n> me.\n\nOK, I had a chance to read through your patches more carefully. Assuming\nthat the strbuf offset technique is acceptable (and I think it is worth\nthe increased speed and simplicity), they look great to me. So:\n\nAcked-by: Jeff King <peff@peff.net>\n\n-Peff\n"},{"id":"59304","messageId":"7vejexf53b.fsf@gitster.siamese.dyndns.org","threadId":"10745","inReplyTo":"47359382.1010600@lsrfire.ath.cx","subject":"Re: [PATCH 3/3] --format=pretty: avoid calculating expensive expansions twice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-11T10:29:12Z","receivedAt":"2007-11-11T10:29:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> +static int add_again(struct strbuf *sb, struct chunk *chunk)\n> +{\n> +\tif (chunk->len) {\n> +\t\tstrbuf_adddup(sb, chunk->off, chunk->len);\n> +\t\treturn 1;\n> +\t}\n> +\n> +\t/*\n> +\t * We haven't seen this chunk before.  Our caller is surely\n> +\t * going to add it the hard way now.  Remember the most likely\n> +\t * start of the to-be-added chunk: the current end of the\n> +\t * struct strbuf.\n> +\t */\n> +\tchunk->off = sb->len;\n> +\treturn 0;\n> +}\n> +\n>  static void parse_commit_header(struct format_commit_context *context)\n>  {\n>  \tconst char *msg = context->commit->buffer;\n> @@ -447,15 +469,21 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,\n>  \t\tstrbuf_addstr(sb, sha1_to_hex(commit->object.sha1));\n>  \t\treturn;\n>  \tcase 'h':\t\t/* abbreviated commit hash */\n> +\t\tif (add_again(sb, &c->abbrev_commit_hash))\n> +\t\t\treturn;\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\nBrilliant.  Doubly brilliant is the adddup abstraction that does\nnot suffer from underlying strbuf being reallocated.\n\nMe likee..\n"}]}