threads / discuss / 30369

git log -z still outputting newlines?

Subject: git log -z still outputting newlines?

## tl;dr

17 messages between Apr 30, 2012 and May 1, 2012.

replies: 16people: 6as markdown or json

Randal L. Schwartz· Apr 30, 2012, 17:58 UTC · lore

$ git log -z --format='%cE' -5 | od -c 0000000 g i t s t e r @ p o b o x . c o 0000020 m \n g i t s t e r @ p o b o x . 0000040 c o m \n g i t s t e r @ p o b o 0000060 x . c o m \n g i t s t e r @ p o 0000100 b o x . c o m \n g i t s t e r @ 0000120 p o b o x . c o m \n 0000132

Why are all those newlines in there? Bug? Misfeature? Feature? If feature, how do I ensure \0 in my output? If I add %x00, I get both \0 *and* \n in output. :(

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.posterous.com/ for Smalltalk discussion
Andreas Schwab· Apr 30, 2012, 18:34 UTC · re: Randal L. Schwartz · lore

Re: git log -z still outputting newlines?

merlyn@stonehenge.com (Randal L. Schwartz) writes:
Show 12 quoted lines
> $ git log -z --format='%cE' -5 | od -c
> 0000000    g   i   t   s   t   e   r   @   p   o   b   o   x   .   c   o
> 0000020    m  \n   g   i   t   s   t   e   r   @   p   o   b   o   x   .
> 0000040    c   o   m  \n   g   i   t   s   t   e   r   @   p   o   b   o
> 0000060    x   .   c   o   m  \n   g   i   t   s   t   e   r   @   p   o
> 0000100    b   o   x   .   c   o   m  \n   g   i   t   s   t   e   r   @
> 0000120    p   o   b   o   x   .   c   o   m  \n                        
> 0000132
>
> Why are all those newlines in there?  Bug?  Misfeature?  Feature?  If
> feature, how do I ensure \0 in my output?  If I add %x00, I get both \0
> *and* \n in output. :(
--format=format:%cE respects the -z option.
Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Thomas Rast· Apr 30, 2012, 19:02 UTC · re: Andreas Schwab · lore

Re: git log -z still outputting newlines?

Andreas Schwab <schwab@linux-m68k.org> writes:
Show 16 quoted lines
> merlyn@stonehenge.com (Randal L. Schwartz) writes:
>
>> $ git log -z --format='%cE' -5 | od -c
>> 0000000    g   i   t   s   t   e   r   @   p   o   b   o   x   .   c   o
>> 0000020    m  \n   g   i   t   s   t   e   r   @   p   o   b   o   x   .
>> 0000040    c   o   m  \n   g   i   t   s   t   e   r   @   p   o   b   o
>> 0000060    x   .   c   o   m  \n   g   i   t   s   t   e   r   @   p   o
>> 0000100    b   o   x   .   c   o   m  \n   g   i   t   s   t   e   r   @
>> 0000120    p   o   b   o   x   .   c   o   m  \n                        
>> 0000132
>>
>> Why are all those newlines in there?  Bug?  Misfeature?  Feature?  If
>> feature, how do I ensure \0 in my output?  If I add %x00, I get both \0
>> *and* \n in output. :(
>
> --format=format:%cE respects the -z option.

The underlying problem is apparently that --format=%cE triggers the format-guessing logic, which assumes you meant --pretty=tformat:%cE instead of --pretty=format:%cE.

It's probably a bug that --pretty=tformat:%cE does not use \0 here. After all the manual states

  · tformat:
    The tformat: format works exactly like format:, except that it
    provides "terminator" semantics instead of "separator" semantics.

Fixing it may be as easy as the patch below, but I haven't spent much time on it.

diff --git i/log-tree.c w/log-tree.c
index 34c49e7..44f0268 100644
--- i/log-tree.c
+++ w/log-tree.c
@@ -682,7 +682,7 @@ void show_log(struct rev_info *opt)
 	if (opt->use_terminator) {
 		if (!opt->missing_newline)
 			graph_show_padding(opt->graph);
-		putchar('\n');
+		putchar(opt->diffopt.line_termination);
 	}
 
 	strbuf_release(&msgbuf);

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Jan Krüger· Apr 30, 2012, 18:45 UTC · re: Randal L. Schwartz · lore

[PATCH] log-tree: use custom line terminator in line termination mode

When using a custom format in line termination mode (as opposed to line separation mode), the configured line terminator is not used, so things like "git log --pretty=tformat:%H -z" do not work properly.

Make it use the line terminator the user ordered.
Signed-off-by: Jan Krüger <jk@jk.gs>
---
 log-tree.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/log-tree.c b/log-tree.c
index 34c49e7..44f0268 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -682,7 +682,7 @@ void show_log(struct rev_info *opt)
 	if (opt->use_terminator) {
 		if (!opt->missing_newline)
 			graph_show_padding(opt->graph);
-		putchar('\n');
+		putchar(opt->diffopt.line_termination);
 	}
 
 	strbuf_release(&msgbuf);

-- 
1.7.10
Junio C Hamano· Apr 30, 2012, 19:36 UTC · re: Jan Krüger · lore

Re: [PATCH] log-tree: use custom line terminator in line termination mode

Jan Krüger <jk@jk.gs> writes:
Show 24 quoted lines
> When using a custom format in line termination mode (as opposed to line
> separation mode), the configured line terminator is not used, so things
> like "git log --pretty=tformat:%H -z" do not work properly.
>
> Make it use the line terminator the user ordered.
>
> Signed-off-by: Jan Krüger <jk@jk.gs>
> ---
>  log-tree.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/log-tree.c b/log-tree.c
> index 34c49e7..44f0268 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -682,7 +682,7 @@ void show_log(struct rev_info *opt)
>  	if (opt->use_terminator) {
>  		if (!opt->missing_newline)
>  			graph_show_padding(opt->graph);
> -		putchar('\n');
> +		putchar(opt->diffopt.line_termination);
>  	}
>  
>  	strbuf_release(&msgbuf);
Looks sensible.  Perhaps we would want to add a test?
Jan Krüger· Apr 30, 2012, 20:28 UTC · re: Junio C Hamano · lore

[PATCH v2] log-tree: use custom line terminator in line termination mode

When using a custom format in line termination mode (as opposed to line separation mode), the configured line terminator is not used, so things like "git log --pretty=tformat:%H -z" do not work properly.

Make it use the line terminator the user ordered.
Signed-off-by: Jan Krüger <jk@jk.gs>
---
 Here are two simple tests, for both format: and tformat: with -z.
 log-tree.c                    |    2 +-
 t/t4205-log-pretty-formats.sh |   12 ++++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/log-tree.c b/log-tree.c
index 34c49e7..44f0268 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -682,7 +682,7 @@ void show_log(struct rev_info *opt)
 	if (opt->use_terminator) {
 		if (!opt->missing_newline)
 			graph_show_padding(opt->graph);
-		putchar('\n');
+		putchar(opt->diffopt.line_termination);
 	}
 
 	strbuf_release(&msgbuf);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 2ae9faa..03a73ba 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -71,4 +71,16 @@ test_expect_success 'alias loop' '
 	test_must_fail git log --pretty=test-foo
 '
 
+printf "add bar\0initial" > expected
+test_expect_success 'NUL separation' '
+	git log -z --pretty="format:%s" >actual &&
+	test_cmp expected actual
+'
+
+printf "add bar\0initial\0" > expected
+test_expect_success 'NUL termination' '
+	git log -z --pretty="tformat:%s" >actual &&
+	test_cmp expected actual
+'
+
 test_done

-- 
1.7.10.406.g0017
Junio C Hamano· Apr 30, 2012, 22:58 UTC · re: Jan Krüger · lore

Re: [PATCH v2] log-tree: use custom line terminator in line termination mode

Jan Krüger <jk@jk.gs> writes:
Show 9 quoted lines
> When using a custom format in line termination mode (as opposed to line
> separation mode), the configured line terminator is not used, so things
> like "git log --pretty=tformat:%H -z" do not work properly.
>
> Make it use the line terminator the user ordered.
>
> Signed-off-by: Jan Krüger <jk@jk.gs>
> ---
>  Here are two simple tests, for both format: and tformat: with -z.
Thanks for being thorough. Very much appreciated.
Having said that, are we sure that
	printf "add bar\0initial"

works per specification, or merely works by accident in some implementation?

In C, we have to write this as printf("add bar%cinitial", 0), and the above makes my stomach feel a bit queasy.

Admittedly we have "printf "\0\0" in t6024 and we haven't seen anybody complain for the past 6 years, so perhaps I shouldn't be worried too much about this.

Show 21 quoted lines
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index 2ae9faa..03a73ba 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -71,4 +71,16 @@ test_expect_success 'alias loop' '
>  	test_must_fail git log --pretty=test-foo
>  '
>  
> +printf "add bar\0initial" > expected
> +test_expect_success 'NUL separation' '
> +	git log -z --pretty="format:%s" >actual &&
> +	test_cmp expected actual
> +'
> +
> +printf "add bar\0initial\0" > expected
> +test_expect_success 'NUL termination' '
> +	git log -z --pretty="tformat:%s" >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_done
Jan Krüger· Apr 30, 2012, 23:28 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] log-tree: use custom line terminator in line termination mode

On 05/01/2012 12:58 AM, Junio C Hamano wrote:
Show 6 quoted lines
> Having said that, are we sure that
> 
> 	printf "add bar\0initial"
> 
> works per specification, or merely works by accident in some
> implementation?

I don't know for certain. In these cases I go by what dash (Debian's sh implementation) does. To quote its manpage:

"Only features designated by POSIX, plus a few Berkeley extensions, are being incorporated into this shell."

It does support this syntax for printf. That's enough for me personally. Is it enough for everyone? I don't know.

-Jan
Andreas Schwab· May 1, 2012, 00:28 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] log-tree: use custom line terminator in line termination mode

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> Having said that, are we sure that
>
> 	printf "add bar\0initial"
>
> works per specification, or merely works by accident in some
> implementation?

Since the backslash is not followed by $ ` " \ <newline> it is not special to the shell.

Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Junio C Hamano· May 1, 2012, 00:43 UTC · re: Andreas Schwab · lore

Re: [PATCH v2] log-tree: use custom line terminator in line termination mode

Andreas Schwab <schwab@linux-m68k.org> writes:
Show 11 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Having said that, are we sure that
>>
>> 	printf "add bar\0initial"
>>
>> works per specification, or merely works by accident in some
>> implementation?
>
> Since the backslash is not followed by $ ` " \ <newline> it is not
> special to the shell.

Yeah, but I wasn't worried about what shell does in the first place. I was worried about what printf(1) does.

Andreas Schwab· May 1, 2012, 07:29 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] log-tree: use custom line terminator in line termination mode

Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
> Andreas Schwab <schwab@linux-m68k.org> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Having said that, are we sure that
>>>
>>> 	printf "add bar\0initial"
>>>
>>> works per specification, or merely works by accident in some
>>> implementation?
>>
>> Since the backslash is not followed by $ ` " \ <newline> it is not
>> special to the shell.
>
> Yeah, but I wasn't worried about what shell does in the first place. I was
> worried about what printf(1) does.

Oh, I was confused becaused you mentioned the C case where the \NNN interpretation is done by the compiler, not printf(3).

Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Randal L. Schwartz· May 1, 2012, 02:51 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] log-tree: use custom line terminator in line termination mode

>>>>> "Junio" == Junio C Hamano <gitster@pobox.com> writes:
Junio> Having said that, are we sure that
Junio> 	printf "add bar\0initial"

Junio> works per specification, or merely works by accident in some Junio> implementation?

>From the POSIX spec
(http://pubs.opengroup.org/onlinepubs/9699919799/utilities/printf.html):
    In addition to the escape sequences shown in XBD File Format Notation (
    '\\' , '\a' , '\b' , '\f' , '\n' , '\r' , '\t' , '\v' ), "\ddd" , where
    ddd is a one, two, or three-digit octal number, shall be written as a
    byte with the numeric value specified by the octal number.
Looks pretty intentional to me.  \0 is a nul.
-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.posterous.com/ for Smalltalk discussion
Junio C Hamano· May 1, 2012, 04:27 UTC · re: Randal L. Schwartz · lore

Re: [PATCH v2] log-tree: use custom line terminator in line termination mode

merlyn@stonehenge.com (Randal L. Schwartz) writes:
Show 19 quoted lines
>>>>>> "Junio" == Junio C Hamano <gitster@pobox.com> writes:
>
> Junio> Having said that, are we sure that
>
> Junio> 	printf "add bar\0initial"
>
>
> Junio> works per specification, or merely works by accident in some
> Junio> implementation?
>
> From the POSIX spec
> (http://pubs.opengroup.org/onlinepubs/9699919799/utilities/printf.html):
>
>     In addition to the escape sequences shown in XBD File Format Notation (
>     '\\' , '\a' , '\b' , '\f' , '\n' , '\r' , '\t' , '\v' ), "\ddd" , where
>     ddd is a one, two, or three-digit octal number, shall be written as a
>     byte with the numeric value specified by the octal number.
>
> Looks pretty intentional to me.  \0 is a nul.

I was staring at the same passage, but somehow it didn't "click" for me that the above is _not_ saying that the NUL would terminate the string.

So I was worried too much and needlessly.  Thanks.
Jeff King· May 1, 2012, 08:56 UTC · re: Junio C Hamano · lore

Re: [PATCH] log-tree: use custom line terminator in line termination mode

On Mon, Apr 30, 2012 at 12:36:07PM -0700, Junio C Hamano wrote:
Show 28 quoted lines
> Jan Krüger <jk@jk.gs> writes:
> 
> > When using a custom format in line termination mode (as opposed to line
> > separation mode), the configured line terminator is not used, so things
> > like "git log --pretty=tformat:%H -z" do not work properly.
> >
> > Make it use the line terminator the user ordered.
> >
> > Signed-off-by: Jan Krüger <jk@jk.gs>
> > ---
> >  log-tree.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/log-tree.c b/log-tree.c
> > index 34c49e7..44f0268 100644
> > --- a/log-tree.c
> > +++ b/log-tree.c
> > @@ -682,7 +682,7 @@ void show_log(struct rev_info *opt)
> >  	if (opt->use_terminator) {
> >  		if (!opt->missing_newline)
> >  			graph_show_padding(opt->graph);
> > -		putchar('\n');
> > +		putchar(opt->diffopt.line_termination);
> >  	}
> >  
> >  	strbuf_release(&msgbuf);
> 
> Looks sensible.  Perhaps we would want to add a test?

Hmm. This came up before, and the issue is (or can be) slightly more complex:

  http://thread.gmane.org/gmane.comp.version-control.git/122478/focus=122568
-Peff
Junio C Hamano· May 1, 2012, 16:12 UTC · re: Jeff King · lore

Re: [PATCH] log-tree: use custom line terminator in line termination mode

Jeff King <peff@peff.net> writes:
Show 35 quoted lines
> On Mon, Apr 30, 2012 at 12:36:07PM -0700, Junio C Hamano wrote:
>
>> Jan Krüger <jk@jk.gs> writes:
>> 
>> > When using a custom format in line termination mode (as opposed to line
>> > separation mode), the configured line terminator is not used, so things
>> > like "git log --pretty=tformat:%H -z" do not work properly.
>> >
>> > Make it use the line terminator the user ordered.
>> >
>> > Signed-off-by: Jan Krüger <jk@jk.gs>
>> > ---
>> >  log-tree.c |    2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/log-tree.c b/log-tree.c
>> > index 34c49e7..44f0268 100644
>> > --- a/log-tree.c
>> > +++ b/log-tree.c
>> > @@ -682,7 +682,7 @@ void show_log(struct rev_info *opt)
>> >  	if (opt->use_terminator) {
>> >  		if (!opt->missing_newline)
>> >  			graph_show_padding(opt->graph);
>> > -		putchar('\n');
>> > +		putchar(opt->diffopt.line_termination);
>> >  	}
>> >  
>> >  	strbuf_release(&msgbuf);
>> 
>> Looks sensible.  Perhaps we would want to add a test?
>
> Hmm. This came up before, and the issue is (or can be) slightly more
> complex:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/122478/focus=122568

Yeah, the test given completely forgets about "log -p" case, as you said in the above:

    But we can't just modify that to use the specified line terminator,
    because sometimes it is acting as a separator between commit message and
    diff, and sometimes it is acting as the terminator of the whole record.

So the patch is not quite right for the "log -p -z" (or "log --stat -z") case.

The correct output would have NUL after each commit, so "-z --format=%s" would have a single-liner subject with the line-terminating LF replaced with NUL, and "-p/--stat -z --format=%s" would have a single-liner subject with its line-terminating LF, followed by the diff/diffstat in which the terminating LF of the last line is replaced with NUL, but to be consistent with what "-p/--stat -z --pretty=format:%s" does, I think it is OK to append NUL to the diff/diffstat part instead of replacing its last LF with NUL.

Junio C Hamano· May 1, 2012, 17:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] log-tree: use custom line terminator in line termination mode

Junio C Hamano <gitster@pobox.com> writes:
Show 25 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> Hmm. This came up before, and the issue is (or can be) slightly more
>> complex:
>>
>>   http://thread.gmane.org/gmane.comp.version-control.git/122478/focus=122568
>
> Yeah, the test given completely forgets about "log -p" case, as you said
> in the above:
>
>     But we can't just modify that to use the specified line terminator,
>     because sometimes it is acting as a separator between commit message and
>     diff, and sometimes it is acting as the terminator of the whole record.
>
> So the patch is not quite right for the "log -p -z" (or "log --stat -z")
> case.
>
> The correct output would have NUL after each commit, so "-z --format=%s"
> would have a single-liner subject with the line-terminating LF replaced
> with NUL, and "-p/--stat -z --format=%s" would have a single-liner subject
> with its line-terminating LF, followed by the diff/diffstat in which the
> terminating LF of the last line is replaced with NUL, but to be consistent
> with what "-p/--stat -z --pretty=format:%s" does, I think it is OK to
> append NUL to the diff/diffstat part instead of replacing its last LF with
> NUL.

In other words, this test on top (the last one only demonstrates the breakage).

 t/t4205-log-pretty-formats.sh |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index f00e446..4afd778 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -83,4 +83,20 @@ test_expect_success 'NUL termination' '
 	test_cmp expected actual
 '
 
+test_expect_success 'NUL separation with --stat' '
+	stat0_part=$(git diff --stat HEAD^ HEAD) &&
+	stat1_part=$(git diff --stat --root HEAD^) &&
+	printf "add bar\n$stat0_part\n\0initial\n$stat1_part\n" >expected &&
+	git log -z --stat --pretty="format:%s" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure 'NUL termination with --stat' '
+	stat0_part=$(git diff --stat HEAD^ HEAD) &&
+	stat1_part=$(git diff --stat --root HEAD^) &&
+	printf "add bar\n$stat0_part\n\0initial\n$stat1_part\n\0" >expected &&
+	git log -z --stat --pretty="tformat:%s" >actual &&
+	test_cmp expected actual
+'
+
 test_done
Junio C Hamano· May 1, 2012, 20:03 UTC · re: Junio C Hamano · lore

Re: [PATCH] log-tree: use custom line terminator in line termination mode

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
>> The correct output would have NUL after each commit, so "-z --format=%s"
>> would have a single-liner subject with the line-terminating LF replaced
>> with NUL, and "-p/--stat -z --format=%s" would have a single-liner subject
>> with its line-terminating LF, followed by the diff/diffstat in which the
>> terminating LF of the last line is replaced with NUL, but to be consistent
>> with what "-p/--stat -z --pretty=format:%s" does, I think it is OK to
>> append NUL to the diff/diffstat part instead of replacing its last LF with
>> NUL.
>
> In other words, this test on top (the last one only demonstrates the
> breakage).

Just a short hint if anybody wants to take a stab at it while I am deeply in today's integration cycle.

The code for "separator" semantics was a well tested code when "terminator" semantics was bolted on, and operated like this:

        for each record
        do
                if we have shown a record already
                then
                        show the termination character
                fi
                show the record
        done

The only difference between the two semantics is if we append the termination character after all the above is done, so the code should be structured that way, but that may not be how we currently do it. So the proper way to add the "terminator" semantics ought to be:

        if we have shown any record in the loop && opt->use_terminator
        then
                show the termination character
        fi

at the end of the above loop. If we see opt->use_terminator anywhere else in the existing code, it is an indication of a bug.

There is one small glitch. If a record is _not_ terminated with a newline, e.g. "log --pretty=format:%s" without --stat/-p, "separator" semantics will end up giving an incomplete line at the end. Most of the time we will be piping out output to the pager so it may not be a problem in the real life, but it would be nicer to also terminate such an output.

So the resulting logic should look something like:
        for each record
        do
                if we have shown a record already
                then
                        show the termination character
                fi
                show the record
                remember if the record ended with a LF
        done
        if we have shown any record in the loop &&
           (opt->use_terminator ||
            (opt->diffopt.line_termination == '\n' &&
             the last record did not end with a LF))
        then
                show the termination character
        fi

← back to recent threads