-
Notifications
You must be signed in to change notification settings - Fork 229
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
DAY_TO_STR() localization #1059
Conversation
Queued after #1058. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, I'll take another look when you resolve the conflicts.
This comment was marked as outdated.
This comment was marked as outdated.
bf3481d
to
62b553e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this changed a bit since my last review.
I am not completely convinced about the changes, but if we make them we should add in the release notes that from now on all the dates printed follow the ISO standard.
src/chage.c
Outdated
@@ -243,7 +243,7 @@ print_day_as_date(long day) | |||
} | |||
|
|||
if (localtime_r(&date, &tm) == NULL) { | |||
(void) printf ("time_t: %lu\n", (unsigned long)date); | |||
puts(_("future")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change printf()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit e78f9782183e88e3973d4634d22c538a09f44b96
Author: Alejandro Colomar <[email protected]>
Date: Wed Jul 31 14:53:12 2024 +0200
src/chage.c: print_day_as_date(): Simplify error handling
If localtime_r(3) fails, just print future, as we do in day_to_str().
It should only fail for unrealistic dates, if at all.
This is a step towards reusing the DAY_TO_STR() function.
Signed-off-by: Alejandro Colomar <[email protected]>
diff --git a/src/chage.c b/src/chage.c
index 0f963090..8b2d7b35 100644
--- a/src/chage.c
+++ b/src/chage.c
@@ -243,7 +243,7 @@ print_day_as_date(long day)
}
if (localtime_r(&date, &tm) == NULL) {
- (void) printf ("time_t: %lu\n", (unsigned long)date);
+ puts(_("future"));
return;
}
This makes it behave like day_to_str(), so that we can just call it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ diff -u \
<(git show shadow/master:src/chage.c \
| grepc -tfd print_day_as_date) \
<(git show shadow/master:lib/time/day_to_str.h \
| grepc -tfd day_to_str);
--- /dev/fd/63 2024-08-02 14:16:39.866679146 +0200
+++ /dev/fd/62 2024-08-02 14:16:39.866679146 +0200
@@ -1,24 +1,24 @@
-(standard input):static void
-print_day_as_date(long day)
+(standard input):inline void
+day_to_str(size_t size, char buf[size], long day)
{
- char buf[80];
time_t date;
struct tm tm;
if (day < 0) {
- puts(_("never"));
+ strtcpy(buf, "never", size);
return;
}
+
if (__builtin_mul_overflow(day, DAY, &date)) {
- puts(_("future"));
+ strtcpy(buf, "future", size);
return;
}
if (localtime_r(&date, &tm) == NULL) {
- (void) printf ("time_t: %lu\n", (unsigned long)date);
+ strtcpy(buf, "future", size);
return;
}
- STRFTIME(buf, iflg ? "%Y-%m-%d" : "%b %d, %Y", &tm);
- (void) puts (buf);
+ if (strftime(buf, size, "%Y-%m-%d", &tm) == 0)
+ strtcpy(buf, "future", size);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, after that commit, the difference is reduced to just:
$ diff -u <(git show e78f9782183e:src/chage.c \
| grepc -tfd print_day_as_date) <(git show e78f9782183e:lib/time/day_to_str.h \
| grepc -tfd day_to_str);
--- /dev/fd/63 2024-08-02 14:16:16.050428094 +0200
+++ /dev/fd/62 2024-08-02 14:16:16.050428094 +0200
@@ -1,24 +1,24 @@
-(standard input):static void
-print_day_as_date(long day)
+(standard input):inline void
+day_to_str(size_t size, char buf[size], long day)
{
- char buf[80];
time_t date;
struct tm tm;
if (day < 0) {
- puts(_("never"));
+ strtcpy(buf, "never", size);
return;
}
+
if (__builtin_mul_overflow(day, DAY, &date)) {
- puts(_("future"));
+ strtcpy(buf, "future", size);
return;
}
if (localtime_r(&date, &tm) == NULL) {
- puts(_("future"));
+ strtcpy(buf, "future", size);
return;
}
- STRFTIME(buf, iflg ? "%F" : "%x", &tm);
- (void) puts (buf);
+ if (strftime(buf, size, "%F", &tm) == 0)
+ strtcpy(buf, "future", size);
}
So the only difference after that change is the support for localized dates (which is also solved in another commit of this patch set).
Nah, I reverted that change. That was only present in v1. :) Since v2 of this patch set, I changed custom(American)-formatted dates and date-times by locale-formatted dates (%x) and date-times (%c). And kept printing ISO dates only where we already printed them as ISO. |
So this unfortunately begs the usual question - how many dark server rooms are running 20 year old scripts depending on the current date format that are going to break from a perfectly reasonable change? I think none. I think this is fine. |
I'm worried about that second one (928172c). |
That's the one I'm less worried about. If they wanted to parse the output, they had the option to pass I was more worried about the first one, since it was unconditionally non-standard. (Or you might be worried about a script that would be written before chage(1) ever got a Edit: |
@alejandro-colomar Ok, for all of the commits (including the already recently merged ones), let's please add an /etc/login.defs switch to dictate whether to use the classic format, or the standards-compliant formats. I'm 100% convinced there's some site out there parsing the date on '-' (%F) or on the %b %d %Y, and 928172c will mess them up. Generally I'd prefer having fewer ways of dictating the format as it's going to make it harder to spot an accidental format printing bug, but I think it's worth it here. |
Hmm, I don't like adding configurability either. How about the following instead? Deprecate running chage(1) to print a date without -i. Print a deprecation warning to stderr when a date is printed in the non-standard format. Keep the nonstandard format untouched, for compatibility reasons. Then we can come back in some years (4 or 5?) and unconditionally enable %F. |
Hi Gus,
On Mon, Aug 26, 2024 at 03:18:44AM GMT, Gus Kenion wrote:
Just for clarification, has a consensus been reached on how to move
forward?
Not yet.
Since change #1058 was reverted,
No, it wasn't reverted.
issue #1057 is still present.
And no, issue 1057 isn't present.
We have a different one now: we print a different date than the one that
is stored, since we print a local date, but store a UTC date. This
might be problematic too.
The solution would be to append a Z to the ????-??-?? date and revert
#1058 to print a UTC date again. However, that might break scripts, so
I would just leave it imprecise. One can always use TZ=UTC to print the actual date stored.
Have a lovely day!
Alex
… For now, any user who updates to 4.14.6 from an earlier version will
see the change in date input/output behavior described in that issue.
--
Reply to this email directly or view it on GitHub:
#1059 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
There were several custom date/time format strings. Replace them by %c so that the user gets the right date/time string, according to their locale. %c is specified by ISO C89. Signed-off-by: Alejandro Colomar <[email protected]>
…ates Signed-off-by: Alejandro Colomar <[email protected]>
62b553e
to
53554dc
Compare
@alejandro-colomar sorry for my misunderstanding. I've gone over the changes and comments again and I see the root cause of #1057 is addressed - thanks again to you and @hallyn. |
No problem. Thanks! :) |
Cc: @eggert
Cc: @timparenti
Cc: @kenion
Revisions:
v2
v3
v4
v5