Bug 153467 - Sort incoming commits via date instead of revision number
Summary: Sort incoming commits via date instead of revision number
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 152345
  Show dependency tree / graph
 
Reported: 2016-01-25 16:48 PST by Jason Marcell
Modified: 2016-01-26 23:45 PST (History)
7 users (show)

See Also:

Attachments
Patch (1.88 KB, patch)
2016-01-25 16:53 PST, Jason Marcell 		no flags Details | Formatted Diff | Diff
Patch (9.62 KB, patch)
2016-01-26 16:22 PST, Jason Marcell 		no flags Details | Formatted Diff | Diff
Patch (9.62 KB, patch)
2016-01-26 17:04 PST, Jason Marcell 		no flags Details | Formatted Diff | Diff
Patch (10.90 KB, patch)
2016-01-26 17:29 PST, Jason Marcell 		ap: review+
Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Marcell 2016-01-25 16:48:13 PST 
Sort incoming commits via date instead of revision number
Comment 1 Jason Marcell 2016-01-25 16:53:51 PST 
Created attachment 269817 [details]
Patch
Comment 2 Dean Johnson 2016-01-25 17:05:28 PST 
Comment on attachment 269817 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269817&action=review

r=me w/ passing tests! :)

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:200
> +            this.recordedCommits = newCommits.concat(this.recordedCommits).sort(function(a, b) { return a.date - b.date; });

I'm assuming this is the case, but just to be sure: these dates are either stored as integers or are data types that have logic to handle arithmetic operations and give correct results?

Actually, tests for this would completely eliminate my concern :)
Comment 3 Jason Marcell 2016-01-26 16:22:35 PST 
Created attachment 269946 [details]
Patch
Comment 4 Jason Marcell 2016-01-26 16:23:31 PST 
(In reply to comment #2)
> Comment on attachment 269817 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269817&action=review
> 
> r=me w/ passing tests! :)
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:200
> > +            this.recordedCommits = newCommits.concat(this.recordedCommits).sort(function(a, b) { return a.date - b.date; });
> 
> I'm assuming this is the case, but just to be sure: these dates are either
> stored as integers or are data types that have logic to handle arithmetic
> operations and give correct results?

Yes, that is the case.

> 
> Actually, tests for this would completely eliminate my concern :)

I've added tests.
Comment 5 Dean Johnson 2016-01-26 16:54:43 PST 
Comment on attachment 269946 [details]
Patch

Looks good to me!
Comment 6 Jason Marcell 2016-01-26 17:04:28 PST 
Created attachment 269953 [details]
Patch
Comment 7 Alexey Proskuryakov 2016-01-26 17:12:46 PST 
Comment on attachment 269953 [details]
Patch

I don't think that it's right for the tests to live inside public_html, why did we put them there?

The patch doesn't apply, but it looks fine to me.
Comment 8 Jason Marcell 2016-01-26 17:19:29 PST 
(In reply to comment #7)
> Comment on attachment 269953 [details]
> Patch
> 
> I don't think that it's right for the tests to live inside public_html, why
> did we put them there?

I'm definitely open to changing the way these unit tests are organized. I welcome feedback on a better organizational layout. If we come up with something better I can address it in a future bug/patch.

> 
> The patch doesn't apply, but it looks fine to me.

I can rebase the patch so that it applies cleanly.
Comment 9 Jason Marcell 2016-01-26 17:29:36 PST 
Created attachment 269957 [details]
Patch
Comment 10 Jason Marcell 2016-01-26 23:45:43 PST 
Landed: http://trac.webkit.org/changeset/195658