Bug 223001 - [resultsdbpy] Make client aware of hashes and revisions
Summary: [resultsdbpy] Make client aware of hashes and revisions
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: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-09 15:20 PST by Jonathan Bedard
Modified: 2021-03-10 16:01 PST (History)
4 users (show)

See Also:


Attachments
Patch (9.63 KB, patch)
2021-03-09 15:27 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.63 KB, patch)
2021-03-10 08:05 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (10.46 KB, patch)
2021-03-10 14:10 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (11.55 KB, patch)
2021-03-10 15:10 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2021-03-09 15:20:31 PST
As part of supporting identifiers in the results database, client-side code needs to distinguish between revisions, hashes and identifiers.
Comment 1 Radar WebKit Bug Importer 2021-03-09 15:21:05 PST
<rdar://problem/75237812>
Comment 2 Jonathan Bedard 2021-03-09 15:27:57 PST
Created attachment 422767 [details]
Patch
Comment 3 Zhifei Fang 2021-03-09 16:28:12 PST
Comment on attachment 422767 [details]
Patch

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

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:132
> +        this.identifier = json.identifier ? json.identifier : json.id;

We have a similar issue in the perf db, it will be very easy to get confused about id and identifier, since id is short for identity... 

From the name, id is the Cassandra id and identifier is "commit identifier" right ?

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:149
> +    repr() {

Maybe "Label" is a better name ?

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:151
> +        return this.identifier.substring(0,12);

So the identifier is the svn number and git hash, not commit identifier ?
Comment 4 Jonathan Bedard 2021-03-09 17:20:14 PST
(In reply to Zhifei Fang from comment #3)
> Comment on attachment 422767 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422767&action=review
> 
> > Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:132
> > +        this.identifier = json.identifier ? json.identifier : json.id;
> 
> We have a similar issue in the perf db, it will be very easy to get confused
> about id and identifier, since id is short for identity... 

This bit is temporary. By the end of next week, there will be no more "id", everything will be identifier, revision or hash.

> 
> From the name, id is the Cassandra id and identifier is "commit identifier"
> right ?
> 
> > Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:149
> > +    repr() {
> 
> Maybe "Label" is a better name ?
> 
> > Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:151
> > +        return this.identifier.substring(0,12);
> 
> So the identifier is the svn number and git hash, not commit identifier ?

It's the SVN revision or the git hash. But that's temporary. Basically what's going to happen is I'll be adding a UI allowing the user to control which representation, revision, identifier or hash, they prefer for each repo. In parallel, I will be populating a new commit table that allows one to request commits by revision, hash and identifier. Finally, I will switch the existing APIs to vend commits from the new table, and identifier will be the commit identifier. Things are going to be a bit weird for the next 2 weeks, my goal is to keep the UI and API functional, at the cost of some temporarily misleading code.
Comment 5 dewei_zhu 2021-03-09 18:13:55 PST
Comment on attachment 422767 [details]
Patch

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

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/templates/commits.html:74
> +                    this.ref.setState(json.map(datum=>new Commit(datum)));

Nit: space before and after '=>' ?
Comment 6 Jonathan Bedard 2021-03-10 08:05:10 PST
Created attachment 422835 [details]
Patch
Comment 7 Jonathan Bedard 2021-03-10 14:10:04 PST
Created attachment 422869 [details]
Patch
Comment 8 dewei_zhu 2021-03-10 14:17:20 PST
Comment on attachment 422869 [details]
Patch

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

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:150
> +        // Per the birthday paradox, 10% change of collision with 7.7 million commits with 12 character commits

You mean `chance`?
Comment 9 Jonathan Bedard 2021-03-10 14:36:28 PST
(In reply to dewei_zhu from comment #8)
> Comment on attachment 422869 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422869&action=review
> 
> > Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:150
> > +        // Per the birthday paradox, 10% change of collision with 7.7 million commits with 12 character commits
> 
> You mean `chance`?

Yes, moving this comment around, not sure how we didn't notice it the first time around.
Comment 10 Jonathan Bedard 2021-03-10 15:10:49 PST
Created attachment 422877 [details]
Patch for landing
Comment 11 EWS 2021-03-10 16:01:18 PST
Committed r274248: <https://commits.webkit.org/r274248>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422877 [details].