Patchwork [1,of,2,STABLE] check-code: do not abort on an unreadable file, only report this

login
register
mail settings
Submitter Simon Heimberg
Date July 22, 2013, 8:33 a.m.
Message ID <e6de3183429487a11ef1.1374482019@lapsi.heimberg.home>
Download mbox | patch
Permalink /patch/1941/
State Accepted
Commit 3119dc155ac276e45d3c223453940e5c1009756c
Headers show

Comments

Simon Heimberg - July 22, 2013, 8:33 a.m.
# HG changeset patch
# User Simon Heimberg <simohe@besonet.ch>
# Date 1374480285 -7200
# Node ID e6de3183429487a11ef1c28bc1562aa85617732b
# Parent  004f965630d907a3417a93e87d056ad4c2dab541
check-code: do not abort on an unreadable file, only report this
Martin Geisler - July 22, 2013, 10:40 a.m.
Simon Heimberg <simohe@besonet.ch> writes:

> # HG changeset patch
> # User Simon Heimberg <simohe@besonet.ch>
> # Date 1374480285 -7200
> # Node ID e6de3183429487a11ef1c28bc1562aa85617732b
> # Parent  004f965630d907a3417a93e87d056ad4c2dab541
> check-code: do not abort on an unreadable file, only report this
>
> diff -r 004f965630d9 -r e6de31834294 contrib/check-code.py
> --- a/contrib/check-code.py	Fre Jul 12 11:14:42 2013 +0900
> +++ b/contrib/check-code.py	Mon Jul 22 10:04:45 2013 +0200
> @@ -407,7 +407,11 @@
>                  print "Skipping %s for %s it doesn't match %s" % (
>                         name, match, f)
>              continue
> -        fp = open(f)
> +        try:
> +            fp = open(f)
> +        except IOError, e:

I think you should include our normal

    if e.errno != errno.ENOENT:
        raise

idiom here. That would also make the split below seem more robust.

> +            print "Skipping %s, %s" % (f, str(e).split(':', 1)[0])
> +            continue
>          pre = post = fp.read()
>          fp.close()
>          if "no-" "check-code" in pre:
> diff -r 004f965630d9 -r e6de31834294 tests/test-check-code.t
> --- a/tests/test-check-code.t	Fre Jul 12 11:14:42 2013 +0900
> +++ b/tests/test-check-code.t	Mon Jul 22 10:04:45 2013 +0200
> @@ -187,8 +187,10 @@
>    > # this next line is okay
>    > raise SomeException(arg1, arg2)
>    > EOF
> -  $ "$check_code" raise-format.py
> +  $ "$check_code" not-existing.py raise-format.py
> +  Skipping*not-existing.py* (glob)
>    raise-format.py:1:
>     > raise SomeException, message
>     don't use old-style two-argument raise, use Exception(message)
>    [1]
> +
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Simon Heimberg - July 23, 2013, 7:28 a.m.
I would prefer to not abort (and print a traceback) on EACCES, EISDIR 
and maybe some more exceptions.

When we only continue on ENOENT, using str(e) is unnecessary. It always 
tells the file does not exist. So it could be replaced by a constant.

All the mentioned exceptions (ENOENT, EACCES, EISDIR) have the filename 
after the colon, so stripping works as expected. When an exception 
without a colonwould be raised, split returns the entire message.
 >>> 'some message'.split(':', 1)[0]
     'some message'



ON 2013-07-22 12:40, Martin Geisler wrote:
> Simon Heimberg <simohe@besonet.ch> writes:
>
>> # HG changeset patch
>> # User Simon Heimberg <simohe@besonet.ch>
>> # Date 1374480285 -7200
>> # Node ID e6de3183429487a11ef1c28bc1562aa85617732b
>> # Parent  004f965630d907a3417a93e87d056ad4c2dab541
>> check-code: do not abort on an unreadable file, only report this
>>
>> diff -r 004f965630d9 -r e6de31834294 contrib/check-code.py
>> --- a/contrib/check-code.py	Fre Jul 12 11:14:42 2013 +0900
>> +++ b/contrib/check-code.py	Mon Jul 22 10:04:45 2013 +0200
>> @@ -407,7 +407,11 @@
>>                   print "Skipping %s for %s it doesn't match %s" % (
>>                          name, match, f)
>>               continue
>> -        fp = open(f)
>> +        try:
>> +            fp = open(f)
>> +        except IOError, e:
> I think you should include our normal
>
>      if e.errno != errno.ENOENT:
>          raise
>
> idiom here. That would also make the split below seem more robust.
>
>> +            print "Skipping %s, %s" % (f, str(e).split(':', 1)[0])
>> +            continue
>>           pre = post = fp.read()
>>           fp.close()
>>           if "no-" "check-code" in pre:
>> diff -r 004f965630d9 -r e6de31834294 tests/test-check-code.t
>> --- a/tests/test-check-code.t	Fre Jul 12 11:14:42 2013 +0900
>> +++ b/tests/test-check-code.t	Mon Jul 22 10:04:45 2013 +0200
>> @@ -187,8 +187,10 @@
>>     > # this next line is okay
>>     > raise SomeException(arg1, arg2)
>>     > EOF
>> -  $ "$check_code" raise-format.py
>> +  $ "$check_code" not-existing.py raise-format.py
>> +  Skipping*not-existing.py* (glob)
>>     raise-format.py:1:
>>      > raise SomeException, message
>>      don't use old-style two-argument raise, use Exception(message)
>>     [1]
>> +
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff -r 004f965630d9 -r e6de31834294 contrib/check-code.py
--- a/contrib/check-code.py	Fre Jul 12 11:14:42 2013 +0900
+++ b/contrib/check-code.py	Mon Jul 22 10:04:45 2013 +0200
@@ -407,7 +407,11 @@ 
                 print "Skipping %s for %s it doesn't match %s" % (
                        name, match, f)
             continue
-        fp = open(f)
+        try:
+            fp = open(f)
+        except IOError, e:
+            print "Skipping %s, %s" % (f, str(e).split(':', 1)[0])
+            continue
         pre = post = fp.read()
         fp.close()
         if "no-" "check-code" in pre:
diff -r 004f965630d9 -r e6de31834294 tests/test-check-code.t
--- a/tests/test-check-code.t	Fre Jul 12 11:14:42 2013 +0900
+++ b/tests/test-check-code.t	Mon Jul 22 10:04:45 2013 +0200
@@ -187,8 +187,10 @@ 
   > # this next line is okay
   > raise SomeException(arg1, arg2)
   > EOF
-  $ "$check_code" raise-format.py
+  $ "$check_code" not-existing.py raise-format.py
+  Skipping*not-existing.py* (glob)
   raise-format.py:1:
    > raise SomeException, message
    don't use old-style two-argument raise, use Exception(message)
   [1]
+