Bug 221861 – [results.webkit.org] Distinguish hash and revision

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

[Jonathan Bedard]

Right now, both hash and revision are treated as an id. We should distinguish the two in the json the results database returns.

[Radar WebKit Bug Importer]

<rdar://problem/74298457>

[Jonathan Bedard]

Created attachment 420196 [details]
Patch

[Aakash Jain]

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.

[Jonathan Bedard]

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

[Jonathan Bedard]

Created attachment 420324 [details]
Patch

[Aakash Jain]

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

[Jonathan Bedard]

Created attachment 420333 [details]
Patch

[EWS]

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

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