Bug 185269

Summary: Range bisector should check the commits for repositories without change in specified range.
Product: WebKit Reporter: dewei_zhu
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: dewei_zhu, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch rniwa: review+

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());