* [PATCH] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix)
@ 2012-12-08 19:09 Steffen Prohaska
2012-12-09 9:36 ` [PATCH] shortlog: Fix wrapping lines of wraplen Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Steffen Prohaska @ 2012-12-08 19:09 UTC (permalink / raw)
To: git, Junio C Hamano; +Cc: Jan H. Schoenherr, Steffen Prohaska
A recent commit [1] fixed a off-by-one wrapping error. As
a side-effect, add_wrapped_shortlog_msg() needs to be changed to always
append a newline.
[1] 14e1a4e1ff70aff36db3f5d2a8b806efd0134d50 utf8: fix off-by-one
wrapping of text
Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
builtin/shortlog.c | 3 +--
t/t4201-shortlog.sh | 24 ++++++++++++++++++++++++
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index b316cf3..db5b57d 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -307,8 +307,7 @@ static void add_wrapped_shortlog_msg(struct strbuf *sb, const char *s,
const struct shortlog *log)
{
int col = strbuf_add_wrapped_text(sb, s, log->in1, log->in2, log->wrap);
- if (col != log->wrap)
- strbuf_addch(sb, '\n');
+ strbuf_addch(sb, '\n');
}
void shortlog_output(struct shortlog *log)
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 6872ba1..02ac978 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -120,6 +120,30 @@ test_expect_success 'shortlog from non-git directory' '
test_cmp expect out
'
+test_expect_success 'shortlog should add newline when input line matches wraplen' '
+ cat >expect <<\EOF &&
+A U Thor (2):
+ bbbbbbbbbbbbbbbbbb: bbbbbbbb bbb bbbb bbbbbbb bb bbbb bbb bbbbb bbbbbb
+ aaaaaaaaaaaaaaaaaaaaaa: aaaaaa aaaaaaaaaa aaaa aaaaaaaa aa aaaa aa aaa
+
+EOF
+ git shortlog -w >out <<\EOF &&
+commit 0000000000000000000000000000000000000001
+Author: A U Thor <author@example.com>
+Date: Thu Apr 7 15:14:13 2005 -0700
+
+ aaaaaaaaaaaaaaaaaaaaaa: aaaaaa aaaaaaaaaa aaaa aaaaaaaa aa aaaa aa aaa
+
+commit 0000000000000000000000000000000000000002
+Author: A U Thor <author@example.com>
+Date: Thu Apr 7 15:14:13 2005 -0700
+
+ bbbbbbbbbbbbbbbbbb: bbbbbbbb bbb bbbb bbbbbbb bb bbbb bbb bbbbb bbbbbb
+
+EOF
+ test_cmp expect out
+'
+
iconvfromutf8toiso88591() {
printf "%s" "$*" | iconv -f UTF-8 -t ISO8859-1
}
--
1.8.1.rc1.2.gfb98a3a
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] shortlog: Fix wrapping lines of wraplen 2012-12-08 19:09 [PATCH] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix) Steffen Prohaska @ 2012-12-09 9:36 ` Junio C Hamano 2012-12-11 5:59 ` [PATCH 0/2] " Steffen Prohaska 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2012-12-09 9:36 UTC (permalink / raw) To: Steffen Prohaska; +Cc: git, Jan H. Schoenherr Steffen Prohaska <prohaska@zib.de> writes: > A recent commit [1] fixed a off-by-one wrapping error. As > a side-effect, add_wrapped_shortlog_msg() needs to be changed to always > append a newline. Could you clarify "As a side effect" a bit more? Do you mean something like this? Earlier strbuf_add_wrapped_text() ended its output with a newline only when the end of the text exactly fitted in wrap length, due to the off-by-one error fixed with 14e1a4e (utf8: fix off-by-one wrapping of text, 2012-10-18). There was a hack in add_wrapped_shortlog_msg() function to compensate for this bug. With the bug fixed, the function never ends its output with a newline, and the caller needs to unconditionally add one. > > [1] 14e1a4e1ff70aff36db3f5d2a8b806efd0134d50 utf8: fix off-by-one > wrapping of text > > Signed-off-by: Steffen Prohaska <prohaska@zib.de> > --- > builtin/shortlog.c | 3 +-- > t/t4201-shortlog.sh | 24 ++++++++++++++++++++++++ > 2 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/builtin/shortlog.c b/builtin/shortlog.c > index b316cf3..db5b57d 100644 > --- a/builtin/shortlog.c > +++ b/builtin/shortlog.c > @@ -307,8 +307,7 @@ static void add_wrapped_shortlog_msg(struct strbuf *sb, const char *s, > const struct shortlog *log) > { > int col = strbuf_add_wrapped_text(sb, s, log->in1, log->in2, log->wrap); > - if (col != log->wrap) > - strbuf_addch(sb, '\n'); > + strbuf_addch(sb, '\n'); > } > > void shortlog_output(struct shortlog *log) > diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh > index 6872ba1..02ac978 100755 > --- a/t/t4201-shortlog.sh > +++ b/t/t4201-shortlog.sh > @@ -120,6 +120,30 @@ test_expect_success 'shortlog from non-git directory' ' > test_cmp expect out > ' > > +test_expect_success 'shortlog should add newline when input line matches wraplen' ' > + cat >expect <<\EOF && > +A U Thor (2): > + bbbbbbbbbbbbbbbbbb: bbbbbbbb bbb bbbb bbbbbbb bb bbbb bbb bbbbb bbbbbb > + aaaaaaaaaaaaaaaaaaaaaa: aaaaaa aaaaaaaaaa aaaa aaaaaaaa aa aaaa aa aaa > + > +EOF > + git shortlog -w >out <<\EOF && > +commit 0000000000000000000000000000000000000001 > +Author: A U Thor <author@example.com> > +Date: Thu Apr 7 15:14:13 2005 -0700 > + > + aaaaaaaaaaaaaaaaaaaaaa: aaaaaa aaaaaaaaaa aaaa aaaaaaaa aa aaaa aa aaa > + > +commit 0000000000000000000000000000000000000002 > +Author: A U Thor <author@example.com> > +Date: Thu Apr 7 15:14:13 2005 -0700 > + > + bbbbbbbbbbbbbbbbbb: bbbbbbbb bbb bbbb bbbbbbb bb bbbb bbb bbbbb bbbbbb > + > +EOF > + test_cmp expect out > +' > + > iconvfromutf8toiso88591() { > printf "%s" "$*" | iconv -f UTF-8 -t ISO8859-1 > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 0/2] Re: [PATCH] shortlog: Fix wrapping lines of wraplen 2012-12-09 9:36 ` [PATCH] shortlog: Fix wrapping lines of wraplen Junio C Hamano @ 2012-12-11 5:59 ` Steffen Prohaska 2012-12-11 5:59 ` [PATCH 1/2] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix) Steffen Prohaska 2012-12-11 5:59 ` [PATCH 2/2] strbuf_add_wrapped*(): Remove unused return value Steffen Prohaska 0 siblings, 2 replies; 6+ messages in thread From: Steffen Prohaska @ 2012-12-11 5:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jan H. Schoenherr, Steffen Prohaska On Dec 9, 2012, at 10:36 AM, Junio C Hamano <gitster@pobox.com> wrote: > Steffen Prohaska <prohaska@zib.de> writes: > > > A recent commit [1] fixed a off-by-one wrapping error. As > > a side-effect, add_wrapped_shortlog_msg() needs to be changed to always > > append a newline. > > Could you clarify "As a side effect" a bit more? Do you mean > something like this? See updated patches below. Steffen Steffen Prohaska (2): shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix) strbuf_add_wrapped*(): Remove unused return value builtin/shortlog.c | 5 ++--- t/t4201-shortlog.sh | 24 ++++++++++++++++++++++++ utf8.c | 13 ++++++------- utf8.h | 4 ++-- 4 files changed, 34 insertions(+), 12 deletions(-) -- 1.8.1.rc1.2.gfb98a3a ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix) 2012-12-11 5:59 ` [PATCH 0/2] " Steffen Prohaska @ 2012-12-11 5:59 ` Steffen Prohaska 2012-12-11 10:24 ` "Jan H. Schönherr" 2012-12-11 5:59 ` [PATCH 2/2] strbuf_add_wrapped*(): Remove unused return value Steffen Prohaska 1 sibling, 1 reply; 6+ messages in thread From: Steffen Prohaska @ 2012-12-11 5:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jan H. Schoenherr, Steffen Prohaska A recent commit [1] fixed a off-by-one wrapping error. As a side-effect, the conditional in add_wrapped_shortlog_msg() whether to append a newline needs to be removed. add_wrapped_shortlog_msg() should always append a newline, which was the case before the off-by-one fix, because strbuf_add_wrapped_text() never returned a value of wraplen. [1] 14e1a4e1ff70aff36db3f5d2a8b806efd0134d50 utf8: fix off-by-one wrapping of text Signed-off-by: Steffen Prohaska <prohaska@zib.de> --- builtin/shortlog.c | 5 ++--- t/t4201-shortlog.sh | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/builtin/shortlog.c b/builtin/shortlog.c index b316cf3..8360514 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -306,9 +306,8 @@ parse_done: static void add_wrapped_shortlog_msg(struct strbuf *sb, const char *s, const struct shortlog *log) { - int col = strbuf_add_wrapped_text(sb, s, log->in1, log->in2, log->wrap); - if (col != log->wrap) - strbuf_addch(sb, '\n'); + strbuf_add_wrapped_text(sb, s, log->in1, log->in2, log->wrap); + strbuf_addch(sb, '\n'); } void shortlog_output(struct shortlog *log) diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh index 6872ba1..02ac978 100755 --- a/t/t4201-shortlog.sh +++ b/t/t4201-shortlog.sh @@ -120,6 +120,30 @@ test_expect_success 'shortlog from non-git directory' ' test_cmp expect out ' +test_expect_success 'shortlog should add newline when input line matches wraplen' ' + cat >expect <<\EOF && +A U Thor (2): + bbbbbbbbbbbbbbbbbb: bbbbbbbb bbb bbbb bbbbbbb bb bbbb bbb bbbbb bbbbbb + aaaaaaaaaaaaaaaaaaaaaa: aaaaaa aaaaaaaaaa aaaa aaaaaaaa aa aaaa aa aaa + +EOF + git shortlog -w >out <<\EOF && +commit 0000000000000000000000000000000000000001 +Author: A U Thor <author@example.com> +Date: Thu Apr 7 15:14:13 2005 -0700 + + aaaaaaaaaaaaaaaaaaaaaa: aaaaaa aaaaaaaaaa aaaa aaaaaaaa aa aaaa aa aaa + +commit 0000000000000000000000000000000000000002 +Author: A U Thor <author@example.com> +Date: Thu Apr 7 15:14:13 2005 -0700 + + bbbbbbbbbbbbbbbbbb: bbbbbbbb bbb bbbb bbbbbbb bb bbbb bbb bbbbb bbbbbb + +EOF + test_cmp expect out +' + iconvfromutf8toiso88591() { printf "%s" "$*" | iconv -f UTF-8 -t ISO8859-1 } -- 1.8.1.rc1.2.gfb98a3a ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix) 2012-12-11 5:59 ` [PATCH 1/2] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix) Steffen Prohaska @ 2012-12-11 10:24 ` "Jan H. Schönherr" 0 siblings, 0 replies; 6+ messages in thread From: "Jan H. Schönherr" @ 2012-12-11 10:24 UTC (permalink / raw) To: Steffen Prohaska; +Cc: Junio C Hamano, git Am 11.12.2012 06:59, schrieb Steffen Prohaska: > A recent commit [1] fixed a off-by-one wrapping error. As > a side-effect, the conditional in add_wrapped_shortlog_msg() whether to > append a newline needs to be removed. add_wrapped_shortlog_msg() should > always append a newline, which was the case before the off-by-one fix, > because strbuf_add_wrapped_text() never returned a value of wraplen. I agree with this explanation, although there exists a case where wraplen (or wraplen+1 after the off-by-one fix) is returned: This happens when there is not a single space within the string and it has just the correct length. But also in this case, the newline must be added to get a correctly formatted output. So your patch is good as it is. :) But I still wonder about the original motivation for the removed conditional. It looks like, it wasn't even needed in the very first version (3714e7c8)?! (And it wasn't present in the version on the mailing list: http://article.gmane.org/gmane.comp.version-control.git/35221) Regards Jan ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] strbuf_add_wrapped*(): Remove unused return value 2012-12-11 5:59 ` [PATCH 0/2] " Steffen Prohaska 2012-12-11 5:59 ` [PATCH 1/2] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix) Steffen Prohaska @ 2012-12-11 5:59 ` Steffen Prohaska 1 sibling, 0 replies; 6+ messages in thread From: Steffen Prohaska @ 2012-12-11 5:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jan H. Schoenherr, Steffen Prohaska Since shortlog isn't using the return value anymore (see previous commit), the functions can be changed to void. Signed-off-by: Steffen Prohaska <prohaska@zib.de> --- utf8.c | 13 ++++++------- utf8.h | 4 ++-- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/utf8.c b/utf8.c index 5c61bbe..a4ee665 100644 --- a/utf8.c +++ b/utf8.c @@ -323,7 +323,7 @@ static size_t display_mode_esc_sequence_len(const char *s) * If indent is negative, assume that already -indent columns have been * consumed (and no extra indent is necessary for the first line). */ -int strbuf_add_wrapped_text(struct strbuf *buf, +void strbuf_add_wrapped_text(struct strbuf *buf, const char *text, int indent1, int indent2, int width) { int indent, w, assume_utf8 = 1; @@ -332,7 +332,7 @@ int strbuf_add_wrapped_text(struct strbuf *buf, if (width <= 0) { strbuf_add_indented_text(buf, text, indent1, indent2); - return 1; + return; } retry: @@ -356,14 +356,14 @@ retry: if (w <= width || !space) { const char *start = bol; if (!c && text == start) - return w; + return; if (space) start = space; else strbuf_addchars(buf, ' ', indent); strbuf_add(buf, start, text - start); if (!c) - return w; + return; space = text; if (c == '\t') w |= 0x07; @@ -405,13 +405,12 @@ new_line: } } -int strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len, +void strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len, int indent, int indent2, int width) { char *tmp = xstrndup(data, len); - int r = strbuf_add_wrapped_text(buf, tmp, indent, indent2, width); + strbuf_add_wrapped_text(buf, tmp, indent, indent2, width); free(tmp); - return r; } int is_encoding_utf8(const char *name) diff --git a/utf8.h b/utf8.h index 93ef600..a214238 100644 --- a/utf8.h +++ b/utf8.h @@ -9,9 +9,9 @@ int is_utf8(const char *text); int is_encoding_utf8(const char *name); int same_encoding(const char *, const char *); -int strbuf_add_wrapped_text(struct strbuf *buf, +void strbuf_add_wrapped_text(struct strbuf *buf, const char *text, int indent, int indent2, int width); -int strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len, +void strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len, int indent, int indent2, int width); #ifndef NO_ICONV -- 1.8.1.rc1.2.gfb98a3a ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-12-11 10:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-08 19:09 [PATCH] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix) Steffen Prohaska 2012-12-09 9:36 ` [PATCH] shortlog: Fix wrapping lines of wraplen Junio C Hamano 2012-12-11 5:59 ` [PATCH 0/2] " Steffen Prohaska 2012-12-11 5:59 ` [PATCH 1/2] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix) Steffen Prohaska 2012-12-11 10:24 ` "Jan H. Schönherr" 2012-12-11 5:59 ` [PATCH 2/2] strbuf_add_wrapped*(): Remove unused return value Steffen Prohaska
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).