Bazaar

Merge lp:~spiv/bzr/remove-args_received into lp:bzr/2.0

  1. remove-args_received
  2. Merge into 2.0
Proposed by Andrew Bennetts
Status: Merged
Merge reported by: Andrew Bennetts
Merged at revision: not available
Proposed branch: lp:~spiv/bzr/remove-args_received
Merge into: lp:bzr/2.0
Diff against target: None lines
To merge this branch: bzr merge lp:~spiv/bzr/remove-args_received
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+11582@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

This is just a small cleanup I did as part of a larger change. It removes SmartServerRequest.dispatch_command, which I've been intending to do for ages. Instead callers now use args_received, which is more consistent with headers_received, etc. So now there's just one method to do this, and it is named consistently with the other methods.

It also fixes args_received to pass root_client_path to the command's constructor! This wasn't causing trouble in before because nothing (aside from a couple of tests) were calling args_received, but obviously matters now that dispatch_command is gone.

I'm not certain if this is appropriate for the 2.0 branch or just bzr.dev. The branch was created against 2.0 for maximum flexibility.

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

2009/9/11 Andrew Bennetts <email address hidden>:
> Andrew Bennetts has proposed merging lp:~spiv/bzr/remove-args_received into lp:bzr/2.0.
>
> Requested reviews:
>    bzr-core (bzr-core)
>
> This is just a small cleanup I did as part of a larger change.  It removes SmartServerRequest.dispatch_command, which I've been intending to do for ages.  Instead callers now use args_received, which is more consistent with headers_received, etc.  So now there's just one method to do this, and it is named consistently with the other methods.
>
> It also fixes args_received to pass root_client_path to the command's constructor!  This wasn't causing trouble in before because nothing (aside from a couple of tests) were calling args_received, but obviously matters now that dispatch_command is gone.
>
> I'm not certain if this is appropriate for the 2.0 branch or just bzr.dev.  The branch was created against 2.0 for maximum flexibility.

Thanks for doing it off 2.0.

I think generally (before reading this patch) we shouldn't be merging
cleanup-only fixes to that branch, because the risk that we will
inadvertently break something isn't justified by the benefits of
cleanups in code that we expect to make only moderate changes to.
However, if you/we think that a cleanup is going to enable fixing
other bugs, or interact with the fixes for other bugs, then it might
be worth merging. Or we could merge it later, if one of those cases
develops, though then you have to remember to do it.

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

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

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

Martin Pool wrote:
> 2009/9/11 Andrew Bennetts <email address hidden>:
>> Andrew Bennetts has proposed merging lp:~spiv/bzr/remove-args_received into lp:bzr/2.0.
>>
>> Requested reviews:
>> bzr-core (bzr-core)
>>
>> This is just a small cleanup I did as part of a larger change. It removes SmartServerRequest.dispatch_command, which I've been intending to do for ages. Instead callers now use args_received, which is more consistent with headers_received, etc. So now there's just one method to do this, and it is named consistently with the other methods.
>>
>> It also fixes args_received to pass root_client_path to the command's constructor! This wasn't causing trouble in before because nothing (aside from a couple of tests) were calling args_received, but obviously matters now that dispatch_command is gone.
>>
>> I'm not certain if this is appropriate for the 2.0 branch or just bzr.dev. The branch was created against 2.0 for maximum flexibility.
>
> Thanks for doing it off 2.0.
>
> I think generally (before reading this patch) we shouldn't be merging
> cleanup-only fixes to that branch, because the risk that we will
> inadvertently break something isn't justified by the benefits of
> cleanups in code that we expect to make only moderate changes to.
> However, if you/we think that a cleanup is going to enable fixing
> other bugs, or interact with the fixes for other bugs, then it might
> be worth merging. Or we could merge it later, if one of those cases
> develops, though then you have to remember to do it.
>

So I think what Martin is saying is:
  review: approve

but merge it into trunk instead of 2.0 for now, unless you think it is
really necessary there. Instead, we still have the branch if we decide
we need the cleanup to build future bugfixes easily.

And if *he* isn't saying approve, *I* am :)
John
=:->

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

iEYEARECAAYFAkqqUeIACgkQJdeBCYSNAAPFbwCaAmLocfnuObVPgzYr26EJNWwz
VEsAnjN5CAAHBSYkh/L6/5mLuwhAIMF9
=bE7w
-----END PGP SIGNATURE-----

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

>>>>> "jam" == John A Meinel <email address hidden> writes:

<snip/>

    jam> So I think what Martin is saying is:
    jam> review: approve

    jam> but merge it into trunk instead of 2.0 for now, unless you think it is
    jam> really necessary there. Instead, we still have the branch if we decide
    jam> we need the cleanup to build future bugfixes easily.

I'm not sure about that bit.

I read it like: if it's cleanup -> trunk, if it helps catch bugs -> 2.0.

And in fact, Andrew said that it *fixes* latent bug[s] so I'd be
inclined to say merge to trunk *and* 2.0.

Just my 2.0 cents (haha couldn't resist :)

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

2009/9/12 Vincent Ladeuil <email address hidden>:
>    jam> So I think what Martin is saying is:
>    jam>   review: approve
>
>    jam> but merge it into trunk instead of 2.0 for now, unless you think it is
>    jam> really necessary there. Instead, we still have the branch if we decide
>    jam> we need the cleanup to build future bugfixes easily.

That's what I meant.

>
> I'm not sure about that bit.
>
> I read it like: if it's cleanup -> trunk, if it helps catch bugs -> 2.0.

That's the short form.

> And in fact, Andrew said that it *fixes* latent bug[s] so I'd be
> inclined to say merge to trunk *and* 2.0.

I didn't see him say that..?

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

Revision history for this message
Andrew Bennetts (spiv) wrote :

> > And in fact, Andrew said that it *fixes* latent bug[s] so I'd be
> > inclined to say merge to trunk *and* 2.0.
>
> I didn't see him say that..?

I didn't.

Thanks everyone for the feedback. I'm going to submit it to trunk only for now, as it really is a cleanup rather than a bug fix. It doesn't fix any practical bugs that would affect 2.0; it's essentially just changing from having two methods for the same thing, one deprecated and one unused and buggy, to just one method (undeprecated and unbuggy).

It's good to have a clearer idea of what is suitable for 2.0 vs. trunk.

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

>>>>> "Andrew" == Andrew Bennetts <email address hidden> writes:

    >> > And in fact, Andrew said that it *fixes* latent bug[s] so I'd
    Andrew> be
    >> > inclined to say merge to trunk *and* 2.0.
    >>
    >> I didn't see him say that..?

    Andrew> I didn't.

    Andrew> Thanks everyone for the feedback. I'm going to submit it to
    Andrew> trunk only for now, as it really is a cleanup rather than a bug
    Andrew> fix. It doesn't fix any practical bugs that would affect 2.0;
    Andrew> it's essentially just changing from having two methods for the
    Andrew> same thing, one deprecated and one unused and buggy,

Yeah, I misread that as buggy and potentially used (when in fact
it wasn't used at all).

   Vincent

Revision history for this message
Andrew Bennetts (spiv) wrote :

This landed in lp:bzr. I'm marking this merge proposal as "Merged" to get it off the list of unmerged proposals. (Ideally perhaps I'd be able to retarget this proposal to lp:bzr instead.)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/smart/message.py'
2--- bzrlib/smart/message.py 2009-07-28 22:27:04 +0000
3+++ bzrlib/smart/message.py 2009-09-11 06:07:05 +0000
4@@ -134,7 +134,7 @@
5
6 def _args_received(self, args):
7 self.expecting = 'body'
8- self.request_handler.dispatch_command(args[0], args[1:])
9+ self.request_handler.args_received(args)
10 if self.request_handler.finished_reading:
11 self._response_sent = True
12 self.responder.send_response(self.request_handler.response)
13
14=== modified file 'bzrlib/smart/protocol.py'
15--- bzrlib/smart/protocol.py 2009-07-21 03:11:33 +0000
16+++ bzrlib/smart/protocol.py 2009-09-11 06:07:05 +0000
17@@ -145,7 +145,7 @@
18 self.request = request.SmartServerRequestHandler(
19 self._backing_transport, commands=request.request_handlers,
20 root_client_path=self._root_client_path)
21- self.request.dispatch_command(req_args[0], req_args[1:])
22+ self.request.args_received(req_args)
23 if self.request.finished_reading:
24 # trivial request
25 self.unused_data = self.in_buffer
26
27=== modified file 'bzrlib/smart/request.py'
28--- bzrlib/smart/request.py 2009-08-27 05:22:14 +0000
29+++ bzrlib/smart/request.py 2009-09-11 06:07:05 +0000
30@@ -292,15 +292,6 @@
31 # cannot read after this.
32 self.finished_reading = True
33
34- def dispatch_command(self, cmd, args):
35- """Deprecated compatibility method.""" # XXX XXX
36- try:
37- command = self._commands.get(cmd)
38- except LookupError:
39- raise errors.UnknownSmartMethod(cmd)
40- self._command = command(self._backing_transport, self._root_client_path)
41- self._run_handler_code(self._command.execute, args, {})
42-
43 def _run_handler_code(self, callable, args, kwargs):
44 """Run some handler specific code 'callable'.
45
46@@ -343,7 +334,8 @@
47 command = self._commands.get(cmd)
48 except LookupError:
49 raise errors.UnknownSmartMethod(cmd)
50- self._command = command(self._backing_transport)
51+ self._command = command(
52+ self._backing_transport, self._root_client_path)
53 self._run_handler_code(self._command.execute, args, {})
54
55 def end_received(self):
56
57=== modified file 'bzrlib/tests/test_smart_transport.py'
58--- bzrlib/tests/test_smart_transport.py 2009-07-08 07:03:38 +0000
59+++ bzrlib/tests/test_smart_transport.py 2009-09-11 06:07:05 +0000
60@@ -1248,7 +1248,7 @@
61
62 def test_hello(self):
63 handler = self.build_handler(None)
64- handler.dispatch_command('hello', ())
65+ handler.args_received(('hello',))
66 self.assertEqual(('ok', '2'), handler.response.args)
67 self.assertEqual(None, handler.response.body)
68
69@@ -1268,7 +1268,7 @@
70 """The response for a read-only error is ('ReadOnlyError')."""
71 handler = self.build_handler(self.get_readonly_transport())
72 # send a mkdir for foo, with no explicit mode - should fail.
73- handler.dispatch_command('mkdir', ('foo', ''))
74+ handler.args_received(('mkdir', 'foo', ''))
75 # and the failure should be an explicit ReadOnlyError
76 self.assertEqual(("ReadOnlyError", ), handler.response.args)
77 # XXX: TODO: test that other TransportNotPossible errors are
78@@ -1279,14 +1279,14 @@
79 def test_hello_has_finished_body_on_dispatch(self):
80 """The 'hello' command should set finished_reading."""
81 handler = self.build_handler(None)
82- handler.dispatch_command('hello', ())
83+ handler.args_received(('hello',))
84 self.assertTrue(handler.finished_reading)
85 self.assertNotEqual(None, handler.response)
86
87 def test_put_bytes_non_atomic(self):
88 """'put_...' should set finished_reading after reading the bytes."""
89 handler = self.build_handler(self.get_transport())
90- handler.dispatch_command('put_non_atomic', ('a-file', '', 'F', ''))
91+ handler.args_received(('put_non_atomic', 'a-file', '', 'F', ''))
92 self.assertFalse(handler.finished_reading)
93 handler.accept_body('1234')
94 self.assertFalse(handler.finished_reading)
95@@ -1300,7 +1300,7 @@
96 """'readv' should set finished_reading after reading offsets."""
97 self.build_tree(['a-file'])
98 handler = self.build_handler(self.get_readonly_transport())
99- handler.dispatch_command('readv', ('a-file', ))
100+ handler.args_received(('readv', 'a-file'))
101 self.assertFalse(handler.finished_reading)
102 handler.accept_body('2,')
103 self.assertFalse(handler.finished_reading)
104@@ -1315,7 +1315,7 @@
105 """'readv' when a short read occurs sets the response appropriately."""
106 self.build_tree(['a-file'])
107 handler = self.build_handler(self.get_readonly_transport())
108- handler.dispatch_command('readv', ('a-file', ))
109+ handler.args_received(('readv', 'a-file'))
110 # read beyond the end of the file.
111 handler.accept_body('100,1')
112 handler.end_of_body()
113@@ -2542,8 +2542,8 @@
114 self.calls.append(('end_received',))
115 self.finished_reading = True
116
117- def dispatch_command(self, cmd, args):
118- self.calls.append(('dispatch_command', cmd, args))
119+ def args_received(self, args):
120+ self.calls.append(('args_received', args))
121
122 def accept_body(self, bytes):
123 self.calls.append(('accept_body', bytes))

Subscribers

People subscribed via source and target branches