Bug 153818

Summary: Format revisions for display according to repository type
Product: WebKit Reporter: Jason Marcell <jmarcell>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, ap, dbates, dburkart, dean_johnson, dino, jmarcell, lforschler, matthew_hanson
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=152345
Attachments:
Description Flags
Patch
none
Patch ap: review+, ap: commit-queue-

Jason Marcell
Reported 2016-02-02 21:19:35 PST
Add special logic for Git revisions in the popover
Attachments
Patch (1.91 KB, patch)
2016-02-02 21:29 PST, Jason Marcell
no flags
Patch (5.54 KB, patch)
2016-02-03 17:02 PST, Jason Marcell
ap: review+
ap: commit-queue-
Jason Marcell
Comment 1 2016-02-02 21:29:14 PST
Alexey Proskuryakov
Comment 2 2016-02-02 22:11:25 PST
Comment on attachment 270547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270547&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:131 > + revision = commit.revisionNumber.substr(0, 7); Should there be a utility function with a helpful name for substr(0, 7)?
Jason Marcell
Comment 3 2016-02-03 17:02:19 PST
Jason Marcell
Comment 4 2016-02-03 17:06:14 PST
(In reply to comment #2) > Comment on attachment 270547 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270547&action=review > > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:131 > > + revision = commit.revisionNumber.substr(0, 7); > > Should there be a utility function with a helpful name for substr(0, 7)? I started to write a utility function just for the Git part but then I went another route and made a more generic function called _formatRevisionForDisplay that handles both the Git and the Subversion part. I also noticed another section of the codebase where we were doing almost the exact same thing so it ended up being useful there as well.
Alexey Proskuryakov
Comment 5 2016-02-03 21:42:36 PST
Comment on attachment 270610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270610&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:127 > - linkElement.textContent = "r" + commit.revisionNumber; > + linkElement.textContent = this._formatRevisionForDisplay(commit.revisionNumber, branch.repository); Is "revision number" the right terminology for git? Just curious, no need to update all the naming for this patch even if it's not quite right. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:339 > + _formatRevisionForDisplay: function (revision, repository) Please remove the space after "function". > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:347 > + if (repository.isSVN) > + return "r" + revision; > + else if (repository.isGit) > + // Truncating for display. Git traditionally uses seven characters for a short hash. > + return revision.substr(0, 7); > + else > + console.assert(false, "Should not get here; " + repository.name + " did not specify a known VCS type."); WebKit style is to not use else after return. Here is how I would write this: console.assert(repository.isSVN || repository.isGit); if (repository.isSVN) return "r" + revision; // Truncating for display. Git traditionally uses seven characters for a short hash. return revision.substr(0, 7);
Jason Marcell
Comment 6 2016-02-03 21:56:58 PST
Comment on attachment 270610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270610&action=review >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:127 >> + linkElement.textContent = this._formatRevisionForDisplay(commit.revisionNumber, branch.repository); > > Is "revision number" the right terminology for git? > > Just curious, no need to update all the naming for this patch even if it's not quite right. I've thought about that before. With the introduction of Git, the revision is a hash, which you might argue is not a number because we're working with it in string form everywhere. On the other hand, a Git hash is actually a hexadecimal number. I think maybe the term "revision" instead of "revision number" might be more appropriate. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:339 >> + _formatRevisionForDisplay: function (revision, repository) > > Please remove the space after "function". Will do. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:347 >> + console.assert(false, "Should not get here; " + repository.name + " did not specify a known VCS type."); > > WebKit style is to not use else after return. Here is how I would write this: > > console.assert(repository.isSVN || repository.isGit); > if (repository.isSVN) > return "r" + revision; > // Truncating for display. Git traditionally uses seven characters for a short hash. > return revision.substr(0, 7); Thanks. I'll use this.
Jason Marcell
Comment 7 2016-02-03 22:08:49 PST
Note You need to log in before you can comment on or make changes to this bug.