if you run rebaseline-chromium-webkit-tests using a Git checkout of WebKit, it can't produce the correct results.html file to show the diffs - it assumes you can do an 'svn cat' of the old -expected file. See http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py#L697 in particular around line 719.
This script should use scm.py to abstract away the differences between svn and git to avoid this entire class of bugs instead of re-inventing scmy.py as it does now.
Created attachment 55425 [details] Patch
here's a minimal patch to make things work in Git; we should really abstract all the SCM stuff out though. There's probably code already written to do this. Also, we should diff the two files directly rather using the port routines rather than using svn diff. Haven't made that change yet.
whoops; didn't see Adam's comment, but I agree completely.
(In reply to comment #4) > whoops; didn't see Adam's comment, but I agree completely. Agreed. There are a couple of things I like to refactor the rebaseline tool after it has been upstreamed. Working with git (the tool only works with svn when it was written last year) is one item on the list, other items like better summarized messages, better working with the upstream infrastructure (port, multiple test_expectations etc). I have some other tasks on my Q2 list, but should be able to get into this in later Q2 or earlier Q3.
Created attachment 59887 [details] patch v0
As we introduce SCM abstraction to rebaseline_chromium_webkit_test, now we can replace diff and cat with them. Here is an attempt.
Comment on attachment 59887 [details] patch v0 WebKitTools/Scripts/webkitpy/common/checkout/scm.py:441 + self._bogus_dir = os.path.join(parent_dir, "temp_svn_config") Should _bogus_dir be created for win? WebKitTools/Scripts/webkitpy/common/checkout/scm.py:462 + return self.run(['svn', 'diff', path]) Missing "--config-dir"? WebKitTools/Scripts/webkitpy/common/checkout/scm.py:467 + return self.run(['svn', 'cat', path]) Add "-r BASE" to svn cat?
Created attachment 59986 [details] patch v1
Hi Victor, thank you for reviewing! I updated the patch. (In reply to comment #8) > (From update of attachment 59887 [details]) > WebKitTools/Scripts/webkitpy/common/checkout/scm.py:441 > + self._bogus_dir = os.path.join(parent_dir, "temp_svn_config") > Should _bogus_dir be created for win? Fixed. (I broke the code during copying...) > > WebKitTools/Scripts/webkitpy/common/checkout/scm.py:462 > + return self.run(['svn', 'diff', path]) > Missing "--config-dir"? Fixed. > > WebKitTools/Scripts/webkitpy/common/checkout/scm.py:467 > + return self.run(['svn', 'cat', path]) > Add "-r BASE" to svn cat? Fixed, and added test for SVN.show_head()
Ok to me, you need a reviewer to review this. (In reply to comment #10) > Hi Victor, thank you for reviewing! > I updated the patch. > > (In reply to comment #8) > > (From update of attachment 59887 [details] [details]) > > WebKitTools/Scripts/webkitpy/common/checkout/scm.py:441 > > + self._bogus_dir = os.path.join(parent_dir, "temp_svn_config") > > Should _bogus_dir be created for win? > Fixed. (I broke the code during copying...) > > > > > WebKitTools/Scripts/webkitpy/common/checkout/scm.py:462 > > + return self.run(['svn', 'diff', path]) > > Missing "--config-dir"? > Fixed. > > > > > WebKitTools/Scripts/webkitpy/common/checkout/scm.py:467 > > + return self.run(['svn', 'cat', path]) > > Add "-r BASE" to svn cat? > Fixed, and added test for SVN.show_head()
Comment on attachment 59986 [details] patch v1 Cannot we test diff_for_file? WebKitTools/Scripts/webkitpy/common/checkout/scm.py:228 + def diff_for_file(self, path, optios={}): Hmm... I'd prefer using keyword arguments for all possible options. def diff_for_file(self, use_bogus_dir=True, git_cached=True, ...) so we won't make typo. I think this way is more consistent with other code in this file (see changed_file for example) By the way, do we really need these options?
Created attachment 60197 [details] patch v2
Attachment 60197 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKitTools/Scripts/webkitpy/common/checkout/scm.py:433: trailing whitespace [pep8/W291] [5] WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:1172: trailing whitespace [pep8/W291] [5] WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:1187: trailing whitespace [pep8/W291] [5] Total errors found: 3 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 60197 [details] patch v2 WebKitTools/Scripts/webkitpy/common/checkout/scm.py:455 + def diff_for_file(self, path, use_bogus_dir=False, log=None, **kwargs): Still this code allows typo. I'd just remove kwargs.
Created attachment 60198 [details] patch v2.1; fixed style violation
Created attachment 60202 [details] patch v3; removed redundant options
Hi Hamaji-san, thank you reviewing again. I updated the patch. > Still this code allows typo. I'd just remove kwargs. Ah, I get what you meant to say. And I removed options which can be altered by appropriate default behavior.
Comment on attachment 60202 [details] patch v3; removed redundant options Looks good. Thanks for your update!
Committed r62236: <http://trac.webkit.org/changeset/62236>