Format revisions for display according to repository type

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jason Marcell]

Add special logic for Git revisions in the popover

[Jason Marcell]

Created attachment 270547 [details]
Patch

[Alexey Proskuryakov]

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]

Created attachment 270610 [details]
Patch

[Jason Marcell]

(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 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 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]

Landed: https://trac.webkit.org/changeset/196109