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

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

> This can be done completely non-racily with os.open - a really picky version
> could be a function something like:
>
> lomode = os.O_WRONLY | os.O_APPEND | osutils.O_TEXT
> while True:
> try:
> return os.open(filename, lomode)
> except OSError, e:
> if e.errno != errno.ENOENT:
> raise
> try:
> fd = os.open(filename, lomode | os.O_CREAT | os.O_EXCL)
> except OSError, e:
> if e.errno != errno.EEXIST:
> raise
> else:
> copy_ownership(filename, ownership_src)
> return fd
>
> And os.fdopen to make a regular file object.
>

Thanks for this.
This certainly looks good to me. Would this approach work on Mac? In the python docs[1] the availability of os.open is shown as Unix and Windows. I don't know much about Mac so I ask.

> Raises the question though, is it ever going to be right for a
> osutils.open_with_ownership function to chown the file if it's *not* just
> created it? Makes me wonder again whether bundling the chown into the open
> function makes any sense.

I agree, I don't see too much use of open_with_ownership in a non-create case.
I can try to create and test a patch based on the above approach which also deals with open_with_ownership. Its used only in one another place IIRC.

[1] http://docs.python.org/library/os.html#os.open

« Back to merge proposal