1
The Code Review Process
2
#######################
4
All code changes coming in to Bazaar are reviewed by someone else.
1
Reviewing proposed changes to Bazaar
2
####################################
4
All non-trivial code changes coming in to Bazaar are reviewed by someone else.
6
Anyone is welcome to review any patch. You don't need to have a full
7
understanding of the codebase to find problems in the code, the documentation,
8
or the concept of the patch.
5
10
Normally changes by core contributors are reviewed by one other core
6
11
developer, and changes from other people are reviewed by two core
7
developers. Use intelligent discretion if the patch is trivial.
9
Good reviews do take time. They also regularly require a solid
10
understanding of the overall code base. In practice, this means a small
11
number of people often have a large review burden - with knowledge comes
12
responsibility. No one likes their merge requests sitting in a queue going
13
nowhere, so reviewing sooner rather than later is strongly encouraged.
20
Please put a "cover letter" on your merge request explaining:
22
* the reason **why** you're making this change
24
* **how** this change achieves this purpose
26
* anything else you may have fixed in passing
28
* anything significant that you thought of doing, such as a more
29
extensive fix or a different approach, but didn't or couldn't do now
31
A good cover letter makes reviewers' lives easier because they can decide
32
from the letter whether they agree with the purpose and approach, and then
33
assess whether the patch actually does what the cover letter says.
34
Explaining any "drive-by fixes" or roads not taken may also avoid queries
35
from the reviewer. All in all this should give faster and better reviews.
36
Sometimes writing the cover letter helps the submitter realize something
37
else they need to do. The size of the cover letter should be proportional
38
to the size and complexity of the patch.
12
developers. Use intelligent discretion about whether if the patch is trivial.
14
No one likes their merge requests sitting in a queue going nowhere: this
15
is pure waste. We prioritize reviewing existing proposals.
16
Canonical dedicates some staff time to providing prompt helpful reviews.
17
(See <http://wiki.bazaar.canonical.com/PatchPilot/>.)
19
From late 2009 on, we do all our code reviews through Launchpad's
20
merge proposal interface.
41
23
Reviewing proposed changes
42
24
==========================
44
Anyone is welcome to review code, and reply to the thread with their
47
The simplest way to review a proposed change is to just read the patch on
48
the list or in Bundle Buggy. For more complex changes it may be useful
49
to make a new working tree or branch from trunk, and merge the proposed
50
change into it, so you can experiment with the code or look at a wider
53
26
There are three main requirements for code to get in:
55
28
* Doesn't reduce test coverage: if it adds new methods or commands,
109
82
Reviews on Launchpad
110
83
====================
112
From May 2009 on, we prefer people to propose code reviews through
115
* <https://launchpad.net/+tour/code-review>
117
* <https://help.launchpad.net/Code/Review>
119
85
Anyone can propose or comment on a merge proposal just by creating a
120
86
Launchpad account.
122
There are two ways to create a new merge proposal: through the web
123
interface or by email.
126
Proposing a merge through the web
127
---------------------------------
129
To create the proposal through the web, first push your branch to Launchpad.
130
For example, a branch dealing with documentation belonging to the Launchpad
131
User mbp could be pushed as ::
133
bzr push lp:~mbp/bzr/doc
135
Then go to the branch's web page, which in this case would be
136
<https://code.launchpad.net/~mbp/bzr/doc>. You can simplify this step by just
141
You can then click "Propose for merging into another branch", and enter your
142
cover letter (see above) into the web form. Typically you'll want to merge
143
into ``~bzr/bzr/trunk`` which will be the default; you might also want to
144
nominate merging into a release branch for a bug fix. There is the option to
145
specify a specific reviewer or type of review, and you shouldn't normally
148
Submitting the form takes you to the new page about the merge proposal
149
containing the diff of the changes, comments by interested people, and
150
controls to comment or vote on the change.
152
Proposing a merge by mail
153
-------------------------
155
To propose a merge by mail, send a bundle to ``merge@code.launchpad.net``.
157
You can generate a merge request like this::
159
bzr send -o bug-1234.diff
161
``bzr send`` can also send mail directly if you prefer; see the help.
166
88
From <https://code.launchpad.net/bzr/+activereviews> you can see all
167
89
currently active reviews, and choose one to comment on. This page also
168
90
shows proposals that are now approved and should be merged by someone with
93
<https://help.launchpad.net/Code/Review> explains the various merge proposal
94
states. Note that we don't use state *Approved* until the patch is completely
98
Landing approved changes
99
========================
101
Once a merge proposal is approved and finished, it's sent to PQM (the patch
102
queue manager) which will automatically test and integrate it. The recommended
103
way to start this off is by running the ``feed-pqm`` script from
104
<https://launchpad.net/hydrazine/>.
added
removed
doc/developers/code-review.txt
