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

Proposed by Andrew Bennetts
Status: Rejected
Rejected by: Andrew Bennetts
Proposed branch: lp://staging/~spiv/bzr/checkout-tags-propagation-603395-2.2
Merge into: lp://staging/bzr/2.2
Diff against target: 423 lines (+272/-16)
8 files modified
NEWS (+13/-0)
bzrlib/branch.py (+1/-1)
bzrlib/builtins.py (+3/-1)
bzrlib/tag.py (+69/-13)
bzrlib/tests/blackbox/test_merge.py (+1/-1)
bzrlib/tests/blackbox/test_tags.py (+12/-0)
bzrlib/tests/per_branch/test_tags.py (+170/-0)
bzrlib/tests/test_tag.py (+3/-0)
To merge this branch: bzr merge lp://staging/~spiv/bzr/checkout-tags-propagation-603395-2.2
Reviewer Review Type Date Requested Status
Vincent Ladeuil Disapprove
John A Meinel Pending
Review via email: mp+40406@code.staging.launchpad.net

This proposal supersedes a proposal from 2010-11-01.

Description of the change

This is part of the fix for bug 603395. It makes BasicTags.merge_to also merge to the master branch, if there is one. This is consistent with e.g. set_tag. bzr-builddeb directly calls that API, and presumably expects merge_to in a checkout branch to update the master as well. Because bzr-builddeb does call this API directly I don't believe <https://code.launchpad.net/~spiv/bzr/tags-commit-propagation-603395-2.2/+merge/39733> (which has already landed) alone is enough to fix the UDD bug. bzr-svn also calls this API, so it is likely affected too. I know John commented on the other proposal that he thought it alone would be sufficient, but I'm unconvinced. (And even if this isn't necessary it still seems like a reasonable change to me, the previous behaviour was too low on the Rusty API usability scale IMO.)

I've dealt with my reservations on the original merge proposal: merge_to now accepts an optional kwarg, ignore_master, that cmd_merge uses to avoid prematurely propagating changes to the master. So I think this avoids any chance of regressing at all on #99137 (bzr revert will still not remove the new tags from the child branch, but that's not a regression). This does mean that external implementations of merge_to should be updated. I've documented the API change in NEWS, and made sure the callsite in bzr that does pass that kwarg falls back to not passing the kwarg. I've tested the fallback manually with bzr-svn (which has an external implementation of merge_to), and it appears to work as intended.

The tests take care to deal with all the permutations, including those that can only be triggered by an unfixed bzr operating on the checkout. See the test comments for details.

To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

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.

review: Needs Fixing
Revision history for this message
Andrew Bennetts (spiv) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Vincent Ladeuil (vila) wrote :

Our policy for stable branches is a strict 'no API changes'.

I realize you have gone very far in ensuring compatibility and that this impacts UDD but this still doesn't justify breaking our policy IMHO.

Are there some other ways to propagate the tags to the master branch without this patch ?

I'm otherwise +1 for landing this on trunk.

review: Disapprove
Revision history for this message
Vincent Ladeuil (vila) wrote :

More thoughts on this as I realize it's always frustrating to have a mp rejected.

My concerns here of course are: what if we break something ? Would that means we'll get into a whack-a-mole frenzy updating bzr and plugins introducing even more problems ?

Instead of focusing on the right fix on trunk...

What are your thoughts here and what do you plan to do next ?

Is there some way we can have some real-world testing here ? May be asking Jelmer for bzr-svn fallouts ? Do we have a clear understanding of *which* plugins are (or could be) impacted there ?

Revision history for this message
John A Meinel (jameinel) wrote :

I think I'm happy with the change, but I would agree with Vincent's hesitancy to land it in 2.2, vs just landing it in trunk.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches