Code review comment for lp:~vila/bzr/525571-lock-bazaar-conf-files

Ok, actual review stuff:
 the docstring layout is wrong, please nuke the \.

We should check for branches first, not config files, because branch locks are the common case and break-lock doesn't need to be slow.

This change is suspicous:

152 def _write_config_file(self):
153 - f = file(self._get_filename(), "wb")
154 + fname = self._get_filename()
155 + conf_dir = os.path.dirname(fname)
156 + ensure_config_dir_exists(conf_dir)
157 + f = file(fname, "wb")
158 try:
159 - osutils.copy_ownership_from_path(f.name)
160 + osutils.copy_ownership_from_path(fname)
161 self._get_parser().write(f)
162 finally:
163 f.close()
164

It appears to be adding a new stat/mkdir check, at the wrong layer.

missing VWS:

172 + """
173 + lock_name = 'lock'

Ditto here:

181 + def lock_write(self, token=None):
182 + if self.lock is None:
183 + ensure_config_dir_exists(self.dir)
184 + self._lock = lockdir.LockDir(self.transport, self.lock_name)
185 + self._lock.lock_write(token)
186 + return lock.LogicalLockResult(self.unlock)

If the dir doesn't exist, something is wrong - we really shouldn't have gotten this far without it, should we?

Docstrings!!!

I'll review the tests after the core is good.

Uh, you raise NoLockDir, but use it to mean 'A config directory that is not locked' which is very odd. Please consider something that means what you need. Also see my ordering suggestion which may help you not to need this at all.