threads / patch / 17152

patchgit-notes: fix printing of multi-line notes

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

## tl;dr

17 messages between Jan 13, 2009 and Jan 18, 2009. Diffs are folded; open one to read it.

replies: 16people: 6as markdown or json

Tor Arne Vestbø· Jan 13, 2009, 19:57 UTC · lore

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(-)
Show changes to notes.c +7 −6
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
Johannes Schindelin· Jan 13, 2009, 22:40 UTC · re: Tor Arne Vestbø · lore

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

Hi,
On Tue, 13 Jan 2009, Tor Arne Vestbø wrote:
Show 9 quoted lines
> 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

Junio C Hamano· Jan 14, 2009, 06:48 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 17 quoted lines
> 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?
Johannes Schindelin· Jan 14, 2009, 10:14 UTC · re: Junio C Hamano · lore

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

Hi,
On Tue, 13 Jan 2009, Junio C Hamano wrote:
Show 22 quoted lines
> 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

Tor Arne Vestbø· Jan 14, 2009, 14:39 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to t/t3301-notes.sh +32 −3
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
Johannes Schindelin· Jan 14, 2009, 15:34 UTC · re: Tor Arne Vestbø · lore

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

Hi,
On Wed, 14 Jan 2009, Tor Arne Vestbø wrote:
Show 6 quoted lines
> 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.

Show 12 quoted lines
> 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.

Show 8 quoted lines
> +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...

Show 15 quoted lines
> +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

Tor Arne Vestbø· Jan 14, 2009, 16:28 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to t/t3301-notes.sh +32 −3
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
Jeff King· Jan 14, 2009, 16:56 UTC · re: Tor Arne Vestbø · lore

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

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
Boyd Stephen Smith Jr.· Jan 14, 2009, 17:09 UTC · re: Jeff King · lore

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

On Wednesday 2009 January 14 10:56:33 Jeff King wrote:
Show 6 quoted lines
>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/                      \_/
Jeff King· Jan 14, 2009, 17:13 UTC · re: Boyd Stephen Smith Jr. · lore

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

On Wed, Jan 14, 2009 at 11:09:52AM -0600, Boyd Stephen Smith Jr. wrote:
Show 8 quoted lines
> >  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
Johannes Sixt· Jan 14, 2009, 17:14 UTC · re: Jeff King · lore

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

Jeff King schrieb:
Show 9 quoted lines
> 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
Jeff King· Jan 14, 2009, 17:19 UTC · re: Johannes Sixt · lore

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

On Wed, Jan 14, 2009 at 06:14:31PM +0100, Johannes Sixt wrote:
Show 13 quoted lines
> 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
Johannes Schindelin· Jan 14, 2009, 18:01 UTC · re: Johannes Sixt · lore

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

Hi,
On Wed, 14 Jan 2009, Johannes Sixt wrote:
Show 24 quoted lines
> 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

Tor Arne Vestbø· Jan 14, 2009, 20:57 UTC · re: Johannes Sixt · lore

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

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(-)
Show changes to t/t3301-notes.sh +31 −1
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
Johannes Schindelin· Jan 14, 2009, 21:10 UTC · re: Tor Arne Vestbø · lore

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

Hi,
On Wed, 14 Jan 2009, Tor Arne Vestbø wrote:
Show 6 quoted lines
> 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

Tor Arne Vestbø· Jan 16, 2009, 13:06 UTC · re: Johannes Schindelin · lore

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

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

notes.c, t/t3301-notes.sh

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
Junio C Hamano· Jan 18, 2009, 21:27 UTC · re: Tor Arne Vestbø · lore

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

Tor Arne Vestbø <tavestbo@trolltech.com> writes:
Show 15 quoted lines
> 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.

← back to recent threads