Bug 222897 – [perf dashboard] Add commit revision label support

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zhifei Fang]

Add commit identifier support

[Zhifei Fang]

Created attachment 422544 [details]
Patch

Comment on attachment 422544 [details]
Patch

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

I believe we also need some changes to report-processor.php around line 163.

> Websites/perf.webkit.org/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

`Reviewed by` should be the line before the detailed commit description.

> Websites/perf.webkit.org/ChangeLog:15
> +        * init-database.sql:
> +        * migrate-database.sql:

Maybe a brief description for the change here would be since

> Websites/perf.webkit.org/init-database.sql:105
> +    commit_identifier varchar(64) DEFAULT NULL,

Let's also add a constraint for commit_identifier as well 
CONSTRAINT commit_identifier_in_repository_must_be_unique UNIQUE(commit_repository, commit_identifier));
Also add a unit test for that.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:56
> +        let revision_format = revision;

Do you mean `formatted_revision` here?

> Websites/perf.webkit.org/server-tests/resources/test-server.conf:20
> +LoadModule php7_module /usr/local/Cellar/php@7.4/7.4.16/lib/httpd/modules/libphp7.so

Let's remove this line for now until we finalized way to setup an instance on recent macOS.

[Zhifei Fang]

Created attachment 422615 [details]
Patch

Comment on attachment 422615 [details]
Patch

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

> Websites/perf.webkit.org/server-tests/resources/test-server.conf:20
> +LoadModule php7_module /usr/local/Cellar/php@7.4/7.4.16/lib/httpd/modules/libphp7.so

Let's remove this for now.

[Zhifei Fang]

Created attachment 422619 [details]
Patch

Comment on attachment 422619 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:10
> +        Add new column commit identifier
> +        Make all commit api also can work with commit identifier
> +        Change the commit label display with commit identiier

Those sentences should end with '.'.
Typo in `identiier` -> `identifier`.
`Change the commit label display with commit identiier` -> `Change commit label to display commit *with* identifier.`

[Ryosuke Niwa]

Comment on attachment 422619 [details]
Patch

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

> Websites/perf.webkit.org/browser-tests/commit-log-viewer-tests.js:137
> +            expect(viewer.content('spinner-container').offsetHeight).to.not.be(0);
> +            await waitForComponentsToRender(context);

What is this test change about? It should be explained in the change log.

> Websites/perf.webkit.org/browser-tests/commit-log-viewer-tests.js:142
> -            expect(viewer.content('commits-list').textContent).to.contain('r210950');
> +            expect(viewer.content('commits-list').textContent).to.contain('184278@main (r210950)');

Please add a new test instead of mutating an existing test case.

> Websites/perf.webkit.org/init-database.sql:105
> +    commit_identifier varchar(64) DEFAULT NULL,

Calling this "identifier" is a bit confusing because we also have commit_id.
Maybe commit_revision_identifier?

> Websites/perf.webkit.org/migrate-database.sql:19
> +    ALTER TABLE commits ADD COLUMN IF NOT EXISTS commit_identifier;
> +    ALTER TABLE commits DROP CONSTRAINT commit_identifier_in_repository_must_be_unique;
> +    ALTER TABLE commits ADD CONSTRAINT commit_identifier_in_repository_must_be_unique UNIQUE(commit_repository, commit_identifier);

r-. This can't be right. This migration is nothing to do with having build_number.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:9
>      static function find_commit_id_by_revision($db, $repository_id, $revision)

Huh, I think this function is really a duplicate of fetch_revision.
Not sure how we ended up with two functions here.
We should probably clean this up in a separate patch.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:13
> +        $column_name = 'revision';
> +        if ($this->is_commit_identifier($revision))
> +            $column_name = 'identifier';

Please put the whole column name into the variable.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:201
> +    private function is_commit_identifier($commit_revision_or_identifier) {
> +        return preg_match('/\d+@\S+/', $commit_revision_or_identifier);

I don't think \S+ is right. We want 1234@ is match 1234@main.
Or are we doing that transformation in the frontend?

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:208
> -        $commit_info = array('repository' => $repository_id, 'revision' => $revision);
> +        if (!$this->is_commit_identifier($revision))
> +            $commit_info = array('repository' => $repository_id, 'revision' => $revision);
> +        else 
> +            $commit_info = array('repository' => $repository_id, 'identifier' => $revision);

Looks like this can just be:
$commit_info = array('repository' => $repository_id);
$commit_info[$this->is_commit_identifier($revision) ? 'identifier' : 'revision'] = $revision;

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:218
> +        $column_name = 'revision';
> +        if ($this->is_commit_identifier($revision_prefix))
> +            $column_name = 'identifier';

Please use a ternary expression here as well.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:220
> +        $rows = $this->db->query_and_fetch_all('SELECT * FROM commits WHERE commit_repository = $1 AND commit_'.$column_name.' LIKE $2 ORDER BY commit_'.$column_name.' LIMIT 2', array($repository_id, Database::escape_for_like($revision_prefix) . '%'));

Nit: Need spaces around each ".".

> Websites/perf.webkit.org/public/include/commit-updater.php:89
> +                $commit_identifiers[$commit_info['identifier']] = true;
> +            }

We should also verify the format of identifier.
r- due to the lack of the check here and the lack of tests for that check.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:39
>      testability() { return this._rawData['testability']; }

We also need to update CommitLog.prototype.updateSingleton
so that we can either assert that commit identifiers don't change or update it dynamically.
r- due to this missing code and the lack of tests thereof.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:42
> +    identifier() { return this._rawData['identifier']}

Same issue here. It's confusing to have id() and identifier().
Probably we should call it revisionIdentifier().
Also nit: missing ; and a space at the end.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:56
> +        let formatted_revision = revision;

Nit: Use camelCase.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:66
>  

We also need to update diff(previousCommit).
r- due to the lack of change there and test cases thereof.

> Websites/perf.webkit.org/server-tests/api-commits-tests.js:533
> +        it("should return commit with commit identifier", () => {
> +            return addSlaveForReport(subversionCommits).then(() => {

Use async & await.

> Websites/perf.webkit.org/server-tests/api-commits-tests.js:756
> +            ]).then(() => {
> +                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?precedingRevision=184276@main&lastRevision=184278@main');

Ditto.

> Websites/perf.webkit.org/server-tests/api-commits-tests.js:806
> +            return Promise.all([
> +                db.insert('repositories', {'id': 1, 'name': 'WebKit'}),

Ditto.

> Websites/perf.webkit.org/server-tests/api-commits-tests.js:825
>      });

We need tests for when querying with a partial match.
Also, we need to check that, latest, oldest, etc... all support the commit identifiers.
r- due to the lack of these test cases.

> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:172
> +    it("should reject with duplicated commit identifier", () => {
> +        return addSlaveForReport(subversionCommit).then(() => {

Use async & await.

> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:181
> +

We also need tests for updating commit identifiers on an existing commit,
both an existing commit doesn't have any commit identifier and when it already has some other commit.

Comment on attachment 422619 [details]
Patch

I should have done the check more carefully in the first patch.
By searching the reference of `commit_revision` in our codebase, It looks like we may also want to expose the identifier info in `api/measurement-set.php: line 169 & line 263`, `include/build-requests-fetcher.php: line 159`, otherwise, the UI may not be able to show commit with identifiers for CommitSet from BuildRequests and MeasurementSet.

[Ryosuke Niwa]

(In reply to dewei_zhu from comment #8)
> Comment on attachment 422619 [details]
> Patch
> 
> I should have done the check more carefully in the first patch.
> By searching the reference of `commit_revision` in our codebase, It looks
> like we may also want to expose the identifier info in
> `api/measurement-set.php: line 169 & line 263`,
> `include/build-requests-fetcher.php: line 159`, otherwise, the UI may not be
> able to show commit with identifiers for CommitSet from BuildRequests and
> MeasurementSet.

Also MeasurementCommitSet.

We need to make sure CommitSetRangeBisector._orderCommitSetsByTimeAndOrderThenDeduplicate works when commits have identifiers as well.

We probably want to do sometime about commits with identifiers in CommitLogViewer.

CustomAnalysisTaskConfigurator._computeCommitSet also has some funky logic with regards to revisions although it may not matter much if we're rewriting that whole thing.

[Zhifei Fang]

(In reply to Ryosuke Niwa from comment #7)
> Comment on attachment 422619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422619&action=review
> 
> > Websites/perf.webkit.org/browser-tests/commit-log-viewer-tests.js:137
> > +            expect(viewer.content('spinner-container').offsetHeight).to.not.be(0);
> > +            await waitForComponentsToRender(context);
> 
> What is this test change about? It should be explained in the change log.

Yeah, this is a kind of strange, since we request the remote, it should firstly prompt the spinner, then hide the spinner to display the resolved list, not sure why this is passing at the very beginning.
> 
> > Websites/perf.webkit.org/browser-tests/commit-log-viewer-tests.js:142
> > -            expect(viewer.content('commits-list').textContent).to.contain('r210950');
> > +            expect(viewer.content('commits-list').textContent).to.contain('184278@main (r210950)');
> 
> Please add a new test instead of mutating an existing test case.

Got it
> 
> > Websites/perf.webkit.org/init-database.sql:105
> > +    commit_identifier varchar(64) DEFAULT NULL,
> 
> Calling this "identifier" is a bit confusing because we also have commit_id.
> Maybe commit_revision_identifier?

I used this name because that's the "official name" somehow.
> 
> > Websites/perf.webkit.org/migrate-database.sql:19
> > +    ALTER TABLE commits ADD COLUMN IF NOT EXISTS commit_identifier;
> > +    ALTER TABLE commits DROP CONSTRAINT commit_identifier_in_repository_must_be_unique;
> > +    ALTER TABLE commits ADD CONSTRAINT commit_identifier_in_repository_must_be_unique UNIQUE(commit_repository, commit_identifier);
> 
> r-. This can't be right. This migration is nothing to do with having
> build_number.

good point
> 
> > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:9
> >      static function find_commit_id_by_revision($db, $repository_id, $revision)
> 
> Huh, I think this function is really a duplicate of fetch_revision.
> Not sure how we ended up with two functions here.
> We should probably clean this up in a separate patch.
> 
> > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:13
> > +        $column_name = 'revision';
> > +        if ($this->is_commit_identifier($revision))
> > +            $column_name = 'identifier';
> 
> Please put the whole column name into the variable.
> 
> > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:201
> > +    private function is_commit_identifier($commit_revision_or_identifier) {
> > +        return preg_match('/\d+@\S+/', $commit_revision_or_identifier);
> 
> I don't think \S+ is right. We want 1234@ is match 1234@main.
> Or are we doing that transformation in the frontend?
> 
> > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:208
> > -        $commit_info = array('repository' => $repository_id, 'revision' => $revision);
> > +        if (!$this->is_commit_identifier($revision))
> > +            $commit_info = array('repository' => $repository_id, 'revision' => $revision);
> > +        else 
> > +            $commit_info = array('repository' => $repository_id, 'identifier' => $revision);
> 
> Looks like this can just be:
> $commit_info = array('repository' => $repository_id);
> $commit_info[$this->is_commit_identifier($revision) ? 'identifier' :
> 'revision'] = $revision;
> 
> > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:218
> > +        $column_name = 'revision';
> > +        if ($this->is_commit_identifier($revision_prefix))
> > +            $column_name = 'identifier';
> 
> Please use a ternary expression here as well.
> 
> > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:220
> > +        $rows = $this->db->query_and_fetch_all('SELECT * FROM commits WHERE commit_repository = $1 AND commit_'.$column_name.' LIKE $2 ORDER BY commit_'.$column_name.' LIMIT 2', array($repository_id, Database::escape_for_like($revision_prefix) . '%'));
> 
> Nit: Need spaces around each ".".
> 
> > Websites/perf.webkit.org/public/include/commit-updater.php:89
> > +                $commit_identifiers[$commit_info['identifier']] = true;
> > +            }
> 
> We should also verify the format of identifier.
> r- due to the lack of the check here and the lack of tests for that check.
> 
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:39
> >      testability() { return this._rawData['testability']; }
> 
> We also need to update CommitLog.prototype.updateSingleton
> so that we can either assert that commit identifiers don't change or update
> it dynamically.
> r- due to this missing code and the lack of tests thereof.
> 
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:42
> > +    identifier() { return this._rawData['identifier']}
> 
> Same issue here. It's confusing to have id() and identifier().
> Probably we should call it revisionIdentifier().
> Also nit: missing ; and a space at the end.
> 
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:56
> > +        let formatted_revision = revision;
> 
> Nit: Use camelCase.
> 
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:66
> >  
> 
> We also need to update diff(previousCommit).
> r- due to the lack of change there and test cases thereof.

Got it.
> 
> > Websites/perf.webkit.org/server-tests/api-commits-tests.js:533
> > +        it("should return commit with commit identifier", () => {
> > +            return addSlaveForReport(subversionCommits).then(() => {
> 
> Use async & await.
> 
> > Websites/perf.webkit.org/server-tests/api-commits-tests.js:756
> > +            ]).then(() => {
> > +                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?precedingRevision=184276@main&lastRevision=184278@main');
> 
> Ditto.
> 
> > Websites/perf.webkit.org/server-tests/api-commits-tests.js:806
> > +            return Promise.all([
> > +                db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
> 
> Ditto.
> 
> > Websites/perf.webkit.org/server-tests/api-commits-tests.js:825
> >      });
> 
> We need tests for when querying with a partial match.
> Also, we need to check that, latest, oldest, etc... all support the commit
> identifiers.
> r- due to the lack of these test cases.
> 
> > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:172
> > +    it("should reject with duplicated commit identifier", () => {
> > +        return addSlaveForReport(subversionCommit).then(() => {
> 
> Use async & await.
> 
> > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:181
> > +
> 
> We also need tests for updating commit identifiers on an existing commit,
> both an existing commit doesn't have any commit identifier and when it
> already has some other commit.

[Ryosuke Niwa]

Having said that, it's okay to add the basic support for the backend code first.

By the way, the most critical API: /api/report should probably get a support for this as well. It would be annoying if we didn't even see commit identifiers just because commit syncing script had gone down for one reason or another so we should be reporting commit identifiers along with subversion revision / Git hash there as well.

[Ryosuke Niwa]

(In reply to Zhifei Fang from comment #10)
> (In reply to Ryosuke Niwa from comment #7)
> > Comment on attachment 422619 [details]
> > Patch
> > 
> > > Websites/perf.webkit.org/init-database.sql:105
> > > +    commit_identifier varchar(64) DEFAULT NULL,
> > 
> > Calling this "identifier" is a bit confusing because we also have commit_id.
> > Maybe commit_revision_identifier?
> 
> I used this name because that's the "official name" somehow.

I get that. But it's simply too confusing to have two columns named commit_id and commit_identifier. Maybe we could use something like commit_preferred_revision or commit_revision_label or something.

[Zhifei Fang]

(In reply to Ryosuke Niwa from comment #12)
> (In reply to Zhifei Fang from comment #10)
> > (In reply to Ryosuke Niwa from comment #7)
> > > Comment on attachment 422619 [details]
> > > Patch
> > > 
> > > > Websites/perf.webkit.org/init-database.sql:105
> > > > +    commit_identifier varchar(64) DEFAULT NULL,
> > > 
> > > Calling this "identifier" is a bit confusing because we also have commit_id.
> > > Maybe commit_revision_identifier?
> > 
> > I used this name because that's the "official name" somehow.
> 
> I get that. But it's simply too confusing to have two columns named
> commit_id and commit_identifier. Maybe we could use something like
> commit_preferred_revision or commit_revision_label or something.

How about commit_tag ? This also give us some abilities to extend this field to more staffs, commit identifier will only apply to webkit GitHub for now. the OS commit won't have such staff, same as other normal git repo, but with this name, it may be reused for git tag or some other customized label for OS commit as well...

[Ryosuke Niwa]

(In reply to Zhifei Fang from comment #13)
> (In reply to Ryosuke Niwa from comment #12)
> > (In reply to Zhifei Fang from comment #10)
> > > (In reply to Ryosuke Niwa from comment #7)
> > > > Comment on attachment 422619 [details]
> > > > Patch
> > > > 
> > > > > Websites/perf.webkit.org/init-database.sql:105
> > > > > +    commit_identifier varchar(64) DEFAULT NULL,
> > > > 
> > > > Calling this "identifier" is a bit confusing because we also have commit_id.
> > > > Maybe commit_revision_identifier?
> > > 
> > > I used this name because that's the "official name" somehow.
> > 
> > I get that. But it's simply too confusing to have two columns named
> > commit_id and commit_identifier. Maybe we could use something like
> > commit_preferred_revision or commit_revision_label or something.
> 
> How about commit_tag ? This also give us some abilities to extend this field
> to more staffs, commit identifier will only apply to webkit GitHub for now.
> the OS commit won't have such staff, same as other normal git repo, but with
> this name, it may be reused for git tag or some other customized label for
> OS commit as well...

Hm... wouldn't that be confusing with Git's tag? Like I could imagine we'd be eventually adding a support for branch/tag information in this class.

Comment on attachment 422619 [details]
Patch

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

>>>>>> Websites/perf.webkit.org/init-database.sql:105
>>>>>> +    commit_identifier varchar(64) DEFAULT NULL,
>>>>> 
>>>>> Calling this "identifier" is a bit confusing because we also have commit_id.
>>>>> Maybe commit_revision_identifier?
>>>> 
>>>> I used this name because that's the "official name" somehow.
>>> 
>>> I get that. But it's simply too confusing to have two columns named commit_id and commit_identifier. Maybe we could use something like commit_preferred_revision or commit_revision_label or something.
>> 
>> How about commit_tag ? This also give us some abilities to extend this field to more staffs, commit identifier will only apply to webkit GitHub for now. the OS commit won't have such staff, same as other normal git repo, but with this name, it may be reused for git tag or some other customized label for OS commit as well...
> 
> Hm... wouldn't that be confusing with Git's tag? Like I could imagine we'd be eventually adding a support for branch/tag information in this class.

How about `commit_serial` or `commit_serial_label` serial implies it has some ordering?

[Zhifei Fang]

(In reply to Ryosuke Niwa from comment #14)
> (In reply to Zhifei Fang from comment #13)
> > (In reply to Ryosuke Niwa from comment #12)
> > > (In reply to Zhifei Fang from comment #10)
> > > > (In reply to Ryosuke Niwa from comment #7)
> > > > > Comment on attachment 422619 [details]
> > > > > Patch
> > > > > 
> > > > > > Websites/perf.webkit.org/init-database.sql:105
> > > > > > +    commit_identifier varchar(64) DEFAULT NULL,
> > > > > 
> > > > > Calling this "identifier" is a bit confusing because we also have commit_id.
> > > > > Maybe commit_revision_identifier?
> > > > 
> > > > I used this name because that's the "official name" somehow.
> > > 
> > > I get that. But it's simply too confusing to have two columns named
> > > commit_id and commit_identifier. Maybe we could use something like
> > > commit_preferred_revision or commit_revision_label or something.
> > 
> > How about commit_tag ? This also give us some abilities to extend this field
> > to more staffs, commit identifier will only apply to webkit GitHub for now.
> > the OS commit won't have such staff, same as other normal git repo, but with
> > this name, it may be reused for git tag or some other customized label for
> > OS commit as well...
> 
> Hm... wouldn't that be confusing with Git's tag? Like I could imagine we'd
> be eventually adding a support for branch/tag information in this class.


I'd prefere commit_revision_label then, we can reused this for something in the future with this more general name.

[Ryosuke Niwa]

(In reply to dewei_zhu from comment #15)
> Comment on attachment 422619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422619&action=review
> 
> >>>>>> Websites/perf.webkit.org/init-database.sql:105
> >>>>>> +    commit_identifier varchar(64) DEFAULT NULL,
> >>>>> 
> >>>>> Calling this "identifier" is a bit confusing because we also have commit_id.
> >>>>> Maybe commit_revision_identifier?
> >>>> 
> >>>> I used this name because that's the "official name" somehow.
> >>> 
> >>> I get that. But it's simply too confusing to have two columns named commit_id and commit_identifier. Maybe we could use something like commit_preferred_revision or commit_revision_label or something.
> >> 
> >> How about commit_tag ? This also give us some abilities to extend this field to more staffs, commit identifier will only apply to webkit GitHub for now. the OS commit won't have such staff, same as other normal git repo, but with this name, it may be reused for git tag or some other customized label for OS commit as well...
> > 
> > Hm... wouldn't that be confusing with Git's tag? Like I could imagine we'd be eventually adding a support for branch/tag information in this class.
> 
> How about `commit_serial` or `commit_serial_label` serial implies it has
> some ordering?

I've considered that but then what's serial / monotonic is the leading numerical part, not the whole thing.

[Zhifei Fang]

Created attachment 422891 [details]
Patch

[Zhifei Fang]

Comment on attachment 422619 [details]
Patch

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

>> Websites/perf.webkit.org/server-tests/api-commits-tests.js:825
>>      });
> 
> We need tests for when querying with a partial match.
> Also, we need to check that, latest, oldest, etc... all support the commit identifiers.
> r- due to the lack of these test cases.

Added the partial match tests.

For latest, oldest, etc, by adding the revisionLabel in the "subversionCommits", the existing tests will automatically check this field as well, since we use something like "assertCommitIsSameAsOneSubmitted(result['commits'][0], subversionCommits['commits'][0]);"

[Ryosuke Niwa]

(In reply to Zhifei Fang from comment #19)
> Comment on attachment 422619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422619&action=review
> 
> >> Websites/perf.webkit.org/server-tests/api-commits-tests.js:825
> >>      });
> > 
> > We need tests for when querying with a partial match.
> > Also, we need to check that, latest, oldest, etc... all support the commit identifiers.
> > r- due to the lack of these test cases.
> 
> Added the partial match tests.
> 
> For latest, oldest, etc, by adding the revisionLabel in the
> "subversionCommits", the existing tests will automatically check this field
> as well, since we use something like
> "assertCommitIsSameAsOneSubmitted(result['commits'][0],
> subversionCommits['commits'][0]);"

What I mean is that we need to add tests cases for accessing those APIs with commit identifiers, not necessarily verifying that the results contain them, which we obviously should check as well.

[Zhifei Fang]

(In reply to Ryosuke Niwa from comment #20)
> (In reply to Zhifei Fang from comment #19)
> > Comment on attachment 422619 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=422619&action=review
> > 
> > >> Websites/perf.webkit.org/server-tests/api-commits-tests.js:825
> > >>      });
> > > 
> > > We need tests for when querying with a partial match.
> > > Also, we need to check that, latest, oldest, etc... all support the commit identifiers.
> > > r- due to the lack of these test cases.
> > 
> > Added the partial match tests.
> > 
> > For latest, oldest, etc, by adding the revisionLabel in the
> > "subversionCommits", the existing tests will automatically check this field
> > as well, since we use something like
> > "assertCommitIsSameAsOneSubmitted(result['commits'][0],
> > subversionCommits['commits'][0]);"
> 
> What I mean is that we need to add tests cases for accessing those APIs with
> commit identifiers, not necessarily verifying that the results contain them,
> which we obviously should check as well.

Wont be those API taking repository and order only ?

/api/commits/<repository>/oldest
/api/commits/<repository>/latest
/api/commits/<repository>/last-reported
/api/commits/<repository>/last-reported?from=<start_order>&to=<end_order>

Please let me know if I miss something

[Ryosuke Niwa]

(In reply to Zhifei Fang from comment #21)
> (In reply to Ryosuke Niwa from comment #20)
> > (In reply to Zhifei Fang from comment #19)
> > What I mean is that we need to add tests cases for accessing those APIs with
> > commit identifiers, not necessarily verifying that the results contain them,
> > which we obviously should check as well.
> 
> Wont be those API taking repository and order only ?
> 
> /api/commits/<repository>/oldest
> /api/commits/<repository>/latest
> /api/commits/<repository>/last-reported
> /api/commits/<repository>/last-reported?from=<start_order>&to=<end_order>

Oh, that's a good point. FWIW, we should also add a test case for owned-commits to make sure the API rejects a request with commit identifier instead of revision.

Comment on attachment 422891 [details]
Patch

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

> Websites/perf.webkit.org/browser-tests/commit-log-viewer-tests.js:160
> +    it('should show revision label', () => {

Let's use async with await here so that we can get rid of Promise.then

> Websites/perf.webkit.org/init-database.sql:114
> +    CONSTRAINT commit_revision_label_in_repository_must_be_unique UNIQUE(commit_repository, commit_revision_label));

Maybe we should do
`CREATE UNIQUE INDEX commit_revision_label_in_repository_must_be_unique_if_not_null ON commits (commit_repository, commit_revision_label)
WHERE commit_revision_label IS NOT NULL;`

> Websites/perf.webkit.org/public/include/commit-updater.php:86
> +                    $this->exit_with_error('DuplicatedCommitIdentifier', array('commit' => $commit_info));

Maybe the error name should be `DuplicatedRevisionLabel`?

> Websites/perf.webkit.org/public/include/report-processor.php:167
> +            $commit_data = array('repository' => $repository_id, 'revision' => $revision_data['revision'], 'time' => array_get($revision_data, 'timestamp'), 'revision_label' => array_get($revision_data, 'revisionLabel', null));

We should not set

> Websites/perf.webkit.org/public/v3/models/commit-log.js:81
> +        const to_revision = this.revision();
> +        const from_revison = previousCommit.revision();
> +        const to_revision_label = this.revisionLabel();
> +        const from_revision_label = previousCommit.revisionLabel();
> +        let label = `${previousCommit.label()} - ${this.label()}`;

use camelCase?

> Websites/perf.webkit.org/server-tests/api-commits-tests.js:13
> +    const subversionCommits =  {

Nit: extra space

> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:200
> +    it("should reject an ivalid revision label", async () => {

`invalid`

> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:208
> +    it("should reject with duplicated commit revision labels", async () => {

Maybe `should reject with 'DuplicatedRevisionLabel' when same commit revision labels from different commits are reported`?

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:183
> +        it('should prefix commit identifier with svn r + revision', () => {

Do you mean `should prefix with commit revision label followed by 'r' prefixed svn revision`?
Also, we should avoid calling it identifier any more since we decided to call it commit revision label.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:191
> +        it('should prefix commit identifier with git hash truncated', () => {

-> `should prefix with commit revision label followed by truncated git hash`? or maybe even say:
"should follow `{commit revision label} ({12 character truncated git hash})` format"

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:205
> +        it('should prefix commit identifier', () => {

`should prefix SVN revision with commit revision label`

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:209
>          it('should truncate a Git hash at 8th character', function () {

this should be updated from '8th' -> '12th'

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:213
> +        it('should prefix commit identifier with git hash truncated',  () => {

-> `should prefix Git hash with commit revision label followed by truncated git hash`?

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:276
> +        it('should prefix with commit revision label and use "-", if both have revision label', () => {

should follow `{A commit revision label} (r{A svn revision}) - {B commit revision label} (r{B svn revision})` or other more descriptive way.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:284
> +        it('should only contains revison, if one of the commit doesn`t have the revision label', () => {

I think this behavior may need more discussion. To me,  it would be still useful if we have commit revision label on one side. What do you think? Even though in reality this may not happen.

Comment on attachment 422891 [details]
Patch

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

>> Websites/perf.webkit.org/public/include/report-processor.php:167
>> +            $commit_data = array('repository' => $repository_id, 'revision' => $revision_data['revision'], 'time' => array_get($revision_data, 'timestamp'), 'revision_label' => array_get($revision_data, 'revisionLabel', null));
> 
> We should not set

NVM. If we allow it to be null, it should be OK.

[Zhifei Fang]

(In reply to dewei_zhu from comment #23)
> Comment on attachment 422891 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422891&action=review
> 
> > Websites/perf.webkit.org/browser-tests/commit-log-viewer-tests.js:160
> > +    it('should show revision label', () => {
> 
> Let's use async with await here so that we can get rid of Promise.then
> 
> > Websites/perf.webkit.org/init-database.sql:114
> > +    CONSTRAINT commit_revision_label_in_repository_must_be_unique UNIQUE(commit_repository, commit_revision_label));
> 
> Maybe we should do
> `CREATE UNIQUE INDEX
> commit_revision_label_in_repository_must_be_unique_if_not_null ON commits
> (commit_repository, commit_revision_label)
> WHERE commit_revision_label IS NOT NULL;`
> 
Discussed with Dewei, since suggest this change for letting the unique constraint allow NULL value, in fact we do allow multiple NULL in the column that has a unique constraint.

https://www.postgresql.org/docs/9.6/ddl-constraints.html

Comment on attachment 422891 [details]
Patch

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

> Websites/perf.webkit.org/public/v3/models/commit-log.js:80
> +        const to_revision = this.revision();
> +        const from_revison = previousCommit.revision();
> +        const to_revision_label = this.revisionLabel();
> +        const from_revision_label = previousCommit.revisionLabel();

use camelCase?

[Zhifei Fang]

Created attachment 422988 [details]
Patch

[Zhifei Fang]

Created attachment 422992 [details]
Patch

[Zhifei Fang]

Created attachment 422997 [details]
Patch

Comment on attachment 422997 [details]
Patch

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

> Websites/perf.webkit.org/browser-tests/commit-log-viewer-tests.js:148
> +            expect(viewer.content('spinner-container').offsetHeight).not.to.be(0);
> +            await waitForComponentsToRender(context);

This doesn't seem to help with the flakiness of this test.

[Zhifei Fang]

Created attachment 423073 [details]
Patch

So far, looks good to me.

You may want to mention the browser test fix we did in this patch in the change log. Also, maybe we should apply same fix for other the browser tests those are flaky.

[Radar WebKit Bug Importer]

<rdar://problem/75419827>

[Zhifei Fang]

Any more comments for this one ?

[Ryosuke Niwa]

Comment on attachment 423073 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:12
> +        * browser-tests/commit-log-viewer-tests.js: Fix a failed test case, while requesting the remote api, we should wait for those promises resolved and then wait for commponent to render.

Can we hard-wrap this really long line? Maybe after "the remote api,".

> Websites/perf.webkit.org/init-database.sql:114
> +    CONSTRAINT commit_revision_label_in_repository_must_be_unique UNIQUE(commit_repository, commit_revision_label));

Hm... it's counter-intuitive that label needs to be unique.
Maybe we should call this commit_revision_identifier instead.
Or maybe commit_string_identifier?

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:13
> +        $commit_rows = $db->query_and_fetch_all('SELECT commit_id FROM commits WHERE commit_repository = $1 AND ' . $column_name . ' = $2', array($repository_id, $revision));

Can we just do this: "SELECT commit_id FROM commits WHERE commit_repository = \$1 AND $column_name = \$2"

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:20
> +        $commit_rows = $db->query_and_fetch_all('SELECT commit_id FROM commits WHERE commit_repository = $1 AND ' . $column_name . ' LIKE $2 LIMIT 2', array($repository_id, Database::escape_for_like($revision) . '%'));

Ditto.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:198
> +    private static function is_commit_revision_label($commit_revision_or_revision_label) {

Nit: { should appear on the next line.
I know a lot of existing code doesn't follow this style guideline but we should do that in the new code.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:199
> +        return preg_match('/^\d+@\S*$/', $commit_revision_or_revision_label);

I don't think \S is right. We don't wanna allow random symbols here including @.
So I'd suggest we explicitly list character types: [A-Za-z0-9\.\-_]
We also need a test for this (at least for @ and random other symbols like $ and /)

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:213
> +        $rows = $this->db->query_and_fetch_all('SELECT * FROM commits WHERE commit_repository = $1 AND commit_' . $column_name . ' LIKE $2 ORDER BY commit_'.$column_name.' LIMIT 2', array($repository_id, Database::escape_for_like($revision_prefix) . '%'));

Ditto about using double quotes.

> Websites/perf.webkit.org/public/include/commit-updater.php:161
> +    private static function validate_own_commits(&$commit_info)

This seems like a subset of validate_commits.
Please share the share code.

> Websites/perf.webkit.org/public/include/commit-updater.php:163
> +        require_format('OwnCommitRevision', $commit_info['revision'], '/^[A-Za-z0-9 \.]+$/');

I don't think we need to differentiate ownCommitRevision vs. revision since we're reporting the revision.

> Websites/perf.webkit.org/public/include/report-processor.php:167
> +            $commit_data = array('repository' => $repository_id, 'revision' => $revision_data['revision'], 'time' => array_get($revision_data, 'timestamp'), 'revision_label' => array_get($revision_data, 'revisionLabel', null));

Please wrap the line after "'time' => array_get($revision_data, 'timestamp'),",
and start the next line 4 spaces to the right of $commit_data. i.e.
$commit_data = array('rep...,
     'revision_label' =>

> Websites/perf.webkit.org/public/v3/models/commit-log.js:61
> +        let formattedRevision = revision;
>          if (parseInt(revision) == revision) // e.g. r12345
> -            return 'r' + revision;
> +            formattedRevision = 'r' + revision;
>          if (revision.length == 40) // e.g. git hash
> -            return revision.substring(0, 12);
> -        return revision;
> +            formattedRevision = revision.substring(0, 12);

Let's extract this logic as a helper function:
_formattedRevision()
{
    const revision = this.revision();
    if (parseInt(revision) == revision) // e.g. r12345
        return 'r' + revision;
    if (revision.length == 40) // e.g. git hash
        return revision.substring(0, 12);
    return revision;
}

> Websites/perf.webkit.org/public/v3/models/commit-log.js:81
> +        let label = `${previousCommit.label()} - ${this.label()}`;

This is going to be very ugly for commit identifiers because we're gonna have to different numbers split by -.
Let's do something better. Something like this:
const toRevision = this._formattedRevision();
const fromRevision = previousCommit._formattedRevision();
const toRevisionLabel = this.revisionLabel();
const fromRevisionLabel = previousCommit.revisionLabel();
console.assert(!!toRevisionLabel == !!fromRevisionLabel);

const withRevisionLabel (label) => {
    if (!toRevisionLabel || !fromRevisionLabel)
        return label;

    const branchName = (revisionLabel) => (revisionLabel.match(/\@.+/) || [''])[0].substring(1);
    const toBranch = branchName(toRevisionLabel);
    const fromBranch = branchName(fromRevisionLabel);
    if (toBranch && toBranch == fromBranch)
        fromRevisionLabel = fromRevisionLabel.substring(0, fromRevisionLabel.search(toBranch))
    return `${fromRevisionLabel}-${toRevisionLabel} (${label})`;
}
if (parseInt(fromRevision) == fromRevision && parseInt(toRevision) == toRevision)
    return withRevisionLabel(`{fromRevision}-${toRevision}`);
if (fromRevision.length == 40 && toRevision.length == 40)
    return withRevisionLabel(`{fromRevision}:${toRevision}`);
return `{fromRevision} - ${toRevision}`;

Note that we need to keep a space around for things like OS versions.

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:278
> +                label: '175605@main (r200574) - 184276@main (r200805)',

With the suggested change above, this should become:
'175605-184276@main (r200574-r200805)'

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

Please add a test for Git hashes with commit identifiers & rebaseline existing test cases.

[Ryosuke Niwa]

Sorry for the delay.

[Ryosuke Niwa]

Comment on attachment 423073 [details]
Patch

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

>> Websites/perf.webkit.org/public/v3/models/commit-log.js:81
>> +        let label = `${previousCommit.label()} - ${this.label()}`;
> 
> This is going to be very ugly for commit identifiers because we're gonna have to different numbers split by -.
> Let's do something better. Something like this:
> const toRevision = this._formattedRevision();
> const fromRevision = previousCommit._formattedRevision();
> const toRevisionLabel = this.revisionLabel();
> const fromRevisionLabel = previousCommit.revisionLabel();
> console.assert(!!toRevisionLabel == !!fromRevisionLabel);
> 
> const withRevisionLabel (label) => {
>     if (!toRevisionLabel || !fromRevisionLabel)
>         return label;
> 
>     const branchName = (revisionLabel) => (revisionLabel.match(/\@.+/) || [''])[0].substring(1);
>     const toBranch = branchName(toRevisionLabel);
>     const fromBranch = branchName(fromRevisionLabel);
>     if (toBranch && toBranch == fromBranch)
>         fromRevisionLabel = fromRevisionLabel.substring(0, fromRevisionLabel.search(toBranch))
>     return `${fromRevisionLabel}-${toRevisionLabel} (${label})`;
> }
> if (parseInt(fromRevision) == fromRevision && parseInt(toRevision) == toRevision)
>     return withRevisionLabel(`{fromRevision}-${toRevision}`);
> if (fromRevision.length == 40 && toRevision.length == 40)
>     return withRevisionLabel(`{fromRevision}:${toRevision}`);
> return `{fromRevision} - ${toRevision}`;
> 
> Note that we need to keep a space around for things like OS versions.

actually, (revisionLabel.match(/\@.+/) || [''])[0].substring(1) can be:
(revisionLabel.match(/\@(.+)/) || [])[1] or revisionLabel.match(/\d+\@(.+)|()/)[1]

[Zhifei Fang]

(In reply to Ryosuke Niwa from comment #35)
> Comment on attachment 423073 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423073&action=review
> 
> > Websites/perf.webkit.org/ChangeLog:12
> > +        * browser-tests/commit-log-viewer-tests.js: Fix a failed test case, while requesting the remote api, we should wait for those promises resolved and then wait for commponent to render.
> 
> Can we hard-wrap this really long line? Maybe after "the remote api,".

OK

> 
> > Websites/perf.webkit.org/init-database.sql:114
> > +    CONSTRAINT commit_revision_label_in_repository_must_be_unique UNIQUE(commit_repository, commit_revision_label));
> 
> Hm... it's counter-intuitive that label needs to be unique.
> Maybe we should call this commit_revision_identifier instead.
> Or maybe commit_string_identifier?
> 
> > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:13
> > +        $commit_rows = $db->query_and_fetch_all('SELECT commit_id FROM commits WHERE commit_repository = $1 AND ' . $column_name . ' = $2', array($repository_id, $revision));
> 
> Can we just do this: "SELECT commit_id FROM commits WHERE commit_repository
> = \$1 AND $column_name = \$2"
> 
> > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:20
> > +        $commit_rows = $db->query_and_fetch_all('SELECT commit_id FROM commits WHERE commit_repository = $1 AND ' . $column_name . ' LIKE $2 LIMIT 2', array($repository_id, Database::escape_for_like($revision) . '%'));
> 
> Ditto.
> 
> > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:198
> > +    private static function is_commit_revision_label($commit_revision_or_revision_label) {
> 
> Nit: { should appear on the next line.
> I know a lot of existing code doesn't follow this style guideline but we
> should do that in the new code.
> 
> > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:199
> > +        return preg_match('/^\d+@\S*$/', $commit_revision_or_revision_label);
> 
> I don't think \S is right. We don't wanna allow random symbols here
> including @.
> So I'd suggest we explicitly list character types: [A-Za-z0-9\.\-_]
> We also need a test for this (at least for @ and random other symbols like $
> and /)

OK, I am doing this because the git branch name are actually allow '@', it just won't allow '@' alone. I can make it fully worked as git ref name format describes.
see details with man git-check-ref-format

> 
> > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:213
> > +        $rows = $this->db->query_and_fetch_all('SELECT * FROM commits WHERE commit_repository = $1 AND commit_' . $column_name . ' LIKE $2 ORDER BY commit_'.$column_name.' LIMIT 2', array($repository_id, Database::escape_for_like($revision_prefix) . '%'));
> 
> Ditto about using double quotes.
> 
> > Websites/perf.webkit.org/public/include/commit-updater.php:161
> > +    private static function validate_own_commits(&$commit_info)
> 
> This seems like a subset of validate_commits.
> Please share the share code.
> 
> > Websites/perf.webkit.org/public/include/commit-updater.php:163
> > +        require_format('OwnCommitRevision', $commit_info['revision'], '/^[A-Za-z0-9 \.]+$/');
> 
> I don't think we need to differentiate ownCommitRevision vs. revision since
> we're reporting the revision.

OK, I am doing this for better error message for developer to debug. So that we can know which part of the data is wrong.

> 
> > Websites/perf.webkit.org/public/include/report-processor.php:167
> > +            $commit_data = array('repository' => $repository_id, 'revision' => $revision_data['revision'], 'time' => array_get($revision_data, 'timestamp'), 'revision_label' => array_get($revision_data, 'revisionLabel', null));
> 
> Please wrap the line after "'time' => array_get($revision_data,
> 'timestamp'),",
> and start the next line 4 spaces to the right of $commit_data. i.e.
> $commit_data = array('rep...,
>      'revision_label' =>
> 
OK
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:61
> > +        let formattedRevision = revision;
> >          if (parseInt(revision) == revision) // e.g. r12345
> > -            return 'r' + revision;
> > +            formattedRevision = 'r' + revision;
> >          if (revision.length == 40) // e.g. git hash
> > -            return revision.substring(0, 12);
> > -        return revision;
> > +            formattedRevision = revision.substring(0, 12);
> 
> Let's extract this logic as a helper function:
> _formattedRevision()
> {
>     const revision = this.revision();
>     if (parseInt(revision) == revision) // e.g. r12345
>         return 'r' + revision;
>     if (revision.length == 40) // e.g. git hash
>         return revision.substring(0, 12);
>     return revision;
> }
> 
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:81
> > +        let label = `${previousCommit.label()} - ${this.label()}`;
> 
> This is going to be very ugly for commit identifiers because we're gonna
> have to different numbers split by -.
> Let's do something better. Something like this:
> const toRevision = this._formattedRevision();
> const fromRevision = previousCommit._formattedRevision();
> const toRevisionLabel = this.revisionLabel();
> const fromRevisionLabel = previousCommit.revisionLabel();
> console.assert(!!toRevisionLabel == !!fromRevisionLabel);
> 
> const withRevisionLabel (label) => {
>     if (!toRevisionLabel || !fromRevisionLabel)
>         return label;
> 
>     const branchName = (revisionLabel) => (revisionLabel.match(/\@.+/) ||
> [''])[0].substring(1);
>     const toBranch = branchName(toRevisionLabel);
>     const fromBranch = branchName(fromRevisionLabel);
>     if (toBranch && toBranch == fromBranch)
>         fromRevisionLabel = fromRevisionLabel.substring(0,
> fromRevisionLabel.search(toBranch))
>     return `${fromRevisionLabel}-${toRevisionLabel} (${label})`;
> }
> if (parseInt(fromRevision) == fromRevision && parseInt(toRevision) ==
> toRevision)
>     return withRevisionLabel(`{fromRevision}-${toRevision}`);
> if (fromRevision.length == 40 && toRevision.length == 40)
>     return withRevisionLabel(`{fromRevision}:${toRevision}`);
> return `{fromRevision} - ${toRevision}`;
> 
> Note that we need to keep a space around for things like OS versions.
> 
> > Websites/perf.webkit.org/unit-tests/commit-log-tests.js:278
> > +                label: '175605@main (r200574) - 184276@main (r200805)',
> 
> With the suggested change above, this should become:
> '175605-184276@main (r200574-r200805)'
> 
> > Websites/perf.webkit.org/unit-tests/commit-log-tests.js:290
> > +        });
> 
> Please add a test for Git hashes with commit identifiers & rebaseline
> existing test cases.

OK

[Ryosuke Niwa]

(In reply to Zhifei Fang from comment #38)
> > > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:199
> > > +        return preg_match('/^\d+@\S*$/', $commit_revision_or_revision_label);
> > 
> > I don't think \S is right. We don't wanna allow random symbols here
> > including @.
> > So I'd suggest we explicitly list character types: [A-Za-z0-9\.\-_]
> > We also need a test for this (at least for @ and random other symbols like $
> > and /)
> 
> OK, I am doing this because the git branch name are actually allow '@', it
> just won't allow '@' alone. I can make it fully worked as git ref name
> format describes.
> see details with man git-check-ref-format

Are we planning to do that in our commit identifiers? Since commit identifier is our own format, we can put more restrictions on it.
Since we've had a number of regressions in the past where we started submitting erroneous revision numbers in the past, I'd rather enforce the strictest format we can enforce. It's easy to loosen it up when a new character is introduced but it's harder to fix the database entries once bad data creeps in.

> > > Websites/perf.webkit.org/public/include/commit-updater.php:163
> > > +        require_format('OwnCommitRevision', $commit_info['revision'], '/^[A-Za-z0-9 \.]+$/');
> > 
> > I don't think we need to differentiate ownCommitRevision vs. revision since
> > we're reporting the revision.
> 
> OK, I am doing this for better error message for developer to debug. So that
> we can know which part of the data is wrong.

I don't think that's necessary. Definitely not worth duplicating all this code.

[Zhifei Fang]

(In reply to Ryosuke Niwa from comment #39)
> (In reply to Zhifei Fang from comment #38)
> > > > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:199
> > > > +        return preg_match('/^\d+@\S*$/', $commit_revision_or_revision_label);
> > > 
> > > I don't think \S is right. We don't wanna allow random symbols here
> > > including @.
> > > So I'd suggest we explicitly list character types: [A-Za-z0-9\.\-_]
> > > We also need a test for this (at least for @ and random other symbols like $
> > > and /)
> > 
> > OK, I am doing this because the git branch name are actually allow '@', it
> > just won't allow '@' alone. I can make it fully worked as git ref name
> > format describes.
> > see details with man git-check-ref-format
> 
> Are we planning to do that in our commit identifiers? Since commit
> identifier is our own format, we can put more restrictions on it.
> Since we've had a number of regressions in the past where we started
> submitting erroneous revision numbers in the past, I'd rather enforce the
> strictest format we can enforce. It's easy to loosen it up when a new
> character is introduced but it's harder to fix the database entries once bad
> data creeps in.

Right, I agreed that for now we only have those in our branch names, I mention this because currently we defined commit identifier as <number>@<branch name>, I am OK with that we put more restriction first and then make it lose when we have something special.
> 
> > > > Websites/perf.webkit.org/public/include/commit-updater.php:163
> > > > +        require_format('OwnCommitRevision', $commit_info['revision'], '/^[A-Za-z0-9 \.]+$/');
> > > 
> > > I don't think we need to differentiate ownCommitRevision vs. revision since
> > > we're reporting the revision.
> > 
> > OK, I am doing this for better error message for developer to debug. So that
> > we can know which part of the data is wrong.
> 
> I don't think that's necessary. Definitely not worth duplicating all this
> code.

[Zhifei Fang]

Created attachment 424098 [details]
Patch

[Zhifei Fang]

Any more review comments ?

[Ryosuke Niwa]

Comment on attachment 424098 [details]
Patch

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

r=me assuming the changes suggested below are made.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:13
> +        $commit_rows = $db->query_and_fetch_all("SELECT commit_id FROM commits WHERE commit_repository = $1 AND $column_name  = $2", array($repository_id, $revision));

Nit: two spaces between $column_name and =.

> Websites/perf.webkit.org/public/include/commit-updater.php:112
> +                self::validate_commits($owned_commit_info, true);

Nice! This is way better than the old patch with code duplication.

> Websites/perf.webkit.org/public/include/commit-updater.php:161
> +    private static function validate_commits(&$commit_info, $is_own_commit=false)

Nit: spaces around =. Since this is not python, we follow the usual WebKit coding style.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:55
> +    static getRevisionType(revision)

This should be _repositoryType.
We don't use "get" prefix unless it has an out argument:
https://webkit.org/code-style-guidelines/#names-setter-getter

> Websites/perf.webkit.org/public/v3/models/commit-log.js:63
> +    static getformatttedRevision(revision, revisionIdentifier=null) 

Nit: Always use camelCase. Also need spaces around =.
But in this case, this should be called _formattedRevision instead.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:65
> +        let formattedRevision = revision;

I don't think we need this variable.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:68
> +            formattedRevision = 'r' + revision;

Use return.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:70
> +            formattedRevision = revision.substring(0, 12);

Use return.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:76
> +    revisionIdentifierParts() { 

Nit: { should be on the next line.
Also, this should be probably called _revisionIdentifier.
But this shouldn't really be a separate method since there is no utility outside diff at least for now.
So this should really be a lambda function inside diff.
But defining a method for this is really an overkill.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:77
> +        const revisionIdentifier =  this.revisionIdentifier();

Nit: two spaces between after =.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:80
> +        const identifierRe = /(?<number>\d+)@(?<branch>[\w\.\-]+)/;

There is no need to have this local variable.
Just do: /~/.exec

> Websites/perf.webkit.org/public/v3/models/commit-log.js:100
> +        const toRevisionIdentifierParts = this.revisionIdentifierParts();
> +        const fromRevisionIdentifierParts = previousCommit.revisionIdentifierParts();

These are really long names! How about toIdentifier & fromIdentifier or toMatch & fromMatch

> Websites/perf.webkit.org/public/v3/models/commit-log.js:116
> +        let connector = ' - ';
> +        if (revisionType == 'git')
> +            connector = '..';
> +        if (revisionType == 'svn')
> +            connector = '-';
> +        let label = `${previousCommit.label()}${connector}${this.label()}`;
> +        if (fromRevisionIdentifierParts || toRevisionIdentifierParts) {
> +            if (fromRevisionIdentifierParts && toRevisionIdentifierParts) {
> +                console.assert(fromRevisionIdentifierParts.branch == toRevisionIdentifierParts.branch);
> +                label = `${fromRevisionIdentifierParts.number} - ${toRevisionIdentifierParts.number}@${fromRevisionIdentifierParts.branch} (${CommitLog.getformatttedRevision(fromRevision)}${connector}${CommitLog.getformatttedRevision(toRevision)})`
> +            } else {
> +                connector = ' - ';
> +                label = `${previousCommit.label()}${connector}${this.label()}`;
> +            }
> +        }

This should really be a lambda.
Also, if we're gonna just assert that branches are same, we should just use early return,
which is WebKit's preferred style.

const identifierPattern = /^(?<number>\d+)\@(?<branch>.+)$/;
const label = ((fromMatch, toMatch) => {
    const separator = repositoryType == 'git' ? '..' : (repositoryType == 'svn' ? '-' : ' - ');
    if (fromMatch && toMatch) {
        console.assert(fromRevisionIdentifierParts.branch == toRevisionIdentifierParts.branch);
        return `${fromRevisionIdentifierParts.number}-${toRevisionIdentifierParts.number}@${fromRevisionIdentifierParts.branch} (${CommitLog.getformatttedRevision(fromRevision)}${connector}${CommitLog.getformatttedRevision(toRevision)})`;
    }
    if (fromMatch || toMatch)
        return `${previousCommit.label()}$ - ${this.label()}`;
    return `${previousCommit.label()}${connector}${this.label()}`;
})(identifierPattern.exec(previousCommit.revisionIdentifier()), identifierPattern.exec(this.revisionIdentifier()));

> Websites/perf.webkit.org/unit-tests/commit-log-tests.js:289
> +                label: '175605 - 184276@main (r200574-r200805)',

This looks wrong. It should be: 175605-184276@main (r200574-r200805)

[Zhifei Fang]

Created attachment 424429 [details]
Patch

[Zhifei Fang]

Created attachment 424430 [details]
Patch

[Zhifei Fang]

(In reply to Ryosuke Niwa from comment #43)

I have upload a new one with some explanation for some comments I don't take
> Comment on attachment 424098 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424098&action=review
> 
> r=me assuming the changes suggested below are made.
> 
> > Websites/perf.webkit.org/public/include/commit-log-fetcher.php:13
> > +        $commit_rows = $db->query_and_fetch_all("SELECT commit_id FROM commits WHERE commit_repository = $1 AND $column_name  = $2", array($repository_id, $revision));
> 
> Nit: two spaces between $column_name and =.

Got it.
> 
> > Websites/perf.webkit.org/public/include/commit-updater.php:112
> > +                self::validate_commits($owned_commit_info, true);
> 
> Nice! This is way better than the old patch with code duplication.
> 
> > Websites/perf.webkit.org/public/include/commit-updater.php:161
> > +    private static function validate_commits(&$commit_info, $is_own_commit=false)
> 
> Nit: spaces around =. Since this is not python, we follow the usual WebKit
> coding style.
> 
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:55
> > +    static getRevisionType(revision)
> 
> This should be _repositoryType.
> We don't use "get" prefix unless it has an out argument:
> https://webkit.org/code-style-guidelines/#names-setter-getter
> 
Got it 
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:63
> > +    static getformatttedRevision(revision, revisionIdentifier=null) 
> 
> Nit: Always use camelCase. Also need spaces around =.
> But in this case, this should be called _formattedRevision instead.
> 
Got it
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:65
> > +        let formattedRevision = revision;
> 
> I don't think we need this variable.
> 

The reason here we do have an extra variable is to share this function with label and diff. For label, we want to it to display something like 1234@main (r1234), how ever for the diff, we want to it to display 1234-1235@main (r1234-r1235), note the following parts in the bracket r1234-r1235 we don't want the revision identifier prefix, so we can simply don't pass revision identifier to the _formatttedRevision, it will only gives the r1234 part back. For label, we pass with the revision identifier, so this function will add the revision identifier prefix, directly return will be wrong. 

> > Websites/perf.webkit.org/public/v3/models/commit-log.js:68
> > +            formattedRevision = 'r' + revision;
> 
> Use return.
> 
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:70
> > +            formattedRevision = revision.substring(0, 12);
> 
> Use return.
> 
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:76
> > +    revisionIdentifierParts() { 
> 
> Nit: { should be on the next line.
> Also, this should be probably called _revisionIdentifier.
> But this shouldn't really be a separate method since there is no utility
> outside diff at least for now.
> So this should really be a lambda function inside diff.
> But defining a method for this is really an overkill.

Yeah, I agree for this patch to make a function seems an overkill, but I have feeling that we will use the first part of revision identifier a lot soon, for example perform bi-selection when the svn revision becomes git hashes. So that's why I make it a function.
> 
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:77
> > +        const revisionIdentifier =  this.revisionIdentifier();
> 
> Nit: two spaces between after =.

Got it
> 
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:80
> > +        const identifierRe = /(?<number>\d+)@(?<branch>[\w\.\-]+)/;
> 
> There is no need to have this local variable.
> Just do: /~/.exec
> 
got it
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:100
> > +        const toRevisionIdentifierParts = this.revisionIdentifierParts();
> > +        const fromRevisionIdentifierParts = previousCommit.revisionIdentifierParts();
> 
> These are really long names! How about toIdentifier & fromIdentifier or
> toMatch & fromMatch

got it
> 
> > Websites/perf.webkit.org/public/v3/models/commit-log.js:116
> > +        let connector = ' - ';
> > +        if (revisionType == 'git')
> > +            connector = '..';
> > +        if (revisionType == 'svn')
> > +            connector = '-';
> > +        let label = `${previousCommit.label()}${connector}${this.label()}`;
> > +        if (fromRevisionIdentifierParts || toRevisionIdentifierParts) {
> > +            if (fromRevisionIdentifierParts && toRevisionIdentifierParts) {
> > +                console.assert(fromRevisionIdentifierParts.branch == toRevisionIdentifierParts.branch);
> > +                label = `${fromRevisionIdentifierParts.number} - ${toRevisionIdentifierParts.number}@${fromRevisionIdentifierParts.branch} (${CommitLog.getformatttedRevision(fromRevision)}${connector}${CommitLog.getformatttedRevision(toRevision)})`
> > +            } else {
> > +                connector = ' - ';
> > +                label = `${previousCommit.label()}${connector}${this.label()}`;
> > +            }
> > +        }
> 
> This should really be a lambda.
> Also, if we're gonna just assert that branches are same, we should just use
> early return,
> which is WebKit's preferred style.
> 
> const identifierPattern = /^(?<number>\d+)\@(?<branch>.+)$/;
> const label = ((fromMatch, toMatch) => {
>     const separator = repositoryType == 'git' ? '..' : (repositoryType ==
> 'svn' ? '-' : ' - ');
>     if (fromMatch && toMatch) {
>         console.assert(fromRevisionIdentifierParts.branch ==
> toRevisionIdentifierParts.branch);
>         return
> `${fromRevisionIdentifierParts.number}-${toRevisionIdentifierParts.
> number}@${fromRevisionIdentifierParts.branch}
> (${CommitLog.getformatttedRevision(fromRevision)}${connector}${CommitLog.
> getformatttedRevision(toRevision)})`;
>     }
>     if (fromMatch || toMatch)
>         return `${previousCommit.label()}$ - ${this.label()}`;
>     return `${previousCommit.label()}${connector}${this.label()}`;
> })(identifierPattern.exec(previousCommit.revisionIdentifier()),
> identifierPattern.exec(this.revisionIdentifier()));
> 
> > Websites/perf.webkit.org/unit-tests/commit-log-tests.js:289
> > +                label: '175605 - 184276@main (r200574-r200805)',
> 
> This looks wrong. It should be: 175605-184276@main (r200574-r200805)

[Ryosuke Niwa]

(In reply to Zhifei Fang from comment #46)
> > > Websites/perf.webkit.org/public/v3/models/commit-log.js:76
> > > +    revisionIdentifierParts() { 
> > 
> > Nit: { should be on the next line.
> > Also, this should be probably called _revisionIdentifier.
> > But this shouldn't really be a separate method since there is no utility
> > outside diff at least for now.
> > So this should really be a lambda function inside diff.
> > But defining a method for this is really an overkill.
> 
> Yeah, I agree for this patch to make a function seems an overkill, but I
> have feeling that we will use the first part of revision identifier a lot
> soon, for example perform bi-selection when the svn revision becomes git
> hashes. So that's why I make it a function.

If that's the case, let's add such a function when another use case like that comes up. In my experience, structuring code in the anticipation for something that may happen in the future often backfires.

[Zhifei Fang]

Created attachment 424433 [details]
Patch

[Zhifei Fang]

(In reply to Ryosuke Niwa from comment #47)
> (In reply to Zhifei Fang from comment #46)
> > > > Websites/perf.webkit.org/public/v3/models/commit-log.js:76
> > > > +    revisionIdentifierParts() { 
> > > 
> > > Nit: { should be on the next line.
> > > Also, this should be probably called _revisionIdentifier.
> > > But this shouldn't really be a separate method since there is no utility
> > > outside diff at least for now.
> > > So this should really be a lambda function inside diff.
> > > But defining a method for this is really an overkill.
> > 
> > Yeah, I agree for this patch to make a function seems an overkill, but I
> > have feeling that we will use the first part of revision identifier a lot
> > soon, for example perform bi-selection when the svn revision becomes git
> > hashes. So that's why I make it a function.
> 
> If that's the case, let's add such a function when another use case like
> that comes up. In my experience, structuring code in the anticipation for
> something that may happen in the future often backfires.

Ok, I removed that function, but out of curiosity, why such a function that is doing the right staff and only get called in one function will create a backfire?

[Ryosuke Niwa]

Comment on attachment 424430 [details]
Patch

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

> Websites/perf.webkit.org/public/v3/models/commit-log.js:55
> +    static repositoryType(revision)

Since this function doesn't have any external use, we should call this _repositoryType

> Websites/perf.webkit.org/public/v3/models/commit-log.js:61
> +    }

We should probably explicitly return null here.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:63
> +    static _formatttedRevision(revision, revisionIdentifier=null) 

Nit: Spaces around =.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:70
> +        let formattedRevision = revision;
> +        let repositoryType = CommitLog.repositoryType(revision);
> +        if (repositoryType == 'svn') // e.g. r12345
> +            formattedRevision =  'r' + revision;
> +        if (repositoryType == 'git') // e.g. git hash
> +            formattedRevision = revision.substring(0, 12);

In general, having a variable that gets updated later makes the code harder to follow
because we'd be creating an implicit dependency between different control statements.

Here, we should probably define a nested lambda like this:
const formattedRevision = (() => {
    if (repositoryType == 'svn') // e.g. r12345
        return 'r' + revision;
    if (repositoryType == 'git')
        return revision.substring(0, 12);
    return revision;
})();

Or better yet, we should just use a switch statement now that we have _repositoryType:
const formattedRevision = (() => {
    switch (this._repositoryType(revision)) {
    case 'svn':
        return 'r' + revision; // e.g. r12345
    case 'git':
        return revision.substring(0, 12);
    }
    return revision;
})();

I really wish we had switch expression like Rust or C#.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:72
> +            formattedRevision = `${revisionIdentifier} (${formattedRevision})`;

This should be an early return:
if (revisionIdentifier)
    return `${revisionIdentifier} (${formattedRevision})`;

> Websites/perf.webkit.org/public/v3/models/commit-log.js:76
> +    revisionIdentifierParts() 

Let's not add this function prematurely.
If we later find that the numerical part of a revision identifier is used often, then we should add a function to get that.
We should structure code based on real world use cases, not based on our anticipation.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:103
> +        let label = (() => {

const.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:107
> +                return `${fromMatch.number}-${toMatch.number}@${fromMatch.branch} (${CommitLog._formatttedRevision(fromRevision)}${separator}${CommitLog._formatttedRevision(toRevision)})`

This is a really long statement.
Maybe we should format revisions in a separate lines like this:
const formattedFrom = CommitLog._formatttedRevision(fromRevision);
const formattedTo = CommitLog._formatttedRevision(toRevision);

> Websites/perf.webkit.org/public/v3/models/commit-log.js:114
> +        return {repository: repository, label: label, url: repository.urlForRevisionRange(fromRevision, toRevision)};

Use shorthand: return {repository, label, url: repository.urlForRevisionRange(fromRevision, toRevision)};

[Ryosuke Niwa]

Comment on attachment 424433 [details]
Patch

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

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:11
> -        $commit_rows = $db->query_and_fetch_all('SELECT commit_id FROM commits WHERE commit_repository = $1 AND commit_revision = $2', array($repository_id, $revision));
> +        $column_name = CommitLogFetcher::is_commit_revision_identifier($revision) ? 'commit_revision_identifier' : 'commit_revision';

I'm actually having second thoughts that maybe we shouldn't detect revision identifier like this in the backend.
But we can address that sometime in the future.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:63
> +    static _formatttedRevision(revision, revisionIdentifier=null) 

Please address the review comment I just posted.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:95
> +                console.assert(fromMatch.groups.branch == toMatch.groups.branch);

Ditto.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:103
> +        return {repository: repository, label: label, url: repository.urlForRevisionRange(fromRevision, toRevision)};

Ditto.

[Zhifei Fang]

Created attachment 424437 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 424437 [details]
Patch

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

> Websites/perf.webkit.org/public/v3/models/commit-log.js:61
>      }

Like I pointed out earlier, we should return null explicitly here.

> Websites/perf.webkit.org/public/v3/models/commit-log.js:65
> +        let formattedRevision =  (() => {

Nit: use const. Two spaces after =

> Websites/perf.webkit.org/public/v3/models/commit-log.js:105
> +            return `${previousCommit.label()}${separator}${this.label()}`;

Actually, this is basically same as ${formattedFrom}${separator}${formattedTo} above.
So maybe it's better to do something like this:
        const label = ((fromMatch, toMatch) => {
            const separator = repositoryType == 'git' ? '..' : (repositoryType == 'svn' ? '-' : ' - ');
            const revisionRange = `${CommitLog._formatttedRevision(fromRevision)}${separator}${CommitLog._formatttedRevision(toRevision);}`;
            if (fromMatch && toMatch) {
                console.assert(fromMatch.groups.branch == toMatch.groups.branch);
                return `${fromMatch.groups.number}-${toMatch.groups.number}@${fromMatch.groups.branch} (${revisionRange})`;
            }
            if (fromMatch || toMatch)
                return `${previousCommit.label()} - ${this.label()}`;
            return revisionRange;
        })(identifierPattern.exec(previousCommit.revisionIdentifier()), identifierPattern.exec(this.revisionIdentifier()));

[Zhifei Fang]

Created attachment 424442 [details]
Patch

[Ryosuke Niwa]

(In reply to Zhifei Fang from comment #49)
>
> > If that's the case, let's add such a function when another use case like
> > that comes up. In my experience, structuring code in the anticipation for
> > something that may happen in the future often backfires.
> 
> Ok, I removed that function, but out of curiosity, why such a function that
> is doing the right staff and only get called in one function will create a
> backfire?

Because it adds more code and indirection. Adding any unnecessary abstraction will make the code harder to maintain by separating different pieces of code that are related / dependent.

In this particular case, it's not immediately obvious what "parts" are in "revisionIdentifierParts". This is not a concept readily defined anywhere. If we're having a hard time coming up with a self-explanatory function name, it usually means we have a bad abstraction / interface. If, on the other hand, we're only interested in the leading digits, then we should be just calling parseInt instead of running a full regular expression.

[Ryosuke Niwa]

Comment on attachment 424442 [details]
Patch

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

r=me but it seems like this patch needs be updated for ToT revision?

> Websites/perf.webkit.org/public/v3/models/commit-log.js:61
> +            return 'git';
>      }

Again, we should return null here.

> Websites/perf.webkit.org/server-tests/api-report-tests.js:394
> +        for (let repository of repositories)

Nit: const.

> Websites/perf.webkit.org/server-tests/api-report-tests.js:398
> +        for (let commit of commits)

Nit: const.

[Zhifei Fang]

Created attachment 424566 [details]
Patch

[Zhifei Fang]

Created attachment 424796 [details]
Patch

[Zhifei Fang]

Created attachment 424813 [details]
Patch

[Zhifei Fang]

Created attachment 424815 [details]
Patch for landing

[EWS]

Found 1 new test failure: media/media-fragments/TC0051.html

[EWS]

Committed r275329: <https://commits.webkit.org/r275329>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424815 [details].