Bug 225985 - [git-webkit] Add setup function
Summary: [git-webkit] Add setup function
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: 228867
  Show dependency treegraph
 
Reported: 2021-05-19 15:07 PDT by Jonathan Bedard
Modified: 2021-08-06 15:16 PDT (History)
6 users (show)

See Also:


Attachments
Patch (14.61 KB, patch)
2021-05-19 15:59 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (28.42 KB, patch)
2021-08-03 17:34 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (11.59 KB, patch)
2021-08-04 12:10 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (31.23 KB, patch)
2021-08-05 10:33 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (32.50 KB, patch)
2021-08-05 14:12 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (32.80 KB, patch)
2021-08-05 16:04 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (32.84 KB, patch)
2021-08-05 17:24 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-05-19 15:07:18 PDT
The idea behind a 'setup' function is to provide a central location for us to configure git settings for a specific repository. Right now, the most important part of this function is setting the merge strategy, although more will be added in the future.
Comment 1 Radar WebKit Bug Importer 2021-05-19 15:08:02 PDT
<rdar://problem/78226729>
Comment 2 Jonathan Bedard 2021-05-19 15:59:40 PDT
Created attachment 429107 [details]
Patch
Comment 3 Dean Johnson 2021-05-21 14:11:50 PDT
Comment on attachment 429107 [details]
Patch

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

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:68
> +class hybridmethod(object):

Let's chat about this one on Monday.

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/terminal.py:44
> +        return default or options[0]

I don't think we should automatically choose any value other than the default, otherwise someone who provides an invalid answer to 'Yes' / 'No' would always get yes, even if they were required to enter something.

We also may want to consider a 'required' option where this can loop if we fail to parse the input from the user.

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:56
> +        if isinstance(context, type):

Is this trying to figure out if there is an instance of the class, or the object, because of @decorators.hybridmethod? This seems excessively complex -- can we implement this function with less abstractions?

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/setup.py:42
> +    def git(cls, args, repository, **kwargs):

I wonder if this code should instead be on the repository object, so SVN / Git / SCM-X could implement their own setup functions?
Comment 4 Jonathan Bedard 2021-05-21 14:33:12 PDT
Comment on attachment 429107 [details]
Patch

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

>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/terminal.py:44
>> +        return default or options[0]
> 
> I don't think we should automatically choose any value other than the default, otherwise someone who provides an invalid answer to 'Yes' / 'No' would always get yes, even if they were required to enter something.
> 
> We also may want to consider a 'required' option where this can loop if we fail to parse the input from the user.

Perhaps if no default is specified, then we loop until a matching answer is found?

>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:56
>> +        if isinstance(context, type):
> 
> Is this trying to figure out if there is an instance of the class, or the object, because of @decorators.hybridmethod? This seems excessively complex -- can we implement this function with less abstractions?

The essential problem here is that there exists both a global git config and a local git config, and we need access to both. The current implementation (using the hybrid method), makes this distinction by treating Git.config() and Git().config() differently, because that is the essence of what `git config -l` does, behave differently when in the global context vs the local context.

>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/setup.py:42
>> +    def git(cls, args, repository, **kwargs):
> 
> I wonder if this code should instead be on the repository object, so SVN / Git / SCM-X could implement their own setup functions?

I did think about this, but there really isn't a case for running the setup except through the command-line script. I didn't want to muddy the waters for other potentials clients of webkitscmpy, setup is a function that should only be preformed by a script explicitly called by the user.
Comment 5 Jonathan Bedard 2021-08-03 17:34:13 PDT
Created attachment 434874 [details]
Patch
Comment 6 Jonathan Bedard 2021-08-03 17:35:52 PDT
Comment on attachment 434874 [details]
Patch

An initial crack at a setup function which configures your WebKit checkout for interacting with GitHub
Comment 7 Jonathan Bedard 2021-08-04 12:10:12 PDT
Created attachment 434922 [details]
Patch
Comment 8 Jonathan Bedard 2021-08-05 10:33:19 PDT
Created attachment 435003 [details]
Patch
Comment 9 Jonathan Bedard 2021-08-05 14:12:38 PDT
Created attachment 435024 [details]
Patch
Comment 10 Jonathan Bedard 2021-08-05 16:04:36 PDT
Created attachment 435035 [details]
Patch
Comment 11 Jonathan Bedard 2021-08-05 17:24:14 PDT
Created attachment 435043 [details]
Patch
Comment 12 dewei_zhu 2021-08-06 15:08:33 PDT
Comment on attachment 435043 [details]
Patch

r=me
Comment 13 EWS 2021-08-06 15:16:41 PDT
Committed r280739 (240325@main): <https://commits.webkit.org/240325@main>

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