Bug 198847 - Custom analysis task configurator should allow supplying commit prefix and revision starts 'r'.
Summary: Custom analysis task configurator should allow supplying commit prefix and re...
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: dewei_zhu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-13 19:13 PDT by dewei_zhu
Modified: 2019-06-17 16:13 PDT (History)
3 users (show)

See Also:


Attachments
Patch (21.20 KB, patch)
2019-06-13 19:19 PDT, dewei_zhu
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description dewei_zhu 2019-06-13 19:13:26 PDT
Custom analysis task configurator should allow supplying commit prefix and revision starts 'r'.
Comment 1 dewei_zhu 2019-06-13 19:19:59 PDT
Created attachment 372097 [details]
Patch
Comment 2 dewei_zhu 2019-06-17 11:13:09 PDT
rdar://49238080
rdar://49237819
Comment 3 Ryosuke Niwa 2019-06-17 14:17:22 PDT
Comment on attachment 372097 [details]
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.
Comment 4 dewei_zhu 2019-06-17 16:12:25 PDT
Landed in r246522.
Comment 5 dewei_zhu 2019-06-17 16:13:25 PDT
This change is landed in r246522.