[webkit-reviews] review granted: [Bug 222897] [perf dashboard] Add commit revision label support : [Attachment 424098] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Zhifei Fang
<zhifei_fang at apple.com>'s request for review:
Bug 222897: [perf dashboard] Add commit revision label support
https://bugs.webkit.org/show_bug.cgi?id=222897

Attachment 424098: Patch

https://bugs.webkit.org/attachment.cgi?id=424098&action=review




--- Comment #43 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 424098
  --> https://bugs.webkit.org/attachment.cgi?id=424098
Patch

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

r=me assuming the changes suggested below are made.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:13
> +	   $commit_rows = $db->query_and_fetch_all("SELECT commit_id FROM
commits WHERE commit_repository = $1 AND $column_name  = $2",
array($repository_id, $revision));

Nit: two spaces between $column_name and =.

> Websites/perf.webkit.org/public/include/commit-updater.php:112
> +		   self::validate_commits($owned_commit_info, true);

Nice! This is way better than the old patch with code duplication.

> Websites/perf.webkit.org/public/include/commit-updater.php:161
> +    private static function validate_commits(&$commit_info,
$is_own_commit=false)

Nit: spaces around =. Since this is not python, we follow the usual WebKit
coding style.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:55
> +    static getRevisionType(revision)

This should be _repositoryType.
We don't use "get" prefix unless it has an out argument:
https://webkit.org/code-style-guidelines/#names-setter-getter

> Websites/perf.webkit.org/public/v3/models/commit-log.js:63
> +    static getformatttedRevision(revision, revisionIdentifier=null) 

Nit: Always use camelCase. Also need spaces around =.
But in this case, this should be called _formattedRevision instead.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:65
> +	   let formattedRevision = revision;

I don't think we need this variable.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:68
> +	       formattedRevision = 'r' + revision;

Use return.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:70
> +	       formattedRevision = revision.substring(0, 12);

Use return.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:76
> +    revisionIdentifierParts() { 

Nit: { should be on the next line.
Also, this should be probably called _revisionIdentifier.
But this shouldn't really be a separate method since there is no utility
outside diff at least for now.
So this should really be a lambda function inside diff.
But defining a method for this is really an overkill.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:77
> +	   const revisionIdentifier =  this.revisionIdentifier();

Nit: two spaces between after =.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:80
> +	   const identifierRe = /(?<number>\d+)@(?<branch>[\w\.\-]+)/;

There is no need to have this local variable.
Just do: /~/.exec

> Websites/perf.webkit.org/public/v3/models/commit-log.js:100
> +	   const toRevisionIdentifierParts = this.revisionIdentifierParts();
> +	   const fromRevisionIdentifierParts =
previousCommit.revisionIdentifierParts();

These are really long names! How about toIdentifier & fromIdentifier or toMatch
& fromMatch

> Websites/perf.webkit.org/public/v3/models/commit-log.js:116
> +	   let connector = ' - ';
> +	   if (revisionType == 'git')
> +	       connector = '..';
> +	   if (revisionType == 'svn')
> +	       connector = '-';
> +	   let label = `${previousCommit.label()}${connector}${this.label()}`;
> +	   if (fromRevisionIdentifierParts || toRevisionIdentifierParts) {
> +	       if (fromRevisionIdentifierParts && toRevisionIdentifierParts) {
> +		   console.assert(fromRevisionIdentifierParts.branch ==
toRevisionIdentifierParts.branch);
> +		   label = `${fromRevisionIdentifierParts.number} -
${toRevisionIdentifierParts.number}@${fromRevisionIdentifierParts.branch}
(${CommitLog.getformatttedRevision(fromRevision)}${connector}${CommitLog.getfor
matttedRevision(toRevision)})`
> +	       } else {
> +		   connector = ' - ';
> +		   label =
`${previousCommit.label()}${connector}${this.label()}`;
> +	       }
> +	   }

This should really be a lambda.
Also, if we're gonna just assert that branches are same, we should just use
early return,
which is WebKit's preferred style.

const identifierPattern = /^(?<number>\d+)\@(?<branch>.+)$/;
const label = ((fromMatch, toMatch) => {
    const separator = repositoryType == 'git' ? '..' : (repositoryType == 'svn'
? '-' : ' - ');
    if (fromMatch && toMatch) {
	console.assert(fromRevisionIdentifierParts.branch ==
toRevisionIdentifierParts.branch);
	return
`${fromRevisionIdentifierParts.number}-${toRevisionIdentifierParts.number}@${fr
omRevisionIdentifierParts.branch}
(${CommitLog.getformatttedRevision(fromRevision)}${connector}${CommitLog.getfor
matttedRevision(toRevision)})`;
    }
    if (fromMatch || toMatch)
	return `${previousCommit.label()}$ - ${this.label()}`;
    return `${previousCommit.label()}${connector}${this.label()}`;
})(identifierPattern.exec(previousCommit.revisionIdentifier()),
identifierPattern.exec(this.revisionIdentifier()));

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:289
> +		   label: '175605 - 184276 at main (r200574-r200805)',

This looks wrong. It should be: 175605-184276 at main (r200574-r200805)