Code review comment for lp:~parthm/bzr/2.0_376388_dot_bazaar_ownership

Revision history for this message
Martin Pool (mbp) wrote :

Having a special case for /dev/null and NUL is a bit gross and probably unnecessary.

I think we should set the permissions only when the files are first created; changing the perms on existing files is more dangerous and more likely to cause nasty surprises.

I think we should catch errors in doing the chmod and turn them into user warnings to avoid bzr crashing entirely if you do have a strange situation.

I'd like to see the 'create a thing inheriting permissions' factored out; probably you need a variation for mkdir and for making a file.

Once that is split out, the cleanest way to test it is probably to monkeypatch os.chmod and then run that function. You can use overrideAttr to do this.

review: Needs Fixing

« Back to merge proposal