Merge lp://staging/~spiv/bzr/checkout-tags-propagation-603395-2.2 into lp://staging/bzr/2.2

I should point out: this does have the perhaps unwanted side-effect that "bzr merge" can cause tags to be added the master even if the next command is "bzr commit". That's really <https://bugs.launchpad.net/bzr/+bug/99137> (tags are "permanently" propagated by merge), and this patch is just making the overall behaviour more consistent. I can see an argument that undoing the accidental limiting of the the effect of bug 99137 might not be acceptable in a stable branch, though.

A compromise would be to add a new optional argument to merge_to (or perhaps a whole new method) that callers can use to explicitly enable/disable propagation. I'm not sure if it would be better to default to enabled (so that plugins like builddeb can automatically benefit), or disabled (to minimise the change in behaviour, and also perhaps to avoid needing to tweak the API layers between cmd_merge and br.tags.merge_to to pass the flag). With 'bzr merge' not propagating we'd then rely on <https://code.launchpad.net/~spiv/bzr/tags-commit-propagation-603395-2.2> to propagate at commit time rather than merge time.

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.

John A Meinel wrote:
[...]
> 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.

Good point!

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

Really what I want is to use add_cleanup. It seemed like it was perhaps
overkill, so I didn't do that initially. But this review makes it clear
to me that it really would be worthwhile, despite incurring a larger
diff.