Bug 184368 – Added a helper function in CommitSet which will be shared between multiple independent incoming changes.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Added two helper functions in CommitSet which will be shared between multiple independent incoming changes.

Created attachment 337392 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 337392 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:4
> +        Added two helper functions in CommitSet which will be shared between multiple independent incoming changes.
> +        https://bugs.webkit.org/show_bug.cgi?id=184368

Don't add random helper functions like this.
Just add a function to do either.
Each WebKit bug should fix a very specific problem.
r- because of this.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:91
> +    withoutRootPatchOrOwnedCommit()

Again, the semantics of this function should be reversed, and this should be called containsRootPatchOrOwnedCommit()
r- because of this.

Created attachment 337412 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 337412 [details]
Patch

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

r=me assuming the following comments are addressed.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:149
> +    static createNameWithoutCollision(name, hasDuplicateTestGroupName)

Why don't we make this function take a set of names instead of a closure
since most of other call sites you're adding will be using a set.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:204
> +                nameParts.push(`${repository.name()}: Patch(${nameForFirstPatch}) - Patch(${nameForSecondPath})`);

We shouldn't make up a new syntax like Patch(a.patch).
Just say: "WebKit: a.patch - b.patch"
Filenames and revision/git hashes look sufficiently different that the confusion
between the two isn't really a serious practical concern as far as I can tell.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:207
> +                nameParts.push(`${repository.name()}: ${firstCommit.label()} with Patch(${nameForFirstPatch}) - ${secondCommit.label()} with Patch(${nameForSecondPath})`);

Ditto. We shouldn't be using unnecessarily verbose syntax like Patch(a.patch).
Just say: WebKit: r12345 with a.patch - r12346 with b.patch.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:217
> +            const leftRootFileDescription = uniqueInFirstCommitSet.map((rootFile) => nameGenerator(rootFile.filename())).join(', ');
> +            const rightRootFileDescription = uniqueInSecondCommitSet.map((rootFile) => nameGenerator(rootFile.filename())).join(', ');

These should firstDescription and secondDescription.
Left & right are very LTR centric way of looking at things.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:218
> +            nameParts.push(`Roots: ${leftRootFileDescription.length ? leftRootFileDescription : 'none'} - ${rightRootFileDescription.length ? rightRootFileDescription : 'none'}`);

"leftRootFileDescription || 'none'" would be sufficient here since '' is falsey.

> Websites/perf.webkit.org/unit-tests/commit-set-tests.js:334
> +        });

You need a test case where two repositories' commits differ.

[Ryosuke Niwa]

Comment on attachment 337412 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:3
> +        Added a helper function in CommitSet which will be shared between multiple independent incoming changes.

Please also rename this to say we're adding CommitSet.diff

[Ryosuke Niwa]

https://trac.webkit.org/changeset/230441/webkit

Ugh... I asked you to update the bug title to reflect what you're doing :(

[Radar WebKit Bug Importer]

<rdar://problem/39304875>