« back to all changes in this revision
Viewing changes to doc/developers/HACKING.txt
- browse files at revision 5227
- compare with another revision
- compare with revision 45
- download diff
- download tarball
- view history from revision 5227
- Committer: Canonical.com Patch Queue Manager
- Date: 2010-05-12 13:57:49 UTC
- mfrom: (5225.2.3 doc)
- Revision ID: pqm@pqm.ubuntu.com-20100512135749-8b0et7ntvh8dgm2t
(mbp) (mbp) mention babune in testing guide;
split out review guide (Martin Pool)
split out review guide (Martin Pool)
added
removed
doc/developers/HACKING.txt
179
179
180
180
* Bazaar - http://bazaar-vcs.org/
181
181
182
* Bundle Buggy - http://bundlebuggy.aaronbentley.com/
183
184
182
* Patch Queue Manager - https://launchpad.net/pqm/
185
183
186
For further information, see http://bazaar-vcs.org/BzrDevelopment.
184
For further information, see <http://wiki.bazaar.canonical.com/BzrDevelopment>.
187
185
188
186
189
187
272
270
<http://doc.bazaar-vcs.org/developers/overview.html>`_.
273
271
274
272
275
The Code Review Process
276
#######################
277
278
All code changes coming in to Bazaar are reviewed by someone else.
279
Normally changes by core contributors are reviewed by one other core
280
developer, and changes from other people are reviewed by two core
281
developers. Use intelligent discretion if the patch is trivial.
282
283
Good reviews do take time. They also regularly require a solid
284
understanding of the overall code base. In practice, this means a small
285
number of people often have a large review burden - with knowledge comes
286
responsibility. No one likes their merge requests sitting in a queue going
287
nowhere, so reviewing sooner rather than later is strongly encouraged.
288
289
290
291
292
293
Review cover letters
294
====================
295
296
Please put a "cover letter" on your merge request explaining:
297
298
* the reason **why** you're making this change
299
300
* **how** this change achieves this purpose
301
302
* anything else you may have fixed in passing
303
304
* anything significant that you thought of doing, such as a more
305
extensive fix or a different approach, but didn't or couldn't do now
306
307
A good cover letter makes reviewers' lives easier because they can decide
308
from the letter whether they agree with the purpose and approach, and then
309
assess whether the patch actually does what the cover letter says.
310
Explaining any "drive-by fixes" or roads not taken may also avoid queries
311
from the reviewer. All in all this should give faster and better reviews.
312
Sometimes writing the cover letter helps the submitter realize something
313
else they need to do. The size of the cover letter should be proportional
314
to the size and complexity of the patch.
315
316
317
Reviewing proposed changes
318
==========================
319
320
Anyone is welcome to review code, and reply to the thread with their
321
opinion or comments.
322
323
The simplest way to review a proposed change is to just read the patch on
324
the list or in Bundle Buggy. For more complex changes it may be useful
325
to make a new working tree or branch from trunk, and merge the proposed
326
change into it, so you can experiment with the code or look at a wider
327
context.
328
329
There are three main requirements for code to get in:
330
331
* Doesn't reduce test coverage: if it adds new methods or commands,
332
there should be tests for them. There is a good test framework
333
and plenty of examples to crib from, but if you are having trouble
334
working out how to test something feel free to post a draft patch
335
and ask for help.
336
337
* Doesn't reduce design clarity, such as by entangling objects
338
we're trying to separate. This is mostly something the more
339
experienced reviewers need to help check.
340
341
* Improves bugs, features, speed, or code simplicity.
342
343
Code that goes in should not degrade any of these aspects. Patches are
344
welcome that only cleanup the code without changing the external
345
behaviour. The core developers take care to keep the code quality high
346
and understandable while recognising that perfect is sometimes the enemy
347
of good.
348
349
It is easy for reviews to make people notice other things which should be
350
fixed but those things should not hold up the original fix being accepted.
351
New things can easily be recorded in the Bug Tracker instead.
352
353
It's normally much easier to review several smaller patches than one large
354
one. You might want to use ``bzr-loom`` to maintain threads of related
355
work, or submit a preparatory patch that will make your "real" change
356
easier.
357
358
359
Checklist for reviewers
360
=======================
361
362
* Do you understand what the code's doing and why?
363
364
* Will it perform reasonably for large inputs, both in memory size and
365
run time? Are there some scenarios where performance should be
366
measured?
367
368
* Is it tested, and are the tests at the right level? Are there both
369
blackbox (command-line level) and API-oriented tests?
370
371
* If this change will be visible to end users or API users, is it
372
appropriately documented in NEWS?
373
374
* Does it meet the coding standards below?
375
376
* If it changes the user-visible behaviour, does it update the help
377
strings and user documentation?
378
379
* If it adds a new major concept or standard practice, does it update the
380
developer documentation?
381
382
* (your ideas here...)
383
384
385
Reviews on Launchpad
386
====================
387
388
From May 2009 on, we prefer people to propose code reviews through
389
Launchpad.
390
391
* <https://launchpad.net/+tour/code-review>
392
393
* <https://help.launchpad.net/Code/Review>
394
395
Anyone can propose or comment on a merge proposal just by creating a
396
Launchpad account.
397
398
There are two ways to create a new merge proposal: through the web
399
interface or by email.
400
401
402
Proposing a merge through the web
403
---------------------------------
404
405
To create the proposal through the web, first push your branch to Launchpad.
406
For example, a branch dealing with documentation belonging to the Launchpad
407
User mbp could be pushed as ::
408
409
bzr push lp:~mbp/bzr/doc
410
411
Then go to the branch's web page, which in this case would be
412
<https://code.launchpad.net/~mbp/bzr/doc>. You can simplify this step by just
413
running ::
414
415
bzr lp-open
416
417
You can then click "Propose for merging into another branch", and enter your
418
cover letter (see above) into the web form. Typically you'll want to merge
419
into ``~bzr/bzr/trunk`` which will be the default; you might also want to
420
nominate merging into a release branch for a bug fix. There is the option to
421
specify a specific reviewer or type of review, and you shouldn't normally
422
change those.
423
424
Submitting the form takes you to the new page about the merge proposal
425
containing the diff of the changes, comments by interested people, and
426
controls to comment or vote on the change.
427
428
Proposing a merge by mail
429
-------------------------
430
431
To propose a merge by mail, send a bundle to ``merge@code.launchpad.net``.
432
433
You can generate a merge request like this::
434
435
bzr send -o bug-1234.diff
436
437
``bzr send`` can also send mail directly if you prefer; see the help.
438
439
Reviewing changes
440
-----------------
441
442
From <https://code.launchpad.net/bzr/+activereviews> you can see all
443
currently active reviews, and choose one to comment on. This page also
444
shows proposals that are now approved and should be merged by someone with
445
PQM access.
446
447
448
Reviews through Bundle Buggy
449
============================
450
451
The Bundle Buggy tool used up to May 2009 is still available as a review
452
mechanism.
453
454
Sending patches for review
455
--------------------------
456
457
If you'd like to propose a change, please post to the
458
bazaar@lists.canonical.com list with a bundle, patch, or link to a
459
branch. Put ``[PATCH]`` or ``[MERGE]`` in the subject so Bundle Buggy
460
can pick it out, and explain the change in the email message text.
461
Remember to update the NEWS file as part of your change if it makes any
462
changes visible to users or plugin developers. Please include a diff
463
against mainline if you're giving a link to a branch.
464
465
You can generate a merge request like this::
466
467
bzr send -o bug-1234.patch
468
469
A ``.patch`` extension is recommended instead of .bundle as many mail clients
470
will send the latter as a binary file.
471
472
``bzr send`` can also send mail directly if you prefer; see the help.
473
474
Please do **NOT** put [PATCH] or [MERGE] in the subject line if you don't
475
want it to be merged. If you want comments from developers rather than
476
to be merged, you can put ``[RFC]`` in the subject line.
477
478
If this change addresses a bug, please put the bug number in the subject
479
line too, in the form ``[#1]`` so that Bundle Buggy can recognize it.
480
481
If the change is intended for a particular release mark that in the
482
subject too, e.g. ``[1.6]``.
483
Anyone can "vote" on the mailing list by expressing an opinion. Core
484
developers can also vote using Bundle Buggy. Here are the voting codes and
485
their explanations.
486
487
:approve: Reviewer wants this submission merged.
488
:tweak: Reviewer wants this submission merged with small changes. (No
489
re-review required.)
490
:abstain: Reviewer does not intend to vote on this patch.
491
:resubmit: Please make changes and resubmit for review.
492
:reject: Reviewer doesn't want this kind of change merged.
493
:comment: Not really a vote. Reviewer just wants to comment, for now.
494
495
If a change gets two approvals from core reviewers, and no rejections,
496
then it's OK to come in. Any of the core developers can bring it into the
497
bzr.dev trunk and backport it to maintenance branches if required. The
498
Release Manager will merge the change into the branch for a pending
499
release, if any. As a guideline, core developers usually merge their own
500
changes and volunteer to merge other contributions if they were the second
501
reviewer to agree to a change.
502
503
To track the progress of proposed changes, use Bundle Buggy. See
504
http://bundlebuggy.aaronbentley.com/help for a link to all the
505
outstanding merge requests together with an explanation of the columns.
506
Bundle Buggy will also mail you a link to track just your change.
507
508
273
Coding Style Guidelines
509
274
#######################
510
275
Loggerhead is a web-based interface for Breezy
Version: 2.0.1