Code review comment for lp:~spiv/bzr/checkout-tags-propagation-603395-2.2

38 - dest_dict = to_tags.get_tag_dict()
39 - result, conflicts = self._reconcile_tags(source_dict, dest_dict,
40 - overwrite)
41 - if result != dest_dict:
42 - to_tags._set_tag_dict(result)
43 + master = to_tags.branch.get_master_branch()
44 + try:
45 + if master is not None:
46 + master.lock_write()
47 + conflicts = self._merge_to(to_tags, source_dict, overwrite)
48 + if master is not None:
49 + conflicts += self._merge_to(master.tags, source_dict,
50 + overwrite)
51 + finally:
52 + if master is not None:
53 + master.unlock()

Isn't this better written as:

master = to_tags.branch.get_master_branch()
if master is not None:
    master.lock_write()
    try:
      conflicts = ...
    finally:
      master.unlock()

Note that the way you wrote it, if we fail to take the write lock in the first place (no such permission), we end up still calling unlock.

I think you wrote it this way because you check the conflicts anyway, but the code looks clearer to me if you just add an 'else: conflicts = ...' to the above statement.

I suppose either way it is a bit clumsy, since you want the merge to fail immediately if you can't propagate the tags to the master branch before you try to merge to the local one...

I think the logic is otherwise ok.