Code review comment for lp:~parthm/bzr/376388-dot-bazaar-ownership

Revision history for this message
Martin Packman (gz) wrote :

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.

review: Needs Fixing

« Back to merge proposal