Bug 170932 – Introduce 'commit_group' concept to be able to distinguish between commits with same revision but from different groups.

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

Introduce 'commit_group' concept to be able to distinguish between commits with same revision but from different groups.

Created attachment 307351 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 307351 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:12
> +            CREATE TABLE commit_groups (
> +                commit_group_id serial PRIMARY KEY,
> +                commit_group_name varchar(64),
> +                CONSTRAINT commit_group_name_must_be_unique UNIQUE(commit_group_name));

I think we should just add a new column to commits which is varchar(64) instead of adding a new table.

> Websites/perf.webkit.org/ChangeLog:23
> +            

Nit: Whitespace.

> Websites/perf.webkit.org/public/api/commits.php:29
> +        $group = array_get($_GET, 'group');
> +        $commits = $fetcher->fetch_between($repository_id, $preceding_revision, $last_revision, $group, $keyword);

We need to make the JSON API's parameters consistent.
Here, group is assumed to be an ID.

> Websites/perf.webkit.org/public/api/commits.php:47
> +        $group_name = array_get($_GET, 'group');
> +        if ($group_name)
> +            $commits = $fetcher->fetch_last_reported_for_commit_group($repository_id, $group_name);

And it's assumed to be a string.

> Websites/perf.webkit.org/public/api/report-commits.php:42
> +
> +

Nit: two spaces.

> Websites/perf.webkit.org/public/api/report-commits.php:104
> -    $inserted_commit_id = $db->update_or_insert_row('commits', 'commit', array('repository' => $repository_id, 'revision' => $data['revision']), $data);
> +    $inserted_commit_id = $db->update_or_insert_row('commits', 'commit', array('repository' => $repository_id, 'revision' => $data['revision'], 'group' => $data['group']), $data);

This is problematic because the existing database rows would have group being set to null.
So this results in all macOS & iOS builds being inserted again.
Also, /api/report doesn't set group name so this might end up creating a new row as well.

[Ryosuke Niwa]

Didn't we conclude that this approach was bad?