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

You have some PEP8 issues (see doc/developpers/HACKING.txt section Code layout),
namely:
- no spaces around '='
27 + f = osutils.open_with_ownership(path, 'wb', ownership_src = parent)

- lines should be no more than 79 characters (watch for your tests docstrings
  in particular)

58 +def parent_dir(path):

This looks weird, why not just embed that code in copy_ownership like your docstring
seem to imply ?

87 +def open_with_ownership(filename, mode='r', bufsize=-1, ownership_src=None):
88 + """This function wraps the python builtin open. filename, mode and bufsize

From "Code Layout" cited above:
One often-missed requirement is that the first line of docstrings
should be a self-contained one-sentence summary.

135 + # we can't overrideAttr os.chown on OS that doesn't support chown
136 + if os.name == 'posix' and hasattr(os, 'chown'):
137 + self.overrideAttr(os, 'chown', self._dummy_chown)

You don't need to protect the call since your tests already depends on tests.ChownFeature.

145 + def test_mkdir_with_ownership_no_chown(self):
146 + """ensure that osutils.mkdir_with_ownership does not chown without ownership_src arg"""
147 + self.requireFeature(tests.ChownFeature)

You can avoid avoid duplicating that check for each test by doing:

class TestCreationOps(tests.TestCaseInTempDir):

    _test_needs_features = [tests.ChownFeature]

142 + def _dummy_chown(self, path, uid, gid):
143 + self.path, self.uid, self.gid = path, uid, gid

Hmm, using a dummy here is questionable since you don't really test calling chown(),
I'd prefer a recording variant like:

def _instrumented_chown(self, path, uid, gid):
    self.calls.append((path, uid, gid))
    return os.chown(path, uid, gid)

so you can both check that it was called *and* verify what was done.

180 + ownsrc = '/'

Ouch ! You're playing with fire here, I realize you may want to test with a different
uid/gid than the current user but... you'd better create such a reference file explicitly.

On the overall, I'm still a bit doubtful on the whole approach, it seems fine for root
but what happens if we use 'sudo -Ujoe' ?