Bug 131415 – Improve support for SVN statuses in scm.py

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Matthew Hanson]

When using the webkitpy.common.checkout.scm module, it is often useful to determine the status of the current checkout. The SVN class provides a solution using regular expressions, but it is not very extensible and duplicates quite a bit of code. Since the status codes correspond to a well-defined bitmask-like syntax, we can do better than resorting to calls like svn_instance.run_status_and_extract_filenames(self.status_command(), self._status_regexp("?")) when something like  svn_instance.unversioned_files() would be much more clear.

[Matthew Hanson]

Created attachment 231854 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 231854 [details]
Patch

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

Do we really need to support all of them?

> Tools/Scripts/webkitpy/common/checkout/changelog.py:31
> +import codecs

Why do we need to import codecs now?

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:-241
> -    def added_files(self):
> -        return self.run_status_and_extract_filenames(self.status_command(), self._status_regexp("A"))
> -
> -    def deleted_files(self):
> -        return self.run_status_and_extract_filenames(self.status_command(), self._status_regexp("D"))
> -

Doesn't this break git?

[Matthew Hanson]

Comment on attachment 231854 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/checkout/changelog.py:31
>> +import codecs
> 
> Why do we need to import codecs now?

This is an artifact. I will remove it.

>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:-241
>> -
> 
> Doesn't this break git?

This does not break git, because the git implementation of status_command() is not `git st`, but `git diff --name-status --no-renames HEAD`

[Ryosuke Niwa]

Comment on attachment 231854 [details]
Patch

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

> Tools/Scripts/webkitpy/common/checkout/scm/status.py:6
> +        self.file_statuses = [FileStatus(line, checkout_root) for line in text.split("\n") if line]

We usually prefix private variables with _.

> Tools/Scripts/webkitpy/common/checkout/scm/status.py:45
> +class FileStatus(object):

FileStatus is an extremely generic name.  Please give it a more descriptive name.

> Tools/Scripts/webkitpy/common/checkout/scm/status.py:73
> +    # Status convenience methods

Can't we use __getattr__ to implement these instead?

> Tools/Scripts/webkitpy/common/checkout/scm/status.py:175
> +class Code(object):

Code is an extremely generic class name.  Please give it a more descriptive name.

> Tools/Scripts/webkitpy/common/checkout/scm/status.py:194
> +ItemReplaced = Code(1, "R", "Item has been replaced in your working "
> +                            "copy. This means the file was scheduled "
> +                            "for deletion, and then a new file with "
> +                            "the same name was scheduled for addition "
> +                            "in its place.")

We don't align text line this with indentations.  Please indent each line with exactly 4 spaces.
Also, these descriptions are split into so many lines.  We can probably fit this into much fewer lines.

[Matthew Hanson]

Comment on attachment 231854 [details]
Patch

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

I will send a v2 with Ryosuke's feedback incorporated.

> Tools/Scripts/webkitpy/common/checkout/scm/status.py:4
> +class CheckoutStatus(object):

SVNStyleCheckoutStatus

>> Tools/Scripts/webkitpy/common/checkout/scm/status.py:6
>> +        self.file_statuses = [FileStatus(line, checkout_root) for line in text.split("\n") if line]
> 
> We usually prefix private variables with _.

Noted. I will add the _ prefix.

>> Tools/Scripts/webkitpy/common/checkout/scm/status.py:45
>> +class FileStatus(object):
> 
> FileStatus is an extremely generic name.  Please give it a more descriptive name.

SVNStyleStatusCodeString

>> Tools/Scripts/webkitpy/common/checkout/scm/status.py:175
>> +class Code(object):
> 
> Code is an extremely generic class name.  Please give it a more descriptive name.

SVNStyleStatusCode

>> Tools/Scripts/webkitpy/common/checkout/scm/status.py:194
>> +                            "in its place.")
> 
> We don't align text line this with indentations.  Please indent each line with exactly 4 spaces.
> Also, these descriptions are split into so many lines.  We can probably fit this into much fewer lines.

Will do.

[Matthew Hanson]

Comment on attachment 231854 [details]
Patch

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

>>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:-241
>>> -
>> 
>> Doesn't this break git?
> 
> This does not break git, because the git implementation of status_command() is not `git st`, but `git diff --name-status --no-renames HEAD`

I will move the previous SCM implementation into git.py, so as to avoid any changes when using the scm module with Git clients.

>> Tools/Scripts/webkitpy/common/checkout/scm/status.py:4
>> +class CheckoutStatus(object):
> 
> SVNStyleCheckoutStatus

EDIT: SVNCheckoutStatus

>>> Tools/Scripts/webkitpy/common/checkout/scm/status.py:45
>>> +class FileStatus(object):
>> 
>> FileStatus is an extremely generic name.  Please give it a more descriptive name.
> 
> SVNStyleStatusCodeString

EDIT: SVNStatusLineItem

>> Tools/Scripts/webkitpy/common/checkout/scm/status.py:73
>> +    # Status convenience methods
> 
> Can't we use __getattr__ to implement these instead?

By this, I assume you mean that we should use the Dispatch pattern with __getattr__? We already have that, in essence, with the matches_code method. There is an alternative way to do this with setattr by defining a method like:

    def _define_matching_function(self, svn_status_code):
        def matching_function():
            return self.matches_status_code(svn_status_code)
        return matching_function

and then looping over all of the property constants and setting their snake_case representation to the function returned by _define_matching_function(). I find this hard to follow and unnecessarily complicated for a set of constants that will not be changing anytime soon.

Are you ok with keeping these properties explicitly defined?

>>> Tools/Scripts/webkitpy/common/checkout/scm/status.py:175
>>> +class Code(object):
>> 
>> Code is an extremely generic class name.  Please give it a more descriptive name.
> 
> SVNStyleStatusCode

EDIT: SVNStatusCode

[Matthew Hanson]

Created attachment 231979 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 231979 [details]
Patch

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

(In reply to comment #6)
>
> By this, I assume you mean that we should use the Dispatch pattern with __getattr__? We already have that, in essence, with the matches_code method. There is an alternative way to do this with setattr by defining a method like:
> 
>     def _define_matching_function(self, svn_status_code):
>         def matching_function():
>             return self.matches_status_code(svn_status_code)
>         return matching_function
> 
> and then looping over all of the property constants and setting their snake_case representation to the function returned by _define_matching_function(). I find this hard to follow and unnecessarily complicated for a set of constants that will not be changing anytime soon.
> 
> Are you ok with keeping these properties explicitly defined?

Why do they need to be actual properties?  Can't they just be method calls in call sites?
If we did that, then we can simply iterate through the list of status codes and check whether it matches or not.

Alternatively, why can't we just export status objects and let call sites manually call matches_status_code?

I don't like the fact we're listing every status code name almost thrice in SVNCheckoutStatus, SVNStatusLineItem, and for status code objects themselves.

> Tools/Scripts/webkitpy/common/checkout/scm/svn_status.py:78
> +    def _define_matching_function(self, svn_status_code):
> +        def matching_function():
> +            return self.matches_status_code(svn_status_code)
> +        return matching_function

Where is this used?

[Brent Fulgham]

Comment on attachment 231979 [details]
Patch

This patch is too old to apply now. Can you revise against current sources?

[Ahmad Saleem]

@Brent & @Ryosuke - do we need this considering we have now moved to Github? Thanks!

[Ryosuke Niwa]

Yeah, this is won't fix.