Bug 225985 – [git-webkit] Add setup function

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

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.

[Radar WebKit Bug Importer]

<rdar://problem/78226729>

[Jonathan Bedard]

Created attachment 429107 [details]
Patch

[Dean Johnson]

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?

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 434874 [details]
Patch

[Jonathan Bedard]

Comment on attachment 434874 [details]
Patch

An initial crack at a setup function which configures your WebKit checkout for interacting with GitHub

[Jonathan Bedard]

Created attachment 434922 [details]
Patch

[Jonathan Bedard]

Created attachment 435003 [details]
Patch

[Jonathan Bedard]

Created attachment 435024 [details]
Patch

[Jonathan Bedard]

Created attachment 435035 [details]
Patch

[Jonathan Bedard]

Created attachment 435043 [details]
Patch

Comment on attachment 435043 [details]
Patch

r=me

[EWS]

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