[webkit-reviews] review granted: [Bug 198847] Custom analysis task configurator should allow supplying commit prefix and revision starts 'r'. : [Attachment 372097] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 198847: Custom analysis task configurator should allow supplying commit
prefix and revision starts 'r'.
https://bugs.webkit.org/show_bug.cgi?id=198847

Attachment 372097: Patch

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




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

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

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:204
> +	   $rows = $this->db->query_and_fetch_all('SELECT * FROM commits WHERE
commit_repository = $1 AND commit_revision LIKE $2 LIMIT 2',
array($repository_id, Database::escape_for_like($revision_prefix) . '%'));

This is not right. In the case of subversion revision, we should always check
with =.
It makes no sense to match r123 against revision 123456 or 456123.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:181
> -	   const data = await
this.cachedFetch(`/api/commits/${repository.id()}/${revision}`);
> +	   const data = await
this.cachedFetch(`/api/commits/${repository.id()}/${revision}${prefixMatch ?
'?prefix-match=true' : ''}`);

Just do: this.cachedFetch(`/api/commits/${repository.id()}/${revision}`,
prefixMatch ? {prefixMatch} : {})
We could also update cachedFetch to not include the param when it's set to
"false"

> Websites/perf.webkit.org/server-tests/api-commits-tests.js:401
> +	       const result = await
remote.getJSON('/api/commits/WebKit/210949?prefix-match=false');

Need another test to make sure we default to false when prefix-match is not
set.