{"thread":{"id":"10644","subject":"[PATCH 5/5] pretty describe: add %ds, %dn, %dd placeholders","startedAt":"2007-11-04T11:49:04Z","lastAt":"2007-11-05T01:20:25Z","messageCount":7,"participants":["René Scharfe","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"58256","messageId":"472DB1B0.1050904@lsrfire.ath.cx","threadId":"10644","inReplyTo":null,"subject":"[PATCH 5/5] pretty describe: add %ds, %dn, %dd placeholders","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-04T11:49:04Z","receivedAt":"2007-11-04T11:49:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The new placeholders %ds (description string, git-describe style), %dn\n(the name part) and %dd (the depth part) are added.\n\nTo avoid imposing the significant cost of running describe_commit() on\nevery format string, even if none of the new placeholders are used, a\nnew function, interp_count(), is introduced.  It is a stripped down\nversion of interpolate(), that simply counts the placeholders in a\nformat string.  If the describe placeholders are not found, setting up\nthe corresponding replacements is skipped.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n Documentation/pretty-formats.txt |    3 +++\n commit.c                         |   38 ++++++++++++++++++++++++++++++++++++++\n interpolate.c                    |   36 ++++++++++++++++++++++++++++++++++++\n interpolate.h                    |    2 ++\n t/t6120-describe.sh              |   22 ++++++++++++++++++++++\n 5 files changed, 101 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 0193c3c..ec86415 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -100,6 +100,9 @@ The placeholders are:\n - '%t': abbreviated tree hash\n - '%P': parent hashes\n - '%p': abbreviated parent hashes\n+- '%ds': description\n+- '%dn': tag name part of the description\n+- '%dd': depth part of the description\n - '%an': author name\n - '%ae': author email\n - '%ad': author date\ndiff --git a/commit.c b/commit.c\nindex 2e52a2f..06d5cec 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -781,6 +781,9 @@ void format_commit_message(struct commit *commit,\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{ \"%ds\" },\t/* description */\n+\t\t{ \"%dn\" },\t/* tag name part of the description */\n+\t\t{ \"%dd\" },\t/* depth part of the description */\n \t\t{ \"%an\" },\t/* author name */\n \t\t{ \"%ae\" },\t/* author email */\n \t\t{ \"%ad\" },\t/* author date */\n@@ -809,6 +812,7 @@ void format_commit_message(struct commit *commit,\n \t\tIHASH = 0, IHASH_ABBREV,\n \t\tITREE, ITREE_ABBREV,\n \t\tIPARENTS, IPARENTS_ABBREV,\n+\t\tIDESC, IDESC_NAME, IDESC_DEPTH,\n \t\tIAUTHOR_NAME, IAUTHOR_EMAIL,\n \t\tIAUTHOR_DATE, IAUTHOR_DATE_RFC2822, IAUTHOR_DATE_RELATIVE,\n \t\tIAUTHOR_TIMESTAMP, IAUTHOR_ISO8601,\n@@ -829,10 +833,13 @@ void format_commit_message(struct commit *commit,\n \tint i;\n \tenum { HEADER, SUBJECT, BODY } state;\n \tconst char *msg = commit->buffer;\n+\tunsigned long occurs[ARRAY_SIZE(table)];\n \n \tif (ILEFT_RIGHT + 1 != ARRAY_SIZE(table))\n \t\tdie(\"invalid interp table!\");\n \n+\tinterp_count(occurs, format, table, ARRAY_SIZE(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@@ -875,6 +882,37 @@ void format_commit_message(struct commit *commit,\n \t\t\t\tDEFAULT_ABBREV));\n \tinterp_set_entry(table, IPARENTS_ABBREV, parents + 1);\n \n+\tif (occurs[IDESC] || occurs[IDESC_DEPTH] || occurs[IDESC_NAME]) {\n+\t\tstruct strbuf desc;\n+\t\tchar *name;\n+\t\tint depth = 0;\n+\t\tchar *depthstr;\n+\t\tconst char *abbr;\n+\n+\t\tload_commit_names(2);\n+\t\tname = describe_commit(commit, 10, 0, &depth);\n+\t\tclear_commit_name_flags(commit);\n+\t\tabbr = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n+\n+\t\tstrbuf_init(&desc, 32);\n+\t\tstrbuf_addstr(&desc, name ? name + 5 : abbr);\n+\t\tinterp_set_entry(table, IDESC_NAME, desc.buf);\n+\t\tif (name) {\n+\t\t\tif (depth) {\n+\t\t\t\tstrbuf_addch(&desc, '-');\n+\t\t\t\tdepthstr = desc.buf + desc.len;\n+\t\t\t\tstrbuf_addf(&desc, \"%d\", depth);\n+\t\t\t\tinterp_set_entry(table, IDESC_DEPTH, depthstr);\n+\t\t\t\tstrbuf_addstr(&desc, \"-g\");\n+\t\t\t\tstrbuf_addstr(&desc, abbr);\n+\t\t\t} else {\n+\t\t\t\tinterp_set_entry(table, IDESC_DEPTH, \"0\");\n+\t\t\t}\n+\t\t}\n+\t\tinterp_set_entry(table, IDESC, desc.buf);\n+\t\tstrbuf_release(&desc);\n+\t}\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++)\ndiff --git a/interpolate.c b/interpolate.c\nindex 6ef53f2..8f51649 100644\n--- a/interpolate.c\n+++ b/interpolate.c\n@@ -102,3 +102,39 @@ unsigned long interpolate(char *result, unsigned long reslen,\n \t\t*dest = '\\0';\n \treturn newlen;\n }\n+\n+/*\n+ * interp_count - count occurences of placeholders\n+ */\n+void interp_count(unsigned long *result, const char *orig,\n+                  const struct interp *interps, int ninterps)\n+{\n+\tconst char *src = orig;\n+\tconst char *name;\n+\tunsigned long namelen;\n+\tint i;\n+\tchar c;\n+\n+\tfor (i = 0; i < ninterps; i++)\n+\t\tresult[i] = 0;\n+\n+\twhile ((c = *src)) {\n+\t\tif (c == '%') {\n+\t\t\t/* Try to match an interpolation string. */\n+\t\t\tfor (i = 0; i < ninterps; i++) {\n+\t\t\t\tname = interps[i].name;\n+\t\t\t\tnamelen = strlen(name);\n+\t\t\t\tif (strncmp(src, name, namelen) == 0)\n+\t\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\t\t/* Check for valid interpolation. */\n+\t\t\tif (i < ninterps) {\n+\t\t\t\tresult[i]++;\n+\t\t\t\tsrc += namelen;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\t\tsrc++;\n+\t}\n+}\ndiff --git a/interpolate.h b/interpolate.h\nindex 77407e6..c41ea18 100644\n--- a/interpolate.h\n+++ b/interpolate.h\n@@ -22,5 +22,7 @@ extern void interp_clear_table(struct interp *table, int ninterps);\n extern unsigned long interpolate(char *result, unsigned long reslen,\n \t\t\t\t const char *orig,\n \t\t\t\t const struct interp *interps, int ninterps);\n+extern void interp_count(unsigned long *result, const char *orig,\n+                         const struct interp *interps, int ninterps);\n \n #endif /* INTERPOLATE_H */\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex ae8ee11..3be43c4 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -23,6 +23,28 @@ check_describe () {\n \t\tfalse ;;\n \tesac\n \t'\n+\n+\t# %ds, %dn and %dd don't work with --tags or --all\n+\tcase \"$@\" in\n+\t*--tags*|*--all*) return ;;\n+\tesac\n+\n+\tR=$(git log -n 1 --pretty=format:%ds \"$@\") &&\n+\ttest_expect_success \"log --pretty=format:%ds $*\" '\n+\tcase \"$R\" in\n+\t$expect)\techo happy ;;\n+\t*)\techo \"Oops - $R is not $expect\";\n+\t\tfalse ;;\n+\tesac\n+\t'\n+\tR=$(git log -n 1 --pretty=format:%dn-%dd-g%h \"$@\" | sed 's/-0-g.*//') &&\n+\ttest_expect_success \"log --pretty=format:%dn-%dd-g%h $*\" '\n+\tcase \"$R\" in\n+\t$expect)\techo happy ;;\n+\t*)\techo \"Oops - $R is not $expect\";\n+\t\tfalse ;;\n+\tesac\n+\t'\n }\n \n test_expect_success setup '\n-- \n1.5.3.5.529.ge3d6d\n"},{"id":"58270","messageId":"472DD277.2070406@lsrfire.ath.cx","threadId":"10644","inReplyTo":"472DB1B0.1050904@lsrfire.ath.cx","subject":"[PATCH 7/5] pretty describe: add min_prio parameter to describe_commit()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-04T14:08:55Z","receivedAt":"2007-11-04T14:08:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If load_commit_names() is called with a certain priority and later with\na higher priority, all the old, low-priority names are still kept and\ndescribe_commit() will happily take them into account.  It can thus\nreport heads, even if the user asked for annotated tags.\n\nIn the current code, this can't happen, because builtin-describe.c calls\nload_commit_names() only once, and commit.c always asks for the highest\npriority (annotated tags).  This patch fixes the problem anyway, for the\nbenefit of future users.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n\n builtin-describe.c |    6 ++++--\n cache.h            |    2 +-\n commit.c           |    2 +-\n describe.c         |    7 ++++---\n 4 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin-describe.c b/builtin-describe.c\nindex fcd93f4..481d92f 100644\n--- a/builtin-describe.c\n+++ b/builtin-describe.c\n@@ -14,6 +14,7 @@ static int all;\t/* Default to annotated tags only */\n static int tags;\t/* But allow any tags if --tags is specified */\n static int abbrev = DEFAULT_ABBREV;\n static int max_candidates = 10;\n+static int min_prio;\n \n static void describe(const char *arg, int last_one)\n {\n@@ -28,7 +29,7 @@ static void describe(const char *arg, int last_one)\n \tif (!cmit)\n \t\tdie(\"%s is not a valid '%s' object\", arg, commit_type);\n \n-\tname = describe_commit(cmit, max_candidates, debug, &depth);\n+\tname = describe_commit(cmit, max_candidates, min_prio, debug, &depth);\n \tif (!name)\n \t\tdie(\"cannot describe '%s'\", sha1_to_hex(cmit->object.sha1));\n \n@@ -74,7 +75,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t * If --tags, then any tags are used.\n \t * Otherwise only annotated tags are used.\n \t */\n-\tload_commit_names(all ? 0 : (tags ? 1 : 2));\n+\tmin_prio = all ? 0 : (tags ? 1 : 2);\n+\tload_commit_names(min_prio);\n \n \tif (argc == 0) {\n \t\tdescribe(\"HEAD\", 1);\ndiff --git a/cache.h b/cache.h\nindex 84423a3..703d6a9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -605,6 +605,6 @@ void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, i\n /* describe.c */\n struct commit;\n extern void load_commit_names(int min_prio);\n-extern char *describe_commit(struct commit *cmit, int max_candidates, int debug, int *depthp);\n+extern char *describe_commit(struct commit *cmit, int max_candidates, int min_prio, int debug, int *depthp);\n \n #endif /* CACHE_H */\ndiff --git a/commit.c b/commit.c\nindex 9ff4735..624005b 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -897,7 +897,7 @@ void format_commit_message(struct commit *commit,\n \t\tconst char *abbr;\n \n \t\tload_commit_names(2);\n-\t\tname = describe_commit(commit, 10, 0, &depth);\n+\t\tname = describe_commit(commit, 10, 2, 0, &depth);\n \t\tclear_commit_name_flags(commit);\n \t\tabbr = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n \ndiff --git a/describe.c b/describe.c\nindex 18c7abc..b6de4c1 100644\n--- a/describe.c\n+++ b/describe.c\n@@ -102,7 +102,8 @@ static unsigned long finish_depth_computation(\n \treturn seen_commits;\n }\n \n-char *describe_commit(struct commit *cmit, int max_candidates, int debug, int *depthp)\n+char *describe_commit(struct commit *cmit, int max_candidates, int min_prio,\n+                      int debug, int *depthp)\n {\n \tstruct commit *gave_up_on = NULL;\n \tstruct commit_list *list;\n@@ -110,7 +111,7 @@ char *describe_commit(struct commit *cmit, int max_candidates, int debug, int *d\n \tunsigned int match_cnt = 0, annotated_cnt = 0, cur_match;\n \tunsigned long seen_commits = 0;\n \n-\tif (cmit->name) {\n+\tif (cmit->name && cmit->name_prio >= min_prio) {\n \t\t*depthp = 0;\n \t\treturn cmit->name;\n \t}\n@@ -130,7 +131,7 @@ char *describe_commit(struct commit *cmit, int max_candidates, int debug, int *d\n \t\tstruct commit *c = pop_commit(&list);\n \t\tstruct commit_list *parents = c->parents;\n \t\tseen_commits++;\n-\t\tif (c->name) {\n+\t\tif (c->name && c->name_prio >= min_prio) {\n \t\t\tif (match_cnt < max_candidates) {\n \t\t\t\tstruct possible_tag *t = &all_matches[match_cnt++];\n \t\t\t\tt->name = c->name;\n"},{"id":"58271","messageId":"Pine.LNX.4.64.0711041356330.4362@racer.site","threadId":"10644","inReplyTo":"472DB1B0.1050904@lsrfire.ath.cx","subject":"Re: [PATCH 5/5] pretty describe: add %ds, %dn, %dd placeholders","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-04T14:11:24Z","receivedAt":"2007-11-04T14:11:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 4 Nov 2007, Ren? Scharfe wrote:\n\n> The new placeholders %ds (description string, git-describe style), %dn \n> (the name part) and %dd (the depth part) are added.\n> \n> To avoid imposing the significant cost of running describe_commit() on \n> every format string, even if none of the new placeholders are used, a \n> new function, interp_count(), is introduced.  It is a stripped down \n> version of interpolate(), that simply counts the placeholders in a \n> format string.  If the describe placeholders are not found, setting up \n> the corresponding replacements is skipped.\n\nThe way I read this, those are two really quite independent patches \nsquashed into one.\n\n> +\tunsigned long occurs[ARRAY_SIZE(table)];\n\nYou do not ever use the counts.  So, longs are overkill.  Even ints might \nbe overkill, but probably the most convenient.  I would have gone with \nchars.  If I knew how to memset() an array of unsigned:1 entries to all \nzero, I would even have gone with that, but the runtime cost of this is \nprobably higher than the chars.\n\nBut the even more fundamental problem is that you count the needed \ninterpolations _every_ single time you output a commit message.\n\nA much better place would be get_commit_format().  Yes that means \nrestructuring the code a bit more, but I would say that this definitely \nwould help.  My preference would even be introducing a new source file for \nthe user format handling (commit-format.[ch]).\n\n> +\n> +/*\n> + * interp_count - count occurences of placeholders\n> + */\n> +void interp_count(unsigned long *result, const char *orig,\n> +                  const struct interp *interps, int ninterps)\n> +{\n> +\tconst char *src = orig;\n\nYou do not ever use orig again.  So why not just use that variable instead \nof introducing a new one?\n\n> +\tconst char *name;\n> +\tunsigned long namelen;\n> +\tint i;\n> +\tchar c;\n> +\n> +\tfor (i = 0; i < ninterps; i++)\n> +\t\tresult[i] = 0;\n\nmemset()?\n\n> +\n> +\twhile ((c = *src)) {\n> +\t\tif (c == '%') {\n> +\t\t\t/* Try to match an interpolation string. */\n> +\t\t\tfor (i = 0; i < ninterps; i++) {\n> +\t\t\t\tname = interps[i].name;\n> +\t\t\t\tnamelen = strlen(name);\n> +\t\t\t\tif (strncmp(src, name, namelen) == 0)\n\nprefixcmp()?\n\n> +\t\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\t/* Check for valid interpolation. */\n> +\t\t\tif (i < ninterps) {\n\nThis is ugly.  You already had a successful if() for that case earlier.\n\n> +\t\t\t\tresult[i]++;\n> +\t\t\t\tsrc += namelen;\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t}\n> +\t\tsrc++;\n> +\t}\n> +}\n\nI'd rewrite this whole loop as\n\n\twhile ((c = *(orig++)))\n\t\tif (c == '%')\n\t\t\t/* Try to match an interpolation string. */\n\t\t\tfor (i = 0; i < ninterps; i++)\n\t\t\t\tif (prefixcmp(orig, interps[i].name)) {\n\t\t\t\t\tresult[i] = 1;\n\t\t\t\t\torig += strlen(interps[i].name);\n\t\t\t\t\tbreak;\n\t\t\t\t}\n\nCiao,\nDscho\n\n\t\t\t\n"},{"id":"58276","messageId":"472DDA3B.4090100@lsrfire.ath.cx","threadId":"10644","inReplyTo":"Pine.LNX.4.64.0711041356330.4362@racer.site","subject":"Re: [PATCH 5/5] pretty describe: add %ds, %dn, %dd placeholders","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-04T14:42:03Z","receivedAt":"2007-11-04T14:42:03Z","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 Sun, 4 Nov 2007, Ren? Scharfe wrote:\n> \n>> The new placeholders %ds (description string, git-describe style), %dn \n>> (the name part) and %dd (the depth part) are added.\n>>\n>> To avoid imposing the significant cost of running describe_commit() on \n>> every format string, even if none of the new placeholders are used, a \n>> new function, interp_count(), is introduced.  It is a stripped down \n>> version of interpolate(), that simply counts the placeholders in a \n>> format string.  If the describe placeholders are not found, setting up \n>> the corresponding replacements is skipped.\n> \n> The way I read this, those are two really quite independent patches \n> squashed into one.\n\nBusted!  I didn't want to introduce a performance regression with the\n%ds parsing code, but I also didn't want to add a function without\nusers.  Patch 6 was then glued on as an afterthought -- the rest of the\nseries was ready when I saw Paul's mail.\n\nSo, a better way might be to move patch 6 to the head of the series and\nintroduce interp_count() in it, too.\n\n>> +\tunsigned long occurs[ARRAY_SIZE(table)];\n> \n> You do not ever use the counts.  So, longs are overkill.  Even ints might \n> be overkill, but probably the most convenient.  I would have gone with \n> chars.  If I knew how to memset() an array of unsigned:1 entries to all \n> zero, I would even have gone with that, but the runtime cost of this is \n> probably higher than the chars.\n\nWell, it isn't used in format_commit_message() currently, but it could\nbe.  Multiply the count and and the length of each substitution (minus\nthe length of the placeholder) and you get the number of bytes you need\nto allocate.  interpolate() wouldn't need to be called twice anymore.\n\n> But the even more fundamental problem is that you count the needed \n> interpolations _every_ single time you output a commit message.\n> \n> A much better place would be get_commit_format().  Yes that means \n> restructuring the code a bit more, but I would say that this definitely \n> would help.  My preference would even be introducing a new source file for \n> the user format handling (commit-format.[ch]).\n\nCounting the interpolations is easier than actually interpolating.\nWherever the code goes, the calls to interpolate() and interp_count()\nshould stay together.\n\n>> +\n>> +/*\n>> + * interp_count - count occurences of placeholders\n>> + */\n>> +void interp_count(unsigned long *result, const char *orig,\n>> +                  const struct interp *interps, int ninterps)\n>> +{\n>> +\tconst char *src = orig;\n> \n> You do not ever use orig again.  So why not just use that variable instead \n> of introducing a new one?\n\nI simply copied interpolate() and then chopped off the parts not needed\nfor counting, to make it easy to see that this is the smaller brother.\n\n> \n>> +\tconst char *name;\n>> +\tunsigned long namelen;\n>> +\tint i;\n>> +\tchar c;\n>> +\n>> +\tfor (i = 0; i < ninterps; i++)\n>> +\t\tresult[i] = 0;\n> \n> memset()?\n\nIn earlier versions there was a memset() there.  I replaced it to make\nthe intent even more clear, but I guess not using memset() is simply\nsuperstition..\n\n> \n>> +\n>> +\twhile ((c = *src)) {\n>> +\t\tif (c == '%') {\n>> +\t\t\t/* Try to match an interpolation string. */\n>> +\t\t\tfor (i = 0; i < ninterps; i++) {\n>> +\t\t\t\tname = interps[i].name;\n>> +\t\t\t\tnamelen = strlen(name);\n>> +\t\t\t\tif (strncmp(src, name, namelen) == 0)\n> \n> prefixcmp()?\n\nCopied from interpolate()..\n\n>> +\t\t\t\t\tbreak;\n>> +\t\t\t}\n>> +\n>> +\t\t\t/* Check for valid interpolation. */\n>> +\t\t\tif (i < ninterps) {\n> \n> This is ugly.  You already had a successful if() for that case earlier.\n\nDito..\n\n> \n>> +\t\t\t\tresult[i]++;\n>> +\t\t\t\tsrc += namelen;\n>> +\t\t\t\tcontinue;\n>> +\t\t\t}\n>> +\t\t}\n>> +\t\tsrc++;\n>> +\t}\n>> +}\n> \n> I'd rewrite this whole loop as\n> \n> \twhile ((c = *(orig++)))\n> \t\tif (c == '%')\n> \t\t\t/* Try to match an interpolation string. */\n> \t\t\tfor (i = 0; i < ninterps; i++)\n> \t\t\t\tif (prefixcmp(orig, interps[i].name)) {\n> \t\t\t\t\tresult[i] = 1;\n> \t\t\t\t\torig += strlen(interps[i].name);\n> \t\t\t\t\tbreak;\n> \t\t\t\t}\n\nCleanups are sure possible, but they should be done on top, and to both\ninterpolate() and interp_count().  Let's first see how far we get with\ndumb code-copying and reusing the result in new ways. :)\n\nThanks,\nRené\n"},{"id":"58280","messageId":"Pine.LNX.4.64.0711041518130.4362@racer.site","threadId":"10644","inReplyTo":"472DDA3B.4090100@lsrfire.ath.cx","subject":"Re: [PATCH 5/5] pretty describe: add %ds, %dn, %dd placeholders","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-04T15:25:47Z","receivedAt":"2007-11-04T15:25:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 4 Nov 2007, Ren? Scharfe wrote:\n\n> Johannes Schindelin schrieb:\n> \n> > On Sun, 4 Nov 2007, Ren? Scharfe wrote:\n> > \n> >> +\tunsigned long occurs[ARRAY_SIZE(table)];\n> > \n> > You do not ever use the counts.  So, longs are overkill.  Even ints \n> > might be overkill, but probably the most convenient.  I would have \n> > gone with chars.  If I knew how to memset() an array of unsigned:1 \n> > entries to all zero, I would even have gone with that, but the runtime \n> > cost of this is probably higher than the chars.\n> \n> Well, it isn't used in format_commit_message() currently, but it could \n> be.  Multiply the count and and the length of each substitution (minus \n> the length of the placeholder) and you get the number of bytes you need \n> to allocate.  interpolate() wouldn't need to be called twice anymore.\n\nThe better change, of course, would be to migrate interpolate() to strbuf.  \nThen you do not have to play clever tricks anymore.\n\n> > But the even more fundamental problem is that you count the needed \n> > interpolations _every_ single time you output a commit message.\n> > \n> > A much better place would be get_commit_format().  Yes that means \n> > restructuring the code a bit more, but I would say that this definitely \n> > would help.  My preference would even be introducing a new source file for \n> > the user format handling (commit-format.[ch]).\n> \n> Counting the interpolations is easier than actually interpolating. \n> Wherever the code goes, the calls to interpolate() and interp_count() \n> should stay together.\n\nNo.\n\nThe purpose of calling interp_count() is to know what placeholders have to \nbe filled with substitutes.  As a consequence, the _logical_ thing to do \nis call interp_count() _once_.\n\nIt makes absolutely no sense to call the function over and over again, \nonly to end up with the same result over and over again.\n\n> >> +\n> >> +/*\n> >> + * interp_count - count occurences of placeholders\n> >> + */\n> >> +void interp_count(unsigned long *result, const char *orig,\n> >> +                  const struct interp *interps, int ninterps)\n> >> +{\n> >> +\tconst char *src = orig;\n> > \n> > You do not ever use orig again.  So why not just use that variable instead \n> > of introducing a new one?\n> \n> I simply copied interpolate() and then chopped off the parts not needed\n> for counting, to make it easy to see that this is the smaller brother.\n\nIt is not.  It does not do any substitution.  It is a pure helper for the \nprocess of filling the interpolation table.\n\n> > I'd rewrite this whole loop as\n> > \n> > \twhile ((c = *(orig++)))\n> > \t\tif (c == '%')\n> > \t\t\t/* Try to match an interpolation string. */\n> > \t\t\tfor (i = 0; i < ninterps; i++)\n> > \t\t\t\tif (prefixcmp(orig, interps[i].name)) {\n> > \t\t\t\t\tresult[i] = 1;\n> > \t\t\t\t\torig += strlen(interps[i].name);\n> > \t\t\t\t\tbreak;\n> > \t\t\t\t}\n> \n> Cleanups are sure possible, but they should be done on top, and to both \n> interpolate() and interp_count().  Let's first see how far we get with \n> dumb code-copying and reusing the result in new ways. :)\n\nCode copying is one of the primary sources for bad code.  Let's not even \nstart.\n\nIMHO there have to be _very_ good reasons to commit something that you \nplan to fix later anyway.\n\nOne such good reason would be that it is too hard to do in one go.  \nAnother good reason would be that you think the fix is not even needed \n(like I did when I wrote format: in the first place; I am quite surprised \nthat after _that_ long a time people complain -- I'd have expected \ncomplaints right away or never).\n\nIn this case, I see no reason why we should go for suboptimal code first.\n\nBut hey, if you do not want to do it, I'll do it.  Just say so.\n\nCiao,\nDscho\n"},{"id":"58285","messageId":"472E010B.5070602@lsrfire.ath.cx","threadId":"10644","inReplyTo":"Pine.LNX.4.64.0711041518130.4362@racer.site","subject":"Re: [PATCH 5/5] pretty describe: add %ds, %dn, %dd placeholders","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-04T17:27:39Z","receivedAt":"2007-11-04T17:27:39Z","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 Sun, 4 Nov 2007, Ren? Scharfe wrote:\n> \n>> Johannes Schindelin schrieb:\n>>\n>>> On Sun, 4 Nov 2007, Ren? Scharfe wrote:\n>>>\n>>>> +\tunsigned long occurs[ARRAY_SIZE(table)];\n>>> You do not ever use the counts.  So, longs are overkill.  Even ints \n>>> might be overkill, but probably the most convenient.  I would have \n>>> gone with chars.  If I knew how to memset() an array of unsigned:1 \n>>> entries to all zero, I would even have gone with that, but the runtime \n>>> cost of this is probably higher than the chars.\n>> Well, it isn't used in format_commit_message() currently, but it could \n>> be.  Multiply the count and and the length of each substitution (minus \n>> the length of the placeholder) and you get the number of bytes you need \n>> to allocate.  interpolate() wouldn't need to be called twice anymore.\n> \n> The better change, of course, would be to migrate interpolate() to strbuf.  \n> Then you do not have to play clever tricks anymore.\n>\n>>> But the even more fundamental problem is that you count the needed \n>>> interpolations _every_ single time you output a commit message.\n>>>\n>>> A much better place would be get_commit_format().  Yes that means \n>>> restructuring the code a bit more, but I would say that this definitely \n>>> would help.  My preference would even be introducing a new source file for \n>>> the user format handling (commit-format.[ch]).\n>> Counting the interpolations is easier than actually interpolating. \n>> Wherever the code goes, the calls to interpolate() and interp_count() \n>> should stay together.\n> \n> No.\n> \n> The purpose of calling interp_count() is to know what placeholders have to \n> be filled with substitutes.  As a consequence, the _logical_ thing to do \n> is call interp_count() _once_.\n> \n> It makes absolutely no sense to call the function over and over again, \n> only to end up with the same result over and over again.\n\nTo allow this optimization, you need to make the (not yet filled)\ninterpolation table available to the new callsite of interp_count().\nAnd you need to somehow pass the result of interp_count() from every\ncaller of it to the setup code in format_commit_message().\n\nTo see if it's worthwhile, I've just replaced the array \"occurs\" and the\ncall to interp_count() with a static array, and measured the runtime.\nThe speed difference was lost in the noise.\n\n>>>> +\n>>>> +/*\n>>>> + * interp_count - count occurences of placeholders\n>>>> + */\n>>>> +void interp_count(unsigned long *result, const char *orig,\n>>>> +                  const struct interp *interps, int ninterps)\n>>>> +{\n>>>> +\tconst char *src = orig;\n>>> You do not ever use orig again.  So why not just use that variable instead \n>>> of introducing a new one?\n>> I simply copied interpolate() and then chopped off the parts not needed\n>> for counting, to make it easy to see that this is the smaller brother.\n> \n> It is not.  It does not do any substitution.  It is a pure helper for the \n> process of filling the interpolation table.\n\nSure.  It's important, though, that it reports the same number of\nsubstitutions as interpolate() later actually performs.  Correctness\ntrumps cleanliness, and its easier to check that a copy is correct, even\nif certain pieces are missing.\n\n>>> I'd rewrite this whole loop as\n>>>\n>>> \twhile ((c = *(orig++)))\n>>> \t\tif (c == '%')\n>>> \t\t\t/* Try to match an interpolation string. */\n>>> \t\t\tfor (i = 0; i < ninterps; i++)\n>>> \t\t\t\tif (prefixcmp(orig, interps[i].name)) {\n>>> \t\t\t\t\tresult[i] = 1;\n>>> \t\t\t\t\torig += strlen(interps[i].name);\n>>> \t\t\t\t\tbreak;\n>>> \t\t\t\t}\n>> Cleanups are sure possible, but they should be done on top, and to both \n>> interpolate() and interp_count().  Let's first see how far we get with \n>> dumb code-copying and reusing the result in new ways. :)\n> \n> Code copying is one of the primary sources for bad code.  Let's not even \n> start.\n> \n> IMHO there have to be _very_ good reasons to commit something that you \n> plan to fix later anyway.\n\nCode copying can be bad if one copies bugs.  But code copying allows a\nstrange feat: new code can inherit maturity.  If you copy known good\ncode and then change it in trivial ways (keeping the structure etc.) to\nmake it do new things, then the chance of a bug creeping in is lower\nthan if you wrote that piece of code anew.\n\n> One such good reason would be that it is too hard to do in one go.  \n> Another good reason would be that you think the fix is not even needed \n> (like I did when I wrote format: in the first place; I am quite surprised \n> that after _that_ long a time people complain -- I'd have expected \n> complaints right away or never).\n\nNot everybody is as fast as you, Dscho. ;-)\n\nAnother idea that I was kicking around, but didn't get time to\nimplement: a performance regression test suite, i.e. make test for\ntimings and memory usages.\n\n> In this case, I see no reason why we should go for suboptimal code first.\n> \n> But hey, if you do not want to do it, I'll do it.  Just say so.\n\nBusted again!  I wanted to see if someone else would pick up the\njanitorial work for me. :-)\n\nIn any case, interpolate.c needs some attention, with or without my\npatch.  I agree that a native strbuf version would be nice.  How about\nan interface like that:\n\n\ttypedef const char *(*expand_fn_t)\n\t\t(const char *placeholder, void *context);\n\tvoid strbuf_addexpand(struct strbuf *sb, const char *format,\n\t                      const char **placeholders,\n\t                      expand_fn_t fn, void *context);\n\nstrbuf_addexpand() would call fn() when it recognizes a placeholder,\navoiding unneeded setup code.  It could cache the result, so that fn()\ngets called at most a single time for each given placeholder.  context\nwould be passed through to fn(), e.g. a struct commit in case of\nformat_commit_message().  Makes sense?\n\nThanks,\nRené\n"},{"id":"58332","messageId":"472E6FD9.7030603@lsrfire.ath.cx","threadId":"10644","inReplyTo":"472E010B.5070602@lsrfire.ath.cx","subject":"Re: [PATCH 5/5] pretty describe: add %ds, %dn, %dd placeholders","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2007-11-05T01:20:25Z","receivedAt":"2007-11-05T01:20:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"René Scharfe schrieb:\n> In any case, interpolate.c needs some attention, with or without my\n> patch.  I agree that a native strbuf version would be nice.  How about\n> an interface like that:\n> \n> \ttypedef const char *(*expand_fn_t)\n> \t\t(const char *placeholder, void *context);\n> \tvoid strbuf_addexpand(struct strbuf *sb, const char *format,\n> \t                      const char **placeholders,\n> \t                      expand_fn_t fn, void *context);\n> \n> strbuf_addexpand() would call fn() when it recognizes a placeholder,\n> avoiding unneeded setup code.  It could cache the result, so that fn()\n> gets called at most a single time for each given placeholder.  context\n> would be passed through to fn(), e.g. a struct commit in case of\n> format_commit_message().  Makes sense?\n\nOK, for discussion, I've experimented a bit, and here's what I could\ncome up with.  This version doesn't do any caching.  Is it even needed?\n\nAnyway, strbuf_expand() is really simple, and the callback is expected\nto add it's output directly to the strbuf.  That means that the number\nof strdups and memdupz is reduced significantly, speeding up the whole\nthing.  Converting code to use strbuf_expand is easy, but not trivial\n(i.e. not doable by a script).\n\nRené\n\n\n commit.c |  275 ++++++++++++++++++++++++++++++++++---------------------------\n strbuf.c |   22 +++++\n strbuf.h |    3 +\n 3 files changed, 178 insertions(+), 122 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 8262f6a..4a47b0e 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -698,7 +698,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@@ -710,7 +711,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@@ -722,7 +726,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@@ -733,7 +740,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@@ -744,123 +754,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+\tstruct 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@@ -868,29 +862,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,\ndiff --git a/strbuf.c b/strbuf.c\nindex f4201e1..b71da99 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -129,6 +129,28 @@ 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 *fmt,\n+                   const char **placeholders, expand_fn_t fn, void *context)\n+{\n+\tchar c;\n+\tconst char **p;\n+\n+\twhile ((c = *fmt++)) {\n+\t\tif (c != '%') {\n+\t\t\tstrbuf_addch(sb, c);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tfor (p = placeholders; *p; p++) {\n+\t\t\tif (!prefixcmp(fmt, *p)) {\n+\t\t\t\tfn(sb, *p, context);\n+\t\t\t\tfmt += strlen(*p);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\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 9b9e861..b9c3e79 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 *fmt, 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 \n"}]}