Bug 223001 – [resultsdbpy] Make client aware of hashes and revisions

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

As part of supporting identifiers in the results database, client-side code needs to distinguish between revisions, hashes and identifiers.

[Radar WebKit Bug Importer]

<rdar://problem/75237812>

[Jonathan Bedard]

Created attachment 422767 [details]
Patch

[Zhifei Fang]

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 ?

[Jonathan Bedard]

(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 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 '=>' ?

[Jonathan Bedard]

Created attachment 422835 [details]
Patch

[Jonathan Bedard]

Created attachment 422869 [details]
Patch

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`?

[Jonathan Bedard]

(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.

[Jonathan Bedard]

Created attachment 422877 [details]
Patch for landing

[EWS]

Committed r274248: <https://commits.webkit.org/r274248>

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