Bug 221861 - [results.webkit.org] Distinguish hash and revision
Summary: [results.webkit.org] Distinguish hash and revision
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-02-12 16:52 PST by Jonathan Bedard
Modified: 2021-02-15 11:58 PST (History)
2 users (show)

See Also:


Attachments
Patch (2.95 KB, patch)
2021-02-12 16:54 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (2.76 KB, patch)
2021-02-15 10:00 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (3.06 KB, patch)
2021-02-15 10:45 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-02-12 16:52:08 PST
Right now, both hash and revision are treated as an id. We should distinguish the two in the json the results database returns.
Comment 1 Radar WebKit Bug Importer 2021-02-12 16:52:40 PST
<rdar://problem/74298457>
Comment 2 Jonathan Bedard 2021-02-12 16:54:08 PST
Created attachment 420196 [details]
Patch
Comment 3 Aakash Jain 2021-02-15 09:18:04 PST
Comment on attachment 420196 [details]
Patch

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

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:-48
> -    def __init__(self, repository_id, branch, id, timestamp=None, order=None, committer=None, message=None):

can we keep it one line for readability.

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:55
> +    ):

We should probably have a check at the beginning of this method which limits passing only one of (id, revision, identifier). I believe passing multiple ones isn't expected.

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:76
> +            self.revision = self.revision if self.revision else self.id

Better to keep it explicit multi-line if else. When written that way, I don't see a reason to do: self.revision = self.revision

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:78
> +            self.hash = self.hash if self.hash else self.id

Ditto.

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:84
> +                raise ValueError(f"'{self.revision}' is an revision")

This error message doesn't look like an error, more like an information. Can be improved to look like an error and actually indicate what's the error here.
Comment 4 Jonathan Bedard 2021-02-15 09:49:57 PST
Comment on attachment 420196 [details]
Patch

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

>> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:55
>> +    ):
> 
> We should probably have a check at the beginning of this method which limits passing only one of (id, revision, identifier). I believe passing multiple ones isn't expected.

We will be passing multiple ones, as an example, https://commits.webkit.org/234094@main would be:

id=234094@main
hash= ee0111014bf107a0958f023114778b75eddc8a4d
revision=272858
Comment 5 Jonathan Bedard 2021-02-15 10:00:08 PST
Created attachment 420324 [details]
Patch
Comment 6 Aakash Jain 2021-02-15 10:08:09 PST
Comment on attachment 420324 [details]
Patch

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

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:70
> +        if re.match(r'[0-9]+$', self.id):

Might be good to make this re a variable, like RE_SVN_REVISION

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:73
> +        elif re.match(r'[a-fA-F0-9]+$', self.id):

Might be good to make this re a variable, like RE_GIT_HASH
Comment 7 Jonathan Bedard 2021-02-15 10:45:46 PST
Created attachment 420333 [details]
Patch
Comment 8 EWS 2021-02-15 11:58:40 PST
Committed r272872: <https://commits.webkit.org/r272872>

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