WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
224890
[webkitcorepy] Add API to efficiently create a sequence of commits
https://bugs.webkit.org/show_bug.cgi?id=224890
Summary
[webkitcorepy] Add API to efficiently create a sequence of commits
Jonathan Bedard
Reported
2021-04-21 13:13:35 PDT
Creating an individual commit (with an identifier) is relatively expensive. However, if we already have a fully defined commit, it is often the case that neighboring commits can be defined much more efficiently.
Attachments
Patch
(29.39 KB, patch)
2021-04-21 13:15 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch
(52.17 KB, patch)
2021-04-27 13:49 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch
(52.25 KB, patch)
2021-04-27 21:27 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch
(50.19 KB, patch)
2021-05-04 14:43 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch
(50.67 KB, patch)
2021-05-04 17:00 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch for landing
(50.68 KB, patch)
2021-05-06 10:37 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2021-04-21 13:13:53 PDT
<
rdar://problem/76975733
>
Jonathan Bedard
Comment 2
2021-04-21 13:15:39 PDT
Created
attachment 426736
[details]
Patch
Jonathan Bedard
Comment 3
2021-04-27 13:49:42 PDT
Created
attachment 427192
[details]
Patch
Jonathan Bedard
Comment 4
2021-04-27 21:27:39 PDT
Created
attachment 427234
[details]
Patch
Jonathan Bedard
Comment 5
2021-05-04 14:43:12 PDT
Created
attachment 427702
[details]
Patch
Dean Johnson
Comment 6
2021-05-04 16:14:59 PDT
Comment on
attachment 427702
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=427702&action=review
I like the changes so far -- the code definitely feels easier to read through. Left a few minor comments, but overall LGTM. Unofficial r+.
> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/scm_base.py:79 > + for arg, value in kwargs.items():
Can you give some example arguments we expect to end up in kwargs here? Is it possible for us to pass arguments directly called "begin" and "end" instead? I suspect including new data in **kwargs could end up having an unintended effect in the future.
> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:379 > + if commit.hash == previous[0].hash:
Could you add some comments for these if/elif/else blocks to help a reader better understand what's happening here, and why?
> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/svn.py:417 > + return dict(
Is it possible to refactor the Git function to return a similarly structured dictionary (defined in this way)? This function makes it really easy to understand what data we expect to be returned.
> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/svn.py:450 > + if line and line.rstrip() != '-' * 72:
This might be premature optimization, but maybe we should consider defining a variable / regex for `'-' * 72`? Currently it's producing that string on every line read.
> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:497 > + for branch, commits in self.commits.items():
It seems like when we're looking for `commits_in_range` / on the default branch we could avoid running this loop entirely. If we plan on changing this code in the future to make use of a hash => identifier lookup table, you can ignore this comment (but it would be good to mention it in the code).
> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:519 > + if previous and branch != self.default_branch:
Nit: It might be cleaner to write this as an early return, rather than nesting the if block: ``` if not previous or branch = self.default_branch: continue ```
Jonathan Bedard
Comment 7
2021-05-04 16:57:33 PDT
Comment on
attachment 427702
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=427702&action=review
>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/scm_base.py:79 >> + for arg, value in kwargs.items(): > > Can you give some example arguments we expect to end up in kwargs here? Is it possible for us to pass arguments directly called "begin" and "end" instead? I suspect including new data in **kwargs could end up having an unintended effect in the future.
I like the point about "begin" and "end" dictionaries, will change
>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:497 >> + for branch, commits in self.commits.items(): > > It seems like when we're looking for `commits_in_range` / on the default branch we could avoid running this loop entirely. If we plan on changing this code in the future to make use of a hash => identifier lookup table, you can ignore this comment (but it would be good to mention it in the code).
This is a helper function for our mock code. It's only ever gathering from an in-memory dictionary, and turning that dictionary into the output of various git commands so we can test against them.
Jonathan Bedard
Comment 8
2021-05-04 17:00:50 PDT
Created
attachment 427713
[details]
Patch
Aakash Jain
Comment 9
2021-05-06 08:59:51 PDT
rs=me
Jonathan Bedard
Comment 10
2021-05-06 10:37:32 PDT
Created
attachment 427905
[details]
Patch for landing
EWS
Comment 11
2021-05-06 11:32:48 PDT
Committed
r277102
(
237404@main
): <
https://commits.webkit.org/237404@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 427905
[details]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug