[webkit-reviews] review denied: [Bug 35498] check-webkit-style: Path-specific logic should work when invoking with files syntax : [Attachment 50617] Proposed patch 4

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied Chris Jerdonek
<cjerdonek at webkit.org>'s request for review:
Bug 35498: check-webkit-style: Path-specific logic should work when invoking
with files syntax
https://bugs.webkit.org/show_bug.cgi?id=35498

Attachment 50617: Proposed patch 4
https://bugs.webkit.org/attachment.cgi?id=50617&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
Sorry for the latency... I've been somewhat sick and busy these days.

Overall this change looks pretty good! But I believe we can improve this a bit.


I think it's nice to rename UnitTestLogStream in separate change. Also, I'm not
sure why TestLogStream is the better naming.

> -def configure_logging(stream, logger=None):
> +def configure_logging(stream, logger=None, logging_level=logging.INFO):

Why? It seems this change is used in this patch. If my understanding is
correct, I'm appreciated to see this change in separated patch.

> +def _rel_path(path, start_path, os_path_abspath=None):
> +    """Return a path relative to the given start path, or None.
> +
> +    Returns None if the path is not contained in the directory start_path.
> +
> +    Args:
> +	 path: An absolute or relative path to convert to a relative path.
> +	 start_path: The path relative to which the given path should be
> +		     converted.
> +	 os_path_abspath: A replacement function for unit testing.
> +			  Defaults to os.path.abspath.
> +
> +    """
> +    if os_path_abspath is None:
> +	   os_path_abspath = os.path.abspath
> +
> +    start_path = os_path_abspath(start_path)
> +    path = os_path_abspath(path)
> +
> +    if not path.lower().startswith(start_path.lower()):
> +	   # Then path is outside the directory given by start_path.
> +	   return None
> +
> +    path = path[len(start_path):]
> +    # Trim off any leading slash from path in case start_path
> +    # does not end in a slash.
> +    path = path.lstrip("/\\")
> +
> +    return path

What happens if a user call this function like

_rel_path("/tmp/foobar", "/tmp/foo")

? I guess the return value will be "bar". I guess it's better to check if
path.lstrip actually modified the string. Otherwise, this function should
return None, I think.

> +def handle_scm(paths=None, mock_detect_scm=None, mock_os=None):

Why cannot we just do mock_os=os ? I know default_value=[] is bad, but I think
it's OK to assign os module here. Sometimes I've seen this idiom in your patch,
so I should have asked earlier.

> +	   _log.warn("Source root not detected: path-specific logic may yield "

> +		     "unexpected results.\n"
> +		     " If you are running this script from a known source "
> +		     "root, ignore this warning.")

This message is for end-users, right? I guess this message isn't super helpful
for end-users. I would say "WebKit source root" or something instead of "source
root". Also, I would provide an example of unexpected results.

> +	       if rel_path is None:
> +		   # Then the path is outside the source tree.	Since all
> +		   # paths should be interpreted relative to the same root,
> +		   # do not interpret any of the paths as relative to the
> +		   # source root.  Interpret all of them relative to the
> +		   # current working directory, and do not change the current
> +		   # working directory.
> +		   _log.warn("One of the given files is outside the source "
> +			     "tree: path-specific logic may yield unexpected "
> +			     "results.\n Ensure that all files are within "
> +			     "the source tree to achieve expected results.")
> +		   return (paths, scm)

I'd add the value of path into the warning message.

> +    def test_rel_path(self):
> +	   rel_path = self._rel_path("/WebKit/test/Foo.txt", "/WebKit")
> +	   self.assertEquals(rel_path, "test/Foo.txt")
> +
> +	   rel_path = self._rel_path("/WebKit/test/Foo.txt", "/WebKit/")
> +	   self.assertEquals(rel_path, "test/Foo.txt")
> +
> +	   rel_path = self._rel_path(r"c:\WebKit\test\Foo.txt", r"c:\WebKit")
> +	   self.assertEquals(rel_path, r"test\Foo.txt")
> +
> +	   rel_path = self._rel_path(r"c:\WebKit\test\Foo.txt", "c:\\WebKit\\")

> +	   self.assertEquals(rel_path, r"test\Foo.txt")

I'd check //WebKit//test//Foo.txt, too. I'd also check
self._rel_path("/WebKitTools", "/WebKit") .