threads / patch / 32292

patchshortlog: Fix wrapping lines of wraplen (was broken since recent off-by-one fix)

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

## tl;dr

6 messages between Dec 8, 2012 and Dec 11, 2012. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Steffen Prohaska· Dec 8, 2012, 19:09 UTC · lore

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(-)
Show changes to 2 files +25 −2

builtin/shortlog.c, t/t4201-shortlog.sh

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
Junio C Hamano· Dec 9, 2012, 09:36 UTC · re: Steffen Prohaska · lore

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

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.
Show 59 quoted lines
>
> [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
>  }
Steffen Prohaska· Dec 11, 2012, 05:59 UTC · re: Junio C Hamano · lore

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

On Dec 9, 2012, at 10:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> 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
Steffen Prohaska· Dec 11, 2012, 05:59 UTC · re: Steffen Prohaska · lore

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

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(-)
Show changes to 2 files +26 −3

builtin/shortlog.c, t/t4201-shortlog.sh

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
Jan H. Schönherr· Dec 11, 2012, 10:24 UTC · re: Steffen Prohaska · lore

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

Am 11.12.2012 06:59, schrieb Steffen Prohaska:
Show 5 quoted lines
> 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

Steffen Prohaska· Dec 11, 2012, 05:59 UTC · re: Steffen Prohaska · lore

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

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(-)
Show changes to 2 files +8 −9

utf8.c, utf8.h

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

← back to recent threads