Code review comment for lp:~parthm/bzr/no-chown-if-bzrlog-exists

Revision history for this message
Parth Malwankar (parthm) wrote :

> + """Create a log file in while avoiding race.
>
> some kind of typo here.
>
> I would make the docstring be:
>
> Create the .bzr.log file.
>
> It inherits the ownership and permissions (masked by umask) from the
> containing
> directory to cope better with being run under sudo with $HOME still set to
> the
> user's homedir.
>

Done.

> Also the fact that you have a comment next to every copy_ownership()
> invocation suggests it should be renamed to copy_ownership_from_directory.
>

I have renamed it to copy_ownership_from_path as both file and
directory are accepted, though we use only parent directory in
the existing cases.

> To set the permissions you need to pass a third parameter to os.open(), which
> here should be 0666. This is separate from the O_APPEND etc mode.
>

I have added permission of 0644 and tested it out.
Thanks.

« Back to merge proposal