<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<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#c2">Comment # 2</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=302923&action=diff" name="attach_302923" title="Patch">attachment 302923</a> <a href="attachment.cgi?id=302923&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=302923&action=review">https://bugs.webkit.org/attachment.cgi?id=302923&action=review</a>
<span class="quote">> Websites/perf.webkit.org/ChangeLog:9
> + On existing production server, run '''</span >
You should probably use ``` instead.
<span class="quote">> Websites/perf.webkit.org/ChangeLog:11
> + commit_owner integer NOT NULL REFERENCES commits ON DELETE CASCADE,</span >
We should rename repository_parent to repository_owner if we're going with owner/owned.
<span class="quote">> Websites/perf.webkit.org/public/api/report-commits.php:5
> +function get_repository_id($repository_name, $parent_repoistory_id, $db) {</span >
$db must be the first argument.
Also { should appear on the following line on its own: <a href="https://webkit.org/code-style-guidelines/#braces">https://webkit.org/code-style-guidelines/#braces</a>
and no get_ prefix: <a href="https://webkit.org/code-style-guidelines/#names-out-argument">https://webkit.org/code-style-guidelines/#names-out-argument</a>
We should probably call this ensure_repository_id or select_or_insert_repository_row.
<span class="quote">> Websites/perf.webkit.org/public/api/report-commits.php:10
> + $repository_id = $db->select_or_insert_row('repositories', 'repository', $repository_info);</span >
This would happily insert (WebKit, non-null) value yet that would break
ReportProcessor::resolve_build_id, ManifestGenerator::repositories (we should include all repositories there with parent repository set),
and CommitLogFetcher::repository_id_from_name (should only find top-level repository).
<span class="quote">> Websites/perf.webkit.org/public/api/report-commits.php:18
> +function process_one_commit($commit_info, $repository_id, $owner_commit_id, $db) {</span >
We should probably call insert_commit.
Again, $db should be the first argument.
<span class="quote">> Websites/perf.webkit.org/public/api/report-commits.php:59
> + $db->insert_row('commit_ownerships', '', array('commit_owner' => $owner_commit_id, 'commit_ownee' => $inserted_commit_id), NULL);</span >
Just pass in "commit" as the prefix.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:90
> + "WebKit": {</span >
We should add a test case where WebKit is a non-sub repository,
and make sure submitting it as a sub commit would either fail or insert a new repository.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:92
> + "time": "2013-02-06T08:55:20.9Z",</span >
I think we should enforce that sub-commit never specifies commit time
because it makes no sense for a sub-commit to have a time and its parent to not have one or them to be different.
I think it's best to forbid altogether for simplify for now.
<span class="quote">> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:198
> + const osxCommit = find_first_matching(commits, {'revision': 'Sierra16D32'});</span >
This would not catch this particular revision getting inserted into a wrong repository.
It's probably better if we manually fetched repository list, and fetch commit for each instead.
Something like this would catch more bugs:
(() => {
return Promise.all[db.selectAll('repositories'), db.selectAll('commits')];
}).then((result) => {
const repositories = result[0];
const repositoryNames = repositories.map((row) => row['repository_name']).sort();
asset.deepEqual(repositoryNames, ["WebKit", ...]);
const repositoryIdByName = {};
const commitByRepository = {};
for (let commit of result[1]) {
const repsoitoryId = commit['commit_repository'];
assert(!(repsoitoryId in commitByRepository));
commitByRepository[repsoitoryId] = commit;
}
assert(repsoitoryId[repositoryIdByName["WebKit"]]['commit_revision'], '1235');
});</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>