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

Jeff King [off-list ref] writes:

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

quoted

quoted

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

On Fri, Mar 21, 2008 at 12:09 AM, Junio C Hamano [off-list ref] wrote:

Jeff King [off-list ref] 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.

On Fri, Mar 21, 2008 at 4:42 PM, Govind Salinas
[off-list ref] 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.

"Govind Salinas" [off-list ref] writes:

On Fri, Mar 21, 2008 at 12:09 AM, Junio C Hamano [off-list ref] wrote:

quoted

Jeff King [off-list ref] 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 */