Defect #6346

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

Added by Felix Schäfer about 7 years ago. Updated over 6 years ago.

Status:ClosedStart date:2010-09-09
Priority:NormalDue date:
Assignee:Eric Davis% Done:

100%

Category:SCM
Target version:1.0.2
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).

6346.patch Magnifier (1.15 KB) Felix Schäfer, 2010-09-09 21:55

6346-new.diff Magnifier (1.65 KB) Toshi MARUYAMA, 2010-09-17 17:23

git.png (118 KB) Toshi MARUYAMA, 2010-09-20 02:59

Associated revisions

Revision 4187
Added by Eric Davis about 7 years ago

Parse the timezone in #last_rev for git to correct display the Age diplay. #6346

Contributed by Felix Schäfer

Revision 4823
Added by Toshi MARUYAMA almost 7 years ago

scm: git: remove localtime (#6346).

No needs to use localtime.
If we use localtime, we should clone.

See r4794 and r4802.

History

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

#2 Updated by Toshi MARUYAMA about 7 years ago

I think this issue is related with #5357.

#3 Updated by Felix Schäfer about 7 years ago

Toshi MARUYAMA wrote:

I think this issue is related with #5357.

No.

#4 Updated by Toshi MARUYAMA about 7 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" 

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

#6 Updated by Toshi MARUYAMA about 7 years ago

#7 Updated by Toshi MARUYAMA about 7 years ago

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

#8 Updated by Felix Schäfer about 7 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, JB: could one of you review and commit this? Thanks.

@Toshi: 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.

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

#10 Updated by Felix Schäfer about 7 years ago

  • Assignee set to Felix Schäfer

Felix Schäfer wrote:

@Eric, 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.

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

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

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

#14 Updated by Eric Davis about 7 years ago

  • Status changed from Resolved to Closed

Merged into 1.0-stable

#15 Updated by Toshi MARUYAMA over 6 years ago

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

Also available in: Atom PDF