PATCH: ztrftime: Handle all non-zsh format chars with strftime if present

PATCH: ztrftime: Handle all non-zsh format chars with strftime if present

[Mikael Magnusson]

Jun wrote:
>2015/07/08 08:21, Mikael Magnusson <mikachu@gmail.com> wrote:
>> % print -P %D\{%x\}
>> 2015年07月08日
>> % print -P %D\{%Ex\}
>> 平成27年07月08日
>
> It works for %Ex but not for %Ey.
> ztrftime() does not send %Ey to strftime() but ignores E and
> handles %y by itself.
> 
> % date +%y
> 15
> % date +%Ey
> 27
> % print -P '%D{%y %Ey}'
> 15 15
> 
> Is it possible to pass the entire format string to strftime()
> if HAVE_STRFTIME is defined?

This is one way to do it, but it makes this statement in the manpage
untrue. Perhaps this is a good tradeoff though? I'm sure someone on a
non-GNU system will disagree :).

  The  GNU  extension  that a `-' between the % and the format character
  causes a leading zero or space to be stripped is handled  directly  by
  the  shell for the format characters d, f, H, k, l, m, M, S and y; any
  other format characters are provided to strftime()  with  any  leading
  `-', present, so the handling is system dependent.  Further GNU exten‐
  sions are not supported at present.

We could keep track of which modifiers have been seen and handle %-f (for
example), but it'll be a mess... Maybe at the start of the switch, we
can check if (fmt - fmtstart is <= 2 && strip), or something like that? I'll
poke at it.

---
 Src/utils.c | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/Src/utils.c b/Src/utils.c
index 2d53a00..e03e180 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2948,6 +2948,12 @@ morefmt:
 		sprintf(buf, "%0*ld", digs, usec);
 		buf += digs;
 		break;
+	    case '\0':
+		/* Guard against premature end of string */
+		*buf++ = '%';
+		fmt--;
+		break;
+#ifndef HAVE_STRFTIME
 	    case 'd':
 		if (tm->tm_mday > 9 || !strip)
 		    *buf++ = '0' + tm->tm_mday / 10;
@@ -3012,12 +3018,6 @@ morefmt:
 		    *buf++ = '0' + (tm->tm_year / 10) % 10;
 		*buf++ = '0' + tm->tm_year % 10;
 		break;
-	    case '\0':
-		/* Guard against premature end of string */
-		*buf++ = '%';
-		fmt--;
-		break;
-#ifndef HAVE_STRFTIME
 	    case 'Y':
 	    {
 		/*
@@ -3065,6 +3065,38 @@ morefmt:
 	    case '-':
 	    case '0' ... '9':
 		goto morefmt;
+	    case 'f': /* copy of code above */
+		strip = 1;
+		if (tm->tm_mday > 9)
+		    *buf++ = '0' + tm->tm_mday / 10;
+		else if (!strip)
+		    *buf++ = ' ';
+		*buf++ = '0' + tm->tm_mday % 10;
+		break;
+	    case 'K': /* copy of code above */
+		strip = 1;
+		if (tm->tm_hour > 9)
+		    *buf++ = '0' + tm->tm_hour / 10;
+		else if (!strip) {
+		    if (fmt[-1] == 'H')
+			*buf++ = '0';
+		    else
+			*buf++ = ' ';
+		}
+		*buf++ = '0' + tm->tm_hour % 10;
+		break;
+	    case 'L': /* copy of code above */
+		strip = 1;
+		hr12 = tm->tm_hour % 12;
+		if (hr12 == 0)
+		    hr12 = 12;
+	        if (hr12 > 9)
+		    *buf++ = '1';
+		else if (!strip)
+		    *buf++ = ' ';
+
+		*buf++ = '0' + (hr12 % 10);
+		break;
 	    default:
 		/*
 		 * Remember we've already allowed for two characters
-- 
2.4.0

PATCH: ztrftime: Handle all non-zsh format chars with strftime if present (v2)

[Mikael Magnusson]

At this point, even I find the code slightly questionable. And this doesn't even work properly yet:
% print -P %D\{aa%14a\}
aa%
% print -P %D\{aaa%14a\}
aaa           Wed

---
 Src/utils.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/Src/utils.c b/Src/utils.c
index 2d53a00..c18a8de 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2933,6 +2933,11 @@ ztrftime(char *buf, int bufsize, char *fmt, struct tm *tm, long usec)
 	    if (ztrftimebuf(&bufsize, 2))
 		return -1;
 morefmt:
+	    if (!((fmt - fmtstart == 1) || (fmt - fmtstart == 2 && strip) || *fmt == '.')) {
+		while (*fmt && strchr("OE^#_-0123456789", *fmt++));
+		if (*fmt++)
+		    goto strftimehandling;
+	    }
 	    switch (*fmt++) {
 	    case '.':
 		if (ztrftimebuf(&bufsize, digs))
@@ -2948,10 +2953,10 @@ morefmt:
 		sprintf(buf, "%0*ld", digs, usec);
 		buf += digs;
 		break;
-	    case 'd':
-		if (tm->tm_mday > 9 || !strip)
-		    *buf++ = '0' + tm->tm_mday / 10;
-		*buf++ = '0' + tm->tm_mday % 10;
+	    case '\0':
+		/* Guard against premature end of string */
+		*buf++ = '%';
+		fmt--;
 		break;
 	    case 'f':
 		strip = 1;
@@ -2992,6 +2997,12 @@ morefmt:
 
 		*buf++ = '0' + (hr12 % 10);
 		break;
+#ifndef HAVE_STRFTIME
+	    case 'd':
+		if (tm->tm_mday > 9 || !strip)
+		    *buf++ = '0' + tm->tm_mday / 10;
+		*buf++ = '0' + tm->tm_mday % 10;
+		break;
 	    case 'm':
 		if (tm->tm_mon > 8 || !strip)
 		    *buf++ = '0' + (tm->tm_mon + 1) / 10;
@@ -3012,12 +3023,6 @@ morefmt:
 		    *buf++ = '0' + (tm->tm_year / 10) % 10;
 		*buf++ = '0' + tm->tm_year % 10;
 		break;
-	    case '\0':
-		/* Guard against premature end of string */
-		*buf++ = '%';
-		fmt--;
-		break;
-#ifndef HAVE_STRFTIME
 	    case 'Y':
 	    {
 		/*
@@ -3065,6 +3070,7 @@ morefmt:
 	    case '-':
 	    case '0' ... '9':
 		goto morefmt;
+strftimehandling:
 	    default:
 		/*
 		 * Remember we've already allowed for two characters
-- 
2.4.0

Re: PATCH: ztrftime: Handle all non-zsh format chars with strftime if present (v2)

[Mikael Magnusson]

On Wed, Jul 8, 2015 at 5:06 PM, Mikael Magnusson <mikachu@gmail.com> wrote:
> At this point, even I find the code slightly questionable. And this doesn't even work properly yet:
> % print -P %D\{aa%14a\}
> aa%
> % print -P %D\{aaa%14a\}
> aaa           Wed
>
> ---
>  Src/utils.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/Src/utils.c b/Src/utils.c
> index 2d53a00..c18a8de 100644
> --- a/Src/utils.c
> +++ b/Src/utils.c
> @@ -2933,6 +2933,11 @@ ztrftime(char *buf, int bufsize, char *fmt, struct tm *tm, long usec)
>             if (ztrftimebuf(&bufsize, 2))
>                 return -1;
>  morefmt:
> +           if (!((fmt - fmtstart == 1) || (fmt - fmtstart == 2 && strip) || *fmt == '.')) {
> +               while (*fmt && strchr("OE^#_-0123456789", *fmt++));
> +               if (*fmt++)
> +                   goto strftimehandling;
> +           }
>             switch (*fmt++) {
>             case '.':
>                 if (ztrftimebuf(&bufsize, digs))

Okay, fun thinko here, should be
-               while (*fmt && strchr("OE^#_-0123456789", *fmt++));
+               while (*fmt && strchr("OE^#_-0123456789", *fmt))
+                   fmt++;

I even had to stare at this for a minute to realize why it fixes it.

-- 
Mikael Magnusson

Re: PATCH: ztrftime: Handle all non-zsh format chars with strftime if present (v2)

[Bart Schaefer]

On Jul 9,  6:31am, Mikael Magnusson wrote:
}
} > +               while (*fmt && strchr("OE^#_-0123456789", *fmt++));
} > +               if (*fmt++)
} > +                   goto strftimehandling;
} > +           }
} >             switch (*fmt++) {
} 
} Okay, fun thinko here, should be
} -               while (*fmt && strchr("OE^#_-0123456789", *fmt++));
} +               while (*fmt && strchr("OE^#_-0123456789", *fmt))
} +                   fmt++;

Isn't there still a thinko there?  In the event that the "while" loop
terminates because *fmt == 0, the subsequent "if (*fmt++)" will still
increment fmt past the end of the string, and then "switch (*fmt++)"
becomes an invalid dereference.

Or is that eventuality already prevented by the surrounding code?  I
haven't been applying these patches and still haven't seen the whole
thing in one diff.

Re: PATCH: ztrftime: Handle all non-zsh format chars with strftime if present (v2)

[Mikael Magnusson]

On Thu, Jul 9, 2015 at 7:43 AM, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jul 9,  6:31am, Mikael Magnusson wrote:
> }
> } > +               while (*fmt && strchr("OE^#_-0123456789", *fmt++));
> } > +               if (*fmt++)
> } > +                   goto strftimehandling;
> } > +           }
> } >             switch (*fmt++) {
> }
> } Okay, fun thinko here, should be
> } -               while (*fmt && strchr("OE^#_-0123456789", *fmt++));
> } +               while (*fmt && strchr("OE^#_-0123456789", *fmt))
> } +                   fmt++;
>
> Isn't there still a thinko there?  In the event that the "while" loop
> terminates because *fmt == 0, the subsequent "if (*fmt++)" will still
> increment fmt past the end of the string, and then "switch (*fmt++)"
> becomes an invalid dereference.
>
> Or is that eventuality already prevented by the surrounding code?  I
> haven't been applying these patches and still haven't seen the whole
> thing in one diff.

No, I think you're right. However, I can't get it to fail with my
current code either. It seems better to do the fmt++ only in the true
case though.

-- 
Mikael Magnusson