1
=== modified file 'doc/developers/HACKING.txt'
2
--- doc/developers/HACKING.txt	2010-04-16 12:59:03 +0000
3
+++ doc/developers/HACKING.txt	2010-05-12 11:06:25 +0000
4
@@ -179,11 +179,9 @@
5
179
179
6
180
* Bazaar - http://bazaar-vcs.org/
180
* Bazaar - http://bazaar-vcs.org/
7
181
181
8
182
* Bundle Buggy - http://bundlebuggy.aaronbentley.com/
9
183
10
184
* Patch Queue Manager - https://launchpad.net/pqm/
182
* Patch Queue Manager - https://launchpad.net/pqm/
11
185
183
13
186
For further information, see http://bazaar-vcs.org/BzrDevelopment.
184
For further information, see <http://wiki.bazaar.canonical.com/BzrDevelopment>.
14
187
185
15
188
186
16
189
187
17
@@ -272,239 +270,6 @@
18
272
<http://doc.bazaar-vcs.org/developers/overview.html>`_.
270
<http://doc.bazaar-vcs.org/developers/overview.html>`_.
19
273
271
20
274
272
21
275
The Code Review Process
22
276
#######################
23
277
24
278
All code changes coming in to Bazaar are reviewed by someone else.
25
279
Normally changes by core contributors are reviewed by one other core
26
280
developer, and changes from other people are reviewed by two core
27
281
developers.  Use intelligent discretion if the patch is trivial.
28
282
29
283
Good reviews do take time. They also regularly require a solid
30
284
understanding of the overall code base. In practice, this means a small
31
285
number of people often have a large review burden - with knowledge comes
32
286
responsibility. No one likes their merge requests sitting in a queue going
33
287
nowhere, so reviewing sooner rather than later is strongly encouraged.
34
288
35
289
36
290
37
291
38
292
39
293
Review cover letters
40
294
====================
41
295
42
296
Please put a "cover letter" on your merge request explaining:
43
297
44
298
* the reason **why** you're making this change
45
299
46
300
* **how** this change achieves this purpose
47
301
48
302
* anything else you may have fixed in passing
49
303
50
304
* anything significant that you thought of doing, such as a more
51
305
  extensive fix or a different approach, but didn't or couldn't do now
52
306
53
307
A good cover letter makes reviewers' lives easier because they can decide
54
308
from the letter whether they agree with the purpose and approach, and then
55
309
assess whether the patch actually does what the cover letter says.
56
310
Explaining any "drive-by fixes" or roads not taken may also avoid queries
57
311
from the reviewer.  All in all this should give faster and better reviews.
58
312
Sometimes writing the cover letter helps the submitter realize something
59
313
else they need to do.  The size of the cover letter should be proportional
60
314
to the size and complexity of the patch.
61
315
62
316
63
317
Reviewing proposed changes
64
318
==========================
65
319
66
320
Anyone is welcome to review code, and reply to the thread with their
67
321
opinion or comments.
68
322
69
323
The simplest way to review a proposed change is to just read the patch on
70
324
the list or in Bundle Buggy.  For more complex changes it may be useful
71
325
to make a new working tree or branch from trunk, and merge the proposed
72
326
change into it, so you can experiment with the code or look at a wider
73
327
context.
74
328
75
329
There are three main requirements for code to get in:
76
330
77
331
* Doesn't reduce test coverage: if it adds new methods or commands,
78
332
  there should be tests for them.  There is a good test framework
79
333
  and plenty of examples to crib from, but if you are having trouble
80
334
  working out how to test something feel free to post a draft patch
81
335
  and ask for help.
82
336
83
337
* Doesn't reduce design clarity, such as by entangling objects
84
338
  we're trying to separate.  This is mostly something the more
85
339
  experienced reviewers need to help check.
86
340
87
341
* Improves bugs, features, speed, or code simplicity.
88
342
89
343
Code that goes in should not degrade any of these aspects.  Patches are
90
344
welcome that only cleanup the code without changing the external
91
345
behaviour.  The core developers take care to keep the code quality high
92
346
and understandable while recognising that perfect is sometimes the enemy
93
347
of good.
94
348
95
349
It is easy for reviews to make people notice other things which should be
96
350
fixed but those things should not hold up the original fix being accepted.
97
351
New things can easily be recorded in the Bug Tracker instead.
98
352
99
353
It's normally much easier to review several smaller patches than one large
100
354
one.  You might want to use ``bzr-loom`` to maintain threads of related
101
355
work, or submit a preparatory patch that will make your "real" change
102
356
easier.
103
357
104
358
105
359
Checklist for reviewers
106
360
=======================
107
361
108
362
* Do you understand what the code's doing and why?
109
363
110
364
* Will it perform reasonably for large inputs, both in memory size and
111
365
  run time?  Are there some scenarios where performance should be
112
366
  measured?
113
367
114
368
* Is it tested, and are the tests at the right level?  Are there both
115
369
  blackbox (command-line level) and API-oriented tests?
116
370
117
371
* If this change will be visible to end users or API users, is it
118
372
  appropriately documented in NEWS?
119
373
120
374
* Does it meet the coding standards below?
121
375
122
376
* If it changes the user-visible behaviour, does it update the help
123
377
  strings and user documentation?
124
378
125
379
* If it adds a new major concept or standard practice, does it update the
126
380
  developer documentation?
127
381
128
382
* (your ideas here...)
129
383
130
384
131
385
Reviews on Launchpad
132
386
====================
133
387
134
388
From May 2009 on, we prefer people to propose code reviews through
135
389
Launchpad.
136
390
137
391
 * <https://launchpad.net/+tour/code-review>
138
392
139
393
 * <https://help.launchpad.net/Code/Review>
140
394
141
395
Anyone can propose or comment on a merge proposal just by creating a
142
396
Launchpad account.
143
397
144
398
There are two ways to create a new merge proposal: through the web
145
399
interface or by email.
146
400
147
401
148
402
Proposing a merge through the web
149
403
---------------------------------
150
404
151
405
To create the proposal through the web, first push your branch to Launchpad.
152
406
For example, a branch dealing with documentation belonging to the Launchpad
153
407
User mbp could be pushed as ::
154
408
155
409
  bzr push lp:~mbp/bzr/doc
156
410
157
411
Then go to the branch's web page, which in this case would be
158
412
<https://code.launchpad.net/~mbp/bzr/doc>.  You can simplify this step by just
159
413
running ::
160
414
161
415
  bzr lp-open
162
416
163
417
You can then click "Propose for merging into another branch", and enter your
164
418
cover letter (see above) into the web form.  Typically you'll want to merge
165
419
into ``~bzr/bzr/trunk`` which will be the default; you might also want to
166
420
nominate merging into a release branch for a bug fix.  There is the option to
167
421
specify a specific reviewer or type of review, and you shouldn't normally
168
422
change those.
169
423
170
424
Submitting the form takes you to the new page about the merge proposal
171
425
containing the diff of the changes, comments by interested people, and
172
426
controls to comment or vote on the change.
173
427
174
428
Proposing a merge by mail
175
429
-------------------------
176
430
177
431
To propose a merge by mail, send a bundle to ``merge@code.launchpad.net``.
178
432
179
433
You can generate a merge request like this::
180
434
181
435
  bzr send -o bug-1234.diff
182
436
183
437
``bzr send`` can also send mail directly if you prefer; see the help.
184
438
185
439
Reviewing changes
186
440
-----------------
187
441
188
442
From <https://code.launchpad.net/bzr/+activereviews> you can see all
189
443
currently active reviews, and choose one to comment on.  This page also
190
444
shows proposals that are now approved and should be merged by someone with
191
445
PQM access.
192
446
193
447
194
448
Reviews through Bundle Buggy
195
449
============================
196
450
197
451
The Bundle Buggy tool used up to May 2009 is still available as a review
198
452
mechanism.
199
453
200
454
Sending patches for review
201
455
--------------------------
202
456
203
457
If you'd like to propose a change, please post to the
204
458
bazaar@lists.canonical.com list with a bundle, patch, or link to a
205
459
branch. Put ``[PATCH]`` or ``[MERGE]`` in the subject so Bundle Buggy
206
460
can pick it out, and explain the change in the email message text.
207
461
Remember to update the NEWS file as part of your change if it makes any
208
462
changes visible to users or plugin developers. Please include a diff
209
463
against mainline if you're giving a link to a branch.
210
464
211
465
You can generate a merge request like this::
212
466
213
467
  bzr send -o bug-1234.patch
214
468
215
469
A ``.patch`` extension is recommended instead of .bundle as many mail clients
216
470
will send the latter as a binary file.
217
471
218
472
``bzr send`` can also send mail directly if you prefer; see the help.
219
473
220
474
Please do **NOT** put [PATCH] or [MERGE] in the subject line if you don't
221
475
want it to be merged. If you want comments from developers rather than
222
476
to be merged, you can put ``[RFC]`` in the subject line.
223
477
224
478
If this change addresses a bug, please put the bug number in the subject
225
479
line too, in the form ``[#1]`` so that Bundle Buggy can recognize it.
226
480
227
481
If the change is intended for a particular release mark that in the
228
482
subject too, e.g. ``[1.6]``.
229
483
Anyone can "vote" on the mailing list by expressing an opinion. Core
230
484
developers can also vote using Bundle Buggy. Here are the voting codes and
231
485
their explanations.
232
486
233
487
:approve:  Reviewer wants this submission merged.
234
488
:tweak:    Reviewer wants this submission merged with small changes. (No
235
489
  re-review required.)
236
490
:abstain:  Reviewer does not intend to vote on this patch.
237
491
:resubmit: Please make changes and resubmit for review.
238
492
:reject:   Reviewer doesn't want this kind of change merged.
239
493
:comment:  Not really a vote. Reviewer just wants to comment, for now.
240
494
241
495
If a change gets two approvals from core reviewers, and no rejections,
242
496
then it's OK to come in.  Any of the core developers can bring it into the
243
497
bzr.dev trunk and backport it to maintenance branches if required.  The
244
498
Release Manager will merge the change into the branch for a pending
245
499
release, if any. As a guideline, core developers usually merge their own
246
500
changes and volunteer to merge other contributions if they were the second
247
501
reviewer to agree to a change.
248
502
249
503
To track the progress of proposed changes, use Bundle Buggy. See
250
504
http://bundlebuggy.aaronbentley.com/help for a link to all the
251
505
outstanding merge requests together with an explanation of the columns.
252
506
Bundle Buggy will also mail you a link to track just your change.
253
507
254
508
Coding Style Guidelines
273
Coding Style Guidelines
255
509
#######################
274
#######################
256
510
275
257
511
276
258
=== added file 'doc/developers/code-review.txt'
259
--- doc/developers/code-review.txt	1970-01-01 00:00:00 +0000
260
+++ doc/developers/code-review.txt	2010-05-12 11:06:25 +0000
261
@@ -0,0 +1,170 @@
262
1
The Code Review Process
263
2
#######################
264
3
265
4
All code changes coming in to Bazaar are reviewed by someone else.
266
5
Normally changes by core contributors are reviewed by one other core
267
6
developer, and changes from other people are reviewed by two core
268
7
developers.  Use intelligent discretion if the patch is trivial.
269
8
270
9
Good reviews do take time. They also regularly require a solid
271
10
understanding of the overall code base. In practice, this means a small
272
11
number of people often have a large review burden - with knowledge comes
273
12
responsibility. No one likes their merge requests sitting in a queue going
274
13
nowhere, so reviewing sooner rather than later is strongly encouraged.
275
14
276
15
277
16
278
17
Review cover letters
279
18
====================
280
19
281
20
Please put a "cover letter" on your merge request explaining:
282
21
283
22
* the reason **why** you're making this change
284
23
285
24
* **how** this change achieves this purpose
286
25
287
26
* anything else you may have fixed in passing
288
27
289
28
* anything significant that you thought of doing, such as a more
290
29
  extensive fix or a different approach, but didn't or couldn't do now
291
30
292
31
A good cover letter makes reviewers' lives easier because they can decide
293
32
from the letter whether they agree with the purpose and approach, and then
294
33
assess whether the patch actually does what the cover letter says.
295
34
Explaining any "drive-by fixes" or roads not taken may also avoid queries
296
35
from the reviewer.  All in all this should give faster and better reviews.
297
36
Sometimes writing the cover letter helps the submitter realize something
298
37
else they need to do.  The size of the cover letter should be proportional
299
38
to the size and complexity of the patch.
300
39
301
40
302
41
Reviewing proposed changes
303
42
==========================
304
43
305
44
Anyone is welcome to review code, and reply to the thread with their
306
45
opinion or comments.
307
46
308
47
The simplest way to review a proposed change is to just read the patch on
309
48
the list or in Bundle Buggy.  For more complex changes it may be useful
310
49
to make a new working tree or branch from trunk, and merge the proposed
311
50
change into it, so you can experiment with the code or look at a wider
312
51
context.
313
52
314
53
There are three main requirements for code to get in:
315
54
316
55
* Doesn't reduce test coverage: if it adds new methods or commands,
317
56
  there should be tests for them.  There is a good test framework
318
57
  and plenty of examples to crib from, but if you are having trouble
319
58
  working out how to test something feel free to post a draft patch
320
59
  and ask for help.
321
60
322
61
* Doesn't reduce design clarity, such as by entangling objects
323
62
  we're trying to separate.  This is mostly something the more
324
63
  experienced reviewers need to help check.
325
64
326
65
* Improves bugs, features, speed, or code simplicity.
327
66
328
67
Code that goes in should not degrade any of these aspects.  Patches are
329
68
welcome that only cleanup the code without changing the external
330
69
behaviour.  The core developers take care to keep the code quality high
331
70
and understandable while recognising that perfect is sometimes the enemy
332
71
of good.
333
72
334
73
It is easy for reviews to make people notice other things which should be
335
74
fixed but those things should not hold up the original fix being accepted.
336
75
New things can easily be recorded in the Bug Tracker instead.
337
76
338
77
It's normally much easier to review several smaller patches than one large
339
78
one.  You might want to use ``bzr-loom`` to maintain threads of related
340
79
work, or submit a preparatory patch that will make your "real" change
341
80
easier.
342
81
343
82
344
83
Checklist for reviewers
345
84
=======================
346
85
347
86
* Do you understand what the code's doing and why?
348
87
349
88
* Will it perform reasonably for large inputs, both in memory size and
350
89
  run time?  Are there some scenarios where performance should be
351
90
  measured?
352
91
353
92
* Is it tested, and are the tests at the right level?  Are there both
354
93
  blackbox (command-line level) and API-oriented tests?
355
94
356
95
* If this change will be visible to end users or API users, is it
357
96
  appropriately documented in NEWS?
358
97
359
98
* Does it meet the coding standards below?
360
99
361
100
* If it changes the user-visible behaviour, does it update the help
362
101
  strings and user documentation?
363
102
364
103
* If it adds a new major concept or standard practice, does it update the
365
104
  developer documentation?
366
105
367
106
* (your ideas here...)
368
107
369
108
370
109
Reviews on Launchpad
371
110
====================
372
111
373
112
From May 2009 on, we prefer people to propose code reviews through
374
113
Launchpad.
375
114
376
115
 * <https://launchpad.net/+tour/code-review>
377
116
378
117
 * <https://help.launchpad.net/Code/Review>
379
118
380
119
Anyone can propose or comment on a merge proposal just by creating a
381
120
Launchpad account.
382
121
383
122
There are two ways to create a new merge proposal: through the web
384
123
interface or by email.
385
124
386
125
387
126
Proposing a merge through the web
388
127
---------------------------------
389
128
390
129
To create the proposal through the web, first push your branch to Launchpad.
391
130
For example, a branch dealing with documentation belonging to the Launchpad
392
131
User mbp could be pushed as ::
393
132
394
133
  bzr push lp:~mbp/bzr/doc
395
134
396
135
Then go to the branch's web page, which in this case would be
397
136
<https://code.launchpad.net/~mbp/bzr/doc>.  You can simplify this step by just
398
137
running ::
399
138
400
139
  bzr lp-open
401
140
402
141
You can then click "Propose for merging into another branch", and enter your
403
142
cover letter (see above) into the web form.  Typically you'll want to merge
404
143
into ``~bzr/bzr/trunk`` which will be the default; you might also want to
405
144
nominate merging into a release branch for a bug fix.  There is the option to
406
145
specify a specific reviewer or type of review, and you shouldn't normally
407
146
change those.
408
147
409
148
Submitting the form takes you to the new page about the merge proposal
410
149
containing the diff of the changes, comments by interested people, and
411
150
controls to comment or vote on the change.
412
151
413
152
Proposing a merge by mail
414
153
-------------------------
415
154
416
155
To propose a merge by mail, send a bundle to ``merge@code.launchpad.net``.
417
156
418
157
You can generate a merge request like this::
419
158
420
159
  bzr send -o bug-1234.diff
421
160
422
161
``bzr send`` can also send mail directly if you prefer; see the help.
423
162
424
163
Reviewing changes
425
164
-----------------
426
165
427
166
From <https://code.launchpad.net/bzr/+activereviews> you can see all
428
167
currently active reviews, and choose one to comment on.  This page also
429
168
shows proposals that are now approved and should be merged by someone with
430
169
PQM access.
431
170
432
0
171
433
=== modified file 'doc/developers/index-plain.txt'
434
--- doc/developers/index-plain.txt	2010-02-23 04:12:26 +0000
435
+++ doc/developers/index-plain.txt	2010-05-12 11:06:25 +0000
436
@@ -24,6 +24,8 @@
437
24
24
438
25
* `Testing <testing.html>`_ |--| Guide to writing tests for Bazaar.
25
* `Testing <testing.html>`_ |--| Guide to writing tests for Bazaar.
439
26
26
440
27
* `Code Review <code-review.html>`_.
441
28
442
27
* `Writing plugins <http://doc.bazaar.canonical.com/plugins/en/plugin-development.html>`_
29
* `Writing plugins <http://doc.bazaar.canonical.com/plugins/en/plugin-development.html>`_
443
28
  |--| specific advice on writing Bazaar plugins. (web link)
30
  |--| specific advice on writing Bazaar plugins. (web link)
444
29
31
445
30
32
446
=== modified file 'doc/developers/index.txt'
447
--- doc/developers/index.txt	2010-02-23 04:12:26 +0000
448
+++ doc/developers/index.txt	2010-05-12 11:06:25 +0000
449
@@ -23,6 +23,7 @@
450
23
   bug-handling
23
   bug-handling
451
24
   HACKING
24
   HACKING
452
25
   testing
25
   testing
453
26
   code-review
454
26
27
455
27
* `Contributing to Bazaar Documentation <http://wiki.bazaar.canonical.com/ContributingToTheDocs>`_ (wiki)
28
* `Contributing to Bazaar Documentation <http://wiki.bazaar.canonical.com/ContributingToTheDocs>`_ (wiki)
456
28
29
457
29
30
458
=== modified file 'doc/developers/testing.txt'
459
--- doc/developers/testing.txt	2010-05-06 23:41:35 +0000
460
+++ doc/developers/testing.txt	2010-05-12 11:06:25 +0000
461
@@ -6,7 +6,7 @@
462
6
The Importance of Testing
6
The Importance of Testing
463
7
=========================
7
=========================
464
8
8
466
9
Reliability is a critical success factor for any Version Control System.
9
Reliability is a critical success factor for any version control system.
467
10
We want Bazaar to be highly reliable across multiple platforms while
10
We want Bazaar to be highly reliable across multiple platforms while
468
11
evolving over time to meet the needs of its community.
11
evolving over time to meet the needs of its community.
469
12
12
470
@@ -183,6 +183,16 @@
471
183
183
472
184
.. _testrepository: https://launchpad.net/testrepository
184
.. _testrepository: https://launchpad.net/testrepository
473
185
185
474
186
475
187
Babune continuous integration
476
188
-----------------------------
477
189
478
190
We have a Hudson continuous-integration system that automatically runs 
479
191
tests across various platforms.  In the future we plan to add more 
480
192
combinations including testing plugins.  See 
481
193
<http://babune.ladeuil.net:24842/>.  (Babune = Bazaar Buildbot Network.)
482
194
483
195
484
186
Writing Tests
196
Writing Tests
485
187
=============
197
=============
486
188
198
Reviewer	Review Type	Date Requested	Status
John A Meinel		2010-05-12	Approve on 2010-05-12
Review via email: mp+25137@code.launchpad.net