Bug 181967 – Fix CommitSet.equals bug which will always return false when comparing CommitSet against MeasurementCommitSet.

• Component: New Bugs
• Status: NEW

Fix CommitSet.equals bug which will always return false when comparing CommitSet against MeasurementCommitSet.

Created attachment 331990 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 331990 [details]
Patch

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

r-. There are enough issues in this patch that I don't think we can land as is.

> Websites/perf.webkit.org/ChangeLog:15
> +        (CommitSet.prototype.commitForRepository): Returns null instead of undefined when key does not exist.

Why do we need to do that? null == undefined.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:70
> +    commitForRepository(repository) { return CommitSet.getFromMapWithFallbackReturn(this._repositoryToCommitMap, repository); }

A canonical way to write this would be this._repositoryToCommitOwnerMap.get(repository) || null.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:90
> +    requiresBuildForRepository(repository) { return CommitSet.getFromMapWithFallbackReturn(this._repositoryRequiresBuildMap, repository, false); }

There is no need to do this. !!null and !!undefined are both false.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:117
> +        const neitherIsMeasurementCommitSet = !(this instanceof MeasurementCommitSet) && !(other instanceof MeasurementCommitSet);
> +        if (neitherIsMeasurementCommitSet) {

We shouldn't have a different code path like this.
Both CommitSet and MeasurementCommitSet should have methods to get the list of owned commits in the set instead.

Created attachment 332005 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 332005 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:3
> +        Fix CommitSet.equals bug which will always return false when comparing CommitSet against MeasurementCommitSet.

We should be describing the end impact in the bug title which is that TestGroupResultsViewer creates unnecessary rows.

> Websites/perf.webkit.org/ChangeLog:10
> +        comparison between a CommitSet and a MeasurementCommitSet.
> +        MeasurementCommitSet does not have full information for the commits, thus, it cannot build mappings

Need a blank line between two paragraphs.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:119
> +        const neitherIsMeasurementCommitSet = !(this instanceof MeasurementCommitSet) && !(other instanceof MeasurementCommitSet);
> +        if (neitherIsMeasurementCommitSet) {
> +            if (this._repositoryToPatchMap.size != other._repositoryToPatchMap.size)
> +                return false;
> +            if (this._repositoryToRootMap.size != other._repositoryToRootMap.size)
> +                return false;
> +            if (this._repositoryToCommitOwnerMap.size != other._repositoryToCommitOwnerMap.size)
> +                return false;
> +            if (this._repositoryRequiresBuildMap.size != other._repositoryRequiresBuildMap.size)
> +                return false;
> +        }
> +

We shouldn't have this special branch as I mentioned earlier. r- because of this.

Comment on attachment 332005 [details]
Patch

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

>> Websites/perf.webkit.org/public/v3/models/commit-set.js:119
>> +
> 
> We shouldn't have this special branch as I mentioned earlier. r- because of this.

Sure. Then, we need to repeat below for loop, but loop against other._repositoryToCommitMap instead. Otherwise, this function will give us a incorrect return if 'this' is some sort of subset of 'other'.

Comment on attachment 332005 [details]
Patch

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

>>> Websites/perf.webkit.org/public/v3/models/commit-set.js:119
>>> +
>> 
>> We shouldn't have this special branch as I mentioned earlier. r- because of this.
> 
> Sure. Then, we need to repeat below for loop, but loop against other._repositoryToCommitMap instead. Otherwise, this function will give us a incorrect return if 'this' is some sort of subset of 'other'.

Never mind. As we've already checked the size of repositories.

Created attachment 332129 [details]
Patch

Created attachment 332134 [details]
Patch