Project

General

Profile

Actions
Copy link

Defect #6346

closed

Age column on repository view is skewed for git, probably CVS too

Added by Felix Schäfer about 14 years ago. Updated over 13 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Eric Davis
Category:
SCM
Target version:
1.0.2
Start date:
2010-09-09
Due date:
% Done:

100%

Estimated time:
Resolution:
Fixed
Affected version:

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

Download all files
6346.patch (1.15 KB) 6346.patch Felix Schäfer, 2010-09-09 21:55
6346-new.diff (1.65 KB) 6346-new.diff Toshi MARUYAMA, 2010-09-17 17:23
git.png (118 KB) git.png Toshi MARUYAMA, 2010-09-20 02:59
Actions
Copy link
 #1

Updated by Felix Schäfer about 14 years ago

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.

Actions
Copy link
 #2

Updated by Toshi MARUYAMA about 14 years ago

I think this issue is related with #5357.

Actions
Copy link
 #3

Updated by Felix Schäfer about 14 years ago

Toshi MARUYAMA wrote:

I think this issue is related with #5357.

No.

Actions
Copy link
 #4

Updated by Toshi MARUYAMA about 14 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"
Actions
Copy link
 #5

Updated by Felix Schäfer about 14 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.

Actions
Copy link
 #6

Updated by Toshi MARUYAMA about 14 years ago

Actions
Copy link
 #7

Updated by Toshi MARUYAMA about 14 years ago

I forgot to write description at previous note.
I posted a patch only for git.

Actions
Copy link
 #8

Updated by Felix Schäfer about 14 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.

Actions
Copy link
 #9

Updated by Toshi MARUYAMA about 14 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.

Actions
Copy link
 #10

Updated by Felix Schäfer about 14 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.

Actions
Copy link
 #11

Updated by Felix Schäfer about 14 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

Actions
Copy link
 #12

Updated by Toshi MARUYAMA about 14 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'.

>> 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.
Actions
Copy link
 #13

Updated by Eric Davis about 14 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.

Actions
Copy link
 #14

Updated by Eric Davis about 14 years ago

  • Status changed from Resolved to Closed

Merged into 1.0-stable

Actions
Copy link
 #15

Updated by Toshi MARUYAMA over 13 years ago

I confirmed CVS has this problem. I create new issue #7827.

Actions
Copy link

Also available in: Atom PDF