[PATCH] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix)

[PATCH] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix)

[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

Re: [PATCH] shortlog: Fix wrapping lines of wraplen

[Junio C Hamano]

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
>  }

[PATCH 0/2] Re: [PATCH] shortlog: Fix wrapping lines of wraplen

[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

[PATCH 1/2] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix)

[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

[PATCH 2/2] strbuf_add_wrapped*(): Remove unused return value

[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

Re: [PATCH 1/2] shortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix)

["Jan H. Schönherr"]

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