git log -z still outputting newlines?

git log -z still outputting newlines?

[Randal L. Schwartz]

$ 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

Re: git log -z still outputting newlines?

[Andreas Schwab]

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.

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."

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

[Jan Krüger]

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

Re: git log -z still outputting newlines?

[Thomas Rast]

Andreas Schwab <schwab@linux-m68k.org> writes:

> 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

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

[Junio C Hamano]

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?

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

[Jan Krüger]

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

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

[Junio C Hamano]

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>
> ---
>  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.

> 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

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

[Jan Krüger]

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

On 05/01/2012 12:58 AM, Junio C Hamano wrote:
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]

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

[Andreas Schwab]

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.

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."

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

[Junio C Hamano]

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.

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

[Randal L. Schwartz]

>>>>> "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

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

[Junio C Hamano]

merlyn@stonehenge.com (Randal L. Schwartz) writes:

>>>>>> "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.

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

[Andreas Schwab]

Junio C Hamano <gitster@pobox.com> writes:

> 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."

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

[Jeff King]

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

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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