[PATCH next] git-notes: fix printing of multi-line notes

[PATCH next] git-notes: fix printing of multi-line notes

[Tor Arne Vestbø]

The line length was read from the same position every time,
causing mangled output when printing notes with multiple lines.

Also, adding new-line manually for each line ensures that we
get a new-line between commits, matching git-log for commits
without notes.

Signed-off-by: Tor Arne Vestbø <tavestbo@trolltech.com>
---

This approach uses a msg pointer, but I started out with just using
msg + msgoffset all over the place, so if that's a preferred way
to do things I'm happy to provide an alternate patch.

Also, I'm guessing this printing should go into pretty.c at some
point, so you can reference the notes as part of a custom pretty
format. If so, this code could be converted to use helpers such
as get_one_line().

This is my first patch to Git, so sorry if I messed something up :)

notes.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/notes.c b/notes.c
index ad43a2e..bd73784 100644
--- a/notes.c
+++ b/notes.c
@@ -110,8 +110,8 @@ void get_commit_notes(const struct commit *commit, struct strbuf *sb,
 {
 	static const char *utf8 = "utf-8";
 	unsigned char *sha1;
-	char *msg;
-	unsigned long msgoffset, msglen;
+	char *msg, *msg_p;
+	unsigned long linelen, msglen;
 	enum object_type type;
 
 	if (!initialized) {
@@ -148,12 +148,13 @@ void get_commit_notes(const struct commit *commit, struct strbuf *sb,
 
 	strbuf_addstr(sb, "\nNotes:\n");
 
-	for (msgoffset = 0; msgoffset < msglen;) {
-		int linelen = strchrnul(msg, '\n') - msg;
+	for (msg_p = msg; msg_p < msg + msglen; msg_p += linelen + 1) {
+		linelen = strchrnul(msg_p, '\n') - msg_p;
 
 		strbuf_addstr(sb, "    ");
-		strbuf_add(sb, msg + msgoffset, linelen);
-		msgoffset += linelen;
+		strbuf_add(sb, msg_p, linelen);
+		strbuf_addch(sb, '\n');
 	}
+
 	free(msg);
 }
-- 
1.6.0.2.GIT

Re: [PATCH next] git-notes: fix printing of multi-line notes

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 556 bytes --]

Hi,

On Tue, 13 Jan 2009, Tor Arne Vestbø wrote:

> The line length was read from the same position every time,
> causing mangled output when printing notes with multiple lines.
> 
> Also, adding new-line manually for each line ensures that we
> get a new-line between commits, matching git-log for commits
> without notes.
> 
> Signed-off-by: Tor Arne Vestbø <tavestbo@trolltech.com>
> ---

Patch looks good, so 

Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>

For extra browny points, you could add a test with multi-line notes.

Ciao,
Dscho

Re: [PATCH next] git-notes: fix printing of multi-line notes

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Tue, 13 Jan 2009, Tor Arne Vestbø wrote:
>
>> The line length was read from the same position every time,
>> causing mangled output when printing notes with multiple lines.
>> 
>> Also, adding new-line manually for each line ensures that we
>> get a new-line between commits, matching git-log for commits
>> without notes.
>> 
>> Signed-off-by: Tor Arne Vestbø <tavestbo@trolltech.com>
>> ---
>
> Patch looks good, so 
>
> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> For extra browny points, you could add a test with multi-line notes.

Yeah, not just "extra", having tests is a good way to make sure a new
feature like this evolves healthily.

Tor?

Re: [PATCH next] git-notes: fix printing of multi-line notes

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 875 bytes --]

Hi,

On Tue, 13 Jan 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Tue, 13 Jan 2009, Tor Arne Vestbø wrote:
> >
> >> The line length was read from the same position every time,
> >> causing mangled output when printing notes with multiple lines.
> >> 
> >> Also, adding new-line manually for each line ensures that we
> >> get a new-line between commits, matching git-log for commits
> >> without notes.
> >> 
> >> Signed-off-by: Tor Arne Vestbø <tavestbo@trolltech.com>
> >> ---
> >
> > Patch looks good, so 
> >
> > Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > For extra browny points, you could add a test with multi-line notes.
> 
> Yeah, not just "extra", having tests is a good way to make sure a new
> feature like this evolves healthily.

Oh, and of course I meant "brownie"...

Ducks,
Dscho

[PATCH next] git-notes: add test case for multi-line notes

[Tor Arne Vestbø]

The tests adds a third commit with a multi-line note. The output of
git log -2 is then checked to see if the note lines are wrapped
correctly, and that there's a line separator between the two commits.

Also, changed from using 'git diff' to test expect vs. output to use
'test_cmp', as I had problems getting correct results using the former.

Signed-off-by: Tor Arne Vestbø <tavestbo@trolltech.com>
---
 t/t3301-notes.sh |   35 ++++++++++++++++++++++++++++++++---
 1 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index ba42c45..76bb6dd 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -8,8 +8,8 @@ test_description='Test commit notes'
 . ./test-lib.sh
 
 cat > fake_editor.sh << \EOF
-echo "$MSG" > "$1"
-echo "$MSG" >& 2
+echo -e "$MSG" > "$1"
+echo -e "$MSG" >& 2
 EOF
 chmod a+x fake_editor.sh
 VISUAL=./fake_editor.sh
@@ -59,7 +59,36 @@ EOF
 test_expect_success 'show notes' '
 	! (git cat-file commit HEAD | grep b1) &&
 	git log -1 > output &&
-	git diff expect output
+	test_cmp expect output
+'
+test_expect_success 'create multi-line notes (setup)' '
+	: > a3 &&
+	git add a3 &&
+	test_tick &&
+	git commit -m 3rd &&
+	MSG="b3\nc3c3c3c3\nd3d3d3" git notes edit
+
+'
+
+cat > expect-multiline << EOF
+commit 1584215f1d29c65e99c6c6848626553fdd07fd75
+Author: A U Thor <author@example.com>
+Date:   Thu Apr 7 15:15:13 2005 -0700
+
+    3rd
+
+Notes:
+    b3
+    c3c3c3c3
+    d3d3d3
+EOF
+
+echo >> expect-multiline
+cat expect >> expect-multiline
+
+test_expect_success 'show multi-line notes' '
+	git log -2 > output &&
+	test_cmp expect-multiline output
 '
 
 test_done
-- 
1.6.0.2.GIT

Re: [PATCH next] git-notes: add test case for multi-line notes

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2016 bytes --]

Hi,

On Wed, 14 Jan 2009, Tor Arne Vestbø wrote:

> The tests adds a third commit with a multi-line note. The output of
> git log -2 is then checked to see if the note lines are wrapped
> correctly, and that there's a line separator between the two commits.
> 
> Also, changed from using 'git diff' to test expect vs. output to use
> 'test_cmp', as I had problems getting correct results using the former.

You could skip the part that you had problems, as the test_cmp is 
obviously the correct thing to do.

> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index ba42c45..76bb6dd 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -8,8 +8,8 @@ test_description='Test commit notes'
> . ./test-lib.sh
> 
> cat > fake_editor.sh << \EOF
> -echo "$MSG" > "$1"
> -echo "$MSG" >& 2
> +echo -e "$MSG" > "$1"
> +echo -e "$MSG" >& 2

I seem to recall that we had plenty of fun substituting "echo -e" with 
"printf" whenever it entered the repository (... again...), as some 
platforms -- ahem, macosx, ahem -- are a bit peculiar with such options.

So you might want to make sure no % is passed as "$MSG", and use printf 
instead.

> +test_expect_success 'create multi-line notes (setup)' '
> +	: > a3 &&
> +	git add a3 &&
> +	test_tick &&
> +	git commit -m 3rd &&
> +	MSG="b3\nc3c3c3c3\nd3d3d3" git notes edit
> +
> +'

Minor style nit: maybe you want to have an empty line at the beginning, 
too...

> +cat > expect-multiline << EOF
> +commit 1584215f1d29c65e99c6c6848626553fdd07fd75
> +Author: A U Thor <author@example.com>
> +Date:   Thu Apr 7 15:15:13 2005 -0700
> +
> +    3rd
> +
> +Notes:
> +    b3
> +    c3c3c3c3
> +    d3d3d3
> +EOF
> +
> +echo >> expect-multiline
> +cat expect >> expect-multiline

Yeah.  My initial reaction was: "you could have that echo inside the cat 
<<EOF", but this is clearer.  Except that you should make sure that 
nothing is printed (M$' echo outputs something if you pass no parameters); 
printf "\n" would be my choice.

Other than that, very good: ACK.

Ciao,
Dscho

[PATCH next v2] git-notes: add test case for multi-line notes

[Tor Arne Vestbø]

The tests adds a third commit with a multi-line note. The output of
git log -2 is then checked to see if the note lines are wrapped
correctly, and that there's a line separator between the two commits.

Signed-off-by: Tor Arne Vestbø <tavestbo@trolltech.com>
---

Thanks for the feedback Johannes! Here's an updated patch. I removed
the blank line instead of adding another, as that's the current style
of that file.

 t/t3301-notes.sh |   35 ++++++++++++++++++++++++++++++++---
 1 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index ba42c45..e260d79 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -8,8 +8,9 @@ test_description='Test commit notes'
 . ./test-lib.sh
 
 cat > fake_editor.sh << \EOF
-echo "$MSG" > "$1"
-echo "$MSG" >& 2
+MSG=${MSG//%/}
+printf "$MSG" > "$1"
+printf "$MSG" >& 2
 EOF
 chmod a+x fake_editor.sh
 VISUAL=./fake_editor.sh
@@ -59,7 +60,35 @@ EOF
 test_expect_success 'show notes' '
 	! (git cat-file commit HEAD | grep b1) &&
 	git log -1 > output &&
-	git diff expect output
+	test_cmp expect output
+'
+test_expect_success 'create multi-line notes (setup)' '
+	: > a3 &&
+	git add a3 &&
+	test_tick &&
+	git commit -m 3rd &&
+	MSG="b3\nc3c3c3c3\nd3d3d3" git notes edit
+'
+
+cat > expect-multiline << EOF
+commit 1584215f1d29c65e99c6c6848626553fdd07fd75
+Author: A U Thor <author@example.com>
+Date:   Thu Apr 7 15:15:13 2005 -0700
+
+    3rd
+
+Notes:
+    b3
+    c3c3c3c3
+    d3d3d3
+EOF
+
+printf "\n" >> expect-multiline
+cat expect >> expect-multiline
+
+test_expect_success 'show multi-line notes' '
+	git log -2 > output &&
+	test_cmp expect-multiline output
 '
 
 test_done
-- 
1.6.0.2.GIT

Re: [PATCH next v2] git-notes: add test case for multi-line notes

[Jeff King]

On Wed, Jan 14, 2009 at 05:28:11PM +0100, Tor Arne Vestbø wrote:

> +MSG=${MSG//%/}
> +printf "$MSG" > "$1"
> +printf "$MSG" >& 2

Substitution parameter expansion is a bash-ism, IIRC. How about just

  printf %s "$MSG" ?

-Peff

Re: [PATCH next v2] git-notes: add test case for multi-line notes

[Boyd Stephen Smith Jr.]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

On Wednesday 2009 January 14 10:56:33 Jeff King wrote:
>On Wed, Jan 14, 2009 at 05:28:11PM +0100, Tor Arne Vestbø wrote:
>> +MSG=${MSG//%/}
>> +printf "$MSG" > "$1"
>> +printf "$MSG" >& 2
>
>Substitution parameter expansion is a bash-ism, IIRC. How about just

MSG=$(printf '%s\n' "$MSG" | sed -e 's/%/%%/g')
printf "$MSG" > "$1"
printf "$MSG" >& 2

Is my best attempt at portable and "safe".  It's a few extra processes though.

>  printf %s "$MSG" ?

On my box
$ printf '%s\n' '\n'
\n
$

He wants '\n' in $MSG to be expanded, and what you gave doesn't do that.
-- 
Boyd Stephen Smith Jr.                     ,= ,-_-. =. 
bss@iguanasuicide.net                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH next v2] git-notes: add test case for multi-line notes

[Jeff King]

On Wed, Jan 14, 2009 at 11:09:52AM -0600, Boyd Stephen Smith Jr. wrote:

> >  printf %s "$MSG" ?
> 
> On my box
> $ printf '%s\n' '\n'
> \n
> $
> 
> He wants '\n' in $MSG to be expanded, and what you gave doesn't do that.

Oh, sorry. That's what I get for not reading his patch carefully.

It looks like all of the input is statically included in the test
script. While I think it is nice to be defensive, it is probably
simplest to just assume there is no '%' in this case (which we can
verify by reading the script).

-Peff

Re: [PATCH next v2] git-notes: add test case for multi-line notes

[Johannes Sixt]

Jeff King schrieb:
> On Wed, Jan 14, 2009 at 05:28:11PM +0100, Tor Arne Vestbø wrote:
> 
>> +MSG=${MSG//%/}
>> +printf "$MSG" > "$1"
>> +printf "$MSG" >& 2
> 
> Substitution parameter expansion is a bash-ism, IIRC. How about just
> 
>   printf %s "$MSG" ?

A the point was that $MSG contains \n, which should be turned int LF. IMO,
the easiest way to achieve this is:

MSG='b3
c3c3c3c3
d3d3d3'

test_expect_success ' ... ' '
   ...
   MSG="$MSG" git notes edit
'

and go back to using echo in the part cited above.

-- Hannes

Re: [PATCH next v2] git-notes: add test case for multi-line notes

[Jeff King]

On Wed, Jan 14, 2009 at 06:14:31PM +0100, Johannes Sixt wrote:

> A the point was that $MSG contains \n, which should be turned int LF. IMO,
> the easiest way to achieve this is:
> 
> MSG='b3
> c3c3c3c3
> d3d3d3'
> 
> test_expect_success ' ... ' '
>    ...
>    MSG="$MSG" git notes edit
> '
> 
> and go back to using echo in the part cited above.

Yes, sorry, I hadn't read his original patch carefully. I think that is
a sane solution.

-Peff

Re: [PATCH next v2] git-notes: add test case for multi-line notes

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 705 bytes --]

Hi,

On Wed, 14 Jan 2009, Johannes Sixt wrote:

> Jeff King schrieb:
> > On Wed, Jan 14, 2009 at 05:28:11PM +0100, Tor Arne Vestbø wrote:
> > 
> >> +MSG=${MSG//%/}
> >> +printf "$MSG" > "$1"
> >> +printf "$MSG" >& 2
> > 
> > Substitution parameter expansion is a bash-ism, IIRC. How about just
> > 
> >   printf %s "$MSG" ?
> 
> A the point was that $MSG contains \n, which should be turned int LF. IMO,
> the easiest way to achieve this is:
> 
> MSG='b3
> c3c3c3c3
> d3d3d3'
> 
> test_expect_success ' ... ' '
>    ...
>    MSG="$MSG" git notes edit
> '
> 
> and go back to using echo in the part cited above.

Heh, I almost suggested it, but I know that I get quoting wrong all the 
time.

Ciao,
Dscho

[PATCH next v3] git-notes: add test case for multi-line notes

[Tor Arne Vestbø]

The tests adds a third commit with a multi-line note. The output of
git log -2 is then checked to see if the note lines are wrapped
correctly, and that there's a line separator between the two commits.

Signed-off-by: Tor Arne Vestbø <tavestbo@trolltech.com>
---
 t/t3301-notes.sh |   32 +++++++++++++++++++++++++++++++-
 1 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index ba42c45..9393a25 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -59,7 +59,37 @@ EOF
 test_expect_success 'show notes' '
 	! (git cat-file commit HEAD | grep b1) &&
 	git log -1 > output &&
-	git diff expect output
+	test_cmp expect output
+'
+test_expect_success 'create multi-line notes (setup)' '
+	: > a3 &&
+	git add a3 &&
+	test_tick &&
+	git commit -m 3rd &&
+	MSG="b3
+c3c3c3c3
+d3d3d3" git notes edit
+'
+
+cat > expect-multiline << EOF
+commit 1584215f1d29c65e99c6c6848626553fdd07fd75
+Author: A U Thor <author@example.com>
+Date:   Thu Apr 7 15:15:13 2005 -0700
+
+    3rd
+
+Notes:
+    b3
+    c3c3c3c3
+    d3d3d3
+EOF
+
+printf "\n" >> expect-multiline
+cat expect >> expect-multiline
+
+test_expect_success 'show multi-line notes' '
+	git log -2 > output &&
+	test_cmp expect-multiline output
 '
 
 test_done
-- 
1.6.0.2.GIT

Re: [PATCH next v3] git-notes: add test case for multi-line notes

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 434 bytes --]

Hi,

On Wed, 14 Jan 2009, Tor Arne Vestbø wrote:

> The tests adds a third commit with a multi-line note. The output of
> git log -2 is then checked to see if the note lines are wrapped
> correctly, and that there's a line separator between the two commits.
> 
> Signed-off-by: Tor Arne Vestbø <tavestbo@trolltech.com>
> ---

Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

Maybe squash the test into the fix?

Ciao,
Dscho

[PATCH next v4] git-notes: fix printing of multi-line notes

[Tor Arne Vestbø]

The line length was read from the same position every time,
causing mangled output when printing notes with multiple lines.

Also, adding new-line manually for each line ensures that we
get a new-line between commits, matching git-log for commits
without notes.

Test case added to t3301-notes.sh.

Signed-off-by: Tor Arne Vestbø <tavestbo@trolltech.com>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

Sorry about the delay. Here's a squashed patch.

 notes.c          |   13 +++++++------
 t/t3301-notes.sh |   32 +++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/notes.c b/notes.c
index ad43a2e..bd73784 100644
--- a/notes.c
+++ b/notes.c
@@ -110,8 +110,8 @@ void get_commit_notes(const struct commit *commit, struct strbuf *sb,
 {
 	static const char *utf8 = "utf-8";
 	unsigned char *sha1;
-	char *msg;
-	unsigned long msgoffset, msglen;
+	char *msg, *msg_p;
+	unsigned long linelen, msglen;
 	enum object_type type;
 
 	if (!initialized) {
@@ -148,12 +148,13 @@ void get_commit_notes(const struct commit *commit, struct strbuf *sb,
 
 	strbuf_addstr(sb, "\nNotes:\n");
 
-	for (msgoffset = 0; msgoffset < msglen;) {
-		int linelen = strchrnul(msg, '\n') - msg;
+	for (msg_p = msg; msg_p < msg + msglen; msg_p += linelen + 1) {
+		linelen = strchrnul(msg_p, '\n') - msg_p;
 
 		strbuf_addstr(sb, "    ");
-		strbuf_add(sb, msg + msgoffset, linelen);
-		msgoffset += linelen;
+		strbuf_add(sb, msg_p, linelen);
+		strbuf_addch(sb, '\n');
 	}
+
 	free(msg);
 }
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index ba42c45..9393a25 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -59,7 +59,37 @@ EOF
 test_expect_success 'show notes' '
 	! (git cat-file commit HEAD | grep b1) &&
 	git log -1 > output &&
-	git diff expect output
+	test_cmp expect output
+'
+test_expect_success 'create multi-line notes (setup)' '
+	: > a3 &&
+	git add a3 &&
+	test_tick &&
+	git commit -m 3rd &&
+	MSG="b3
+c3c3c3c3
+d3d3d3" git notes edit
+'
+
+cat > expect-multiline << EOF
+commit 1584215f1d29c65e99c6c6848626553fdd07fd75
+Author: A U Thor <author@example.com>
+Date:   Thu Apr 7 15:15:13 2005 -0700
+
+    3rd
+
+Notes:
+    b3
+    c3c3c3c3
+    d3d3d3
+EOF
+
+printf "\n" >> expect-multiline
+cat expect >> expect-multiline
+
+test_expect_success 'show multi-line notes' '
+	git log -2 > output &&
+	test_cmp expect-multiline output
 '
 
 test_done
-- 
1.6.0.2.GIT

Re: [PATCH next v4] git-notes: fix printing of multi-line notes

[Junio C Hamano]

Tor Arne Vestbø <tavestbo@trolltech.com> writes:

> The line length was read from the same position every time,
> causing mangled output when printing notes with multiple lines.
>
> Also, adding new-line manually for each line ensures that we
> get a new-line between commits, matching git-log for commits
> without notes.
>
> Test case added to t3301-notes.sh.
>
> Signed-off-by: Tor Arne Vestbø <tavestbo@trolltech.com>
> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> Sorry about the delay. Here's a squashed patch.

Thanks.  This exactly matches 22a3d06 (git-notes: fix printing of
multi-line notes, 2009-01-13) I already have, so we are in a good shape.