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.

break_lock() for wt, branch, repo gives no indication about whether it fails or succeeds.
Trying the conf files first was the easiest.

Regarding performance, I think we don't care at all, the user is supposed to first check that
the lock is not hold by a running process (or someone else) which requires seconds in the best case
or occur long after the lock has been created.

>
> 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.

No adding there, code duplication removal instead, ensure_config_dir_exists() was called anyway.

>
> missing VWS:
>
> 172 + """
> 173 + lock_name = 'lock'

Fixed.
>
>
>
> 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?

When the config file didn't exist, the config dir needs to be created.

> Docstrings!!!

A bit terse but I will add some.

> 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.

I need something that means: "Oh, I wanted to break a lock there but there is no lock dir there,
surely I can't break a lock in that case". I fail to see the oddness :-/