Bug 75825 – webkit-patch land should automatically add svn:mime-type for .png files

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

webkit-patch land should automatically add svn:mime-type for .png files

Or maybe just warn when it's missing?  See bug 75824.

[Balazs Ankes]

Can anyone help me, where can I start to solve this problem?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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?

[Ojan Vafai]

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.

[Balazs Ankes]

(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.

[Balazs Ankes]

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)

[Balazs Ankes]

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?

[Balazs Ankes]

Created attachment 138335 [details]
proposed fix

[Balazs Ankes]

Could somebody check my patch, please?

[Adam Barth]

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.

[Dirk Pranke]

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.

[Balazs Ankes]

Created attachment 140028 [details]
proposed fix

The improved patch based on comments.

[Dirk Pranke]

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.

[Balazs Ankes]

(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.

[Balazs Ankes]

Created attachment 140967 [details]
proposed fix

I wrote unittest and use boolean type instead of 1/0.

[Dirk Pranke]

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)?

[Balazs Ankes]

Created attachment 141155 [details]
proposed fix

I did what you suggested, the code is clearer indeed. Thank you!

[Balazs Ankes]

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)

[Csaba Osztrogonác]

Comment on attachment 141181 [details]
proposed fix

Landed in https://trac.webkit.org/changeset/116935

[Stephen Chenney]

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

[Csaba Osztrogonác]

Ouch. :-/ Let me roll out immediately.

[Csaba Osztrogonác]

(In reply to comment #21)
> Ouch. :-/ Let me roll out immediately.

Rollout landed in https://trac.webkit.org/changeset/116947

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

(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.

[Balazs Ankes]

Created attachment 141960 [details]
proposed fix

I fixed the problem and tested on svn, it seems good.

[Balazs Ankes]

Created attachment 147528 [details]
proposed fix

I updated the patch. Could somebody check it, please?

[Dirk Pranke]

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

[Balazs Ankes]

Created attachment 147802 [details]
proposed fix

I renamed the variables and rewrote the comment.

[Peter Gal]

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.

[Csaba Osztrogonác]

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.

[Balazs Ankes]

Could somebody take a look for the patch, please?

[Peter Gal]

(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 :))

[Dirk Pranke]

Comment on attachment 149495 [details]
Patch

looks fine. Sorry for the delay; this fell through the cracks somehow.

[Csaba Osztrogonác]

Comment on attachment 149495 [details]
Patch

Clearing flags on attachment: 149495

Committed r122311: <http://trac.webkit.org/changeset/122311>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.