Code review comment for lp:~vila/bzr/525571-minimal-atomic-write-bazaar-conf-files-2.1

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/525571-minimal-lock-bazaar-conf-files-2.1 into lp:bzr/2.1.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This is a minimal patch to address bug #525571 (concurrent config writers) without any attempt to cleanup anything ;)
>
>

1) If we know we are accessing a local file (since you are using open()
directly), then we could just use AtomicFile directly.

2) I'm a bit concerned about the need for
safe_unicode(self._get_filename()).

3) You can do

dirname, basename = osutils.split(fname)

rather than doing it 2x.

Personally, I'd really rather see AtomicFile than Transport for this.

Specifically, if fname was ever Unicode, this would break. As Transport
expects the 'relpath' to be a URL, and you are handing it a Unicode
string. Oh, and it would probably break if fname had any funny
characters like % in it.

 review: needsfixing

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwt9DoACgkQJdeBCYSNAAOE7ACgzcbdLqynPzKMStWmEZkVgOtA
2jYAn0BNItWLFxQCecwo/NHh2x7+jOCG
=EmU/
-----END PGP SIGNATURE-----