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.

---

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(-)

index ad43a2e..bd73784 100644
@@ -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

Hi,

Patch looks good, so

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

Ciao,
Dscho

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

Tor?

Hi,

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

Ducks,
Dscho

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.

---
t/t3301-notes.sh | 35 ++++++++++++++++++++++++++++++++---

1 files changed, 32 insertions(+), 3 deletions(-)

index ba42c45..76bb6dd 100755
@@ -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 <[email protected]>
+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

Hi,

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

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.

---

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(-)

index ba42c45..e260d79 100755
@@ -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 <[email protected]>
+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

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.

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.                     ,= ,-_-. =. 
[email protected]                     ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy           `-'(. .)`-' 
http://iguanasuicide.net/                      \_/     

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

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

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

-Peff

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.

---
t/t3301-notes.sh | 32 +++++++++++++++++++++++++++++++-

1 files changed, 31 insertions(+), 1 deletions(-)

index ba42c45..9393a25 100755
@@ -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 <[email protected]>
+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

Hi,

Maybe squash the test into the fix?

Ciao,
Dscho

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.

---

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

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

index ad43a2e..bd73784 100644
@@ -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);
}

index ba42c45..9393a25 100755
@@ -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 <[email protected]>
+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

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.

Hi,

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

Ciao,
Dscho