Bug 224519 - [results.webkit.org] Fully report git commits
Summary: [results.webkit.org] Fully report git commits
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-13 17:25 PDT by Jonathan Bedard
Modified: 2021-04-15 12:08 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.74 KB, patch)
2021-04-13 17:30 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (4.94 KB, patch)
2021-04-15 11:17 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2021-04-13 17:25:56 PDT
Git commits in a local checkout are fully defined, we shouldn't rely on the results database defining partial commits for us.
Comment 1 Radar WebKit Bug Importer 2021-04-13 17:26:13 PDT
<rdar://problem/76619007>
Comment 2 Jonathan Bedard 2021-04-13 17:30:47 PDT
Created attachment 425935 [details]
Patch
Comment 3 Aakash Jain 2021-04-15 07:00:39 PDT
Comment on attachment 425935 [details]
Patch

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

> Tools/Scripts/webkitpy/port/base.py:1530
> +            # Don't rely on the results database to define git commits

what does this mean?

> Tools/Scripts/webkitpy/port/base.py:-1532
> -            # Special case for WebKit since we have multiple representations at the moment

Are we removing this special case?
Comment 4 Jonathan Bedard 2021-04-15 08:49:52 PDT
Comment on attachment 425935 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/base.py:1530
>> +            # Don't rely on the results database to define git commits
> 
> what does this mean?

if you send the results database a partially-defined commit (ie, only the commit hash and repository_id), the results database will talk to whatever backend it has defined for the specific repository (GitHub + svn.webkit.org, for example) to determine the author/timestamp/identifier/etc. However, if we have a Git checkout, all of this information is accessible locally, so we might as well just report fully defined commits (ie, commits with populated author/timestamp/identifier) in the first place. The reason we didn't do this originally is because getting timestamp and commit message in Subversion is a network call anyways.

>> Tools/Scripts/webkitpy/port/base.py:-1532
>> -            # Special case for WebKit since we have multiple representations at the moment
> 
> Are we removing this special case?

Because the representation of WebKit on the backend now handles this. Previously, our representation of WebKit was purely Subversion, and had no knowledge of what a git commit was, and so didn't know that "master" and "trunk" were the same branch. However, since the new WebKit repository representation is aware of Git, it will correctly map trunk, main and master as default branches.
Comment 5 Aakash Jain 2021-04-15 09:08:25 PDT
(In reply to Jonathan Bedard from comment #4)
> >> Tools/Scripts/webkitpy/port/base.py:1530
> >> +            # Don't rely on the results database to define git commits
> > 
> > what does this mean?
> 
> if you send the results database a partially-defined commit (ie, only the
> commit hash and repository_id), the results database will talk to whatever
> backend it has defined for the specific repository (GitHub + svn.webkit.org,
> for example) to determine the author/timestamp/identifier/etc. However, if
> we have a Git checkout, all of this information is accessible locally, so we
> might as well just report fully defined commits (ie, commits with populated
> author/timestamp/identifier) in the first place. The reason we didn't do
> this originally is because getting timestamp and commit message in
> Subversion is a network call anyways.

ok, the comments needs to be updated to be more clear.


> Because the representation of WebKit on the backend now handles this.
ok
Comment 6 Jonathan Bedard 2021-04-15 11:17:02 PDT
Created attachment 426119 [details]
Patch for landing
Comment 7 EWS 2021-04-15 12:08:08 PDT
Committed r276038 (236582@main): <https://commits.webkit.org/236582@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426119 [details].