Bug 224890 - [webkitcorepy] Add API to efficiently create a sequence of commits
Summary: [webkitcorepy] Add API to efficiently create a sequence of 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-21 13:13 PDT by Jonathan Bedard
Modified: 2021-05-06 11:32 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 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.
Comment 1 Radar WebKit Bug Importer 2021-04-21 13:13:53 PDT
<rdar://problem/76975733>
Comment 2 Jonathan Bedard 2021-04-21 13:15:39 PDT
Created attachment 426736 [details]
Patch
Comment 3 Jonathan Bedard 2021-04-27 13:49:42 PDT
Created attachment 427192 [details]
Patch
Comment 4 Jonathan Bedard 2021-04-27 21:27:39 PDT
Created attachment 427234 [details]
Patch
Comment 5 Jonathan Bedard 2021-05-04 14:43:12 PDT
Created attachment 427702 [details]
Patch
Comment 6 Dean Johnson 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
```
Comment 7 Jonathan Bedard 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.
Comment 8 Jonathan Bedard 2021-05-04 17:00:50 PDT
Created attachment 427713 [details]
Patch
Comment 9 Aakash Jain 2021-05-06 08:59:51 PDT
rs=me
Comment 10 Jonathan Bedard 2021-05-06 10:37:32 PDT
Created attachment 427905 [details]
Patch for landing
Comment 11 EWS 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].