Make webkit-patch support subdirectories for SVN checkouts

[Maciej Stachowiak]

Make webkit-patch support subdirectories for SVN checkouts

[Maciej Stachowiak]

Created attachment 78613 [details]
Patch

[Maciej Stachowiak]

This is my first Python patch so I don't really know if this was the best way to do it.

[Adam Barth]

Comment on attachment 78613 [details]
Patch

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

Thanks for the patch.  This looks like a good approach.

> Tools/Scripts/webkitpy/common/checkout/scm.py:330
> +        if len(patch_directories) > 0:
> +            self.patch_directories = patch_directories

Private member variables in python are prefixed by _ (by convention).  We're not perfect about this in webkitpy because we learned about it after we started, but we're trying to enforce that for new code.

> Tools/Scripts/webkitpy/common/checkout/scm.py:332
> +            self.patch_directories = [os.path.relpath(cwd, self.checkout_root)]

Generally, using None is better than [] for default behavior, especially default behavior that changes the empty set into the universal set.

> Tools/Scripts/webkitpy/common/checkout/scm.py:439
> -        return self.run_status_and_extract_filenames(self.status_command(), self._status_regexp("ACDMR"))
> +        status_command = ["svn", "status"]
> +        status_command.extend(self.patch_directories)

We don't want to do this for all clients of run_status_and_extract_filenames and/or status_command ?  It seems strange to do this only here.

> Tools/Scripts/webkitpy/tool/main.py:88
> -            self._scm = default_scm()
> +            self._scm = default_scm(self._options)

options is a tool-layer concept (i.e., specific to webkit-patch).  We shouldn't be passing the options collection to the "common" layer because different scripts (e.g., new-run-webkit-tests) might have a different set of options.  Usually what we do in this case is read the value out of the options collection and pass it in a named argument explicitly.

[Maciej Stachowiak]

Created attachment 78619 [details]
Patch

[Adam Barth]

Comment on attachment 78619 [details]
Patch

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

> Tools/Scripts/webkitpy/style_references.py:74
>      def create_patch(self, git_commit, changed_files=None):
>          # FIXME: SCM.create_patch should understand how to handle None.
> -        return self._scm.create_patch(git_commit, changed_files=changed_files or [])
> +        return self._scm.create_patch(git_commit, changed_files=changed_files)

We really should just get rid of this whole references thing.  It hasn't really bought us much.  (This comment isn't really about your patch, just my reaction to seeing this change.)

[Maciej Stachowiak]

Created attachment 78625 [details]
Patch

[Maciej Stachowiak]

Created attachment 78626 [details]
Patch

[Adam Barth]

Comment on attachment 78626 [details]
Patch

Thanks.  Looks good modulo renaming the parameter to default_scm.

[Maciej Stachowiak]

Committed r75575: <http://trac.webkit.org/changeset/75575>

[Alexey Proskuryakov]

*** Bug 52260 has been marked as a duplicate of this bug. ***

[Csaba Osztrogonác]

It doesn't work with python 2.5, because os.path.relpath() introduced in 2.6

File "/home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/Tools/Scripts/webkitpy/common/checkout/scm.py", line 331, in __init__
    self._patch_directories = [os.path.relpath(cwd, self.checkout_root)]
AttributeError: 'module' object has no attribute 'relpath'

[Adam Barth]

> It doesn't work with python 2.5, because os.path.relpath() introduced in 2.6

My mistake.  We should be calling this version:

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/system/ospath.py#L35

[Adam Barth]

Created attachment 78668 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 78668 [details]
Patch

Clearing flags on attachment: 78668

Committed r75600: <http://trac.webkit.org/changeset/75600>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #5)
> > Tools/Scripts/webkitpy/style_references.py:74
> >      def create_patch(self, git_commit, changed_files=None):
> >          # FIXME: SCM.create_patch should understand how to handle None.
> > -        return self._scm.create_patch(git_commit, changed_files=changed_files or [])
> > +        return self._scm.create_patch(git_commit, changed_files=changed_files)

After this change, check-webkit-style doesn't work for me.

$ Tools/Scripts/check-webkit-style
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 134, in <module>
    main()
  File "Tools/Scripts/check-webkit-style", line 119, in main
    patch = checkout.create_patch(options.git_commit, changed_files=changed_files)
  File "/home/oszi/WebKit/Tools/Scripts/webkitpy/style_references.py", line 73, in create_patch
    return self._scm.create_patch(git_commit, changed_files=changed_files)
  File "/home/oszi/WebKit/Tools/Scripts/webkitpy/common/checkout/scm.py", line 748, in create_patch
    return self.run(['git', 'diff', '--binary', "--no-ext-diff", "--full-index", "-M", self.merge_base(git_commit), "--"] + changed_files, decode_output=False, cwd=self.checkout_root)
TypeError: can only concatenate list (not "NoneType") to list

[Eric Seidel (no email)]

I suspect that Git.create_patch is lame and doens't understand None as a default argument.

[Peter Gal]

Created attachment 78847 [details]
proposed fix

I wasn't able to test it (don't have a git repo at hand), but with this it should work.

[Eric Seidel (no email)]

Comment on attachment 78847 [details]
proposed fix

Testing should be easy.  I can help if necessary.

[Peter Gal]

(In reply to comment #19)
> (From update of attachment 78847 [details])
> Testing should be easy.  I can help if necessary.

Well I've checked out a git repo and with my patch it works fine. Also after running Tools/Scripts/test-webkitpy webkitpy.common.checkout.scm_unittest.GitSVNTest there was only one failed test, but it was present before the patch.

[Eric Seidel (no email)]

Yes, it's likely that scm_unittest.py already has failures because it's only run when --all is passed to test-webkitpy.

In this case, you are only really expected to add a new test for this case which woudl be part of scm_unittest.py but would not use a real svn or git repo (like some of the other tests do) and instead pass a MockExecutive to Git() and validate that the right commands are made (using OutputCapture).

Again, I'm happy to help.  You stumbled on a rather archaic piece of our testing infrastructure sadly.

[Eric Seidel (no email)]

Created attachment 78921 [details]
Patch

[Adam Barth]

Comment on attachment 78921 [details]
Patch

THEY SAID IT COULDN'T BE DONE.

[Adam Barth]

Someday we might have SCM tests that aren't amazingly slow.  :)

[Peter Gal]

(In reply to comment #22)
> Created an attachment (id=78921) [details]
> Patch

Cool. Thanks for adding the testcase.

[WebKit Commit Bot]

Comment on attachment 78921 [details]
Patch

Clearing flags on attachment: 78921

Committed r75787: <http://trac.webkit.org/changeset/75787>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.