{"thread":{"id":"32292","subject":"[PATCH] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix)","startedAt":"2012-12-08T19:09:27Z","lastAt":"2012-12-11T10:24:44Z","messageCount":6,"participants":["Steffen Prohaska","Junio C Hamano","Jan H. Schönherr"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"204607","messageId":"1354993767-7455-1-git-send-email-prohaska@zib.de","threadId":"32292","inReplyTo":null,"subject":"[PATCH] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix)","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2012-12-08T19:09:27Z","receivedAt":"2012-12-08T19:09:27Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"A recent commit [1] fixed a off-by-one wrapping error. As\na side-effect, add_wrapped_shortlog_msg() needs to be changed to always\nappend a newline.\n\n[1] 14e1a4e1ff70aff36db3f5d2a8b806efd0134d50 utf8: fix off-by-one\n wrapping of text\n\nSigned-off-by: Steffen Prohaska \n---\n builtin/shortlog.c | 3 +--\n t/t4201-shortlog.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex b316cf3..db5b57d 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -307,8 +307,7 @@ 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+\tstrbuf_addch(sb, '\\n');\n }\n \n void shortlog_output(struct shortlog *log)\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 6872ba1..02ac978 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -120,6 +120,30 @@ test_expect_success 'shortlog from non-git directory' '\n \ttest_cmp expect out\n '\n \n+test_expect_success 'shortlog should add newline when input line matches wraplen' '\n+\tcat >expect <<\\EOF &&\n+A U Thor (2):\n+ bbbbbbbbbbbbbbbbbb: bbbbbbbb bbb bbbb bbbbbbb bb bbbb bbb bbbbb bbbbbb\n+ aaaaaaaaaaaaaaaaaaaaaa: aaaaaa aaaaaaaaaa aaaa aaaaaaaa aa aaaa aa aaa\n+\n+EOF\n+\tgit shortlog -w >out <<\\EOF &&\n+commit 0000000000000000000000000000000000000001\n+Author: A U Thor \n+Date: Thu Apr 7 15:14:13 2005 -0700\n+\n+ aaaaaaaaaaaaaaaaaaaaaa: aaaaaa aaaaaaaaaa aaaa aaaaaaaa aa aaaa aa aaa\n+ \n+commit 0000000000000000000000000000000000000002\n+Author: A U Thor \n+Date: Thu Apr 7 15:14:13 2005 -0700\n+\n+ bbbbbbbbbbbbbbbbbb: bbbbbbbb bbb bbbb bbbbbbb bb bbbb bbb bbbbb bbbbbb\n+ \n+EOF\n+\ttest_cmp expect out\n+'\n+\n iconvfromutf8toiso88591() {\n \tprintf \"%s\" \"$*\" | iconv -f UTF-8 -t ISO8859-1\n }\n-- \n1.8.1.rc1.2.gfb98a3a\n"},{"id":"204622","messageId":"7v8v97efdv.fsf@alter.siamese.dyndns.org","threadId":"32292","inReplyTo":"1354993767-7455-1-git-send-email-prohaska@zib.de","subject":"Re: [PATCH] shortlog: Fix wrapping lines of wraplen","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-09T09:36:28Z","receivedAt":"2012-12-09T09:36:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska writes:\n\n> A recent commit [1] fixed a off-by-one wrapping error. As\n> a side-effect, add_wrapped_shortlog_msg() needs to be changed to always\n> append a newline.\n\nCould you clarify \"As a side effect\" a bit more? Do you mean\nsomething like this?\n\n Earlier strbuf_add_wrapped_text() ended its output with a\n newline only when the end of the text exactly fitted in wrap\n length, due to the off-by-one error fixed with 14e1a4e (utf8:\n fix off-by-one wrapping of text, 2012-10-18). There was a hack\n in add_wrapped_shortlog_msg() function to compensate for this\n bug.\n\n With the bug fixed, the function never ends its output with a\n newline, and the caller needs to unconditionally add one.\n\n\n>\n> [1] 14e1a4e1ff70aff36db3f5d2a8b806efd0134d50 utf8: fix off-by-one\n> wrapping of text\n>\n> Signed-off-by: Steffen Prohaska \n> ---\n> builtin/shortlog.c | 3 +--\n> t/t4201-shortlog.sh | 24 ++++++++++++++++++++++++\n> 2 files changed, 25 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> index b316cf3..db5b57d 100644\n> --- a/builtin/shortlog.c\n> +++ b/builtin/shortlog.c\n> @@ -307,8 +307,7 @@ 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> +\tstrbuf_addch(sb, '\\n');\n> }\n> \n> void shortlog_output(struct shortlog *log)\n> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\n> index 6872ba1..02ac978 100755\n> --- a/t/t4201-shortlog.sh\n> +++ b/t/t4201-shortlog.sh\n> @@ -120,6 +120,30 @@ test_expect_success 'shortlog from non-git directory' '\n> \ttest_cmp expect out\n> '\n> \n> +test_expect_success 'shortlog should add newline when input line matches wraplen' '\n> +\tcat >expect <<\\EOF &&\n> +A U Thor (2):\n> + bbbbbbbbbbbbbbbbbb: bbbbbbbb bbb bbbb bbbbbbb bb bbbb bbb bbbbb bbbbbb\n> + aaaaaaaaaaaaaaaaaaaaaa: aaaaaa aaaaaaaaaa aaaa aaaaaaaa aa aaaa aa aaa\n> +\n> +EOF\n> +\tgit shortlog -w >out <<\\EOF &&\n> +commit 0000000000000000000000000000000000000001\n> +Author: A U Thor \n> +Date: Thu Apr 7 15:14:13 2005 -0700\n> +\n> + aaaaaaaaaaaaaaaaaaaaaa: aaaaaa aaaaaaaaaa aaaa aaaaaaaa aa aaaa aa aaa\n> + \n> +commit 0000000000000000000000000000000000000002\n> +Author: A U Thor \n> +Date: Thu Apr 7 15:14:13 2005 -0700\n> +\n> + bbbbbbbbbbbbbbbbbb: bbbbbbbb bbb bbbb bbbbbbb bb bbbb bbb bbbbb bbbbbb\n> + \n> +EOF\n> +\ttest_cmp expect out\n> +'\n> +\n> iconvfromutf8toiso88591() {\n> \tprintf \"%s\" \"$*\" | iconv -f UTF-8 -t ISO8859-1\n> }\n"},{"id":"204671","messageId":"1355205562-23459-1-git-send-email-prohaska@zib.de","threadId":"32292","inReplyTo":"7v8v97efdv.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/2] Re: [PATCH] shortlog: Fix wrapping lines of wraplen","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2012-12-11T05:59:20Z","receivedAt":"2012-12-11T05:59:20Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"On Dec 9, 2012, at 10:36 AM, Junio C Hamano wrote:\n\n> Steffen Prohaska writes:\n> \n> > A recent commit [1] fixed a off-by-one wrapping error. As\n> > a side-effect, add_wrapped_shortlog_msg() needs to be changed to always\n> > append a newline.\n> \n> Could you clarify \"As a side effect\" a bit more? Do you mean\n> something like this?\n\nSee updated patches below.\n\n Steffen\n\nSteffen Prohaska (2):\n shortlog: Fix wrapping lines of wraplen (was broken since recent\n off-by-one fix)\n strbuf_add_wrapped*(): Remove unused return value\n\n builtin/shortlog.c | 5 ++---\n t/t4201-shortlog.sh | 24 ++++++++++++++++++++++++\n utf8.c | 13 ++++++-------\n utf8.h | 4 ++--\n 4 files changed, 34 insertions(+), 12 deletions(-)\n\n-- \n1.8.1.rc1.2.gfb98a3a\n"},{"id":"204673","messageId":"1355205562-23459-2-git-send-email-prohaska@zib.de","threadId":"32292","inReplyTo":"1355205562-23459-1-git-send-email-prohaska@zib.de","subject":"[PATCH 1/2] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix)","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2012-12-11T05:59:21Z","receivedAt":"2012-12-11T05:59:21Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"A recent commit [1] fixed a off-by-one wrapping error. As\na side-effect, the conditional in add_wrapped_shortlog_msg() whether to\nappend a newline needs to be removed. add_wrapped_shortlog_msg() should\nalways append a newline, which was the case before the off-by-one fix,\nbecause strbuf_add_wrapped_text() never returned a value of wraplen.\n\n[1] 14e1a4e1ff70aff36db3f5d2a8b806efd0134d50 utf8: fix off-by-one\n wrapping of text\n\nSigned-off-by: Steffen Prohaska \n---\n builtin/shortlog.c | 5 ++---\n t/t4201-shortlog.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex b316cf3..8360514 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -306,9 +306,8 @@ parse_done:\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+\tstrbuf_add_wrapped_text(sb, s, log->in1, log->in2, log->wrap);\n+\tstrbuf_addch(sb, '\\n');\n }\n \n void shortlog_output(struct shortlog *log)\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 6872ba1..02ac978 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -120,6 +120,30 @@ test_expect_success 'shortlog from non-git directory' '\n \ttest_cmp expect out\n '\n \n+test_expect_success 'shortlog should add newline when input line matches wraplen' '\n+\tcat >expect <<\\EOF &&\n+A U Thor (2):\n+ bbbbbbbbbbbbbbbbbb: bbbbbbbb bbb bbbb bbbbbbb bb bbbb bbb bbbbb bbbbbb\n+ aaaaaaaaaaaaaaaaaaaaaa: aaaaaa aaaaaaaaaa aaaa aaaaaaaa aa aaaa aa aaa\n+\n+EOF\n+\tgit shortlog -w >out <<\\EOF &&\n+commit 0000000000000000000000000000000000000001\n+Author: A U Thor \n+Date: Thu Apr 7 15:14:13 2005 -0700\n+\n+ aaaaaaaaaaaaaaaaaaaaaa: aaaaaa aaaaaaaaaa aaaa aaaaaaaa aa aaaa aa aaa\n+ \n+commit 0000000000000000000000000000000000000002\n+Author: A U Thor \n+Date: Thu Apr 7 15:14:13 2005 -0700\n+\n+ bbbbbbbbbbbbbbbbbb: bbbbbbbb bbb bbbb bbbbbbb bb bbbb bbb bbbbb bbbbbb\n+ \n+EOF\n+\ttest_cmp expect out\n+'\n+\n iconvfromutf8toiso88591() {\n \tprintf \"%s\" \"$*\" | iconv -f UTF-8 -t ISO8859-1\n }\n-- \n1.8.1.rc1.2.gfb98a3a\n"},{"id":"204672","messageId":"1355205562-23459-3-git-send-email-prohaska@zib.de","threadId":"32292","inReplyTo":"1355205562-23459-1-git-send-email-prohaska@zib.de","subject":"[PATCH 2/2] strbuf_add_wrapped*(): Remove unused return value","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2012-12-11T05:59:22Z","receivedAt":"2012-12-11T05:59:22Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"Since shortlog isn't using the return value anymore (see previous\ncommit), the functions can be changed to void.\n\nSigned-off-by: Steffen Prohaska \n---\n utf8.c | 13 ++++++-------\n utf8.h | 4 ++--\n 2 files changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/utf8.c b/utf8.c\nindex 5c61bbe..a4ee665 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -323,7 +323,7 @@ static size_t display_mode_esc_sequence_len(const char *s)\n * If indent is negative, assume that already -indent columns have been\n * consumed (and no extra indent is necessary for the first line).\n */\n-int strbuf_add_wrapped_text(struct strbuf *buf,\n+void strbuf_add_wrapped_text(struct strbuf *buf,\n \t\tconst char *text, int indent1, int indent2, int width)\n {\n \tint indent, w, assume_utf8 = 1;\n@@ -332,7 +332,7 @@ int strbuf_add_wrapped_text(struct strbuf *buf,\n \n \tif (width <= 0) {\n \t\tstrbuf_add_indented_text(buf, text, indent1, indent2);\n-\t\treturn 1;\n+\t\treturn;\n \t}\n \n retry:\n@@ -356,14 +356,14 @@ retry:\n \t\t\tif (w <= width || !space) {\n \t\t\t\tconst char *start = bol;\n \t\t\t\tif (!c && text == start)\n-\t\t\t\t\treturn w;\n+\t\t\t\t\treturn;\n \t\t\t\tif (space)\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_add(buf, start, text - start);\n \t\t\t\tif (!c)\n-\t\t\t\t\treturn w;\n+\t\t\t\t\treturn;\n \t\t\t\tspace = text;\n \t\t\t\tif (c == '\\t')\n \t\t\t\t\tw |= 0x07;\n@@ -405,13 +405,12 @@ new_line:\n \t}\n }\n \n-int strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len,\n+void strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len,\n \t\t\t int indent, int indent2, int width)\n {\n \tchar *tmp = xstrndup(data, len);\n-\tint r = strbuf_add_wrapped_text(buf, tmp, indent, indent2, width);\n+\tstrbuf_add_wrapped_text(buf, tmp, indent, indent2, width);\n \tfree(tmp);\n-\treturn r;\n }\n \n int is_encoding_utf8(const char *name)\ndiff --git a/utf8.h b/utf8.h\nindex 93ef600..a214238 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -9,9 +9,9 @@ int is_utf8(const char *text);\n int is_encoding_utf8(const char *name);\n int same_encoding(const char *, const char *);\n \n-int strbuf_add_wrapped_text(struct strbuf *buf,\n+void strbuf_add_wrapped_text(struct strbuf *buf,\n \t\tconst char *text, int indent, int indent2, int width);\n-int strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len,\n+void strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len,\n \t\t\t int indent, int indent2, int width);\n \n #ifndef NO_ICONV\n-- \n1.8.1.rc1.2.gfb98a3a\n"},{"id":"204674","messageId":"50C709EC.50102@cs.tu-berlin.de","threadId":"32292","inReplyTo":"1355205562-23459-2-git-send-email-prohaska@zib.de","subject":"Re: [PATCH 1/2] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix)","fromName":"Jan H. Schönherr","fromEmail":"schnhrr@cs.tu-berlin.de","sentAt":"2012-12-11T10:24:44Z","receivedAt":"2012-12-11T10:24:44Z","isPatch":true,"sender":{"key":"schnhrr@cs.tu-berlin.de","avatar":null},"body":"Am 11.12.2012 06:59, schrieb Steffen Prohaska:\n> A recent commit [1] fixed a off-by-one wrapping error. As\n> a side-effect, the conditional in add_wrapped_shortlog_msg() whether to\n> append a newline needs to be removed. add_wrapped_shortlog_msg() should\n> always append a newline, which was the case before the off-by-one fix,\n> because strbuf_add_wrapped_text() never returned a value of wraplen.\n\nI agree with this explanation, although there exists a case where wraplen \n(or wraplen+1 after the off-by-one fix) is returned: This happens\nwhen there is not a single space within the string and it has just the\ncorrect length. But also in this case, the newline must be added to get\na correctly formatted output. So your patch is good as it is. :)\n\nBut I still wonder about the original motivation for the removed\nconditional. It looks like, it wasn't even needed in the very first\nversion (3714e7c8)?! (And it wasn't present in the version on the mailing \nlist: http://article.gmane.org/gmane.comp.version-control.git/35221)\n\nRegards\nJan\n"}]}