Bug 66252 – garden-o-matic frontend needs model.commitDataForRevisionRange function.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dimitri Glazkov (Google)]

garden-o-matic frontend needs model.getCommitDataList function.

[Dimitri Glazkov (Google)]

Created attachment 103949 [details]
Patch

[Adam Barth]

Comment on attachment 103949 [details]
Patch

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

I think the main question in this patch is what behavior we want when oldestRevision or newestRevision isn't in model.state.recentCommits.  This patch returns [], but another reasonable behavior is to return whatever commitData we have that falls inside that range.  For example, when you compute an implied impliedFirstFailingRevision by adding one to newestPassingRevision, you're computing a revision number that might not exist in model.state.recentCommits (e.g, because it was a revision that went into a branch other than trunk).  I'd imagine you'd want this function to return whatever commitData objects we have for the regression range rather than returning the empty list.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:118
> +model.getCommitDataList = function(oldestRevision, newestRevision)

I would have called this commitDataListForRevisionRange to be explicit about the types, but this is fine too.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:121
> +    var commitData = g_commitIndex[oldestRevision];

What if commitData is undefined?  This will return [] rather than whatever commitDatas we have in the range.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:122
> +    var index = model.state.recentCommits.indexOf(commitData);

Is there a reason to use g_commitIndex?  indexOf requires a linear scan over recentCommits anyway.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:126
> +    while(commitData.revision != newestRevision) {

commitData.revision <= newestRevision

The caller might supply a newestRevision that doesn't happen to be in our dataset (e.g., because they added 1 to a rev that was, and the computed rev actually went into a branch).  With the test written this way, we'll walk off the beginning of model.state.recentCommits and return [].

[Dimitri Glazkov (Google)]

Comment on attachment 103949 [details]
Patch

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

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:118
>> +model.getCommitDataList = function(oldestRevision, newestRevision)
> 
> I would have called this commitDataListForRevisionRange to be explicit about the types, but this is fine too.

I'll rename.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/model.js:122
>> +    var index = model.state.recentCommits.indexOf(commitData);
> 
> Is there a reason to use g_commitIndex?  indexOf requires a linear scan over recentCommits anyway.

You're right -- I'll rework the algorithm.

[Dimitri Glazkov (Google)]

Created attachment 103961 [details]
Patch

[Adam Barth]

Comment on attachment 103961 [details]
Patch

Looks great.  Thanks.

[Dimitri Glazkov (Google)]

Committed r93063: <http://trac.webkit.org/changeset/93063>