Bug 75825 - webkit-patch land should automatically add svn:mime-type for .png files
Summary: webkit-patch land should automatically add svn:mime-type for .png files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on: 86373
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-08 21:38 PST by Eric Seidel (no email)
Modified: 2012-07-11 01:34 PDT (History)
12 users (show)

See Also:


Attachments
script to auto-update svn mime-types (2.55 KB, application/octet-stream)
2012-04-02 11:36 PDT, Ojan Vafai
no flags Details
work in progress (12.80 KB, patch)
2012-04-17 06:22 PDT, Balazs Ankes
no flags Details | Formatted Diff | Diff
proposed fix (14.00 KB, patch)
2012-04-23 06:29 PDT, Balazs Ankes
dpranke: review-
abarth: commit-queue-
Details | Formatted Diff | Diff
proposed fix (13.74 KB, patch)
2012-05-03 09:07 PDT, Balazs Ankes
dpranke: review-
dpranke: commit-queue-
Details | Formatted Diff | Diff
proposed fix (17.04 KB, patch)
2012-05-09 10:04 PDT, Balazs Ankes
dpranke: review+
dpranke: commit-queue-
Details | Formatted Diff | Diff
proposed fix (16.58 KB, patch)
2012-05-10 05:58 PDT, Balazs Ankes
no flags Details | Formatted Diff | Diff
proposed fix (16.59 KB, patch)
2012-05-10 09:15 PDT, Balazs Ankes
no flags Details | Formatted Diff | Diff
proposed fix (16.60 KB, patch)
2012-05-15 07:06 PDT, Balazs Ankes
no flags Details | Formatted Diff | Diff
proposed fix (16.57 KB, patch)
2012-06-14 02:23 PDT, Balazs Ankes
dpranke: review+
dpranke: commit-queue-
Details | Formatted Diff | Diff
proposed fix (16.59 KB, patch)
2012-06-15 06:50 PDT, Balazs Ankes
no flags Details | Formatted Diff | Diff
Patch (17.04 KB, patch)
2012-06-26 02:40 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-01-08 21:38:16 PST
webkit-patch land should automatically add svn:mime-type for .png files

Or maybe just warn when it's missing?  See bug 75824.
Comment 1 Balazs Ankes 2012-04-02 04:33:49 PDT
Can anyone help me, where can I start to solve this problem?
Comment 2 Eric Seidel (no email) 2012-04-02 11:21:35 PDT
I believe this can only be done for SVN.  You'd have to edit svn.py to add this ability, and the presumably make a new Step() class which was called by various Command objects and which added this mime type for necessaary png files.
Comment 3 Eric Seidel (no email) 2012-04-02 11:22:23 PDT
I'm no longer convinced this is the right way to solve this problem.  Do we already have a scrxipt which can fix this globally for the svn repository?  Should we just run that scxript every night on a bot somewhere?
Comment 4 Ojan Vafai 2012-04-02 11:36:15 PDT
Created attachment 135140 [details]
script to auto-update svn mime-types

I wrote this script and ran it once through the repo. I don't have time to properly write tests for it and get the patch reviewed/committed. If someone else wants to take it over, they're more than welcome to. Put this script in Tools/Scripts and it should do the right thing.
Comment 5 Balazs Ankes 2012-04-04 02:38:14 PDT
(In reply to comment #4)
> Created an attachment (id=135140) [details]
> script to auto-update svn mime-types
Thank you!
> 
> I wrote this script and ran it once through the repo. I don't have time to properly write tests for it and get the patch reviewed/committed. If someone else wants to take it over, they're more than welcome to. Put this script in Tools/Scripts and it should do the right thing.
I take care of it.
Comment 6 Balazs Ankes 2012-04-16 01:33:53 PDT
I realized I could use the svn config file reader and the config file path getter parts from the webkitpy/style/checkers/png.py. I want to avoid the code duplication. My idea is I outsource those lines to a detached file and use it. The question is where can I place this file? (I would put it to webkitpy/common but maybe I think it wrong)
Comment 7 Balazs Ankes 2012-04-17 06:22:12 PDT
Created attachment 137527 [details]
work in progress

I finished the task, but I have a question. I saw there is no unittest all of the steps (webkitpy/tool/steps), should I write?
Comment 8 Balazs Ankes 2012-04-23 06:29:29 PDT
Created attachment 138335 [details]
proposed fix
Comment 9 Balazs Ankes 2012-05-02 05:14:20 PDT
Could somebody check my patch, please?
Comment 10 Adam Barth 2012-05-02 09:37:16 PDT
Comment on attachment 138335 [details]
proposed fix

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

I took a quick look.  One note below.  Ideally Eric or Dirk would review your patch.

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:43
> +            detector = SCMDetector(self._fs, Executive()).detect_scm_system(self._fs.getcwd())

We should get the Executive from the tool so that it gets mocked properly during testing.
Comment 11 Dirk Pranke 2012-05-02 14:23:44 PDT
Comment on attachment 138335 [details]
proposed fix

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

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:30
> +import sys

It looks like sys is unused in this file ...

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:33
> +def check(self, platform, fs):

Is 'platform' a PlatformInfo object? It looks like it is ... maybe this should take a host or a systemhost instead?

Also, some form of docstring would be good for this function. What is this function supposed to be checking for, and how should the return value be used?

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:72
> +    if platform.startswith('linux') or platform.startswith('darwin'):

This should probably be 

if platform.is_win():
  config_file_path = fs.join(os.environ ...
else:
  config_file_path = fs.join(fs.expand ...

> Tools/Scripts/webkitpy/style/checkers/png.py:45
>          self._platform = platform or sys.platform

Does the style checker have access to a host or a tool object? It should get the platform info from that ...

> Tools/Scripts/webkitpy/style/checkers/png.py:52
> +            errorcode = checksvnconfigfile.check(self, self._platform, self._fs)

Instead of returning an errorcode that is a bit mask, you should consider either returning two values in a tuple, or splitting this into two functions.

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:36
> +        self._fs = FileSystem()

You can also get the filesystem from the tool.
Comment 12 Balazs Ankes 2012-05-03 09:07:30 PDT
Created attachment 140028 [details]
proposed fix

The improved patch based on comments.
Comment 13 Dirk Pranke 2012-05-03 16:48:43 PDT
Comment on attachment 140028 [details]
proposed fix

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

This looks pretty good (with the one quibble), but normally we require unit tests for new code. Can you add some unit tests as well?

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:46
> +        return (1, 1, 1)

You should use True/False instead of 1/0.
Comment 14 Balazs Ankes 2012-05-04 05:51:11 PDT
(In reply to comment #13)
> (From update of attachment 140028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140028&action=review
> 
> This looks pretty good (with the one quibble), but normally we require unit tests for new code. Can you add some unit tests as well?

Yes, I can. I asked it earlier but I didn't get answer.
> 
> > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:46
> > +        return (1, 1, 1)
> 
> You should use True/False instead of 1/0.

OK.
Comment 15 Balazs Ankes 2012-05-09 10:04:55 PDT
Created attachment 140967 [details]
proposed fix

I wrote unittest and use boolean type instead of 1/0.
Comment 16 Dirk Pranke 2012-05-09 12:27:07 PDT
Comment on attachment 140967 [details]
proposed fix

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

marking cq- just so you can take a look at my suggestion and see if it works. looks good otherwise.

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:51
> +    for line in config_file.split('\n'):

I wonder if this would be easier and clearer if you didn't split the file and just did two re.search()'s against the whole string, i.e.

errorcode_autoprops = not re.search("\s*enable-auto-props\s*=\s*yes", config_file)
errorcode_png = not re.search("\s*\*\.png\s*=\s*svn:mime-type=image/png", config_file)

or something like that (I'm not sure how you'd handle commented out lines, so you might have to experiment a bit)?
Comment 17 Balazs Ankes 2012-05-10 05:58:23 PDT
Created attachment 141155 [details]
proposed fix

I did what you suggested, the code is clearer indeed. Thank you!
Comment 18 Balazs Ankes 2012-05-10 09:15:23 PDT
Created attachment 141181 [details]
proposed fix

I rewrote the regex because png_unittest thrown error. (I forgot run unittest before uploaded the previous patch, sorry)
Comment 19 Csaba Osztrogonác 2012-05-14 05:32:56 PDT
Comment on attachment 141181 [details]
proposed fix

Landed in https://trac.webkit.org/changeset/116935
Comment 20 Stephen Chenney 2012-05-14 08:08:35 PDT
Now I can't land anything. :-(

webkit-patch land
Traceback (most recent call last):
  File "/home/schenney/development/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkit-patch", line 69, in <module>
    main()
  File "/home/schenney/development/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkit-patch", line 64, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/usr/local/google/users/schenney/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 311, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/usr/local/google/users/schenney/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 120, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/usr/local/google/users/schenney/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 51, in execute
    self._sequence.run_and_handle_errors(tool, options, state)
  File "/usr/local/google/users/schenney/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 70, in run_and_handle_errors
    self._run(tool, options, state)
  File "/usr/local/google/users/schenney/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/usr/local/google/users/schenney/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py", line 68, in run
    if filename.endswith('.png') and detector.exists(filename) and detector.propget('svn:mime-type', filename) == "":
NameError: global name 'detector' is not defined
Comment 21 Csaba Osztrogonác 2012-05-14 08:09:51 PDT
Ouch. :-/ Let me roll out immediately.
Comment 22 Csaba Osztrogonác 2012-05-14 08:15:01 PDT
(In reply to comment #21)
> Ouch. :-/ Let me roll out immediately.

Rollout landed in https://trac.webkit.org/changeset/116947
Comment 23 Csaba Osztrogonác 2012-05-14 08:17:57 PDT
Comment on attachment 141181 [details]
proposed fix

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

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:68
> +                    if filename.endswith('.png') and detector.exists(filename) and detector.propget('svn:mime-type', filename) == "":

detector -> self._detector

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:70
> +                        detector.propset('svn:mime-type', 'image/png', filename)

detector -> self._detector
Comment 24 Csaba Osztrogonác 2012-05-14 08:18:41 PDT
(In reply to comment #23)
> (From update of attachment 141181 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141181&action=review
> 
> > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:68
> > +                    if filename.endswith('.png') and detector.exists(filename) and detector.propget('svn:mime-type', filename) == "":
> 
> detector -> self._detector
> 
> > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:70
> > +                        detector.propset('svn:mime-type', 'image/png', filename)
> 
> detector -> self._detector

It seems this codepath isn't (unit)tested well.
Comment 25 Balazs Ankes 2012-05-15 07:06:04 PDT
Created attachment 141960 [details]
proposed fix

I fixed the problem and tested on svn, it seems good.
Comment 26 Balazs Ankes 2012-06-14 02:23:33 PDT
Created attachment 147528 [details]
proposed fix

I updated the patch. Could somebody check it, please?
Comment 27 Dirk Pranke 2012-06-14 10:35:41 PDT
Comment on attachment 147528 [details]
proposed fix

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

assuming my comments are correct, fix the comments and variable names and this should be fine.

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:36
> +    was the file read successful, is there enable the auto-props, is there enable the svn:mime-type for png

I'm not sure if this is a bug or if this is just written wrong, but it looks like you want to return True if something failed, right? So I would rewrite this as "is this file missing, is auto-props missing, is the svn:mime-type for png missing"

> Tools/Scripts/webkitpy/style/checkers/png.py:58
> +            (file_read, autoprop, png) = checksvnconfigfile.check(self, self._host, self._fs)

I would rename these variables the same way I renamed the docstring, so file_missing, autoprop_missing, png_missing
Comment 28 Balazs Ankes 2012-06-15 06:50:29 PDT
Created attachment 147802 [details]
proposed fix

I renamed the variables and rewrote the comment.
Comment 29 Peter Gal 2012-06-19 02:04:25 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=147802&action=review

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:32
> +def check(self, host, fs):

I think the 'self' here is a little misleading, and why do we need it at all?

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:52
> +def _config_file_path(self, host, fs):

Why do you need the 'self'? It seems it is unused.

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:53
> +    config_file = ""

I don't see this variable used in this method.

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:57
> +        config_file_path = fs.join(os.environ['APPDATA'], "Subversion\config")
> +    else:
> +        config_file_path = fs.join(fs.expanduser("~"), ".subversion/config")

fs.join(..) can take more than 2 arguments, the config should be the 3.

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:62
> +    return "Have to enable auto props in the subversion config file (" + config_file_path + " \"enable-auto-props = yes\"). "

Instead of escaping the " use ' at the start and end. Oh.. and use the %s formatter, that is more readable.

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:66
> +    return "Have to set the svn:mime-type in the subversion config file (" + config_file_path + " \"*.png = svn:mime-type=image/png\")."

ditto

> Tools/Scripts/webkitpy/style/checkers/png.py:59
> +            config_file_path = checksvnconfigfile._config_file_path(self, self._host, self._fs)

Calling a method prefixed with underscore isn't really nice. Ditch the leading underscore.

> Tools/Scripts/webkitpy/style/checkers/png.py:62
> +                self._handle_style_error(0, 'image/png', 5, "There is no SVN config file. (" + config_file_path + ")")

use %s formatter

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:42
> +        if len(png_files) > 0:

No need for length check, just: "if png_files:"

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:46
> +                (file_read, autoprop, png) = checksvnconfigfile.check(self, self._host, self._fs)

use the same naming like in the PNGChecker.

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:68
> +                    if filename.endswith('.png') and self._detector.exists(filename) and self._detector.propget('svn:mime-type', filename) != 'image/png':

The file extension is already checked in the _check_pngs method. Why do you check it again?

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng_unittest.py:46
> +        options = MockOptions()
> +        options.git_commit = 'MOCK git commit'

Pass the git_commit as a kwarg to the MockOptions.

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng_unittest.py:54
> +            "changed_files": ["test.png"],

Add a non-png file to the list.

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng_unittest.py:59
> +        except SystemExit, e:
> +            self.assertEquals(type(e), type(SystemExit()))

Why do we check if the type is a SystemExit exception? The except will only catch SystemExit ones.
Comment 30 Csaba Osztrogonác 2012-06-26 02:40:28 PDT
Created attachment 149495 [details]
Patch

Fixed patch based on Comment #29. (Balázs isn't online now, but he asked me to upload his updated patch.
Comment 31 Balazs Ankes 2012-07-04 09:29:06 PDT
Could somebody take a look for the patch, please?
Comment 32 Peter Gal 2012-07-05 06:59:32 PDT
(In reply to comment #30)
> Created an attachment (id=149495) [details]
> Patch
> 
> Fixed patch based on Comment #29. (Balázs isn't online now, but he asked me to upload his updated patch.

looks a lot better now (infomral r=me :))
Comment 33 Dirk Pranke 2012-07-08 11:52:45 PDT
Comment on attachment 149495 [details]
Patch

looks fine. Sorry for the delay; this fell through the cracks somehow.
Comment 34 Csaba Osztrogonác 2012-07-11 01:34:37 PDT
Comment on attachment 149495 [details]
Patch

Clearing flags on attachment: 149495

Committed r122311: <http://trac.webkit.org/changeset/122311>
Comment 35 Csaba Osztrogonác 2012-07-11 01:34:48 PDT
All reviewed patches have been landed.  Closing bug.