<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Teach dashboard code to compare non-integer revisions."
href="https://bugs.webkit.org/show_bug.cgi?id=152345#c10">Comment # 10</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Teach dashboard code to compare non-integer revisions."
href="https://bugs.webkit.org/show_bug.cgi?id=152345">bug 152345</a>
from <span class="vcard"><a class="email" href="mailto:jmarcell@apple.com" title="Jason Marcell <jmarcell@apple.com>"> <span class="fn">Jason Marcell</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=267593&action=diff" name="attach_267593" title="Patch">attachment 267593</a> <a href="attachment.cgi?id=267593&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=267593&action=review">https://bugs.webkit.org/attachment.cgi?id=267593&action=review</a>
<span class="quote">>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:276
>> + return undefined;
>
> Can you give an example where we would not have a revision for a repository?</span >
Ideally we shouldn't, but I believe I have seen instances where the defaultBranches() method of a subclass of Buildbot provided branches for a queue/iteration that were not involved in the build. I can look into this further.
<span class="quote">>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:95
>> + if (!trac.latestRecordedRevisionNumber || comparison === undefined || comparison > 0) {
>
> Can you give an example when comparison === undefined?</span >
When the dashboard code first starts up, the Trac instances pull some minimum amount of historical data. The revision(s) in question aren't necessarily present in the "recordCommits" array at first. If they are not in the "recordedCommits" array yet, then we have no way of reasoning about the comparison of the two commits in question. What, then, should I return? I chose to return "undefined".
<span class="quote">>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:100
>> + totalRevisionsBehind += trac.commitsOnBranch(branch.name, function(commit, index, arr) { return index > arr.objectIndexOf(latestProductiveRevisionNumber, "revisionNumber"); }).length;
>
> The name arr is not a good name for the array of commits. I am assuming arr is short for "array". Regardless, neither "arr" nor "array" are good names because the former is an abbreviation of the latter (and I do not find it more canonical than the word "array") and the latter is a generic term that does not convey anything about the contents of the array. Maybe a better name would be commits or allCommits?</span >
It's actually comparing the individual "commit" to the Trac's internal "recordedCommits" array. I'll use the name "recordedCommits" here to remain consistent.
<span class="quote">>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:266
>> + var indexB = this.recordedCommits.objectIndexOf(b, "revisionNumber");
>
> Can you elaborate on when a commit would not have a revision?</span >
The Trac class loads commits incrementally. At first, the revision in question may not be in the recordedCommits array. If that's the case we "loadMoreHistoricalData" and it will be there the next time.
<span class="quote">>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:269
>> + return undefined;
>
> This does not seem like the correct approach. Having a comparison function fetch more data and have a polymorphic return type is weird.</span >
I'm open to changing this. I agree that side effects in functions should generally be avoided. Perhaps we can shift the loading of more data out to the call site of this function.
I might disagree with you, however, on the decision to return "undefined". We need a way to convey that we have an invalid comparison. When the dashboard code first starts up, the Trac instances pull some minimum amount of historical data. The revision(s) in question aren't necessarily present in the "recordCommits" array at first. If they are not in the "recordedCommits" array yet, then we have no way of reasoning about the comparison of the two commits in question. What, then, should I return? I chose to return "undefined".</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>