Bug 75824 – check-webkit-style should warn about missing svn:mime-type for png files

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

[Eric Seidel (no email)]

check-webkit-style should warn about missing svn:mime-type for png files

[Darin Adler]

I think we should also put this check into the pre-commit hook.

[Balazs Ankes]

Created attachment 129058 [details]
patch (work in progress)

I made a png.py file which check the ~/.subversion/config file the auto propset and svn:mime-type=image/png are enabled (based on ttp://trac.webkit.org/wiki/UsingGitWithWebKit Misc. Tips and Tricks) because in git svn there is no propset command. Only auto propset can set the mimetype.
I have some question.
- Is it enough to check the config file?
- I noticed check-webkit-style doesn't support non-text files. I tried to solve the task elegant, but I think I couldn't. What should I do?
- If there is a style error related with pngs I want to add to the global error counter. How can I do this nicely?
Any ideas that should be implement?

(I know I have to write unit tests.)

[Tony Chang]

Comment on attachment 129058 [details]
patch (work in progress)

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

I think checking to see if the user's subversion config file is correct is useful, however, that doesn't actually fix this specific bug where certain files might not have the right svn properties set.  I.e., this should probably be a separate bug.

> Tools/Scripts/webkitpy/style/checkers/png.py:25
>  """Supports checking WebKit style in png files."""

This diff is weird.  I don't see png.py in the tree currently.

> Tools/Scripts/webkitpy/style/checkers/png.py:42
> +            for line in open(os.path.expanduser("~") + "/.subversion/config", 'r').readlines():

Does this work with Windows python?  Does this work with Windows svn?

> Tools/Scripts/webkitpy/style/patchreader.py:65
> +                _log.debug("Using class: " + checker.__class__.__name__)

I would remove this log line.

[Balazs Ankes]

(In reply to comment #3)

Thank you the answer.

> (From update of attachment 129058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129058&action=review
> 
> I think checking to see if the user's subversion config file is correct is useful, however, that doesn't actually fix this specific bug where certain files might not have the right svn properties set.  I.e., this should probably be a separate bug.
> 
> > Tools/Scripts/webkitpy/style/checkers/png.py:25
> >  """Supports checking WebKit style in png files."""
> 
> This diff is weird.  I don't see png.py in the tree currently.
Because it's a new file, I added to my local git repo.
> 
> > Tools/Scripts/webkitpy/style/checkers/png.py:42
> > +            for line in open(os.path.expanduser("~") + "/.subversion/config", 'r').readlines():
> 
> Does this work with Windows python?  Does this work with Windows svn?
Good questions, I will check these.
> 
> > Tools/Scripts/webkitpy/style/patchreader.py:65
> > +                _log.debug("Using class: " + checker.__class__.__name__)
> 
> I would remove this log line.
Ok.

[Balazs Ankes]

Created attachment 131040 [details]
proposed fix

This is the improved patch based on the comments and other observations.
Some note:
- I made this patch with this command: "git diff --binary --no-ext-diff --full-index --no-renames e69b1d148665f162528f0b816a409119f1aef20c --" if it's wrong, please tell a good one. (The check-webkit-style use this command.)
- I can't test the Windows part of the png.py.

[David Levin]

Comment on attachment 131040 [details]
proposed fix

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

I have some concerns/questions but this may be approvable if someone else feels confident in it (or with appropriate explanations).

> Tools/Scripts/webkitpy/style/checker.py:498
> +            return False

Why was this needed for png but no other type?

> Tools/Scripts/webkitpy/style/checkers/png.py:29
> +import sys

sort

> Tools/Scripts/webkitpy/style/checkers/png.py:50
> +                print "There is no ~/.subversion/config"

I don't think we print from these checkers.

Also this won't get flagged as an error at all. Isn't that incorrect given the current code?

> Tools/Scripts/webkitpy/style/checkers/png.py:51
> +                #return 1

Please remove the commented out code.

> Tools/Scripts/webkitpy/style/checkers/png.py:70
> +        if errorstr != "":

Just

if errorstr:

> Tools/Scripts/webkitpy/style/checkers/png.py:71
> +            self.handle_style_error(0, 'image/png', 5, errorstr)

I thought it would look at the diff to determine this. I thought you could set the prop without it being in the config file.

Hopefully someone more familiar with this will look at this part of the code.

> Tools/Scripts/webkitpy/style/checkers/png_unittest.py:41
> +        self.assertEquals(checker.handle_style_error, mock_handle_style_error)

This doesn't look like it verifies that the checker actually finds the error.

[Csaba Osztrogonác]

(In reply to comment #6)
> > Tools/Scripts/webkitpy/style/checkers/png.py:71
> > +            self.handle_style_error(0, 'image/png', 5, errorstr)
> 
> I thought it would look at the diff to determine this. I thought you could set the prop without it being in the config file.

You can if you works with svn, but you can't if you're a git-svn user.

[David Levin]

(In reply to comment #7)
> (In reply to comment #6)
> > > Tools/Scripts/webkitpy/style/checkers/png.py:71
> > > +            self.handle_style_error(0, 'image/png', 5, errorstr)
> > 
> > I thought it would look at the diff to determine this. I thought you could set the prop without it being in the config file.
> 
> You can if you works with svn, but you can't if you're a git-svn user.

Interesting.  Does this mean that this check will never catch anything for patches put up here by git users? (As long as the style checker machine is set-up correctly and runs git.)

[Csaba Osztrogonác]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > > Tools/Scripts/webkitpy/style/checkers/png.py:71
> > > > +            self.handle_style_error(0, 'image/png', 5, errorstr)
> > > 
> > > I thought it would look at the diff to determine this. I thought you could set the prop without it being in the config file.
> > 
> > You can if you works with svn, but you can't if you're a git-svn user.
> 
> Interesting.  Does this mean that this check will never catch anything for patches put up here by git users? (As long as the style checker machine is set-up correctly and runs git.)

As far as I know svn:mime-type settings aren't in the patches. But you have 
to setup your svn config correctly on the machine where you commit from.

If you run check-webkit-style before committing it will warn you if your 
svn config is incorrect.

[Balazs Ankes]

(In reply to comment #6)

> > Tools/Scripts/webkitpy/style/checker.py:498
> > +            return False
> 
> Why was this needed for png but no other type?
Because that function get this parameter:
Tools/Scripts/webkitpy/style/checker.py:312
_SKIPPED_FILES_WITHOUT_WARNING = [
    "LayoutTests" + os.path.sep,
    ]
and pngs are in LayoutTests folder.

I considered your suggestions, thank you.

[David Levin]

Comment on attachment 131040 [details]
proposed fix

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

> Tools/Scripts/webkitpy/style/checker.py:320
> +    'png',

Sort.

> Tools/Scripts/webkitpy/style/checker.py:484
> +    PNG = 9

Sort.

>> Tools/Scripts/webkitpy/style/checkers/png.py:29
>> +import sys
> 
> sort

sort.

[Tony Chang]

(In reply to comment #9)
> (In reply to comment #8)
> > Interesting.  Does this mean that this check will never catch anything for patches put up here by git users? (As long as the style checker machine is set-up correctly and runs git.)
> 
> As far as I know svn:mime-type settings aren't in the patches. But you have 
> to setup your svn config correctly on the machine where you commit from.
> 
> If you run check-webkit-style before committing it will warn you if your 
> svn config is incorrect.

Right, this is what I was saying in comment #3.  For git users, this is the best you can do.  For svn users, it would be better to actually check the properties of png files (although checking the config file doesn't hurt either).

I thought this bug was for the svn user case, but this seems like a reasonable first step.

[Tony Chang]

Comment on attachment 131040 [details]
proposed fix

It might be a bit spammy to run this check on every png file since it'll emit the same error for every png file (we're not actually checking the png file).

[Balazs Ankes]

Created attachment 132787 [details]
proposed fix

Improved working based on comments.

[Balazs Ankes]

I couldn't test the svn and windows parts of code.

[Tony Chang]

Comment on attachment 132787 [details]
proposed fix

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

> Tools/Scripts/webkitpy/style/checker.py:482
>      JSON = 3
> +    PNG = 9
>      PYTHON = 4
>      TEXT = 5
>      WATCHLIST = 6

Please re-number these.

> Tools/Scripts/webkitpy/style/checkers/png.py:29
> +import re
> +import sys
> +import os

Sort.

> Tools/Scripts/webkitpy/style/checkers/png.py:44
> +        self.file_path = file_path
> +        self.handle_style_error = handle_style_error
> +        self.fs = filesystem or FileSystem()
> +        self.det = scm or SCMDetector(self.fs, Executive()).detect_scm_system(self.fs.getcwd())

Member variables only used by the class should be prefixed with _.  I think that applies to all of these.  Also, self._detector or self._scm_detector instead of self.det.

> Tools/Scripts/webkitpy/style/checkers/png.py:53
> +        try:
> +            detection = self.det.display_name()
> +        except:

Do we need this try/except block?  When would this fail?

> Tools/Scripts/webkitpy/style/checkers/png.py:56
> +        if(detection == "git"):

Remove the ()s

> Tools/Scripts/webkitpy/style/checkers/png.py:59
> +                    config_file_path = self.fs.expanduser("~") + "/.subversion/config"

fs.join

> Tools/Scripts/webkitpy/style/checkers/png.py:67
> +                    config_file_path = os.environ['APPDATA'] + "\Subversion\config"

fs.join

> Tools/Scripts/webkitpy/style/checkers/png.py:72
> +                    config_file = self.fs.open_text_file_for_reading(config_file_path)
> +                except IOError:
> +                    errorstr = "There is no " + config_file_path
> +                    self.handle_style_error(0, 'image/png', 5, errorstr)
> +                    return

This is the same as lines 60-64.  We can pull it out of the if.

> Tools/Scripts/webkitpy/style/checkers/png.py:75
> +            for line in config_file.readlines():
> +                linenumber += 1

for linenumber, line in enumerate(config_file.readlines()):

> Tools/Scripts/webkitpy/style/checkers/png.py:77
> +                if match and str(match.group(0))[0] != "e":

str() is unnecessary because match.group(0) is already a string.

> Tools/Scripts/webkitpy/style/checkers/png.py:87
> +        if(detection == "svn"):

Remove the ()s and make this an elif.

> Tools/Scripts/webkitpy/style/checkers/png.py:89
> +            if(prop_get != "image/png"):

Remove the ()s.

> Tools/Scripts/webkitpy/style/checkers/png_unittest.py:57
> +        scm = MockSCMDetector('svn')

Can we add some test cases for git?  For example, a test case with each of the following:

enable-auto-props = yes

#enable-auto-props = yes

enable-auto-props = yes
#enable-auto-props = yes

#enable-auto-props = yes
enable-auto-props = yes

enable-auto-props = no

> Tools/Scripts/webkitpy/style/patchreader.py:60
> +        call_only_once = 1

Use True instead of 1.

> Tools/Scripts/webkitpy/style/patchreader.py:69
> +                    if(call_only_once):

Remove the ()s.

> Tools/Scripts/webkitpy/style/patchreader.py:73
> +                        if(det.display_name() == "git"):

Remove the ()s.

> Tools/Scripts/webkitpy/style/patchreader.py:74
> +                            call_only_once = 0

call_only_once = False

[Balazs Ankes]

(In reply to comment #16)

> > Tools/Scripts/webkitpy/style/checkers/png.py:53
> > +        try:
> > +            detection = self.det.display_name()
> > +        except:
> 
> Do we need this try/except block?  When would this fail?
I thought it's necessary because the detection can get None value and that case display_name() won't working and throw error. (/Tools/Scripts/webkitpy/common/checkout/scm/detection.py, def detect_scm_system(self, path, patch_directories=None):)

[Balazs Ankes]

Created attachment 133272 [details]
proposed fix

fresh version
The windows and the svn parts of the code are not tested! (I'm working on linux and git)

[Tony Chang]

Comment on attachment 133272 [details]
proposed fix

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

> Tools/Scripts/webkitpy/style/checkers/png.py:69
> +                match = re.search("^\s*.*enable-auto-props = (yes|no)", line)
> +                if (match and match.group(0)[0] != "e" and not there_is_enable_line) or (match and match.group(0).endswith("no")):
> +                    errorstr_autoprop = "Have to enable auto props in the subversion config file (" + config_file_path + ", line:" + str(linenumber + 1) + "). "

Can we just look for "^\s*enable-auto-props\s*=\s*yes"? If we don't find the line, we give this error message. I think matching commented out lines is making the logic unnecessarily complex.

> Tools/Scripts/webkitpy/style/checkers/png.py:76
> +                match = re.search("^\s*.*\*\.png = svn:mime-type=image/png", line)
> +                if match and match.group(0)[0] != "*" and not there_is_png_line:

Can we just search for "^\s*\*\.png\s*=\s*svn:mime-type=image/png"?  If we don't find the line, we give the png error message.

> Tools/Scripts/webkitpy/style/checkers/png.py:96
> +        if self._platform.startswith('linux') or self._platform.startswith('darwin'):
> +            config_file_path = self._fs.join(self._fs.expanduser("~") + "/", ".subversion/config")

Nit: You don't need the + "/", os.path.join adds the path separator for you.

> Tools/Scripts/webkitpy/style/checkers/png.py:98
> +            config_file_path = self._fs.join(os.environ['APPDATA'] + "\\", "Subversion\config")

Nit: You don't need the + "\\", os.path.join adds the path separator for you.

> Tools/Scripts/webkitpy/style/checkers/png_unittest.py:50
> +        def mock_handle_style_error(line_number, category, confidence,
> +                                     message):

Nit: Indent is weird.  I would just not wrap here.

[Balazs Ankes]

Created attachment 133465 [details]
proposed fix

[Tony Chang]

Comment on attachment 133465 [details]
proposed fix

It would be great if someone could test this on win and mac before landing.

[Tony Chang]

Comment on attachment 133465 [details]
proposed fix

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

> Tools/Scripts/webkitpy/style/checkers/png.py:86
> +
> +        if detection == "svn":

nit: elif

[Balazs Ankes]

(In reply to comment #21)
> (From update of attachment 133465 [details])
> It would be great if someone could test this on win and mac before landing.

It has been tested, it works good on win and mac also.

[Kristóf Kosztyó]

(In reply to comment #23)
> (In reply to comment #21)
> > (From update of attachment 133465 [details] [details])
> > It would be great if someone could test this on win and mac before landing.
> 
> It has been tested, it works good on win and mac also.

It has been Landed in r112770 http://trac.webkit.org/changeset/112270

And I apologies about this line: "Reviewed by NOBODY Tony Chang." :(
Next time I will be far more careful. Sorry again.

[Tony Chang]

Comment on attachment 133465 [details]
proposed fix

Sounds like this was already committed.

[Eric Seidel (no email)]

Do we need to fix something on the style bot?
https://bugs.webkit.org/show_bug.cgi?id=81936#c13

We obviously don't want it failing like this.  It uses a Git checkout.

[Csaba Osztrogonác]

(In reply to comment #26)
> Do we need to fix something on the style bot?
> https://bugs.webkit.org/show_bug.cgi?id=81936#c13
> 
> We obviously don't want it failing like this.  It uses a Git checkout.

Yes, it isn't good if it fails on the style bot. This warning is to warn git users to set their subversion config correctly before landing patches. (with git-svn). Because if they don't do it, mime-type of png files won't be image/png. This is the only way for git users to committ png files with correct mime-type.

Could you set the style bot, please?
- Have to enable auto props in the subversion config file (/home/ubuntu/.subversion/config "enable-auto-props = yes"). 
- Have to set the svn:mime-type in the subversion config file (/home/ubuntu/.subversion/config "*.png = svn:mime-type=image/png")

These options are commented by default, I think you only have to uncomment them.

[Csaba Osztrogonác]

Oh, after https://bugs.webkit.org/show_bug.cgi?id=81936#c17 everything should work.

[Florin Malita]

This causes .png removal failures: https://bugs.webkit.org/show_bug.cgi?id=82933

[Adam Barth]

This check is really annoying because it doesn't solve the problem for me:

LayoutTests/platform/chromium-win/fast/multicol/progression-reverse-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-win/fast/multicol/progression-reverse-expected.png).  [image/png] [5]
LayoutTests/platform/chromium-mac/fast/multicol/progression-reverse-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-mac/fast/multicol/progression-reverse-expected.png).  [image/png] [5]
LayoutTests/platform/chromium-mac-snowleopard/fast/multicol/progression-reverse-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-mac-snowleopard/fast/multicol/progression-reverse-expected.png).  [image/png] [5]
LayoutTests/platform/chromium-linux/fast/multicol/progression-reverse-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-linux/fast/multicol/progression-reverse-expected.png).  [image/png] [5]

[Csaba Osztrogonác]

(In reply to comment #30)
> This check is really annoying because it doesn't solve the problem for me:
> 
> LayoutTests/platform/chromium-win/fast/multicol/progression-reverse-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-win/fast/multicol/progression-reverse-expected.png).  [image/png] [5]
> LayoutTests/platform/chromium-mac/fast/multicol/progression-reverse-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-mac/fast/multicol/progression-reverse-expected.png).  [image/png] [5]
> LayoutTests/platform/chromium-mac-snowleopard/fast/multicol/progression-reverse-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-mac-snowleopard/fast/multicol/progression-reverse-expected.png).  [image/png] [5]
> LayoutTests/platform/chromium-linux/fast/multicol/progression-reverse-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-linux/fast/multicol/progression-reverse-expected.png).  [image/png] [5]

check-webkit-style won't solve your problem, it only warns you if you do something wrong. We should set svn mimetype for all png files - https://lists.webkit.org/pipermail/webkit-dev/2012-March/019783.html , but you didn't. That's why check-webkit-style warned you and suggested what to do: svn propset svn:mime-type image/png LayoutTests/platform/chromium-mac-snowleopard/fast/multicol/progression-reverse-expected.png )

But https://bugs.webkit.org/show_bug.cgi?id=75825 will do it automatically once the fix is landed.

[Adam Barth]

Thanks.  I will wait patiently for Bug 75825.  :)