Merge lp://staging/~parthm/bzr/376388-dot-bazaar-ownership into lp://staging/bzr

This patch fixes bug #376388 by ensuring the .bazaar,
.bazaar/bazaar.conf and .bzr.log inherit usr and grp ownership
from the containing folder.

Following functions are added to osutils:
* copy_ownership: copies usr/grp ownership from src file/dir to dst file/dir
* mkdir: wraps os.mkdir and adds an optional arg ownership_src. When provided,
  the newly created dir is given ownership based on ownership_src.
* open: wraps __builtin__.open and adds an optional arg ownership_src. When provided,
  the newly created file is given ownership based on ownership_src.
* parent_dir: wraps os.path.dirname handling the special case of dirname returning ''
  by returning '.' instead.

config.py and trace.py are updated to use osutils.{mkdir,open} rather than the
base functions during creation of above mentioned files.

Whitebox tests are added to test_osutils.py to verify mkdir, open and parent_dir.
Manual testing for done using "sudo bzr whoami a@b".

ChmodFeature was added to tests/__init__.py to ensure os.chmod support for test
cases.

This proposal was originally based of 2.0:
https://code.launchpad.net/~parthm/bzr/2.0_376388_dot_bazaar_ownership/+merge/19593/
but is resubmitted now as it is meant to go into trunk and testing requires
overrideAttr support that is not available in 2.0 branch.

Similar issue filed as bug #524306

As mentioned in a different (and now deleted?) merge request, this clashes with that change as they both try and create a shadow of a builtin function in osutils, which I think is bad style.

In this case, I don't see why:

+ ownership_src = osutils.parent_dir(_bzr_log_filename)
+ bzr_log_file = osutils.open(_bzr_log_filename, 'at', buffering, ownership_src)

Is preferable to the more obvious:

+ bzr_log_file = open(_bzr_log_filename, 'at', 0) # unbuffered
+ osutils.copy_ownership(_bzr_log_filename,
+ osutils.parent_dir(_bzr_log_filename))

Would also be nice to avoid this junk entirely on windows, but that doesn't matter much.

> As mentioned in a different (and now deleted?) merge request, this clashes
> with that change as they both try and create a shadow of a builtin function in
> osutils, which I think is bad style.
>
> In this case, I don't see why:
>
> + ownership_src = osutils.parent_dir(_bzr_log_filename)
> + bzr_log_file = osutils.open(_bzr_log_filename, 'at', buffering,
> ownership_src)
>

Good point. Maybe we should have open_with_ownership (or something similar)
for which ownership_src is mandatory. This will keep the usage clean and
also not be confused with the builtin.

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' ?

Thanks for your comments Vincent.
I have updated the code based on your review comments.

The testing approach ( http://pastebin.com/CKhiAgL4 ) didn't work ( http://pastebin.com/ZPk2XGvv ), so it seems like we are stuck with this approach for now unless more ideas come through.

Based on the discussion vila and I had on IRC it seems that user foo cannot change ownership to user bar and chown would work only under sudo. Can such a situation arise? Not having a thorough test case or approach that works under all situations seems somewhat risky.

As discussed I am setting the status to Needs Review so that we can thrash out the solution some more as needed. I would be happy to try out any other ideas. Based on the outcome we could either update the patch or reject it.

See https://bugs.edge.launchpad.net/bzr/+bug/376388/comments/16,
only root can use chown.

> See https://bugs.edge.launchpad.net/bzr/+bug/376388/comments/16,
> only root can use chown.

One approach could be to catch OSError during chown. That way
the ownership would be correct under sudo and in the normal
case the user won't be bothered.

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

Thanks for the review Martin.
I have made the relevant changes.

I'll resolve the conflicts, tweak the feature name to be lowercase, and submit this.

Thanks Martin.