{"thread":{"id":"22733","subject":"[PATCH 0/4] utf8.c: strbuf'ify strbuf_add_wrapped_text()","startedAt":"2010-02-19T22:13:28Z","lastAt":"2010-02-20T19:22:27Z","messageCount":8,"participants":["René Scharfe","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"135114","messageId":"4B7F0D08.6040608@lsrfire.ath.cx","threadId":"22733","inReplyTo":null,"subject":"[PATCH 0/4] utf8.c: strbuf'ify strbuf_add_wrapped_text()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-02-19T22:13:28Z","receivedAt":"2010-02-19T22:13:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The strbuf parameter of strbuf_add_wrapped_text() in utf8.c is\noptional; if it's missing, results are directly written to stdout.\nBy removing this flexibility and converting it to only call \"pure\"\nstrbuf functions, this series cleans up the code a bit and enables\nan optimization, which is added in the last patch:\n\n  [PATCH 1/4] utf8.c: remove print_wrapped_text()\n  [PATCH 2/4] utf8.c: remove print_spaces()\n  [PATCH 3/4] utf8.c: remove strbuf_write()\n\n  [PATCH 4/4] utf8.c: speculatively assume utf-8 in strbuf_add_wrapped_text()\n\n builtin-shortlog.c |   17 ++++++++++++--\n utf8.c             |   61 +++++++++++++++++++++++----------------------------\n utf8.h             |    1 -\n 3 files changed, 42 insertions(+), 37 deletions(-)\n"},{"id":"135115","messageId":"4B7F0D65.1040602@lsrfire.ath.cx","threadId":"22733","inReplyTo":"4B7F0D08.6040608@lsrfire.ath.cx","subject":"[PATCH 1/4] utf8.c: remove print_wrapped_text()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-02-19T22:15:01Z","receivedAt":"2010-02-19T22:15:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"print_wrapped_text() is the only function that calls\nstrbuf_add_wrapped_text() without a strbuf, which then writes its\nresults to stdout.  At its only callside, supply a strbuf, call\nstrbuf_add_wrapped_text() directly and remove the wrapper function.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n builtin-shortlog.c |   17 ++++++++++++++---\n utf8.c             |    5 -----\n utf8.h             |    1 -\n 3 files changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-shortlog.c b/builtin-shortlog.c\nindex b3b055f..ecd2d45 100644\n--- a/builtin-shortlog.c\n+++ b/builtin-shortlog.c\n@@ -304,9 +304,19 @@ parse_done:\n \treturn 0;\n }\n \n+static void add_wrapped_shortlog_msg(struct strbuf *sb, const char *s,\n+\t\t\t\t     const struct shortlog *log)\n+{\n+\tint col = strbuf_add_wrapped_text(sb, s, log->in1, log->in2, log->wrap);\n+\tif (col != log->wrap)\n+\t\tstrbuf_addch(sb, '\\n');\n+}\n+\n void shortlog_output(struct shortlog *log)\n {\n \tint i, j;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n \tif (log->sort_by_number)\n \t\tqsort(log->list.items, log->list.nr, sizeof(struct string_list_item),\n \t\t\tcompare_by_number);\n@@ -321,9 +331,9 @@ void shortlog_output(struct shortlog *log)\n \t\t\t\tconst char *msg = onelines->items[j].string;\n \n \t\t\t\tif (log->wrap_lines) {\n-\t\t\t\t\tint col = print_wrapped_text(msg, log->in1, log->in2, log->wrap);\n-\t\t\t\t\tif (col != log->wrap)\n-\t\t\t\t\t\tputchar('\\n');\n+\t\t\t\t\tstrbuf_reset(&sb);\n+\t\t\t\t\tadd_wrapped_shortlog_msg(&sb, msg, log);\n+\t\t\t\t\tfwrite(sb.buf, sb.len, 1, stdout);\n \t\t\t\t}\n \t\t\t\telse\n \t\t\t\t\tprintf(\"      %s\\n\", msg);\n@@ -337,6 +347,7 @@ void shortlog_output(struct shortlog *log)\n \t\tlog->list.items[i].util = NULL;\n \t}\n \n+\tstrbuf_release(&sb);\n \tlog->list.strdup_strings = 1;\n \tstring_list_clear(&log->list, 1);\n \tclear_mailmap(&log->mailmap);\ndiff --git a/utf8.c b/utf8.c\nindex ab326ac..831cad6 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -405,11 +405,6 @@ new_line:\n \t}\n }\n \n-int print_wrapped_text(const char *text, int indent, int indent2, int width)\n-{\n-\treturn strbuf_add_wrapped_text(NULL, text, indent, indent2, width);\n-}\n-\n int is_encoding_utf8(const char *name)\n {\n \tif (!name)\ndiff --git a/utf8.h b/utf8.h\nindex c9738d8..ebc4d2f 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -8,7 +8,6 @@ int utf8_strwidth(const char *string);\n int is_utf8(const char *text);\n int is_encoding_utf8(const char *name);\n \n-int print_wrapped_text(const char *text, int indent, int indent2, int len);\n int strbuf_add_wrapped_text(struct strbuf *buf,\n \t\tconst char *text, int indent, int indent2, int width);\n \n-- \n1.7.0\n"},{"id":"135116","messageId":"4B7F0D9B.2060904@lsrfire.ath.cx","threadId":"22733","inReplyTo":"4B7F0D08.6040608@lsrfire.ath.cx","subject":"[PATCH 2/4] utf8.c: remove print_spaces()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-02-19T22:15:55Z","receivedAt":"2010-02-19T22:15:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The previous patch made sure that strbuf_add_wrapped_text() (and thus\nstrbuf_add_indented_text(), too) always get a strbuf.  Make use of\nthis fact by adding strbuf_addchars(), a small helper that adds a\nchar the specified number of times to a strbuf, and use it to replace\nprint_spaces().\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n utf8.c |   15 ++++++---------\n 1 files changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/utf8.c b/utf8.c\nindex 831cad6..5146d30 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -288,14 +288,11 @@ static inline void strbuf_write(struct strbuf *sb, const char *buf, int len)\n \t\tfwrite(buf, len, 1, stdout);\n }\n \n-static void print_spaces(struct strbuf *buf, int count)\n+static void strbuf_addchars(struct strbuf *sb, int c, size_t n)\n {\n-\tstatic const char s[] = \"                    \";\n-\twhile (count >= sizeof(s)) {\n-\t\tstrbuf_write(buf, s, sizeof(s) - 1);\n-\t\tcount -= sizeof(s) - 1;\n-\t}\n-\tstrbuf_write(buf, s, count);\n+\tstrbuf_grow(sb, n);\n+\tmemset(sb->buf + sb->len, c, n);\n+\tstrbuf_setlen(sb, sb->len + n);\n }\n \n static void strbuf_add_indented_text(struct strbuf *buf, const char *text,\n@@ -307,7 +304,7 @@ static void strbuf_add_indented_text(struct strbuf *buf, const char *text,\n \t\tconst char *eol = strchrnul(text, '\\n');\n \t\tif (*eol == '\\n')\n \t\t\teol++;\n-\t\tprint_spaces(buf, indent);\n+\t\tstrbuf_addchars(buf, ' ', indent);\n \t\tstrbuf_write(buf, text, eol - text);\n \t\ttext = eol;\n \t\tindent = indent2;\n@@ -366,7 +363,7 @@ int strbuf_add_wrapped_text(struct strbuf *buf,\n \t\t\t\tif (space)\n \t\t\t\t\tstart = space;\n \t\t\t\telse\n-\t\t\t\t\tprint_spaces(buf, indent);\n+\t\t\t\t\tstrbuf_addchars(buf, ' ', indent);\n \t\t\t\tstrbuf_write(buf, start, text - start);\n \t\t\t\tif (!c)\n \t\t\t\t\treturn w;\n-- \n1.7.0\n"},{"id":"135117","messageId":"4B7F0DCD.6040006@lsrfire.ath.cx","threadId":"22733","inReplyTo":"4B7F0D08.6040608@lsrfire.ath.cx","subject":"[PATCH 3/4] utf8.c: remove strbuf_write()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-02-19T22:16:45Z","receivedAt":"2010-02-19T22:16:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The patch before the previous one made sure that all callers of\nstrbuf_add_wrapped_text() supply a strbuf.  Replace all calls of\nstrbuf_write() with regular strbuf functions and remove it.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n utf8.c |   18 +++++-------------\n 1 files changed, 5 insertions(+), 13 deletions(-)\n\ndiff --git a/utf8.c b/utf8.c\nindex 5146d30..87437b0 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -280,14 +280,6 @@ int is_utf8(const char *text)\n \treturn 1;\n }\n \n-static inline void strbuf_write(struct strbuf *sb, const char *buf, int len)\n-{\n-\tif (sb)\n-\t\tstrbuf_insert(sb, sb->len, buf, len);\n-\telse\n-\t\tfwrite(buf, len, 1, stdout);\n-}\n-\n static void strbuf_addchars(struct strbuf *sb, int c, size_t n)\n {\n \tstrbuf_grow(sb, n);\n@@ -305,7 +297,7 @@ static void strbuf_add_indented_text(struct strbuf *buf, const char *text,\n \t\tif (*eol == '\\n')\n \t\t\teol++;\n \t\tstrbuf_addchars(buf, ' ', indent);\n-\t\tstrbuf_write(buf, text, eol - text);\n+\t\tstrbuf_add(buf, text, eol - text);\n \t\ttext = eol;\n \t\tindent = indent2;\n \t}\n@@ -364,7 +356,7 @@ int strbuf_add_wrapped_text(struct strbuf *buf,\n \t\t\t\t\tstart = space;\n \t\t\t\telse\n \t\t\t\t\tstrbuf_addchars(buf, ' ', indent);\n-\t\t\t\tstrbuf_write(buf, start, text - start);\n+\t\t\t\tstrbuf_add(buf, start, text - start);\n \t\t\t\tif (!c)\n \t\t\t\t\treturn w;\n \t\t\t\tspace = text;\n@@ -373,20 +365,20 @@ int strbuf_add_wrapped_text(struct strbuf *buf,\n \t\t\t\telse if (c == '\\n') {\n \t\t\t\t\tspace++;\n \t\t\t\t\tif (*space == '\\n') {\n-\t\t\t\t\t\tstrbuf_write(buf, \"\\n\", 1);\n+\t\t\t\t\t\tstrbuf_addch(buf, '\\n');\n \t\t\t\t\t\tgoto new_line;\n \t\t\t\t\t}\n \t\t\t\t\telse if (!isalnum(*space))\n \t\t\t\t\t\tgoto new_line;\n \t\t\t\t\telse\n-\t\t\t\t\t\tstrbuf_write(buf, \" \", 1);\n+\t\t\t\t\t\tstrbuf_addch(buf, ' ');\n \t\t\t\t}\n \t\t\t\tw++;\n \t\t\t\ttext++;\n \t\t\t}\n \t\t\telse {\n new_line:\n-\t\t\t\tstrbuf_write(buf, \"\\n\", 1);\n+\t\t\t\tstrbuf_addch(buf, '\\n');\n \t\t\t\ttext = bol = space + isspace(*space);\n \t\t\t\tspace = NULL;\n \t\t\t\tw = indent = indent2;\n-- \n1.7.0\n"},{"id":"135118","messageId":"4B7F0EBC.4060209@lsrfire.ath.cx","threadId":"22733","inReplyTo":"4B7F0D08.6040608@lsrfire.ath.cx","subject":"[PATCH 4/4] utf8.c: speculatively assume utf-8 in strbuf_add_wrapped_text()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-02-19T22:20:44Z","receivedAt":"2010-02-19T22:20:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"is_utf8() works by calling utf8_width() for each character at the\nsupplied location.  In strbuf_add_wrapped_text(), we do that anyway\nwhile wrapping the lines.  So instead of checking the encoding\nbeforehand, optimistically assume that it's utf-8 and wrap along\nuntil an invalid character is hit, and when that happens start over.\n\nThis pays off if the text consists only of valid utf-8 characters.\nThe following command was run against the Linux kernel repo with\ngit 1.7.0:\n\n\t$ time git log --format='%b' v2.6.32 >/dev/null\n\n\treal\t0m2.679s\n\tuser\t0m2.580s\n\tsys\t0m0.100s\n\n\t$ time git log --format='%w(60,4,8)%b' >/dev/null\n\n\treal\t0m4.342s\n\tuser\t0m4.230s\n\tsys\t0m0.110s\n\nAnd with this patch series:\n\n\t$ time git log --format='%w(60,4,8)%b' >/dev/null\n\n\treal\t0m3.741s\n\tuser\t0m3.630s\n\tsys\t0m0.110s\n\nSo the cost of wrapping is reduced to 70% in this case.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\nMissing: numbers for a non-utf-8 repo.\n\n utf8.c |   23 +++++++++++++++++------\n 1 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/utf8.c b/utf8.c\nindex 87437b0..84cfc72 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -324,16 +324,21 @@ static size_t display_mode_esc_sequence_len(const char *s)\n  * consumed (and no extra indent is necessary for the first line).\n  */\n int strbuf_add_wrapped_text(struct strbuf *buf,\n-\t\tconst char *text, int indent, int indent2, int width)\n+\t\tconst char *text, int indent1, int indent2, int width)\n {\n-\tint w = indent, assume_utf8 = is_utf8(text);\n-\tconst char *bol = text, *space = NULL;\n+\tint indent, w, assume_utf8 = 1;\n+\tconst char *bol, *space, *start = text;\n+\tsize_t orig_len = buf->len;\n \n \tif (width <= 0) {\n-\t\tstrbuf_add_indented_text(buf, text, indent, indent2);\n+\t\tstrbuf_add_indented_text(buf, text, indent1, indent2);\n \t\treturn 1;\n \t}\n \n+retry:\n+\tbol = text;\n+\tw = indent = indent1;\n+\tspace = NULL;\n \tif (indent < 0) {\n \t\tw = -indent;\n \t\tspace = text;\n@@ -385,9 +390,15 @@ new_line:\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n-\t\tif (assume_utf8)\n+\t\tif (assume_utf8) {\n \t\t\tw += utf8_width(&text, NULL);\n-\t\telse {\n+\t\t\tif (!text) {\n+\t\t\t\tassume_utf8 = 0;\n+\t\t\t\ttext = start;\n+\t\t\t\tstrbuf_setlen(buf, orig_len);\n+\t\t\t\tgoto retry;\n+\t\t\t}\n+\t\t} else {\n \t\t\tw++;\n \t\t\ttext++;\n \t\t}\n-- \n1.7.0\n"},{"id":"135148","messageId":"alpine.DEB.1.00.1002201001290.20986@pacific.mpi-cbg.de","threadId":"22733","inReplyTo":"4B7F0D08.6040608@lsrfire.ath.cx","subject":"Re: [PATCH 0/4] utf8.c: strbuf'ify strbuf_add_wrapped_text()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-02-20T09:03:34Z","receivedAt":"2010-02-20T09:03:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 19 Feb 2010, René Scharfe wrote:\n\n> The strbuf parameter of strbuf_add_wrapped_text() in utf8.c is\n> optional; if it's missing, results are directly written to stdout.\n\nThe reasoning for it is my private 'git-fmt' program, which can reformat \nreally large text files that I did not want to be read in fully, \nconverted, and written out fully.\n\nBut as I am the only user of this feature, it makes sense to simplify the \ncode.\n\nCiao,\nDscho\n"},{"id":"135149","messageId":"alpine.DEB.1.00.1002201012300.20986@pacific.mpi-cbg.de","threadId":"22733","inReplyTo":"4B7F0EBC.4060209@lsrfire.ath.cx","subject":"Re: [PATCH 4/4] utf8.c: speculatively assume utf-8 in strbuf_add_wrapped_text()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-02-20T09:14:57Z","receivedAt":"2010-02-20T09:14:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 19 Feb 2010, René Scharfe wrote:\n\n> Missing: numbers for a non-utf-8 repo.\n\nSince the wrapping code only makes sense for UTF-8, I consider it a pilot \nerror to try to rewrap non-UTF-8 messages. So I think you are safe.\n\nAll of the patches look good to me,\nDscho\n"},{"id":"135180","messageId":"7vljenk8zw.fsf@alter.siamese.dyndns.org","threadId":"22733","inReplyTo":"alpine.DEB.1.00.1002201012300.20986@pacific.mpi-cbg.de","subject":"Re: [PATCH 4/4] utf8.c: speculatively assume utf-8 in strbuf_add_wrapped_text()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-20T19:22:27Z","receivedAt":"2010-02-20T19:22:27Z","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> All of the patches look good to me,\n\nThanks.\n"}]}