Skip to content

Conversation

alejandro-colomar
Copy link
Collaborator

@alejandro-colomar alejandro-colomar commented Mar 13, 2025

This PR removes features of get_date() that we don't really need or want.

It is in preparation of a future replacement of this code by a C implementation.

Cc: @zeha , @timparenti


Revisions:

v1b
Range-diff:
alx@devuan:~/src/shadow/shadow/master$ git range-diff shadow/master gh/gd gd
 1:  0b830153 <  -:  -------- lib/getdate.y: Do not parse am/pm time syntax; just 24h
 2:  ce35738c !  1:  13de308c lib/getdate.y: Do not parse times; just dates
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>
     
      ## Commit message ##
    -    lib/getdate.y: Do not parse times; just dates
    +    lib/getdate.y: Don't parse times; just dates
     
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/getdate.y ##
    +@@ lib/getdate.y: typedef struct _TABLE {
    + } TABLE;
    + 
    + 
    +-/*
    +-**  Meridian:  am, pm, or 24-hour style.
    +-*/
    +-typedef enum _MERIDIAN {
    +-    MERam, MERpm, MER24
    +-} MERIDIAN;
    +-
    +-
    + /*
    + **  Global variables.  We could get rid of most of these by using a good
    + **  union as the yacc stack.  (This routine was originally written before
     @@ lib/getdate.y: static int       yyDayNumber;
      static int        yyHaveDate;
      static int        yyHaveDay;
    @@ lib/getdate.y: static int        yyDayNumber;
      static int        yyMonth;
     -static int        yySeconds;
      static int        yyYear;
    +-static MERIDIAN   yyMeridian;
      static int        yyRelDay;
     -static int        yyRelHour;
     -static int        yyRelMinutes;
    @@ lib/getdate.y: static int        yyDayNumber;
      static int        yyRelYear;
      
      %}
    -@@ lib/getdate.y: static int       yyRelYear;
    + 
    + %union {
          int                   Number;
    +-    enum _MERIDIAN        Meridian;
      }
      
     -%token    tAGO tDAY tDAY_UNIT tHOUR_UNIT tID
    --%token    tMINUTE_UNIT tMONTH tMONTH_UNIT
    +-%token    tMERIDIAN tMINUTE_UNIT tMONTH tMONTH_UNIT
     -%token    tSEC_UNIT tSNUMBER tUNUMBER tYEAR_UNIT
     +%token    tAGO tDAY tDAY_UNIT tID
     +%token    tMONTH tMONTH_UNIT
    @@ lib/getdate.y: static int        yyRelYear;
     +%type     <Number>        tDAY tDAY_UNIT
      %type     <Number>        tMONTH tMONTH_UNIT
     -%type     <Number>        tSEC_UNIT tSNUMBER tUNUMBER tYEAR_UNIT
    +-%type     <Meridian>      tMERIDIAN o_merid
     +%type     <Number>        tSNUMBER tUNUMBER tYEAR_UNIT
      
      %%
    @@ lib/getdate.y: item      : time {
        | number
        ;
      
    --time      : tUNUMBER ':' tUNUMBER {
    +-time      : tUNUMBER tMERIDIAN {
    +-      yyHour = $1;
    +-      yyMinutes = 0;
    +-      yySeconds = 0;
    +-      yyMeridian = $2;
    +-  }
    +-  | tUNUMBER ':' tUNUMBER o_merid {
     -      yyHour = $1;
     -      yyMinutes = $3;
    +-      yySeconds = 0;
    +-      yyMeridian = $4;
    +-  }
    +-  | tUNUMBER ':' tUNUMBER {
    +-      yyHour = $1;
    +-      yyMinutes = $3;
    +-      yyMeridian = MER24;
    +-  }
    +-  | tUNUMBER ':' tUNUMBER ':' tUNUMBER o_merid {
    +-      yyHour = $1;
    +-      yyMinutes = $3;
    +-      yySeconds = $5;
    +-      yyMeridian = $6;
     -  }
     -  | tUNUMBER ':' tUNUMBER ':' tUNUMBER {
     -      yyHour = $1;
     -      yyMinutes = $3;
     -      yySeconds = $5;
    +-      yyMeridian = MER24;
     -  }
     -  ;
     -
    @@ lib/getdate.y: relunit   : tUNUMBER tYEAR_UNIT {
     -                  yyMinutes = $1 % 100;
     -                }
     -              yySeconds = 0;
    +-              yyMeridian = MER24;
     -            }
     -        }
    +-    }
    +-  ;
    +-
    +-o_merid   : /* NULL */
    +-    {
    +-      $$ = MER24;
    +-    }
    +-  | tMERIDIAN
    +-    {
    +-      $$ = $1;
          }
        ;
      
    @@ lib/getdate.y: static TABLE const UnitsTable[] = {
          { "next",             tUNUMBER,       2 },
          { "first",            tUNUMBER,       1 },
      /*  { "second",           tUNUMBER,       2 }, */
    +@@ lib/getdate.y: static int yyerror (MAYBE_UNUSED const char *s)
    +   return 0;
    + }
    + 
    +-static int ToHour (int Hours, MERIDIAN Meridian)
    +-{
    +-  switch (Meridian)
    +-    {
    +-    case MER24:
    +-      if (Hours < 0 || Hours > 23)
    +-  return -1;
    +-      return Hours;
    +-    case MERam:
    +-      if (Hours < 1 || Hours > 12)
    +-  return -1;
    +-      if (Hours == 12)
    +-  Hours = 0;
    +-      return Hours;
    +-    case MERpm:
    +-      if (Hours < 1 || Hours > 12)
    +-  return -1;
    +-      if (Hours == 12)
    +-  Hours = 0;
    +-      return Hours + 12;
    +-    default:
    +-      abort ();
    +-    }
    +-  /* NOTREACHED */
    +-}
    +-
    + static int ToYear (int Year)
    + {
    +   if (Year < 0)
    +@@ lib/getdate.y: static int LookupWord (char *buff)
    +     if (isupper (*p))
    +       *p = tolower (*p);
    + 
    +-  if (streq(buff, "am") || streq(buff, "a.m."))
    +-    {
    +-      yylval.Meridian = MERam;
    +-      return tMERIDIAN;
    +-    }
    +-  if (streq(buff, "pm") || streq(buff, "p.m."))
    +-    {
    +-      yylval.Meridian = MERpm;
    +-      return tMERIDIAN;
    +-    }
    +-
    +   /* See if we have an abbreviation for a month. */
    +   if (strlen (buff) == 3)
    +     abbrev = true;
     @@ lib/getdate.y: time_t get_date (const char *p, const time_t *now)
        yyYear = tmp->tm_year + TM_YEAR_ORIGIN;
        yyMonth = tmp->tm_mon + 1;
    @@ lib/getdate.y: time_t get_date (const char *p, const time_t *now)
     -  yyHour = tmp->tm_hour;
     -  yyMinutes = tmp->tm_min;
     -  yySeconds = tmp->tm_sec;
    +-  yyMeridian = MER24;
     -  yyRelSeconds = 0;
     -  yyRelMinutes = 0;
     -  yyRelHour = 0;
    @@ lib/getdate.y: time_t get_date (const char *p, const time_t *now)
     -  if ((yyHaveTime != 0) ||
     -      ( (yyHaveRel != 0) && (yyHaveDate == 0) && (yyHaveDay == 0) ))
     -    {
    --      tm.tm_hour = yyHour;
    +-      tm.tm_hour = ToHour (yyHour, yyMeridian);
     -      if (tm.tm_hour < 0)
     -  return -1;
     -      tm.tm_min = yyMinutes;
 3:  64d5d8de !  2:  4e143c9f lib/getdate.y: Do not parse relative dates, such as 'yesterday'
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>
     
      ## Commit message ##
    -    lib/getdate.y: Do not parse relative dates, such as 'yesterday'
    +    lib/getdate.y: Don't parse relative dates, such as 'yesterday'
     
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
 4:  0851afca =  3:  7812415b lib/getdate.y: Don't parse week days
 5:  8c581d5e =  4:  3908b9f9 lib/getdate.y: Remove unnecessary variable
 6:  4c923f93 !  5:  05ce266b lib/getdate.y: Don't parse dates in local formats; just YYYY-MM-DD
    @@ Metadata
      ## Commit message ##
         lib/getdate.y: Don't parse dates in local formats; just YYYY-MM-DD
     
    -    Signed-off-by: Alejandro Colomar <alx@kernel.org>
    -
      ## lib/getdate.y ##
     @@
      
    @@ lib/getdate.y
      #include "string/strspn/stpspn.h"
      
      
    +@@ lib/getdate.y: typedef struct _TABLE {
    + **  the %union very rarely.
    + */
    + static const char *yyInput;
    +-static int        yyHaveDate;
    + static int        yyDay;
    + static int        yyMonth;
    + static int        yyYear;
     @@ lib/getdate.y: static int       yyYear;
          int                   Number;
      }
    @@ lib/getdate.y: static int        yyYear;
      %type     <Number>        tSNUMBER tUNUMBER
      
      %%
    -@@ lib/getdate.y: item     : date {
    +@@ lib/getdate.y: spec     : /* NULL */
    +   | spec item
    +   ;
    + 
    +-item      : date {
    +-      yyHaveDate++;
    +-  }
    ++item      : date
        | number
        ;
      
    @@ lib/getdate.y: item      : date {
        ;
      
      number    : tUNUMBER
    +           {
    +-              yyHaveDate++;
    +               yyDay= ($1)%100;
    +               yyMonth= ($1/100)%100;
    +               yyYear = $1/10000;
     @@ lib/getdate.y: number   : tUNUMBER
      
      %%
    @@ lib/getdate.y: number    : tUNUMBER
      ^L
      
      
    -@@ lib/getdate.y: static int ToYear (int Year)
    -   return Year;
    +@@ lib/getdate.y: static int yyerror (MAYBE_UNUSED const char *s)
    +   return 0;
      }
      
    +-static int ToYear (int Year)
    +-{
    +-  if (Year < 0)
    +-    Year = -Year;
    +-
    +-  /* XPG4 suggests that years 00-68 map to 2000-2068, and
    +-     years 69-99 map to 1969-1999.  */
    +-  if (Year < 69)
    +-    Year += 2000;
    +-  else if (Year < 100)
    +-    Year += 1900;
    +-
    +-  return Year;
    +-}
    +-
     -static int LookupWord (char *buff)
     -{
     -  register char *p;
    @@ lib/getdate.y: yylex (void)
            if (c != '(')
        return *yyInput++;
            Count = 0;
    +@@ lib/getdate.y: time_t get_date (const char *p, const time_t *now)
    +   struct tm  tm;
    + 
    +   yyInput = p;
    +-  yyHaveDate = 0;
    + 
    +-  if (yyparse ()
    +-      || yyHaveDate > 1)
    ++  if (yyparse())
    +     return -1;
    + 
    +-  tm.tm_year = ToYear (yyYear) - TM_YEAR_ORIGIN;
    ++  tm.tm_year = yyYear - TM_YEAR_ORIGIN;
    +   tm.tm_mon = yyMonth - 1;
    +   tm.tm_mday = yyDay;
    +   tm.tm_hour = tm.tm_min = tm.tm_sec = 0;
 7:  e385099d <  -:  -------- lib/getdate.y: Remove unnecessary variable
 8:  8928c17e <  -:  -------- lib/getdate.y: Don't parse 2-digit years as if 19xx/20xx years
 9:  a842f13b !  6:  ad501f36 lib/getdate.y: Don't parse a raw number; just a yyyy-mm-dd date
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>
     
      ## Commit message ##
    -    lib/getdate.y: Don't parse a raw number; just a yyyy-mm-dd date
    +    lib/getdate.y: Don't parse a raw number; just a calendar date
     
         Our caller, strtoday(), already handles a raw number.
     
Interdiff:
$ git diff gh/gd..gd
$
v2
$ git range-diff shadow/master gh/gd gd
 1:  13de308c =  1:  13de308c lib/getdate.y: Don't parse times; just dates
 2:  4e143c9f =  2:  4e143c9f lib/getdate.y: Don't parse relative dates, such as 'yesterday'
 3:  7812415b =  3:  7812415b lib/getdate.y: Don't parse week days
 4:  3908b9f9 =  4:  3908b9f9 lib/getdate.y: Remove unnecessary variable
 5:  05ce266b =  5:  05ce266b lib/getdate.y: Don't parse dates in local formats; just YYYY-MM-DD
 6:  ad501f36 =  6:  ad501f36 lib/getdate.y: Don't parse a raw number; just a calendar date
 -:  -------- >  7:  4811959b man/: Consistently express dates in standard format
 -:  -------- >  8:  ea6be66b man/: Localized dates are not accepted anymore
 -:  -------- >  9:  07e7004b lib/strtoday.c: strtoday(): Remove obsolete comment
v3
  • Add comment documenting the name of strtoday().
$ git range-diff shadow/master gh/gd gd
 1:  13de308c =  1:  13de308c lib/getdate.y: Don't parse times; just dates
 2:  4e143c9f =  2:  4e143c9f lib/getdate.y: Don't parse relative dates, such as 'yesterday'
 3:  7812415b =  3:  7812415b lib/getdate.y: Don't parse week days
 4:  3908b9f9 =  4:  3908b9f9 lib/getdate.y: Remove unnecessary variable
 5:  05ce266b =  5:  05ce266b lib/getdate.y: Don't parse dates in local formats; just YYYY-MM-DD
 6:  ad501f36 =  6:  ad501f36 lib/getdate.y: Don't parse a raw number; just a calendar date
 7:  4811959b =  7:  4811959b man/: Consistently express dates in standard format
 8:  ea6be66b =  8:  ea6be66b man/: Localized dates are not accepted anymore
 9:  07e7004b !  9:  c225d27b lib/strtoday.c: strtoday(): Remove obsolete comment
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>
     
      ## Commit message ##
    -    lib/strtoday.c: strtoday(): Remove obsolete comment
    +    lib/strtoday.c: strtoday(): Replace obsolete comment
     
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
    @@ lib/strtoday.c
     - *        24-sep-72
     - *        24sep72
     - */
    ++// string parse-to day-since-Epoch
      long strtoday (const char *str)
      {
        time_t t;
v4
$ git range-diff shadow/master gh/gd gd
 1:  13de308c =  1:  13de308c lib/getdate.y: Don't parse times; just dates
 2:  4e143c9f =  2:  4e143c9f lib/getdate.y: Don't parse relative dates, such as 'yesterday'
 3:  7812415b =  3:  7812415b lib/getdate.y: Don't parse week days
 4:  3908b9f9 =  4:  3908b9f9 lib/getdate.y: Remove unnecessary variable
 5:  05ce266b =  5:  05ce266b lib/getdate.y: Don't parse dates in local formats; just YYYY-MM-DD
 6:  ad501f36 =  6:  ad501f36 lib/getdate.y: Don't parse a raw number; just a calendar date
 7:  4811959b =  7:  4811959b man/: Consistently express dates in standard format
 8:  ea6be66b =  8:  ea6be66b man/: Localized dates are not accepted anymore
 9:  c225d27b =  9:  c225d27b lib/strtoday.c: strtoday(): Replace obsolete comment
 -:  -------- > 10:  87f31e27 man/chage.1.xml: Update -d -E options
 -:  -------- > 11:  1264b960 man/chage.1.xml: wfix
v4b
$ git range-diff db/master gh/gd gd
 1:  13de308c =  1:  13de308c lib/getdate.y: Don't parse times; just dates
 2:  4e143c9f =  2:  4e143c9f lib/getdate.y: Don't parse relative dates, such as 'yesterday'
 3:  7812415b =  3:  7812415b lib/getdate.y: Don't parse week days
 4:  3908b9f9 =  4:  3908b9f9 lib/getdate.y: Remove unnecessary variable
 5:  05ce266b =  5:  05ce266b lib/getdate.y: Don't parse dates in local formats; just YYYY-MM-DD
 6:  ad501f36 =  6:  ad501f36 lib/getdate.y: Don't parse a raw number; just a calendar date
 7:  4811959b =  7:  4811959b man/: Consistently express dates in standard format
 8:  ea6be66b =  8:  ea6be66b man/: Localized dates are not accepted anymore
 9:  c225d27b =  9:  c225d27b lib/strtoday.c: strtoday(): Replace obsolete comment
10:  87f31e27 ! 10:  1e8395be man/chage.1.xml: Update -d -E options
    @@ Commit message
         man/chage.1.xml: Update -d -E options
     
         Signed-off-by: Dominika Borges <dvagnero@redhat.com>
    +    Cc: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## man/chage.1.xml ##
    @@ man/chage.1.xml
     -      Passing the number <emphasis remap='I'>-1</emphasis> as the
     -      <replaceable>EXPIRE_DATE</replaceable> will remove an account
     -      expiration date.
    -+      Passing the value <emphasis remap='I'>-1</emphasis>
    ++      Passing the value <emphasis>-1</emphasis> or an empty string
     +      as the <replaceable>EXPIRE_DATE</replaceable>
     +      removes the account expiration date.
          </para>
11:  1264b960 = 11:  19f564e1 man/chage.1.xml: wfix
v4c
  • Rebase
$ git range-diff db/master..gh/gd shadow/master..gd
 1:  13de308c =  1:  25f1e0eb lib/getdate.y: Don't parse times; just dates
 2:  4e143c9f =  2:  1f7b4913 lib/getdate.y: Don't parse relative dates, such as 'yesterday'
 3:  7812415b =  3:  53b6cfa6 lib/getdate.y: Don't parse week days
 4:  3908b9f9 =  4:  43ea4a50 lib/getdate.y: Remove unnecessary variable
 5:  05ce266b =  5:  d6d5af7a lib/getdate.y: Don't parse dates in local formats; just YYYY-MM-DD
 6:  ad501f36 =  6:  d4edc28e lib/getdate.y: Don't parse a raw number; just a calendar date
 7:  4811959b =  7:  2bf69d03 man/: Consistently express dates in standard format
 8:  ea6be66b =  8:  ead78c5a man/: Localized dates are not accepted anymore
 9:  c225d27b =  9:  fa04e51d lib/strtoday.c: strtoday(): Replace obsolete comment
10:  1e8395be = 10:  90f74d69 man/chage.1.xml: Update -d -E options
11:  19f564e1 = 11:  640a1eb2 man/chage.1.xml: wfix
v5
$ git range-diff gh/master gh/gd gd 
 1:  25f1e0eb =  1:  25f1e0eb lib/getdate.y: Don't parse times; just dates
 2:  1f7b4913 =  2:  1f7b4913 lib/getdate.y: Don't parse relative dates, such as 'yesterday'
 3:  53b6cfa6 =  3:  53b6cfa6 lib/getdate.y: Don't parse week days
 4:  43ea4a50 =  4:  43ea4a50 lib/getdate.y: Remove unnecessary variable
 5:  d6d5af7a =  5:  d6d5af7a lib/getdate.y: Don't parse dates in local formats; just YYYY-MM-DD
 6:  d4edc28e =  6:  d4edc28e lib/getdate.y: Don't parse a raw number; just a calendar date
 7:  2bf69d03 =  7:  2bf69d03 man/: Consistently express dates in standard format
 8:  ead78c5a =  8:  ead78c5a man/: Localized dates are not accepted anymore
 9:  fa04e51d =  9:  fa04e51d lib/strtoday.c: strtoday(): Replace obsolete comment
10:  90f74d69 <  -:  -------- man/chage.1.xml: Update -d -E options
11:  640a1eb2 <  -:  -------- man/chage.1.xml: wfix

@alejandro-colomar alejandro-colomar marked this pull request as ready for review March 13, 2025 13:03
@timparenti
Copy link

This is much clearer to my eyes.

To the extent that a goal here is to separate concerns while keeping a clean minimal commit history for review, I do see two fairly straightforward opportunities for squashes:

  1. 0b83015 (removing am/pm parsing) can be squashed into ce35738 (removing all time-of-day parsing)
  2. 8928c17 (disallowing parsing of two-digit years) can be rolled into 4c923f9 (which already insists dates be in full YYYY-MM-DD format)

I also note that the many global variables referred to by this comment are gradually whittled away, rendering it somewhat outdated by the end of this PR.

As I mentioned on #1217 (comment), this PR should ensure relevant documentation is updated in order to be complete.

@alejandro-colomar
Copy link
Collaborator Author

This is much clearer to my eyes.

To the extent that a goal here is to separate concerns while keeping a clean minimal commit history for review, I do see two fairly straightforward opportunities for squashes:

1. [0b83015](https://github.com/shadow-maint/shadow/commit/0b830153156b602cd648bae061faee9d5aa47f73) (removing am/pm parsing) can be squashed into [ce35738](https://github.com/shadow-maint/shadow/commit/ce35738ce0506897996c037a2ee184993f100770) (removing all time-of-day parsing)

2. [8928c17](https://github.com/shadow-maint/shadow/commit/8928c17ed68b7f4b1b0ded3c7f9e74933462c153) (disallowing parsing of two-digit years) can be rolled into [4c923f9](https://github.com/shadow-maint/shadow/commit/4c923f93a754d733d2f74f07fadbcaabaf2407b4) (which already insists dates be in full YYYY-MM-DD format)

Sounds reasonable. I've squashed them.

I also note that the many global variables referred to by this comment are gradually whittled away, rendering it somewhat outdated by the end of this PR.

I never understood that comment. I'll keep it, just because I don't understand it, so I'll remove it in the commit that gets rid of the weird yacc stuff, in the other PR.

As I mentioned on #1217 (comment), this PR should ensure relevant documentation is updated in order to be complete.

Thanks! I've cherry-picked the documentation changes.

Copy link
Collaborator

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added two minor comments.

In addition, since we are already refactoring the code I think we should improve the existing documentation. I've asked our internal documentation team whether they can help us with this task.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Our caller, strtoday(), already handles a raw number.

Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@ikerexxe
Copy link
Collaborator

@alejandro-colomar do you mind removing Dominika's commits? I think the documentation is not ready yet and we still need to work on improving it before merging.

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Apr 24, 2025

@alejandro-colomar do you mind removing Dominika's commits? I think the documentation is not ready yet and we still need to work on improving it before merging.

Those two commits are good IMO. I think they're useful per se, and maybe we can iterate over them later.

I didn't like the rest of commits as they are, but these two seem a net improvement.

But if you prefer to have more review about them, I can drop them for now. Just let me know what you prefer.

@ikerexxe
Copy link
Collaborator

Yeah, I would prefer to work on them in their own PR and merge all the documentation together.

@alejandro-colomar
Copy link
Collaborator Author

Yeah, I would prefer to work on them in their own PR and merge all the documentation together.

Okay, I've dropped them.

@alejandro-colomar
Copy link
Collaborator Author

container-build fedora is failing, but I don't understand why. @ikerexxe , do you know? :|

@ikerexxe
Copy link
Collaborator

Newly released fedora 42 had a missing dependency. Fix is available at #1252

Copy link
Collaborator

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to come back and approve after I fixed the CI failure. LGTM! Thanks for the patches.

@ikerexxe ikerexxe merged commit 284beee into shadow-maint:master Apr 30, 2025
14 of 16 checks passed
@alejandro-colomar alejandro-colomar deleted the gd branch May 3, 2025 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants