[Mailman-Developers] [Mailman-checkins] [Branch ~mailman-coders/mailman/3.0] (no title)

[Barry Warsaw]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Mark, thanks for fixing this.  Just a couple of quick comments...

On Jun 28, 2007, at 1:27 PM, noreply at launchpad.net wrote:

> @@ -100,20 +101,25 @@
>                  os.chown(path, -1, MAILMAN_GID)
>              else:
>                  print
> -        # All directories must be at least rwxrwsr-x.  Don't check  
> the private
> -        # archive directory or database directory themselves since  
> these are
> -        # checked in checkarchives() and checkarchivedbs() below.
> +        # Most directories must be at least rwxrwsr-x.
> +        # The private archive directory  and database directory  
> must be at
> +        # least rwxrws---.  Their 'other' permissions are checked in
> +        # checkarchives() and checkarchivedbs() below.  Their  
> 'user' and
> +        # 'group' permissions are checked here.
> +        # The directories under qfiles should be rwxrws---.  Their  
> 'user' and
> +        # 'group' permissions are checked here.  Their 'other'  
> permissions
> +        # aren't checked.
>          private = config.PRIVATE_ARCHIVE_FILE_DIR
> -        if path == private or (os.path.commonprefix((path,  
> private)) == private
> -                               and os.path.split(path)[1] ==  
> 'database'):
> -            continue
> -        # The directories under qfiles should have a more limited  
> permission
> -        if os.path.commonprefix((path, config.QUEUE_DIR)) ==  
> config.QUEUE_DIR:
> +        if path == private or \
> +                  (os.path.commonprefix((path, private)) == private
> +                   and os.path.split(path)[1] == 'database'):

This is probably better style:

         if path == private or (
             os.path.commonprefix((path, private)) == private
             and os.path.split(path)[1] == 'database'):
             # then...
             targetperms = PRIVATEPERMS

It eliminates a backslash (always ugly ;) though it kind of begs for  
the 'then...' comment because of the way the columns line up.

> +            targetperms = PRIVATEPERMS
> +        elif os.path.commonprefix((path, config.QUEUE_DIR)) \
> +              == config.QUEUE_DIR:
>              targetperms = QFILEPERMS

Similarly, this removes the need for a backslash:

         elif (os.path.commonprefix((path, config.QUEUE_DIR))
               == config.QUEUE_DIR):
             targetperms = QFILEPERMS

You probably can't do much better without storing config.QUEUE_DIR in  
a local variable.

Cheers,
- -Barry



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)

iQCVAwUBRoP6mHEjvBPtnXfVAQL8qQP9GZzd1T5xdGGmcWvGR+lpZPUpZPGWJhCq
7yH54o8E45pBisH6LVVJR3KoS7/xfJGevj3t/tQ6gLgiOsdeiLaTWQJ05UBBiHAr
r8+7FWWgUrc84o7EvkhgXXsqCVT/iFTW1yVWmlsVGLm0ezdl6oOPTcrpJ2biA63S
3Hn//tQKi2o=
=rA0r
-----END PGP SIGNATURE-----