Bazaar

Merge lp:~mbp/bzr/402022-merge-preview into lp:~bzr/bzr/trunk-old

  1. 402022-merge-preview
  2. Merge into trunk-old
Proposed by Martin Pool
Status: Rejected
Rejected by: Martin Pool
Proposed branch: lp:~mbp/bzr/402022-merge-preview
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 112 lines
To merge this branch: bzr merge lp:~mbp/bzr/402022-merge-preview
Reviewer Review Type Date Requested Status
Martin Pool Disapprove
Review via email: mp+9233@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

This is a minor change to cmd_merge's use of the cleanup list, so that it doesn't break the way qbzr wrapped this command. I think it's at least somewhat cleaner anyhow, and you could make a case that having a list of cleanups to run when the command completes would help extension in other cases...

Revision history for this message
Aaron Bentley (abentley) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/402022-merge-preview into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This is a minor change to cmd_merge's use of the cleanup list, so that it doesn't break the way qbzr wrapped this command.

On the other hand, it's an API break, and it will break bzr-pipeline.
Since we've already had an API break, we might as well stick with the
one we've got.

I'm surprised to see this patch being proposed on the basis of not
breaking qbzr. bzr API updates break bzrtools all the time, and I never
ask for them to be reverted just so that I don't have to update bzrtools.

> I think it's at least somewhat cleaner anyhow, and you could make a case that having a list of cleanups to run when the command completes would help extension in other cases...

I don't really agree, though I might be biased since I wrote those changes.

I don't think it's good practice to initialize instance variables
outside the constructor. Even if you initialize them to None and assign
to them later, they should still be valid names for the lifetime of the
object

I'm not really comfortable with methods having hidden side-effects, such
as the creation and mutation of self.cleanups.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkppSeQACgkQ0F+nu1YWqI3HgACeP1CaOl/wA9AjV7Ma6atRBks7
2s8AoIi57lwoenWUfEVBjZcFuxjnmN+r
=mOAE
-----END PGP SIGNATURE-----

Revision history for this message
Martin Pool (mbp) wrote :

2009/7/24 Aaron Bentley <email address hidden>:
>> This is a minor change to cmd_merge's use of the cleanup list, so that it doesn't break the way qbzr wrapped this command.
>
> On the other hand, it's an API break, and it will break bzr-pipeline.
> Since we've already had an API break, we might as well stick with the
> one we've got.
>
> I'm surprised to see this patch being proposed on the basis of not
> breaking qbzr.  bzr API updates break bzrtools all the time, and I never
> ask for them to be reverted just so that I don't have to update bzrtools.

I don't normally revert things for the sake of qbzr, I just started on
this bug because it was originally reported as a bzr bug, and it
seemed so to me.

If bzr breaks bzrtools and you want to propose a patch to restore an
inadvertently broken api you're welcome to. I think we've taken at
least some of them in the past.

>
>>  I think it's at least somewhat cleaner anyhow, and you could make a case that having a list of cleanups to run when the command completes would help extension in other cases...
>
> I don't really agree, though I might be biased since I wrote those changes.
>
> I don't think it's good practice to initialize instance variables
> outside the constructor.  Even if you initialize them to None and assign
> to them later, they should still be valid names for the lifetime of the
> object

Good point.

> I'm not really comfortable with methods having hidden side-effects, such
> as the creation and mutation of self.cleanups.

Well, 'hidden side effects' sound pretty bad by definition. But if
code is in a method of a stateful object it's reasonable for it to
mutate the object. For me the question is, is it reasonable for that
to be state on the cmd object; at present it almost is. Is that the
main reason you want to pass it?

--
Martin <http://launchpad.net/~mbp/>

Revision history for this message
Aaron Bentley (abentley) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> 2009/7/24 Aaron Bentley <email address hidden>:
>> I'm surprised to see this patch being proposed on the basis of not
>> breaking qbzr. bzr API updates break bzrtools all the time, and I never
>> ask for them to be reverted just so that I don't have to update bzrtools.
>
> I don't normally revert things for the sake of qbzr, I just started on
> this bug because it was originally reported as a bzr bug, and it
> seemed so to me.

If qbzr is failing this way, it must not be properly declaring which
version of bzrlib it supports, so whether or not it's a bug in bzr,
there's definitely a bug in qbzr.

> If bzr breaks bzrtools and you want to propose a patch to restore an
> inadvertently broken api you're welcome to. I think we've taken at
> least some of them in the past.

I would only ask that for a case where an API stopped working, not a
case where it changed it parameters.

>> I'm not really comfortable with methods having hidden side-effects, such
>> as the creation and mutation of self.cleanups.
>
> Well, 'hidden side effects' sound pretty bad by definition. But if
> code is in a method of a stateful object it's reasonable for it to
> mutate the object.

Our command objects aren't usually stateful, but even for stateful
objects, we should not mutate state where it's not expected. For
example, I would never expect list.__str__() to mutate observable state,
even though __str__ is only interesting because list is stateful.

Misunderstanding the side-effects of a method can cause mistakes. For
example, if pipeline overrode cmd_merge.get_merger_from_uncommitted to
call self.run, self.cleanups would get wiped.

> For me the question is, is it reasonable for that
> to be state on the cmd object; at present it almost is. Is that the
> main reason you want to pass it?

I guess I usually treat them as stateless, but they're really somewhere
between functions and classes. If we're going to treat them as
stateful, I'd like to do the initialization in a constructor. Even so,
I worry that an instance cleanups variable could lead to scope
confusion. It would be nice not to run any cleanups that were already
in self.cleanups when run was called.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkpputYACgkQ0F+nu1YWqI3TJACeOpeV66en6AP+wqECx5+t6Pf0
zFcAn0DgH8qZsanrMXuwM9uFsG28WbnA
=B9NY
-----END PGP SIGNATURE-----

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Aaron Bentley wrote:
> Martin Pool wrote:
>> 2009/7/24 Aaron Bentley <email address hidden>:
>>> I'm surprised to see this patch being proposed on the basis of not
>>> breaking qbzr. bzr API updates break bzrtools all the time, and I never
>>> ask for them to be reverted just so that I don't have to update bzrtools.
>> I don't normally revert things for the sake of qbzr, I just started on
>> this bug because it was originally reported as a bzr bug, and it
>> seemed so to me.
>
> If qbzr is failing this way, it must not be properly declaring which
> version of bzrlib it supports, so whether or not it's a bug in bzr,
> there's definitely a bug in qbzr.

Well, it is overriding a private method, so it isn't a method that we
bump the api versioning.

I think the main question is whether something like "cleanups" is valid
as a member variable. Aka state that is preserved between calls. And IMO
it does seem like "hey this is something you need to clean up when you
get a chance" is a reasonable thing to put there. I suppose the major
problem is if various things need to have different lifetimes before
they are cleaned up.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkpp1VMACgkQJdeBCYSNAANWaACgp9pyCQtQQ0i5V+2QAHiydiTl
4AQAoK8dfZ4IEiASRDW6RzDXafQuc8YW
=6UlR
-----END PGP SIGNATURE-----

Revision history for this message
Martin Pool (mbp) wrote :

2009/7/24 Aaron Bentley <email address hidden>:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Martin Pool wrote:
>> 2009/7/24 Aaron Bentley <email address hidden>:
>>> I'm surprised to see this patch being proposed on the basis of not
>>> breaking qbzr.  bzr API updates break bzrtools all the time, and I never
>>> ask for them to be reverted just so that I don't have to update bzrtools.
>>
>> I don't normally revert things for the sake of qbzr, I just started on
>> this bug because it was originally reported as a bzr bug, and it
>> seemed so to me.
>
> If qbzr is failing this way, it must not be properly declaring which
> version of bzrlib it supports, so whether or not it's a bug in bzr,
> there's definitely a bug in qbzr.

It should declare which ones it supports, and yet I think that's not a
really satisfactory way to close the bug. API skew between plugins
and bzr is apparently a pain point for our users, and just reporting
that the plugin doesn't work, while it removes some confusion, is not
a very satisfactory answer for the user.

This is a larger issue than is appropriate to discuss in this
proposal, but I do think if we can avoid breaking existing plugins at
a reasonable cost, we should.

--
Martin <http://launchpad.net/~mbp/>

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

Can we get a final decision on this patch? We're getting really close to the next release, so either we'll break or not. Most likely qbzr has had to sort out the results already.

Personally I'm not a big fan of qbzr hijacking cmd_merge anyway. If you wanted a gui merge, then use "bzr qmerge"...

My personal feeling is this might have been the way to go from the beginning, but we've already broken compatibility such that qbzr has to deal with the fallout. Better to go with our "stable vs development" approach to get more real stability, and not worry about it here.

Revision history for this message
Martin Pool (mbp) wrote :

> Can we get a final decision on this patch? We're getting really close to the
> next release, so either we'll break or not. Most likely qbzr has had to sort
> out the results already.
>
> Personally I'm not a big fan of qbzr hijacking cmd_merge anyway. If you wanted
> a gui merge, then use "bzr qmerge"...
>
> My personal feeling is this might have been the way to go from the beginning,
> but we've already broken compatibility such that qbzr has to deal with the
> fallout. Better to go with our "stable vs development" approach to get more
> real stability, and not worry about it here.

(I thought I already responded to this but apparently not...)

I put this up as what seemed to me like easier than fixing it in qbzr. I'm not particularly attached to the patch and as you say they have probably worked around it. If Alexander or Gary or someone really wants it, they can re-submit it.

review: Disapprove
Revision history for this message
Martin Pool (mbp) wrote :

If qbzr have worked around it, or will, I'm happy to withdraw this patch.

On Jul 30, 2009 3:54 AM, "John A Meinel" <email address hidden> wrote:

Can we get a final decision on this patch? We're getting really close to the
next release, so either we'll break or not. Most likely qbzr has had to sort
out the results already.

Personally I'm not a big fan of qbzr hijacking cmd_merge anyway. If you
wanted a gui merge, then use "bzr qmerge"...

My personal feeling is this might have been the way to go from the
beginning, but we've already broken compatibility such that qbzr has to deal
with the fallout. Better to go with our "stable vs development" approach to
get more real stability, and not worry about it here.

--
https://code.edge.launchpad.net/~mbp/bzr/402022-merge-preview/+merge/9233

You are the owner of lp:~mbp/bzr/402022-merge-preview.

Unmerged revisions

4564. By Martin Pool

News for 402022

4563. By Martin Pool

Make cmd_merge.cleanups an attribute not a passed-around variable

4562. By Canonical.com Patch Queue Manager <email address hidden>

(robertc) Support timestamping subunit streams. (Robert Collins)

4561. By Canonical.com Patch Queue Manager <email address hidden>

(igc) Allow rename of items already removed from the inventory
 (Marius Kruger)

4560. By Canonical.com Patch Queue Manager <email address hidden>

(jam) Add AbsentContentFactory.get_bytes_as,
 which just raises a better error.

4559. By Canonical.com Patch Queue Manager <email address hidden>

(mbp) workaround for ftp servers without APPE

4558. By Canonical.com Patch Queue Manager <email address hidden>

(mbp) various UIFactory cleanups including bug 387717

4557. By Canonical.com Patch Queue Manager <email address hidden>

(robertc) Add interface enforcement for the behaviour of iter_changes
 with missing subtrees with explicit paths - the whole subtree
 is returned. (Robert Collins)

4556. By Canonical.com Patch Queue Manager <email address hidden>

(jam) Bug #375867,
 don't prompt for password if ssh host doesn't support password auth.

4555. By Canonical.com Patch Queue Manager <email address hidden>

(andrew) Fix minor KeyError bug in -Dhpss when logging requests for
 unregistered methods.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-07-28 08:14:15 +0000
3+++ NEWS 2009-07-29 07:36:07 +0000
4@@ -38,6 +38,9 @@
5 * BranchBuilder now accepts timezone to avoid test failures in countries far
6 from GMT. (Vincent Ladeuil, #397716)
7
8+* Fix ``bzr merge --preview`` failing with TypeError when qbzr is
9+ installed. (Martin Pool, #402022)
10+
11 * The environment variable ``BZR_PROGRESS_BAR`` set to either ``text`` or ``none``
12 always forces progress bars either on or off respectively. Otherwise,
13 they're turned on if ``TERM`` is not ``dumb`` and stderr is a terminal.
14
15=== modified file 'bzrlib/builtins.py'
16--- bzrlib/builtins.py 2009-07-27 06:22:57 +0000
17+++ bzrlib/builtins.py 2009-07-29 07:36:07 +0000
18@@ -3645,12 +3645,13 @@
19 view_info = _get_view_info_for_change_reporter(tree)
20 change_reporter = delta._ChangeReporter(
21 unversioned_filter=tree.is_ignored, view_info=view_info)
22- cleanups = []
23+ # callables to be run in reverse order when the command completes
24+ self.cleanups = []
25 try:
26 pb = ui.ui_factory.nested_progress_bar()
27- cleanups.append(pb.finished)
28+ self.cleanups.append(pb.finished)
29 tree.lock_write()
30- cleanups.append(tree.unlock)
31+ self.cleanups.append(tree.unlock)
32 if location is not None:
33 try:
34 mergeable = bundle.read_mergeable_from_url(location,
35@@ -3672,8 +3673,7 @@
36 if revision is not None and len(revision) > 0:
37 raise errors.BzrCommandError('Cannot use --uncommitted and'
38 ' --revision at the same time.')
39- merger = self.get_merger_from_uncommitted(tree, location, pb,
40- cleanups)
41+ merger = self.get_merger_from_uncommitted(tree, location, pb)
42 allow_pending = False
43
44 if merger is None:
45@@ -3698,26 +3698,26 @@
46 return 0
47 merger.check_basis(False)
48 if preview:
49- return self._do_preview(merger, cleanups)
50+ return self._do_preview(merger)
51 elif interactive:
52- return self._do_interactive(merger, cleanups)
53+ return self._do_interactive(merger)
54 else:
55 return self._do_merge(merger, change_reporter, allow_pending,
56 verified)
57 finally:
58- for cleanup in reversed(cleanups):
59+ for cleanup in reversed(self.cleanups):
60 cleanup()
61
62- def _get_preview(self, merger, cleanups):
63+ def _get_preview(self, merger):
64 tree_merger = merger.make_merger()
65 tt = tree_merger.make_preview_transform()
66- cleanups.append(tt.finalize)
67+ self.cleanups.append(tt.finalize)
68 result_tree = tt.get_preview_tree()
69 return result_tree
70
71- def _do_preview(self, merger, cleanups):
72+ def _do_preview(self, merger):
73 from bzrlib.diff import show_diff_trees
74- result_tree = self._get_preview(merger, cleanups)
75+ result_tree = self._get_preview(merger)
76 show_diff_trees(merger.this_tree, result_tree, self.outf,
77 old_label='', new_label='')
78
79@@ -3733,7 +3733,7 @@
80 else:
81 return 0
82
83- def _do_interactive(self, merger, cleanups):
84+ def _do_interactive(self, merger):
85 """Perform an interactive merge.
86
87 This works by generating a preview tree of the merge, then using
88@@ -3741,7 +3741,7 @@
89 and the preview tree.
90 """
91 from bzrlib import shelf_ui
92- result_tree = self._get_preview(merger, cleanups)
93+ result_tree = self._get_preview(merger)
94 writer = bzrlib.option.diff_writer_registry.get()
95 shelver = shelf_ui.Shelver(merger.this_tree, result_tree, destroy=True,
96 reporter=shelf_ui.ApplyReporter(),
97@@ -3812,14 +3812,12 @@
98 allow_pending = True
99 return merger, allow_pending
100
101- def get_merger_from_uncommitted(self, tree, location, pb, cleanups):
102+ def get_merger_from_uncommitted(self, tree, location, pb):
103 """Get a merger for uncommitted changes.
104
105 :param tree: The tree the merger should apply to.
106 :param location: The location containing uncommitted changes.
107 :param pb: The progress bar to use for showing progress.
108- :param cleanups: A list of operations to perform to clean up the
109- temporary directories, unfinalized objects, etc.
110 """
111 location = self._select_branch_location(tree, location)[0]
112 other_tree, other_path = WorkingTree.open_containing(location)