Code review comment for lp:~parthm/bzr/376388-dot-bazaar-ownership

56
57 +def copy_ownership(dst, src=None):
58 + """copy usr/grp ownership from src file/dir to dst file/dir.
59 + If src is None, the containing directory is used as source."""
60 + if os.name != 'posix':
61 + return False
62 +
63 + if src == None:
64 + src = os.path.dirname(dst)
65 + if src == '':
66 + src = '.'
67 +
68 + try:
69 + s = os.stat(src)
70 + os.chown(dst, s.st_uid, s.st_gid)
71 + except OSError, e:
72 + trace.warning("IOError: %s. Unable to copy ownership from '%s' to '%s'" % (e, src, dst))
73 + return True
74 +

Your approach of catching OSError is fne.

Normally we would write the message as:

 Unable to copy ownership from "%s" to "%s": %s

with the message at the end.

I think rather than checking os.name it would be better to check if os.chown exists, ie

chown = getattr(os, 'chown')
if chown is None: return

Why does this return a boolean? If it needs to, you should document and maybe test the value. Otherwise, don't return anything.

For consistency our docstrings have:

 * Sentence capitalization and punctuation
 * If possible a single sentence on the first line
 * A blank line between paragraphs
 * The closing quotes on a separate line

See http://www.python.org/dev/peps/pep-0008/ and the developer docs

114 +
115 +class _ChownFeature(tests.Feature):
116 + """os.chown is supported"""
117 +
118 + def _probe(self):
119 + return os.name == 'posix' and hasattr(os, 'chown')
120 +
121 +ChownFeature = _ChownFeature()
122 +

Normally the instance of the feature object should be named like an instance, ie lowercase chown_feature. Sorry this is confusing, we changed it a while ago.

Thanks!

Martin