[Webkit-unassigned] [Bug 41153] rebaseline-chromium-webkit-tests should add or remove files to local git repository

[bugzilla-daemon at webkit.org]

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





--- Comment #15 from MORITA Hajime <morrita at google.com>  2010-06-27 20:08:41 PST ---
Hamaji-san, Eric, Victor, thank you for taking a look!
I updated the patch.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py: 
>  +  
> I think we need this blank line:
Fixed.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:349
>  +      def _add_directories_recursively(self, path):
> I would say _add_parent_directories or something. "recursively" sounds you will add a directory and its descendants.
Fixed.

> 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:352
>  +          parent, here = os.path.split(path)
> Cannot we just use os.path.dirname here?
Fixed.

> 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:349
>  +      def _add_directories_recursively(self, path):
> Also, I would write a docstring here.
This line is gone ...

> 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:458
>  +      def pset(self, pname, pvalue, path):
> I would use propset as the name of this function.
Fixed.

> 
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:462
>  +      def pget(self, pname, path):
> propget
Fixed.

> 
> WebKitTools/ChangeLog:11
>  +          - add "-U" and "-q" options to rebaseline_chromium_webkit_tests.py
> This can be done in a separate change, but it's OK for now. Please consider splitting your patch next time.
Ah, I agree. I'm sorry for mixing these up.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:477
>  +                      svn_error = True
> This variable should be renamed to scm_error?
Fixed.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:478
>  +                  if not svn_error and isinstance(self._scm, scm.SVN):
> I will comment on this separately.
This line is gone.

(In reply to comment #12)
> (From update of attachment 59732 [details])
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:190
>  +          raise NotImplementedError("subclasses must implement")
> Changed to "raise NotImplementedError, "subclasses must implement"" to be consistent with others.
> 
Fixed. This will be complained by ews bot, though.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:352
>  +          parent, here = os.path.split(path)
> os.path.dirname(path)?
Fixed.

> 
> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:475
>  +                  if 0 != self._scm.add(expected_fullpath, return_exit_code=True):
> What if the file is already under  SVN and here try to add it again?
It' for git. We need to re-add updated files for git.

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