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-

Description Jason Marcell 2016-02-02 21:19:35 PST
Add special logic for Git revisions in the popover
Comment 1 Jason Marcell 2016-02-02 21:29:14 PST
Created attachment 270547 [details]
Patch
Comment 2 Alexey Proskuryakov 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)?
Comment 3 Jason Marcell 2016-02-03 17:02:19 PST
Created attachment 270610 [details]
Patch
Comment 4 Jason Marcell 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.
Comment 5 Alexey Proskuryakov 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);
Comment 6 Jason Marcell 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.
Comment 7 Jason Marcell 2016-02-03 22:08:49 PST
Landed: https://trac.webkit.org/changeset/196109