[Webkit-unassigned] [Bug 50098] New webkit-patch rebaseline2 command.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50098





--- Comment #45 from James Kozianski <koz at chromium.org>  2010-12-12 20:53:23 PST ---
I've split out the first sub-patch at https://bugs.webkit.org/show_bug.cgi?id=50901.

(In reply to comment #39)
> (From update of attachment 76120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76120&action=review
> 
> > WebKitTools/Scripts/webkitpy/common/diraszip.py:36
> > +    def __init__(self, path, fs=FileSystem()):
> 
> filesystem=None is much better.

Done.

> 
> > WebKitTools/Scripts/webkitpy/common/diraszip.py:38
> > +        self._fs = fs
> 
> _filesystem is more readable than _fs in my mind.

Done.

> 
> > WebKitTools/Scripts/webkitpy/common/diraszip.py:40
> > +        if not self._path.endswith(os.path.sep):
> > +            self._path += os.path.sep
> 
> If you always use os.join then you don't need this step.

Done.

> 
> > WebKitTools/Scripts/webkitpy/common/diraszip.py:42
> > +    def handle_to(self, filename):
> 
> So we just call this open?  PYthon doesn't talk about "handles"  you just pass around file-like-objects.

Done.

> 
> > WebKitTools/Scripts/webkitpy/common/diraszip.py:51
> > +    def namelist(self):
> > +        results = []
> > +        for (path, _, filenames) in os.walk(self._path):
> > +            for filename in filenames:
> > +                # We drop the path to the directory from our namelist.
> > +                results.append(os.path.join(path, filename)[len(self._path):])
> > +        return results
> 
> This is a list comprehension. :)

Cool! Done.

> 
> What is the purpose of this?
> os.path.join(path, filename)[len(self._path):]
> 
> Why do we need to append the path only to remove the _path?  Ar the paths from os.walk not relative to the root?  I thought they were.

os.walk() returns results relative to the path that it is called from so all results from os.walk(X) will have X as a prefix. We want to pretend that the directory at X is a zip file (in the same vein as http://docs.python.org/library/zipfile.html), so namelist() shouldn't expose X in its results.

> 
> return list(itertools.chain([filenames for (_, _, filenames) in os.walk(self._path)])
> would have done the same thing, minus the strange path re-writing.

That would only provide a list of the bare filenames under the directory and not include the relative paths to them which we need for extracting files.

> 
> > WebKitTools/Scripts/webkitpy/common/diraszip.py:54
> > +        return self._fs.read_text_file(os.path.join(self._path, filename))
> 
> Seems you want a _full_path(filename) helper function since you do os.path.join all over the place.

Done.

> 
> > WebKitTools/Scripts/webkitpy/common/diraszip.py:61
> > +        path_to_file = os.path.split(filename)[0]
> 
> Here is that funny add-the-path-but-remove-it-again dance.  Seems this should be a function if you really need it.

This is actually a different thing, but in looking at the code I realise it is unnecessarily circuitous, so I've made it more concise. The os.path.split() is used to ensure that the directories above the file we want to extract exist at the destination. In Python ZipFile.extract('src/file', 'dest') will copy 'src/file' to 'dest/src/file'.

> 
> > WebKitTools/Scripts/webkitpy/common/diraszip.py:67
> > +        assert '..' not in filename
> 
> What about leading / ?  Does filename simply need to be underneeth _path?  There is an os.path.relpath which can tell you that.. there may be others.  (os.path.relpath is 2.6 only but we have an implementation in ospath.py in common)

Ah, I see - I didn't know os.path.join() interpreted leading slashes in the second argument. I've added an _is_under() method that uses ospath.relpath() and moved the assert to the new _full_path() method.

(In reply to comment #44)
> I would like to say that I'm *very* supportive of bringing our various rebaseline commands into the 21st century.  The original chromium rebaseline architecture was wholy separate from webkitpy and does not work with WebKit.  I wrote one for webkit on top of webkitpy in a quick hack one evening, but it's obviously incomplete.  A unified rebaseline command which leverages webkitpy's infrastructure (notice how little code webkit-patch rebaseline is!) is a fantastic goal!

Thanks for the encouragement! I must admit I didn't expect this script to be as involved as it has been, but I'll be happy if it is a positive contribution :-)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.