<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Add the ability to report a commit with sub-commits."
href="https://bugs.webkit.org/show_bug.cgi?id=168962">bug 168962</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #303388 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Add the ability to report a commit with sub-commits."
href="https://bugs.webkit.org/show_bug.cgi?id=168962#c4">Comment # 4</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add the ability to report a commit with sub-commits."
href="https://bugs.webkit.org/show_bug.cgi?id=168962">bug 168962</a>
from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=303388&action=diff" name="attach_303388" title="Patch">attachment 303388</a> <a href="attachment.cgi?id=303388&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=303388&action=review">https://bugs.webkit.org/attachment.cgi?id=303388&action=review</a>
<span class="quote">> Websites/perf.webkit.org/ChangeLog:18
> + CREATE UNIQUE INDEX repository_name_unique_index ON repositories (repository_name) WHERE repository_owner IS NOT NULL;</span >
Please also fix this to say "IS NULL".
<span class="quote">> Websites/perf.webkit.org/init-database.sql:48
> +WHERE repository_owner IS NOT NULL;</span >
Nit: indent WHERE by 4 spaces.
<span class="quote">> Websites/perf.webkit.org/init-database.sql:50
> +WHERE repository_owner IS NOT NULL;</span >
Ditto. This should be WHERE repository_owner IS NULL;
<span class="quote">> Websites/perf.webkit.org/public/api/report-commits.php:5
> -function main($post_data) {
> +function insert_commit($db, $commit_info, $repository_id, $owner_commit_id)</span >
Please put this after main before calling main so that they appear in the order of execution.
<span class="quote">> Websites/perf.webkit.org/public/api/report-commits.php:76
> + $owner_repository_id = $db->select_or_insert_repository_row($commit_info['repository'], NULL);
> + if (!$owner_repository_id) {</span >
I think we should keep this repository_id. It's confusing to talk about owner_repository_id when we're in the midst of inserting the owner itself.
<span class="quote">> Websites/perf.webkit.org/public/api/report-commits.php:80
> + $owner_commit_id = insert_commit($db, $commit_info, $owner_repository_id, NULL);</span >
Ditto about keeping this just commit_id.
<span class="quote">> Websites/perf.webkit.org/public/include/db.php:190
> + // FIXME: Should improve _select_update_or_insert_row to handle the NULL column case.</span >
Nit: there should be a blank line above this comment.
<span class="quote">> Websites/perf.webkit.org/public/include/db.php:193
> + $condition = $repository_owner_id != NULL ? '(repository_name, repository_owner) = ($1, $2)' : 'repository_name = $1 AND repository_owner IS NULL';</span >
I think it's cleaner to rewrite this function where we just have two set of queries for when repository_owner_id is null and when it's not.
<span class="quote">> Websites/perf.webkit.org/public/include/db.php:204
> +
> </span >
Nit: two blank lines here.
<span class="quote">> Websites/perf.webkit.org/server-tests/admin-reprocess-report-tests.js:42
> + }</span >
Just place this curly bracket at the end of the pervious line.
<span class="quote">> Websites/perf.webkit.org/server-tests/admin-reprocess-report-tests.js:56
> + assert.equal(repositories.length, 2);</span >
You should assert that the only commit that you can get is assigned to the second repository we just inserted.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-manifest.js:315
> + let webkit1 = Repository.findById(101);</span >
Why don't we call this osWebkit or instead of webkit1?
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:81
> + const systemVersionCommitWithSubcommits = {</span >
Please move this to where it's used.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:180
> + const sameRepositoryNameInSubCommitAndMajorCommit = {</span >
It's better to have this right next to the test.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:419
> + it("should distinguish between repository with same name but different owner", function (done) {</span >
Nit: repositories with the asme name but with a different owner.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:429
> + let webkitRepository0 = result[0];
> + let webkitRepository1 = result[1];</span >
I think we should call one of the osWebKit or something.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:433
> + done();</span >
You should verify that each commit is associated with the right repositories.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:444
> + db.selectRows('commits', {'message': 'WebKit Commit'}),</span >
We should get the list of all commits for WebKit and verify that we have exactly one commit for WebKit.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:457
> + const osxCommit = result[0][0];</span >
Move each of these right next to where length is asserted so that when one of them fails, it's clear which one it is.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:464
> + assert.notEqual(osxCommit, null);</span >
Also move these next to where the variable is declared so that they're next to each other.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:480
> + const ownerCommitForWebKitCommit = result[0][0];</span >
Ditto.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:496
> + db.selectRows('commits', {'message': 'WebKit Commit'}),</span >
Please check the number of commits here again.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:568
> + assert.equal(commits.length, 1);</span >
Please assert that the one commit we have is for the OS.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>