[PATCH 0/2] Fix strptime '%s'

ruby-talk
1

Hi,

There's multiple issues with the way '%s' is handled, starting with the less
controversial (%s):

  Time.strptime('0', '%s')
  => 1969-12-31 18:00:00 -0600
    
If '%s' is meant to imply UTC, then clearly there's a bug here.

After this patch series:
    
  Time.strptime('0', '%s')
  => 1970-01-01 00:00:00 +0000

However, the timezone can be overriden, and in other languages like C and Perl,
it's perfectly fine to use '%s %z'. In fact, Ruby already works fine with
'%s %z', for strftime:

  Time.at(0).localtime('+01:00').strftime('%s %z')
  => "0 +0100"

But it doesn't with strptime():

  Time.strptime('0 +0100', '%s %z').strftime('%s %z')
  => "0 -0600"

After this series:

  Time.strptime('0 +0100', '%s %z').strftime('%s %z')
  => "0 +0100"

The same happens with DateTime.

Rubinious has been fixed already:


Since the RubySpec standard library tests are moving towards the spec tests in
rubysl, Ruby is now failing these:

Time#strptime parses number of seconds since Unix Epoch as UTF FAILED
Expected -21600
to equal 0

Time#strptime parses number of seconds since Unix Epoch with timezone FAILED
Expected "1969-12-31 18:00:00 -0600"
to equal "1970-01-01 01:00:00 +0100"

DateTime#strptime parses seconds and timezone correctly FAILED
Expected "1970-01-01T00:00:00+00:00"
to equal "1970-01-01T01:00:00+01:00"

So why not do the reasonable thing and apply these patches? Nobody woudld be
affected negatively, because not only will '%s' keep working as before, but it
would work better.

Charlie Somerville (1):
  datetime: fix strptime '%s %z'

Felipe Contreras (1):
  time: fix strptime '%s'

ext/date/date_core.c | 8 ++++++--
lib/time.rb | 3 ++-
test/date/test_date_strptime.rb | 6 ++++++
test/test_time.rb | 2 ++
4 files changed, 16 insertions(+), 3 deletions(-)

···

--
1.8.4-fc

2

'%s' is meant to imply UTC, however:

  Time.strptime('0', '%s')
  => 1969-12-31 18:00:00 -0600

After this patch:

  Time.strptime('0', '%s')
  => 1970-01-01 00:00:00 +0000

In addition, '%s %z' is parsed correctly:

  Time.strptime('0 +0100', '%s %z').strftime('%s %z')
  => "0 -0600"

Now:

  Time.strptime('0 +0100', '%s %z').strftime('%s %z')
  => "0 +0100"

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

···

---
lib/time.rb | 3 ++-
test/test_time.rb | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/time.rb b/lib/time.rb
index 91b5b54..4474f27 100644
--- a/lib/time.rb
+++ b/lib/time.rb
@@ -393,7 +393,8 @@ class Time
       d = Date._strptime(date, format)
       raise ArgumentError, "invalid strptime format - `#{format}'" unless d
       if seconds = d[:seconds]
- Time.at(seconds)
+ offset = d[:offset] || 0
+ Time.at(seconds).localtime(offset)
       else
         year = d[:year]
         year = yield(year) if year && block_given?
diff --git a/test/test_time.rb b/test/test_time.rb
index 0a4e8a7..1c95370 100644
--- a/test/test_time.rb
+++ b/test/test_time.rb
@@ -400,6 +400,8 @@ class TestTimeExtension < Test::Unit::TestCase # :nodoc:
   def test_strptime
     assert_equal(Time.utc(2005, 8, 28, 06, 54, 20), Time.strptime("28/Aug/2005:06:54:20 +0000", "%d/%b/%Y:%T %z"))
     assert_equal(Time.at(1).localtime, Time.strptime("1", "%s"))
+ assert_equal(0, Time.strptime('0', '%s').utc_offset)
+ assert_equal(3600, Time.strptime('0 +0100', '%s %z').utc_offset)
   end

   def test_nsec
--
1.8.4-fc

3

Before:

  DateTime.strptime('0 +0100', '%s %z').strftime('%s %z')
  => "0 +0000"

After:

  DateTime.strptime('0 +0100', '%s %z').strftime('%s %z')
  => "0 +0100"

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

···

From: Charlie Somerville <charliesome@ruby-lang.org>
---
ext/date/date_core.c | 8 ++++++--
test/date/test_date_strptime.rb | 6 ++++++
2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/ext/date/date_core.c b/ext/date/date_core.c
index 5455630..5c84017 100644
--- a/ext/date/date_core.c
+++ b/ext/date/date_core.c
@@ -3686,12 +3686,17 @@ date_s_today(int argc, VALUE *argv, VALUE klass)
static VALUE
rt_rewrite_frags(VALUE hash)
{
- VALUE seconds;
+ VALUE seconds, offset;

     seconds = ref_hash("seconds");
     if (!NIL_P(seconds)) {
   VALUE d, h, min, s, fr;

+ offset = ref_hash("offset");
+ if(!NIL_P(offset)) {
+ seconds = f_add(seconds, offset);
+ }
+
   d = f_idiv(seconds, INT2FIX(DAY_IN_SECONDS));
   fr = f_mod(seconds, INT2FIX(DAY_IN_SECONDS));

@@ -3710,7 +3715,6 @@ rt_rewrite_frags(VALUE hash)
   set_hash("sec", s);
   set_hash("sec_fraction", fr);
   del_hash("seconds");
- del_hash("offset");
     }
     return hash;
}
diff --git a/test/date/test_date_strptime.rb b/test/date/test_date_strptime.rb
index f8483f9..e37be57 100644
--- a/test/date/test_date_strptime.rb
+++ b/test/date/test_date_strptime.rb
@@ -308,6 +308,12 @@ class TestDateStrptime < Test::Unit::TestCase
      DateTime.strptime('2002-03-14T11:22:33.123456789-09:00', '%FT%T.%N%Z'))
   end

+ def test_strptime_bug_7445
+ d = DateTime.strptime('0 +0100', '%s %z')
+ assert_equal Rational(1, 24), d.offset
+ assert_equal 0, d.second
+ end
+
   def test_strptime__2
     n = 10**9
     (Date.new(2006,6,1)..Date.new(2007,6,1)).each do |d|
--
1.8.4-fc