Added two helper functions in CommitSet which will be shared between multiple independent incoming changes.
Created attachment 337392 [details] Patch
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
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.
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
https://trac.webkit.org/changeset/230441/webkit Ugh... I asked you to update the bug title to reflect what you're doing :(
<rdar://problem/39304875>