[PATCH] pretty.c: add %z specifier.

[PATCH] pretty.c: add %z specifier.

[Govind Salinas]

This adds a %z format which prints out a null character.  This allows for
easier machine parsing of multiline data.  It is also necessary to use write
to print out the data since printf will terminate at a null.  That in turn
requires that an fflush be executed before the write to preserve the order
the data is printed.

Signed-off-by: Govind Salinas <blix@sophiasuchtig.com>
---
 log-tree.c |    7 +++++--
 pretty.c   |    3 +++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 608f697..e116a1f 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -308,8 +308,11 @@ void show_log(struct rev_info *opt, const char *sep)
 	if (opt->show_log_size)
 		printf("log size %i\n", (int)msgbuf.len);

-	if (msgbuf.len)
-		printf("%s%s%s", msgbuf.buf, extra, sep);
+	if (msgbuf.len) {
+		fflush(stdout);
+		write(STDOUT_FILENO, msgbuf.buf, msgbuf.len);
+		printf("%s%s", extra, sep);
+	}
 	strbuf_release(&msgbuf);
 }

diff --git a/pretty.c b/pretty.c
index 703f521..fd155ec 100644
--- a/pretty.c
+++ b/pretty.c
@@ -478,6 +478,9 @@ static size_t format_commit_item(struct strbuf
*sb, const char *placeholder,
 	case 'n':		/* newline */
 		strbuf_addch(sb, '\n');
 		return 1;
+	case 'z':		/* null */
+		strbuf_addch(sb, '\0');
+		return 1;
 	}

 	/* these depend on the commit */
-- 
1.5.4.4.552.g9987b

Re: [PATCH] pretty.c: add %z specifier.

[Jeff King]

On Thu, Mar 20, 2008 at 07:45:26PM -0500, Govind Salinas wrote:

> This adds a %z format which prints out a null character.  This allows for
> easier machine parsing of multiline data.  It is also necessary to use write
> to print out the data since printf will terminate at a null.  That in turn
> requires that an fflush be executed before the write to preserve the order
> the data is printed.

How about using fwrite instead of write?

-Peff

Re: [PATCH] pretty.c: add %z specifier.

[Junio C Hamano]

"Govind Salinas" <govind@sophiasuchtig.com> writes:

> diff --git a/pretty.c b/pretty.c
> index 703f521..fd155ec 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -478,6 +478,9 @@ static size_t format_commit_item(struct strbuf
> *sb, const char *placeholder,
>  	case 'n':		/* newline */
>  		strbuf_addch(sb, '\n');
>  		return 1;
> +	case 'z':		/* null */
> +		strbuf_addch(sb, '\0');
> +		return 1;
>  	}
>
>  	/* these depend on the commit */

I do not like this at all.  Why aren't we doing %XX (2 hexadecimal digits
for an octet)?

Re: [PATCH] pretty.c: add %z specifier.

[Jeff King]

On Thu, Mar 20, 2008 at 09:48:16PM -0700, Junio C Hamano wrote:

> > +	case 'z':		/* null */
> > +		strbuf_addch(sb, '\0');
> > +		return 1;
> >  	}
> >
> >  	/* these depend on the commit */
> 
> I do not like this at all.  Why aren't we doing %XX (2 hexadecimal digits
> for an octet)?

Because %ad is already taken? :)

%x* is still available, though, so maybe %x00?

-Peff

Re: [PATCH] pretty.c: add %z specifier.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 20, 2008 at 09:48:16PM -0700, Junio C Hamano wrote:
>
>> > +	case 'z':		/* null */
>> > +		strbuf_addch(sb, '\0');
>> > +		return 1;
>> >  	}
>> >
>> >  	/* these depend on the commit */
>> 
>> I do not like this at all.  Why aren't we doing %XX (2 hexadecimal digits
>> for an octet)?
>
> Because %ad is already taken? :)
>
> %x* is still available, though, so maybe %x00?

Perhaps, but before I forget.

My much bigger niggle about the "--pretty=format:<>" code I have is that
the "log" machinery does not change the usual record "delimiter" to record
"terminator" when --pretty=format:<> is in effect.

The "log" family generally treats LF/NUL as record delimiter, not
terminator, and it is by a very good conscious design.  When you are
looking at the output from "git log -2", you would want to have a
delimiting LF between the first commit and the second commit, but you do
not want an extra LF after the second commit.

However, when "--pretty=format:<>" is in effect, it is inconvenient that
the machinery inserts a LF between each record but not at the end.

    $ git log -2 --pretty=format:%s

may look sane when the pager immediately returns the control to you, but
it is not really.  To view it:

    $ git log -2 --pretty=format:%s | cat

This would show that there is no LF after the final output, which is quite
bad.

Re: [PATCH] pretty.c: add %z specifier.

[Govind Salinas]

On Fri, Mar 21, 2008 at 12:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
>  > On Thu, Mar 20, 2008 at 09:48:16PM -0700, Junio C Hamano wrote:
>  >
>  >> > +  case 'z':               /* null */
>  >> > +          strbuf_addch(sb, '\0');
>  >> > +          return 1;
>  >> >    }
>  >> >
>  >> >    /* these depend on the commit */
>  >>
>  >> I do not like this at all.  Why aren't we doing %XX (2 hexadecimal digits
>  >> for an octet)?
>  >
>  > Because %ad is already taken? :)
>  >
>  > %x* is still available, though, so maybe %x00?
>
>  Perhaps, but before I forget.
>
>  My much bigger niggle about the "--pretty=format:<>" code I have is that
>  the "log" machinery does not change the usual record "delimiter" to record
>  "terminator" when --pretty=format:<> is in effect.
>
>  The "log" family generally treats LF/NUL as record delimiter, not
>  terminator, and it is by a very good conscious design.  When you are
>  looking at the output from "git log -2", you would want to have a
>  delimiting LF between the first commit and the second commit, but you do
>  not want an extra LF after the second commit.
>
>  However, when "--pretty=format:<>" is in effect, it is inconvenient that
>  the machinery inserts a LF between each record but not at the end.
>
>     $ git log -2 --pretty=format:%s
>
>  may look sane when the pager immediately returns the control to you, but
>  it is not really.  To view it:
>
>     $ git log -2 --pretty=format:%s | cat
>
>  This would show that there is no LF after the final output, which is quite
>  bad.
>

Sorry, I'm a bit confused.  Should I alter the patch to use a different code
for null, that would be fine by me?  The above seems to be an unrelated issue.


Thanks,
Govind.

Re: [PATCH] pretty.c: add %z specifier.

[David Symonds]

On Fri, Mar 21, 2008 at 4:42 PM, Govind Salinas
<govind@sophiasuchtig.com> wrote:
>
>  Sorry, I'm a bit confused.  Should I alter the patch to use a different code
>  for null, that would be fine by me?  The above seems to be an unrelated issue.

I'm pretty sure the suggestion is that you should change the patch to
allow for *any* specific byte value, where the null byte is just a
special case. %x00 would be used instead of %z, in other words, and
%x20 would be a space character, etc.


Dave.

Re: [PATCH] pretty.c: add %z specifier.

[Junio C Hamano]

"Govind Salinas" <govind@sophiasuchtig.com> writes:

> On Fri, Mar 21, 2008 at 12:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Jeff King <peff@peff.net> writes:
>>
>>  > On Thu, Mar 20, 2008 at 09:48:16PM -0700, Junio C Hamano wrote:
>>  >
>>  >> > +  case 'z':               /* null */
>>  >> > +          strbuf_addch(sb, '\0');
>>  >> > +          return 1;
>>  >> >    }
>>  >> >
>>  >> >    /* these depend on the commit */
>>  >>
>>  >> I do not like this at all.  Why aren't we doing %XX (2 hexadecimal digits
>>  >> for an octet)?
>>  >
>>  > Because %ad is already taken? :)
>>  >
>>  > %x* is still available, though, so maybe %x00?
>>
>>  Perhaps, but before I forget.
>> ...
>
> Sorry, I'm a bit confused.  Should I alter the patch to use a different code
> for null, that would be fine by me?  The above seems to be an unrelated issue.

Sorry for confusing you.  The above is an unrelated issue.  But at least
to me it is much more important one.  I would not be unhappy at all if we
did not have either %z nor %x00, but the above bugs me moderately.  Also I
suspect the proper fix for that issue would involve the part in log-tree
you touched.

By the way, I think Jeff's suggestion of %x00 makes more sense than %z.

 pretty.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/pretty.c b/pretty.c
index 16bfb86..308bfad 100644
--- a/pretty.c
+++ b/pretty.c
@@ -457,6 +457,7 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	const struct commit *commit = c->commit;
 	const char *msg = commit->buffer;
 	struct commit_list *p;
+	int h1, h2;
 
 	/* these are independent of the commit */
 	switch (placeholder[0]) {
@@ -478,6 +479,18 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 	case 'n':		/* newline */
 		strbuf_addch(sb, '\n');
 		return 1;
+	case 'x':
+		/* %x00 == NUL, %x0a == LF, etc. */
+		if (0 <= (h1 = hexval_table[0xff & placeholder[1]]) &&
+		    h1 <= 16 &&
+		    0 <= (h2 = hexval_table[0xff & placeholder[2]]) &&
+		    h2 <= 16) {
+			strbuf_addch(sb, (h1<<4)|h2);
+			return 2;
+		} else {
+			return 0;
+		}
+		
 	}
 
 	/* these depend on the commit */