On 12/28/16, John Vogel <[email protected]> wrote:
> Thanks for the help, Michael. I made a couple adjustments to
> account for tm_year needing to be 'years since 1900'. Tests
> fine here.
Cool. Thanks for the new patch. I just have a few style nits.
> --
> date.1 | 46 ++++++++++++++++++++++++++++++++++++++++++++--
> date.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 96 insertions(+), 8 deletions(-)
>
> diff --git a/date.1 b/date.1
> index 29081a5..0171936 100644
> --- a/date.1
> +++ b/date.1
> @@ -3,12 +3,15 @@
> .Os sbase
> .Sh NAME
> .Nm date
> -.Nd print date and time
> +.Nd print or set date and time
> .Sh SYNOPSIS
> .Nm
> .Op Fl d Ar time
> .Op Fl u
> .Op Cm + Ns Ar format
> +.Sm off
> +.Op Ar mmddHHMM Oo Oo Ar CC Oc Ar yy Oc
> +.Sm on
> .Sh DESCRIPTION
> .Nm
> prints the date and time according to
> @@ -16,7 +19,8 @@ prints the date and time according to
> or
> .Ar format
> using
> -.Xr strftime 3 .
> +.Xr strftime 3
> +or sets the date.
> .Sh OPTIONS
> .Bl -tag -width Ds
> .It Fl d Ar time
> @@ -27,6 +31,44 @@ Unix epoch 1970-01-01T00:00:00Z.
> .It Fl u
> Print UTC time instead of local time.
> .El
> +.Pp
> +An operand with a leading plus
> +.Pq Cm +
> +sign signals a user-defined format string using
> +.Xr strftime 3
> +conversion specifications.
> +.Pp
> +An operand without a leading plus sign is
> +interpreted a value for setting the systems current date and time.
> +The canonical representation for setting the date and time is:
> +.Pp
> +.Bl -tag -width Ds -compact -offset indent
> +.It Ar mm
> +The month of the year, from 01 to 12.
> +.It Ar dd
> +The day of the month, from 01 to 31.
> +.It Ar HH
> +The hour of the day, from 00 to 23.
> +.It Ar MM
> +The minute of the hour, from 00 to 59.
> +.It Ar CC
> +The first two digits of the year (the century).
> +.It Ar yy
> +The second two digits of the year.
> +If
> +.Ar yy
> +is specified, but
> +.Ar CC
> +is not, a value for
> +.Ar yy
> +between 69 and 99 results in a
> +.Ar CC
> +value of 19. Otherwise, a
> +.Ar CC
> +value of 20 is used.
> +.El
> +.Pp
> +The century and year are optional. The default is the current year.
> .Sh STANDARDS
> The
> .Nm
> diff --git a/date.c b/date.c
> index 1671e1f..4a18bd6 100644
> --- a/date.c
> +++ b/date.c
> @@ -1,6 +1,8 @@
> /* See LICENSE file for copyright and license details. */
> #include <stdio.h>
> #include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> #include <time.h>
>
> #include "util.h"
> @@ -11,6 +13,11 @@ usage(void)
> eprintf("usage: %s [-u] [-d time] [+format]\n", argv0);
Let's update this usage message.
> }
>
> +static int date_field(const char *s, size_t i)
The suckless style guide says that `static int` should be on its own
line (sorry, my suggestion had the error too).
> +{
> + return (s[i] - '0') * 10 + (s[i+1] - '0');
> +}
> +
> int
> main(int argc, char *argv[])
> {
> @@ -19,8 +26,6 @@ main(int argc, char *argv[])
> time_t t;
> char buf[BUFSIZ], *fmt = "%c", *tz = "local";
>
> - t = time(NULL);
> -
> ARGBEGIN {
> case 'd':
> t = estrtonum(EARGF(usage()), 0, LLONG_MAX);
> @@ -33,14 +38,55 @@ main(int argc, char *argv[])
> usage();
> } ARGEND
>
> + t = time(NULL);
This is pre-existing, but this call is not checked for errors.
> +
> + if (!(now = tztime(&t)))
> + eprintf("%stime failed\n", tz);
> +
> if (argc) {
> - if (argc != 1 || argv[0][0] != '+')
> + if (argc != 1)
> usage();
> - else
This should use braces (since the else branch needs them).
> + else if (argv[0][0] == '+') {
> fmt = &argv[0][1];
> + }
> + else {
These should be on the same line (`} else {`).
> + struct tm date;
> + struct timespec ts;
> +
> + date.tm_mon = date_field(argv[0], 0) - 1;
> + date.tm_mday = date_field(argv[0], 2);
> + date.tm_hour = date_field(argv[0], 4);
> + date.tm_min = date_field(argv[0], 6);
> +
> + switch (strlen(argv[0])) {
> + case 8:
> + date.tm_year = now->tm_year;
> + break;
> + case 10:
> + date.tm_year = date_field(argv[0], 8);
> + if (date.tm_year < 69)
> + date.tm_year += now->tm_year < 69 ? 0 :
> 100;
I think this needs to be just `date.tm_year += 100` (regardless of the
current year). POSIX says "If century is not specified, then values in
the range [69,99] shall refer to years 1969 to 1999 inclusive, and
values in the range [00,68] shall refer to years 2000 to 2068
inclusive."
> + break;
> + case 12:
> + date.tm_year = ((date_field(argv[0], 8) - 19) *
> 100) +
> date_field(argv[0], 10);
> + break;
> + default:
> + eprintf("invalid date format: %s\n", argv[0]);
> + }
> +
> + t = mktime(&date);
> + if (t == (time_t)-1)
> + eprintf("mktime failed: bad calender date/time:
> %s\n", argv[0]);
> +
> + ts.tv_sec = t;
> + ts.tv_nsec = 0;
> +
> + if (clock_settime(CLOCK_REALTIME, &ts) == -1)
> + eprintf("clock_settime failed: %s\n",
> strerror(errno));
> +
> + return fshut(stdout, "<stdout>");
This is unnecessary since it never prints anything to stdout. Just `return 0`.
> + }
> }
> - if (!(now = tztime(&t)))
> - eprintf("%stime failed\n", tz);
>
> strftime(buf, sizeof(buf), fmt, now);
> puts(buf);
Apart from these nits, this looks good to me.
