Bug 185269 - Range bisector should check the commits for repositories without change in specified range.
Summary: Range bisector should check the commits for repositories without change in sp...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-03 14:42 PDT by dewei_zhu
Modified: 2022-02-09 10:14 PST (History)
3 users (show)

See Also:


Attachments
Patch (9.94 KB, patch)
2018-05-03 14:53 PDT, dewei_zhu
no flags Details | Formatted Diff | Diff
Patch (10.12 KB, patch)
2018-05-03 17:03 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 2018-05-03 14:42:12 PDT
Range bisector should check the commits for repositories without change in specified range.
Comment 1 dewei_zhu 2018-05-03 14:53:32 PDT
Created attachment 339472 [details]
Patch
Comment 2 dewei_zhu 2018-05-03 15:09:18 PDT
rdar://problem/39916741
Comment 3 Ryosuke Niwa 2018-05-03 16:11:22 PDT
Comment on attachment 339472 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:8
> +        For the repositories those do not have change in specified range, we still need to use them

This is kind of wordy. How about "For repositories without a change in the specified range,

> Websites/perf.webkit.org/ChangeLog:9
> +        to filter commit sets.

You need to explain why we need to do that. In a way, what you've stated so far is "what".
We need "why".

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:19
> +        const topLevelRepositoriesWithCommitHasOrdering = firstCommitSet.topLevelRepositories()

This phrase isn't grammatically correct. topLevelRepositoriesWithOrderedCommits?

> Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:666
> +        it('should use still check the repository revision even the repository has no change in the range', async () => {

should still check or should still use? Can't have two verbs.
Comment 4 dewei_zhu 2018-05-03 17:03:43 PDT
Created attachment 339488 [details]
Patch
Comment 5 Ryosuke Niwa 2018-05-08 22:55:55 PDT
Comment on attachment 339488 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:16
> +        (async):

Remove this?

> Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:35
> +            let commits = null;
> +            if (startCommit !== endCommit)
> +                commits = await CommitLog.fetchBetweenRevisions(repository, startCommit.revision(), endCommit.revision());
> +            else
> +                commits = [startCommit];

How about just this?
cont commits = startCommit == endCommit ? [startCommit] : await CommitLog.fetchBetweenRevisions(repository, startCommit.revision(), endCommit.revision());