Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

printf %T formats do not follow strftime(3) #62

Closed
posguy99 opened this issue Jul 5, 2020 · 10 comments · Fixed by #65
Closed

printf %T formats do not follow strftime(3) #62

posguy99 opened this issue Jul 5, 2020 · 10 comments · Fixed by #65
Labels
bug Something is not working

Comments

@posguy99
Copy link

posguy99 commented Jul 5, 2020

It's simple:

[6] iMac $ date +'%l:%M %p'
 8:26 PM

but:

[8] iMac $ printf '%(%l:%M %p)T\n'
Jul  4 20:27:27 PM

The man page for strftime(3) says %l is supposed to be the 12-hour hour without leading zeroes:

     %l    is replaced by the hour (12-hour clock) as a decimal number (1-12); single digits are preceded by
           a blank.

And I didn't ask for the seconds at all, but ksh is displaying them.

93u+ and 93u+m both do the same thing. I can't find %()T anywhere in the new Korn book, but Learning the Korn Shell 2nd ed says it should reformat it "according to the date(1) specification." (p225)

(macOS 10.14.6)

@McDutchie McDutchie added the bug Something is not working label Jul 5, 2020
@McDutchie
Copy link

McDutchie commented Jul 5, 2020

It seems to be particularly %l that is broken:

$ date +%l; printf '%(%l)T\n'
 7
Jul  5 07:54
$ date +%M; printf '%(%M)T\n'
54
54
$ date +%p; printf '%(%p)T\n'
AM
AM

So you weren't getting seconds; you were getting minutes twice.

This is broken in ksh2020 too, so there's no existing fix to backport.

@JohnoKing
Copy link

JohnoKing commented Jul 5, 2020

tmxfmt is treating %l as an alias for %QL:

case 'l': /* (AST) OBSOLETE use %QL */
p = "%QL";
goto push;

$ printf '%(%l)T\n'; printf '%(%QL)T\n'
Jul  4 23:14
Jul  4 23:14

@McDutchie
Copy link

McDutchie commented Jul 5, 2020

Wow. And there's a bunch more of these translations marked obsolete as well. The full list of those marked obsolete:

%f => %Qf
%i => %QI
%k => %QD
%l => %QL
%L => %Ql
%q => %Qz

None of these are specified by POSIX strftime so technically ksh is perfectly within its rights to give these its own meaning in its own implementation of strftime(3).

But it's clearly against widespread expectations, the principle of least astonishment, etc. to act differently from GNU and BSD strftime even for technically non-standard conversion specifiers.

Going by to the multishell ksh93 repo (thanks for linking to that in another comment @JohnoKing, I'd no idea this existed), these have been marked obsolete as of version 2010-06-21.

The ones that conflict with BSD strftime(3) as on my Mac are: %k, %l. The ones that conflict with Linux (really glibc) strftime(3) as on Debian are: %k, %l – the same ones, and they also behave the same on glibc as they do on BSD. So it seems we need to make those two behave like glibc and BSD at the very least.

Or we could be radical and strip out the entire AST implementation of strftime(3) and use the OS's standard strftime(3) instead. But this branch is not really about that, is it? There are probably old ksh scripts that use some of these AST-isms. So, it should also be documented that printf %T uses the AST variant of strftime(3) and not your operating system's. As far as I know, that's not currently mentioned anywhere.

I wonder if even changing the behaviour of %k and %l might break some ksh scripts. But even if it did, I've a feeling that changing these two to match Linux and BSD may fix more breakage than it causes.

Opinions welcome…

@McDutchie
Copy link

On Busybox strftime(3), %k and %l act like on glibc and BSD, but there's an additional conflict: %f

$ ksh --version; date --version
  version         sh (AT&T Research) 93u+ 2012-08-01
date: unrecognized option '--version'
BusyBox v1.31.1 (2020-02-03 04:38:19 CET) multi-call binary.
[...long usage blurb skipped...]
$ date +%f; ksh -c 'printf %(%f)T\\n'
2020.07.05-14:08:00
Sun Jul  5 14:08:00 CEST 2020

@McDutchie
Copy link

One thing that is definitely a bug is how printf %T is documented in sh.1/ksh.1:

              %(date-format)T
                     A %(date-format)T format can be used to treat an argument
                     as a date/time string and to format the date/time accord-
                     ing  to  the  date-format as defined for the date(1) com-
                     mand.

and in printf --man:

    %T    Treat string as a date/time string and format it. The T can be
          preceded by (dformat), where dformat is a date format as defined by
          the date command.

Both of these are obviously wrong. ksh does not use the date(1) command, which uses the OS-provided strftime(3); it uses its own implementation of strftime(3) which will always differ from the OS-provided implementation for non-standard conversion specifiers simply because the OS-provided implementations differ from each other.

So the only way to make ksh behave according to the documentation would be to remove libast's strftime(3) and use the OS-provided one instead. But that risks creating too many backwards compatibility problems.

@JohnoKing
Copy link

JohnoKing commented Jul 5, 2020

Here is a quick patch to make AST %k and %l behave the same as glibc and BSD (this doesn't change %f):

--- a/src/lib/libast/tm/tmxfmt.c
+++ b/src/lib/libast/tm/tmxfmt.c
@@ -316,9 +316,9 @@ tmxfmt(char* buf, size_t len, const char* format, Time_t t)
 		case 'J':	/* Julian date (0 offset) */
 			cp = number(cp, ep, (long)tm->tm_yday, 3, width, pad);
 			continue;
-		case 'k':	/* (AST) OBSOLETE use %QD */
-			p = "%QD";
-			goto push;
+		case 'k':	/* hour (0 - 23) with blank padding */
+			cp = number(cp, ep, (long)tm->tm_hour, -2, width, pad);
+			continue;
 		case 'K':	/* (AST) largest to smallest */
 			switch (alt)
 			{
@@ -333,9 +333,11 @@ tmxfmt(char* buf, size_t len, const char* format, Time_t t)
 				break;
 			}
 			goto push;
-		case 'l':	/* (AST) OBSOLETE use %QL */
-			p = "%QL";
-			goto push;
+		case 'l':	/* hour (0 - 12) with blank padding */
+			if ((n = tm->tm_hour) > 12) n -= 12;
+			else if (n == 0) n = 12;
+			cp = number(cp, ep, (long)n, -2, width, pad);
+			continue;
 		case 'L':	/* (AST) OBSOLETE use %Ql */
 			p = "%Ql";
 			goto push;

number can be passed -2 to give the output blank padding:

/*
* format n with padding p into s
* return end of s
*
* p: <0 blank padding
* 0 no padding
* >0 0 padding
*/
static char*
number(register char* s, register char* e, register long n, register int p, int w, int pad)

@posguy99
Copy link
Author

posguy99 commented Jul 6, 2020

Yup, applied that, works.

[7] mbp13 $ date +'%l:%M %p'
12:56 PM

[8] mbp13 $ printf '%(%l:%M %p)T\n'
12:56 PM

Thanks!

JohnoKing added a commit to JohnoKing/ksh that referenced this issue Jul 7, 2020
This commit changes the behavior of four date formats accepted
by 'printf %()T' because the old behavior is not compatible with
modern implementations of date(1):
- %k and %l now return a blank-padded hour, the former based on a
  24-hour clock and the latter a 12-hour clock (these are common
  extensions present on Linux and BSD systems).
- %f now returns a date with the format %Y.%m.%d-%H:%M:%S'
  (BusyBox extension).
- %q now returns the quarter of the current year (GNU extension).

src/cmd/ksh93/data/builtins.c:
- Copy the date format documentation from date in libcmd to
  the printf man page (for documenting 'printf %T').

src/cmd/ksh93/tests/builtins.sh:
- Add four regression tests for the changed date formats.

src/cmd/ksh93/sh.1:
- Remove inaccurate information about the date formats accepted by
  printf %T'. The KornShell uses a custom version of strftime(3)
  that isn't guaranteed to accepts the same formats as the native
  strftime function.

src/lib/libast/tm/tmxfmt.c:
- Change the behavior of %f, %k, %l and %q to the common behavior.
  %k and %l are implemented as aliases to %_H and %_I to avoid
  duplicating code.

src/lib/libcmd/date.c:
- Update the documentation for the AST date command since it is
  also affected by the changes to 'printf %T'.

Fixes ksh93#62
JohnoKing added a commit to JohnoKing/ksh that referenced this issue Jul 7, 2020
This commit changes the behavior of four date formats accepted
by 'printf %()T' because the old behavior is not compatible with
modern implementations of date(1):
- %k and %l now return a blank-padded hour, the former based on a
  24-hour clock and the latter a 12-hour clock (these are common
  extensions present on Linux and *BSD).
- %f now returns a date with the format '%Y.%m.%d-%H:%M:%S'
  (BusyBox extension).
- %q now returns the quarter of the current year (GNU extension).

src/cmd/ksh93/data/builtins.c:
- Copy the date format documentation from date in libcmd to
  the printf man page (for documenting 'printf %T').

src/cmd/ksh93/tests/builtins.sh:
- Add four regression tests for the changed date formats.

src/cmd/ksh93/sh.1:
- Remove inaccurate information about the date formats accepted by
  printf %T'. The KornShell uses a custom version of strftime(3)
  that isn't guaranteed to accepts the same formats as the native
  strftime function.

src/lib/libast/tm/tmxfmt.c:
- Change the behavior of %f, %k, %l and %q to the common behavior.
  %k and %l are implemented as aliases to %_H and %_I to avoid
  duplicating code.

src/lib/libcmd/date.c:
- Update the documentation for the AST date command since it is
  also affected by the changes to 'printf %T'.

Fixes ksh93#62
@posguy99
Copy link
Author

posguy99 commented Jul 8, 2020

Is the %()T format code actually documented in the new Kornshell book somewhere and I just can't find it? I even went to archive.org's PDF copy of the book and searched in it, but didn't find anything.

Something only tangentially related... is there an electronic copy of that book somewhere (other than the archive.org loaner copy)? I've bought the original one (ksh88), the new one (ksh93), and the O'Reilly, but having an easily searchable I could keep on a tablet....

@McDutchie
Copy link

I noticed before that %T is not documented in the book; it must be a newer addition. No downloadable digital copy is available, AFAIK.

@dannyweldon
Copy link

dannyweldon commented Jul 8, 2020 via email

McDutchie pushed a commit that referenced this issue Jul 9, 2020
This commit changes the behavior of four date formats accepted
by 'printf %()T' because the old behavior is not compatible with
modern implementations of date(1):
- %k and %l now return a blank-padded hour, the former based on a
  24-hour clock and the latter a 12-hour clock (these are common
  extensions present on Linux and *BSD).
- %f now returns a date with the format '%Y.%m.%d-%H:%M:%S'
  (BusyBox extension).
- %q now returns the quarter of the current year (GNU extension).

src/cmd/ksh93/data/builtins.c:
- Copy the date format documentation from date in libcmd to
  the printf man page (for documenting 'printf %T').

src/cmd/ksh93/tests/builtins.sh:
- Add four regression tests for the changed date formats.

src/cmd/ksh93/sh.1:
- Remove inaccurate information about the date formats accepted by
  printf %T'. The KornShell uses a custom version of strftime(3)
  that isn't guaranteed to accepts the same formats as the native
  strftime function.

src/lib/libast/tm/tmxfmt.c:
- Change the behavior of %f, %k, %l and %q to the common behavior.
  %k and %l are implemented as aliases to %_H and %_I to avoid
  duplicating code.

src/lib/libcmd/date.c:
- Update the documentation for the AST date command since it is
  also affected by the changes to 'printf %T'.

Fixes #62
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants