Bug 215960 – [webkitscmpy] Add root, branch and remote for local SCM repositories

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

[Jonathan Bedard]

Start the task of creating local SCM objects to represent git and svn checkouts. The goal here is create a shared API between SVN and Git, along with remote representations of those same repositories.

[Radar WebKit Bug Importer]

<rdar://problem/67968919>

[Jonathan Bedard]

Created attachment 407517 [details]
Patch

[Jonathan Bedard]

Created attachment 407608 [details]
Patch

[Zhifei Fang]

Comment on attachment 407608 [details]
Patch

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

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:42
> +    def root(self):

Not sure if "root" is the right name, this is returning top level directory path of a working tree (git) or a working copy (svn), maybe top_level_path / root_path ?
Besides do we consider to make this an argument ?

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:50
> +        result = run([self.executable, 'rev-parse', '--abbrev-ref', 'HEAD'], cwd=self.root, capture_output=True, encoding='utf-8')

This won't work in detached head state, for example you directly checkout with commit hash, we need to detect this state, otherwise it will return "HEAD" instead of an empty string, "git branch --show-current" will return an empty string, and should make this method failed quickly

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:33
> +        self.remote = remote or '[e-mail]:/mock/{}'.format(os.path.basename(path))

maybe we shouldn't use GitHub in the mock path

[Jonathan Bedard]

Comment on attachment 407608 [details]
Patch

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

>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:42
>> +    def root(self):
> 
> Not sure if "root" is the right name, this is returning top level directory path of a working tree (git) or a working copy (svn), maybe top_level_path / root_path ?
> Besides do we consider to make this an argument ?

The point of this is so that if a user constructs a repository at a location, we know what the root of the working tree is (basically, making it so that "Git(os.path.dirname(__file__))" works.

[Jonathan Bedard]

Created attachment 407630 [details]
Patch

Comment on attachment 407630 [details]
Patch

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

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:32
> +        self.cached = cached

What's the purpose of this variable? Why do we want a memoizer that does not cache result?

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:37
> +            timeout = kwargs.get('timeout', self.timeout)
> +            cached = kwargs.get('cached', self.cached)

What happens if the wrapped function actually uses `timeout` and `cached` for different purpose?

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:48
> +            cache = function(*args)

Also, I think we should pass '**kwargs' here?

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:53
> +        decorator.clear = lambda: self.clear()

Why not just decorator.clear = self.clear?

[Jonathan Bedard]

Comment on attachment 407630 [details]
Patch

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

>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:32
>> +        self.cached = cached
> 
> What's the purpose of this variable? Why do we want a memoizer that does not cache result?

We may want a memoizer which does not cache by default. That would allow you to have a function call which explicitly opted in to the cached behavior. It's basically the reverse of what I would imagine most usages of the class will be: default caching, except for some calls which opt out of caching.

We don't have to keep this, it just seemed silly not to given how decorator was implemented.

>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:37
>> +            cached = kwargs.get('cached', self.cached)
> 
> What happens if the wrapped function actually uses `timeout` and `cached` for different purpose?

We would be passing those arguments in both spots.

Suppose we should probably pop them instead, so they don't get forwarded.

>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:48
>> +            cache = function(*args)
> 
> Also, I think we should pass '**kwargs' here?

Oops, yes!

[Jonathan Bedard]

Created attachment 407833 [details]
Patch

Comment on attachment 407833 [details]
Patch

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

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:46
> +            if cache:
> +                return cache

Does this mean a function that returns None will always not cached even when 'None' is a valid return of the function?
We may want to differentiate that in the code.

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/tests/decorators_unittest.py:31
> +

It would be great to test a function that throws an exception and the exception should be raised as expected.

[Jonathan Bedard]

Comment on attachment 407833 [details]
Patch

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

>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:46
>> +                return cache
> 
> Does this mean a function that returns None will always not cached even when 'None' is a valid return of the function?
> We may want to differentiate that in the code.

Oops! Yes it does

[Jonathan Bedard]

Created attachment 407923 [details]
Patch

[Jonathan Bedard]

Created attachment 408007 [details]
Patch for landing

[EWS]

Committed r266628: <https://trac.webkit.org/changeset/266628>

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