# [PATCH next] git-notes: fix printing of multi-line notes 17 messages from 2009-01-13 to 2009-01-18. Participants: Tor Arne Vestbø, Johannes Schindelin, Junio C Hamano, Jeff King, Boyd Stephen Smith Jr., Johannes Sixt. Thread: https://gitlist.dev/t/17152 ## Tor Arne Vestbø, 2009-01-13 19:57 Subject: [PATCH next] git-notes: fix printing of multi-line notes Message-ID: <496CF21C.2050500@trolltech.com> URL: https://gitlist.dev/e/496CF21C.2050500%40trolltech.com ``` 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ø --- 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 ``` ## Johannes Schindelin, 2009-01-13 22:40 Subject: Re: [PATCH next] git-notes: fix printing of multi-line notes Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901132339270.3586%40pacific.mpi-cbg.de In-Reply-To: <496CF21C.2050500@trolltech.com> ``` 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ø > --- Patch looks good, so Acked-by: Johannes Schindelin For extra browny points, you could add a test with multi-line notes. Ciao, Dscho ``` ## Junio C Hamano, 2009-01-14 06:48 Subject: Re: [PATCH next] git-notes: fix printing of multi-line notes Message-ID: <7v3afm758u.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v3afm758u.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin 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ø >> --- > > Patch looks good, so > > Acked-by: Johannes Schindelin > > 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, 2009-01-14 10:14 Subject: Re: [PATCH next] git-notes: fix printing of multi-line notes Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901141113250.3586%40pacific.mpi-cbg.de In-Reply-To: <7v3afm758u.fsf@gitster.siamese.dyndns.org> ``` Hi, On Tue, 13 Jan 2009, Junio C Hamano wrote: > Johannes Schindelin 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ø > >> --- > > > > Patch looks good, so > > > > Acked-by: Johannes Schindelin > > > > 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ø, 2009-01-14 14:39 Subject: [PATCH next] git-notes: add test case for multi-line notes Message-ID: <496DF936.3060308@trolltech.com> URL: https://gitlist.dev/e/496DF936.3060308%40trolltech.com In-Reply-To: <7v3afm758u.fsf@gitster.siamese.dyndns.org> ``` 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ø --- 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 +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, 2009-01-14 15:34 Subject: Re: [PATCH next] git-notes: add test case for multi-line notes Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901141627440.3586%40pacific.mpi-cbg.de In-Reply-To: <496DF936.3060308@trolltech.com> ``` 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 > +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 < URL: https://gitlist.dev/e/496E129B.3020502%40trolltech.com In-Reply-To: ``` 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ø --- 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 +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, 2009-01-14 16:56 Subject: Re: [PATCH next v2] git-notes: add test case for multi-line notes Message-ID: <20090114165633.GC15758@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090114165633.GC15758%40coredump.intra.peff.net In-Reply-To: <496E129B.3020502@trolltech.com> ``` 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., 2009-01-14 17:09 Subject: Re: [PATCH next v2] git-notes: add test case for multi-line notes Message-ID: <200901141109.56580.bss@iguanasuicide.net> URL: https://gitlist.dev/e/200901141109.56580.bss%40iguanasuicide.net In-Reply-To: <20090114165633.GC15758@coredump.intra.peff.net> ``` 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/ \_/ ``` ## Jeff King, 2009-01-14 17:13 Subject: Re: [PATCH next v2] git-notes: add test case for multi-line notes Message-ID: <20090114171315.GA16063@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090114171315.GA16063%40coredump.intra.peff.net In-Reply-To: <200901141109.56580.bss@iguanasuicide.net> ``` 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 ``` ## Johannes Sixt, 2009-01-14 17:14 Subject: Re: [PATCH next v2] git-notes: add test case for multi-line notes Message-ID: <496E1D77.6000307@viscovery.net> URL: https://gitlist.dev/e/496E1D77.6000307%40viscovery.net In-Reply-To: <20090114165633.GC15758@coredump.intra.peff.net> ``` 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 ``` ## Jeff King, 2009-01-14 17:19 Subject: Re: [PATCH next v2] git-notes: add test case for multi-line notes Message-ID: <20090114171925.GA16199@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090114171925.GA16199%40coredump.intra.peff.net In-Reply-To: <496E1D77.6000307@viscovery.net> ``` 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 ``` ## Johannes Schindelin, 2009-01-14 18:01 Subject: Re: [PATCH next v2] git-notes: add test case for multi-line notes Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901141900490.3586%40pacific.mpi-cbg.de In-Reply-To: <496E1D77.6000307@viscovery.net> ``` 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 ``` ## Tor Arne Vestbø, 2009-01-14 20:57 Subject: [PATCH next v3] git-notes: add test case for multi-line notes Message-ID: <496E51A3.8050908@gmail.com> URL: https://gitlist.dev/e/496E51A3.8050908%40gmail.com In-Reply-To: <496E1D77.6000307@viscovery.net> ``` 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ø --- 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 +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, 2009-01-14 21:10 Subject: Re: [PATCH next v3] git-notes: add test case for multi-line notes Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901142209570.3586%40pacific.mpi-cbg.de In-Reply-To: <496E51A3.8050908@gmail.com> ``` 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ø > --- Acked-by: Johannes Schindelin Maybe squash the test into the fix? Ciao, Dscho ``` ## Tor Arne Vestbø, 2009-01-16 13:06 Subject: [PATCH next v4] git-notes: fix printing of multi-line notes Message-ID: <49708644.7040809@trolltech.com> URL: https://gitlist.dev/e/49708644.7040809%40trolltech.com In-Reply-To: ``` 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ø Acked-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- 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 +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, 2009-01-18 21:27 Subject: Re: [PATCH next v4] git-notes: fix printing of multi-line notes Message-ID: <7vmydouwxl.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmydouwxl.fsf%40gitster.siamese.dyndns.org In-Reply-To: <49708644.7040809@trolltech.com> ``` Tor Arne Vestbø 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ø > Acked-by: Johannes Schindelin > Signed-off-by: Junio C Hamano > --- > > 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. ```