threads / patch / 12784

patchpretty.c: add %z specifier.

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

## tl;dr

8 messages between Mar 21, 2008 and Mar 21, 2008. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Govind Salinas· Mar 21, 2008, 00:45 UTC · lore

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(-)
Show changes to 2 files +8 −2

log-tree.c, pretty.c

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
Jeff King· Mar 21, 2008, 02:13 UTC · re: Govind Salinas · lore

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

On Thu, Mar 20, 2008 at 07:45:26PM -0500, Govind Salinas wrote:
Show 5 quoted lines
> 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
Junio C Hamano· Mar 21, 2008, 04:48 UTC · re: Govind Salinas · lore

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

"Govind Salinas" <govind@sophiasuchtig.com> writes:
Show 15 quoted lines
> 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)?

Jeff King· Mar 21, 2008, 04:51 UTC · re: Junio C Hamano · lore

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

On Thu, Mar 20, 2008 at 09:48:16PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> > +	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
Junio C Hamano· Mar 21, 2008, 05:09 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> 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.

Govind Salinas· Mar 21, 2008, 05:42 UTC · re: Junio C Hamano · lore

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

On Fri, Mar 21, 2008 at 12:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 44 quoted lines
>
> 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.

David Symonds· Mar 21, 2008, 05:50 UTC · re: Govind Salinas · lore

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

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.
Junio C Hamano· Mar 21, 2008, 06:19 UTC · re: Govind Salinas · lore

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

"Govind Salinas" <govind@sophiasuchtig.com> writes:
Show 25 quoted lines
> 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(-)
Show changes to pretty.c +13 −0
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 */

← back to recent threads