Defect #6346
closed
Age column on repository view is skewed for git, probably CVS too
100%
Description
Ok, this is a nasty one and it wasn't easy to find, but I think I figured it out.
The gist of the problem is that Redmine::Scm::Adapters::Entry#lastrev returns a Redmine::Scm::Adapters::Revision for which the #time is with most adapters a Time expressed in local time, with the git adapter a String and with the CVS adapter a Time, but I'm not sure in what timezone. The problem with this is that the view methods, specifically distance_of_time_in_words (source:/trunk/app/views/repositories/_dir_list_content.rhtml#L21), will happily take either Time or String, but fails to parse the included timezone information, at least in the format the git adapter delivers.
In a console:
>> Project.find("redmine-doodles").repository.entry("init.rb").lastrev.time.class
=> String
>> Project.find("redmine-doodles").repository.entry("init.rb").lastrev.time
=> "Fri Sep 3 17:34:00 2010 +0200"
>> Project.find("redmine-doodles").repository.entry("init.rb").lastrev.time.to_time
=> Fri Sep 03 17:34:00 UTC 2010
>> Time.parse(Project.find("redmine-doodles").repository.entry("init.rb").lastrev.time).localtime
=> Fri Sep 03 17:34:00 +0200 2010
>> Project.find("sandbox").repository.entry("tags").lastrev.time.class
=> Time
>> Project.find("sandbox").repository.entry("tags").lastrev.time
=> Thu Dec 03 15:41:08 +0100 2009
>> Project.find("sandbox").repository.entry("tags").lastrev.time.to_time
=> Thu Dec 03 15:41:08 +0100 2009
redmine-doodles has a git repo, sandbox a svn repo, #to_time is what distance_of_time_in_words uses to cast whatever it's given to a Time, which misses the time zone information (at least with String#to_time).
Files
Updated by Felix Schäfer over 13 years ago
- File 6346.patch 6346.patch added
And here's the proposed patch. I made sure all adapters that have a Redmine::Scm::Adapters::Entry#lastrev with a #time also return the time as #localtime because I'm not sure where else it is used, it shouldn't make a difference for distance_of_time_in_words though.
Updated by Toshi MARUYAMA over 13 years ago
I think this issue is related with #5357.
Updated by Felix Schäfer over 13 years ago
Updated by Toshi MARUYAMA over 13 years ago
'localtime' has no effect.
>> include ActionView::Helpers::DateHelper => Object >> str00 = "2010-09-14 11:22:52 +0100" => "2010-09-14 11:22:52 +0100" >> str01 = "2010-09-14 11:22:52 +0900" => "2010-09-14 11:22:52 +0900" >> distance_of_time_in_words(str00,str01) => "less than a minute" >> distance_of_time_in_words(Time.parse(str00),Time.parse(str01)) => "about 8 hours" >> distance_of_time_in_words(Time.parse(str00).localtime,Time.parse(str01)) => "about 8 hours" >> distance_of_time_in_words(Time.parse(str00).localtime,Time.parse(str01).localtime) => "about 8 hours"
Updated by Felix Schäfer over 13 years ago
Toshi MARUYAMA wrote:
'localtime' has no effect.
[...]
Not there, but I don't know at what other places this function is used, so I just wanted to be on the safe side of things.
Updated by Toshi MARUYAMA over 13 years ago
- File 6346-new.diff 6346-new.diff added
Updated by Toshi MARUYAMA over 13 years ago
I forgot to write description at previous note.
I posted a patch only for git.
Updated by Felix Schäfer over 13 years ago
- Status changed from New to 7
- Target version set to 1.0.2
Good catch with the --date=iso parameter Toshi, thanks. I put together both patches and added a complete test for last_rev in the git adapter here http://github.com/thegcat/redmine/commit/1d69507c9cc2bcc5877ac101ba549334a8f81b85 (it might be worth it duplicating this test to the SCMs I guess).
Eric Hulser, JB: could one of you review and commit this? Thanks.
toshio harita: I dug a little deeper into the ruby time classes to see what localtime did and why it could be important, and while it doesn't do anything to the time and to stuff working with the time directly, it is important for things that take specific bits of information:
>> Time.now => Sat Sep 18 13:28:12 +0200 2010 >> Time.now.hour => 13 >> Time.now.utc => Sat Sep 18 11:28:23 UTC 2010 >> Time.now.utc.hour => 11
While in this case it doesn't make a difference, because Time.parse returns a local time, I think forcing it to local time is just a way to make sure that it will always be a local time no matter what.
Updated by Toshi MARUYAMA over 13 years ago
I think 'localtime' is no need.
Because Redmine compare Time.now and entry.lastrev.time at source:tags/1.0.1/app/views/repositories/_dir_list_content.rhtml#L21 .
And Ruby Time object has timezone info in regard of utc or localtime.
Updated by Felix Schäfer over 13 years ago
- Assignee set to Felix Schäfer
Felix Schäfer wrote:
Eric Hulser, JB: could one of you review and commit this? Thanks.
Hold on to that thought, I have a dumb error in the test and it's probably at the wrong place, will post a corrected version this afternoon.
Updated by Felix Schäfer over 13 years ago
- Assignee changed from Felix Schäfer to Jean-Baptiste Barth
Felix Schäfer wrote:
will post a corrected version this afternoon.
The revised commit is here: http://github.com/thegcat/redmine/commit/f8c7ce4e246ac629205f1ec9ed7b0ab110c33b72
Updated by Toshi MARUYAMA over 13 years ago
I finished test and I confirmed 'localtime' is no need.
I pushed new commit at http://github.com/marutosi/redmine/commit/e3adf6bdbc10f1a8aad70c9a97ce5ba222166e2e .
Redmine git adapter uses Time.parse at following lines and no use 'localtime'.
- source:tags/1.0.1/lib/redmine/scm/adapters/git_adapter.rb#L142
- source:tags/1.0.1/lib/redmine/scm/adapters/git_adapter.rb#L191
>> include ActionView::Helpers::DateHelper => Object >> Time.now => Mon Sep 20 09:59:36 +0900 2010 >> str00 = "2010-09-20 09:40:27 +0900" => "2010-09-20 09:40:27 +0900" >> distance_of_time_in_words(Time.now,Time.parse(str00)) => "19 minutes" >> str01= "2010-09-20 01:40:27 +0100" => "2010-09-20 01:40:27 +0100" >> distance_of_time_in_words(Time.now,Time.parse(str01)) => "20 minutes"
$ GIT_AUTHOR_DATE="2010-09-20 01:40:00 +0100" \
GIT_COMMITTER_DATE="2010-09-19 20:40:00 -0400" \
git commit -a -m "git date test at `LANG=C date`."
$ git log --date=iso --pretty=fuller -n 1 | cat
commit fb3c85d484459b59404e1c39bb6e14c72dd5a0a1
Author: Toshi MARUYAMA <XXXXXXXXXXXXXXXXXXXXXXX>
AuthorDate: 2010-09-20 01:40:00 +0100
Commit: Toshi MARUYAMA <XXXXXXXXXXXXXXXXXXXXXXX>
CommitDate: 2010-09-19 20:40:00 -0400
git date test at Mon Sep 20 09:46:04 JST 2010.
Updated by Eric Davis over 13 years ago
- Status changed from 7 to Resolved
- Assignee changed from Jean-Baptiste Barth to Eric Davis
- % Done changed from 0 to 100
- Resolution set to Fixed
Added Felix's patch in r4187.
Toshi MARUYAMA, I don't see the problem with using localtime. If you can update your patch with a failing test, I'd be happy to look at it again.
Updated by Eric Davis over 13 years ago
- Status changed from Resolved to Closed
Merged into 1.0-stable
Updated by Toshi MARUYAMA about 13 years ago
I confirmed CVS has this problem. I create new issue #7827.