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

Revision history for this message
Parth Malwankar (parthm) 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.

Thanks for your comments Martin. I have updated the patch.

« Back to merge proposal