« back to all changes in this revision
Viewing changes to doc/developers/HACKING.txt
- browse files at revision 3691
- compare with another revision
- compare with revision 1185.65.19
- download diff
- download tarball
- view history from revision 3691
- Committer: Canonical.com Patch Queue Manager
- Date: 2008-09-05 01:36:04 UTC
- mfrom: (3683.1.3 doc)
- Revision ID: pqm@pqm.ubuntu.com-20080905013604-6zvq8467ip7l4m9i
Developer docs on review process and stacking
- files added:
- doc/developers/overview.txt
- files modified:
- NEWS
added
removed
doc/developers/HACKING.txt
82
82
Understanding the Development Process
83
83
=====================================
84
84
85
The development team follows many best-practices including:
85
The development team follows many practices including:
86
86
87
87
* a public roadmap and planning process in which anyone can participate
88
88
108
108
For further information, see http://bazaar-vcs.org/BzrDevelopment.
109
109
110
110
111
A Closer Look at the Merge & Review Process
112
===========================================
113
114
If you'd like to propose a change, please post to the
115
bazaar@lists.canonical.com list with a bundle, patch, or link to a
116
branch. Put '[PATCH]' or '[MERGE]' in the subject so Bundle Buggy
117
can pick it out, and explain the change in the email message text.
118
Remember to update the NEWS file as part of your change if it makes any
119
changes visible to users or plugin developers. Please include a diff
120
against mainline if you're giving a link to a branch.
121
122
You can generate a bundle like this::
123
124
bzr bundle > mybundle.patch
125
126
A .patch extension is recommended instead of .bundle as many mail clients
127
will send the latter as a binary file. If a bundle would be too long or your
128
mailer mangles whitespace (e.g. implicitly converts Unix newlines to DOS
129
newlines), use the merge-directive command instead like this::
130
131
bzr merge-directive http://bazaar-vcs.org http://example.org/my_branch > my_directive.patch
132
133
See the help for details on the arguments to merge-directive.
134
135
Please do **NOT** put [PATCH] or [MERGE] in the subject line if you don't
136
want it to be merged. If you want comments from developers rather than
137
to be merged, you can put '[RFC]' in the subject line.
138
139
Anyone is welcome to review code. There are broadly three gates for
140
code to get in:
141
142
* Doesn't reduce test coverage: if it adds new methods or commands,
143
there should be tests for them. There is a good test framework
144
and plenty of examples to crib from, but if you are having trouble
145
working out how to test something feel free to post a draft patch
146
and ask for help.
147
148
* Doesn't reduce design clarity, such as by entangling objects
149
we're trying to separate. This is mostly something the more
150
experienced reviewers need to help check.
151
152
* Improves bugs, features, speed, or code simplicity.
153
154
Code that goes in should pass all three. The core developers take care
155
to keep the code quality high and understandable while recognising that
156
perfect is sometimes the enemy of good. (It is easy for reviews to make
157
people notice other things which should be fixed but those things should
158
not hold up the original fix being accepted. New things can easily be
159
recorded in the Bug Tracker instead.)
160
161
Anyone can "vote" on the mailing list. Core developers can also vote using
162
Bundle Buggy. Here are the voting codes and their explanations.
163
164
:approve: Reviewer wants this submission merged.
165
:tweak: Reviewer wants this submission merged with small changes. (No
166
re-review required.)
167
:abstain: Reviewer does not intend to vote on this patch.
168
:resubmit: Please make changes and resubmit for review.
169
:reject: Reviewer doesn't want this kind of change merged.
170
:comment: Not really a vote. Reviewer just wants to comment, for now.
171
172
If a change gets two approvals from core reviewers, and no rejections,
173
then it's OK to come in. Any of the core developers can bring it into the
174
bzr.dev trunk and backport it to maintenance branches if required. The
175
Release Manager will merge the change into the branch for a pending
176
release, if any. As a guideline, core developers usually merge their own
177
changes and volunteer to merge other contributions if they were the second
178
reviewer to agree to a change.
179
180
To track the progress of proposed changes, use Bundle Buggy. See
181
http://bundlebuggy.aaronbentley.com/help for a link to all the
182
outstanding merge requests together with an explanation of the columns.
183
Bundle Buggy will also mail you a link to track just your change.
184
111
185
112
186
113
Preparing a Sandbox for Making Changes to Bazaar
264
191
<http://starship.python.net/crew/mwh/bzrlibapi/>.
265
192
(There is an experimental editable version at
266
193
<http://starship.python.net/crew/mwh/bzrlibapi-oe/>.)
267
See also the `Essential Domain Classes`_
268
section of this guide.
269
270
271
Essential Domain Classes
272
########################
273
274
Introducing the Object Model
275
============================
276
277
The core domain objects within the bazaar model are:
278
279
* Transport
280
281
* Branch
282
283
* Repository
284
285
* WorkingTree
286
287
Transports are explained below. See http://bazaar-vcs.org/Classes/
288
for an introduction to the other key classes.
289
290
Using Transports
291
================
292
293
The ``Transport`` layer handles access to local or remote directories.
294
Each Transport object acts like a logical connection to a particular
295
directory, and it allows various operations on files within it. You can
296
*clone* a transport to get a new Transport connected to a subdirectory or
297
parent directory.
298
299
Transports are not used for access to the working tree. At present
300
working trees are always local and they are accessed through the regular
301
Python file io mechanisms.
302
303
Filenames vs URLs
304
-----------------
305
306
Transports work in URLs. Take note that URLs are by definition only
307
ASCII - the decision of how to encode a Unicode string into a URL must be
308
taken at a higher level, typically in the Store. (Note that Stores also
309
escape filenames which cannot be safely stored on all filesystems, but
310
this is a different level.)
311
312
The main reason for this is that it's not possible to safely roundtrip a
313
URL into Unicode and then back into the same URL. The URL standard
314
gives a way to represent non-ASCII bytes in ASCII (as %-escapes), but
315
doesn't say how those bytes represent non-ASCII characters. (They're not
316
guaranteed to be UTF-8 -- that is common but doesn't happen everywhere.)
317
318
For example if the user enters the url ``http://example/%e0`` there's no
319
way to tell whether that character represents "latin small letter a with
320
grave" in iso-8859-1, or "latin small letter r with acute" in iso-8859-2
321
or malformed UTF-8. So we can't convert their URL to Unicode reliably.
322
323
Equally problematic if we're given a url-like string containing non-ascii
324
characters (such as the accented a) we can't be sure how to convert that
325
to the correct URL, because we don't know what encoding the server expects
326
for those characters. (Although this is not totally reliable we might still
327
accept these and assume they should be put into UTF-8.)
328
329
A similar edge case is that the url ``http://foo/sweet%2Fsour`` contains
330
one directory component whose name is "sweet/sour". The escaped slash is
331
not a directory separator. If we try to convert URLs to regular Unicode
332
paths this information will be lost.
333
334
This implies that Transports must natively deal with URLs; for simplicity
335
they *only* deal with URLs and conversion of other strings to URLs is done
336
elsewhere. Information they return, such as from ``list_dir``, is also in
337
the form of URL components.
338
194
195
See also the `Bazaar Architectural Overview <../../developers/overview.html>`_.
196
197
198
The Code Review Process
199
#######################
200
201
All code changes coming in to Bazaar are reviewed by someone else.
202
Normally changes by core contributors are reviewed by one other core
203
developer, and changes from other people are reviewed by two core
204
developers. Use intelligent discretion if the patch is trivial.
205
206
Good reviews do take time. They also regularly require a solid
207
understanding of the overall code base. In practice, this means a small
208
number of people often have a large review burden - with knowledge comes
209
responsibility. No one like their merge requests sitting in a queue going
210
nowhere, so reviewing sooner rather than later is strongly encouraged.
211
212
213
214
Sending patches for review
215
==========================
216
217
If you'd like to propose a change, please post to the
218
bazaar@lists.canonical.com list with a bundle, patch, or link to a
219
branch. Put ``[PATCH]`` or ``[MERGE]`` in the subject so Bundle Buggy
220
can pick it out, and explain the change in the email message text.
221
Remember to update the NEWS file as part of your change if it makes any
222
changes visible to users or plugin developers. Please include a diff
223
against mainline if you're giving a link to a branch.
224
225
You can generate a merge request like this::
226
227
bzr send -o bug-1234.diff
228
229
A ``.diff`` extension is recommended instead of .bundle as many mail clients
230
will send the latter as a binary file.
231
232
``bzr send`` can also send mail directly if you prefer; see the help.
233
234
Please do **NOT** put [PATCH] or [MERGE] in the subject line if you don't
235
want it to be merged. If you want comments from developers rather than
236
to be merged, you can put ``[RFC]`` in the subject line.
237
238
If this change addresses a bug, please put the bug number in the subject
239
line too, in the form ``[#1]`` so that Bundle Buggy can recognize it.
240
241
If the change is intended for a particular release mark that in the
242
subject too, e.g. ``[1.6]``.
243
244
245
Review cover letters
246
====================
247
248
Please put a "cover letter" on your merge request explaining:
249
250
* the reason **why** you're making this change
251
252
* **how** this change acheives this purpose
253
254
* anything else you may have fixed in passing
255
256
* anything significant that you thought of doing, such as a more
257
extensive fix or a different approach, but didn't or couldn't do now
258
259
A good cover letter makes reviewers' lives easier because they can decide
260
from the letter whether they agree with the purpose and approach, and then
261
assess whether the patch actually does what the cover letter says.
262
Explaining any "drive-by fixes" or roads not taken may also avoid queries
263
from the reviewer. All in all this should give faster and better reviews.
264
Sometimes writing the cover letter helps the submitter realize something
265
else they need to do. The size of the cover letter should be proportional
266
to the size and complexity of the patch.
267
268
269
Reviewing proposed changes
270
==========================
271
272
Anyone is welcome to review code, and reply to the thread with their
273
opinion or comments.
274
275
The simplest way to review a proposed change is to just read the patch on
276
the list or in Bundle Buggy. For more complex changes it may be useful
277
to make a new working tree or branch from trunk, and merge the proposed
278
change into it, so you can experiment with the code or look at a wider
279
context.
280
281
There are three main requirements for code to get in:
282
283
* Doesn't reduce test coverage: if it adds new methods or commands,
284
there should be tests for them. There is a good test framework
285
and plenty of examples to crib from, but if you are having trouble
286
working out how to test something feel free to post a draft patch
287
and ask for help.
288
289
* Doesn't reduce design clarity, such as by entangling objects
290
we're trying to separate. This is mostly something the more
291
experienced reviewers need to help check.
292
293
* Improves bugs, features, speed, or code simplicity.
294
295
Code that goes in should not degrade any of these aspects. Patches are
296
welcome that only cleanup the code without changing the external
297
behaviour. The core developers take care to keep the code quality high
298
and understandable while recognising that perfect is sometimes the enemy
299
of good.
300
301
It is easy for reviews to make people notice other things which should be
302
fixed but those things should not hold up the original fix being accepted.
303
New things can easily be recorded in the Bug Tracker instead.
304
305
It's normally much easier to review several smaller patches than one large
306
one. You might want to use ``bzr-loom`` to maintain threads of related
307
work, or submit a preparatory patch that will make your "real" change
308
easier.
309
310
311
Checklist for reviewers
312
=======================
313
314
* Do you understand what the code's doing and why?
315
316
* Will it perform reasonably for large inputs, both in memory size and
317
run time? Are there some scenarios where performance should be
318
measured?
319
320
* Is it tested, and are the tests at the right level? Are there both
321
blackbox (command-line level) and API-oriented tests?
322
323
* If this change will be visible to end users or API users, is it
324
appropriately documented in NEWS?
325
326
* Does it meet the coding standards below?
327
328
* If it changes the user-visible behaviour, does it update the help
329
strings and user documentation?
330
331
* If it adds a new major concept or standard practice, does it update the
332
developer documentation?
333
334
* (your ideas here...)
335
336
337
Bundle Buggy and review outcomes
338
================================
339
340
Anyone can "vote" on the mailing list by expressing an opinion. Core
341
developers can also vote using Bundle Buggy. Here are the voting codes and
342
their explanations.
343
344
:approve: Reviewer wants this submission merged.
345
:tweak: Reviewer wants this submission merged with small changes. (No
346
re-review required.)
347
:abstain: Reviewer does not intend to vote on this patch.
348
:resubmit: Please make changes and resubmit for review.
349
:reject: Reviewer doesn't want this kind of change merged.
350
:comment: Not really a vote. Reviewer just wants to comment, for now.
351
352
If a change gets two approvals from core reviewers, and no rejections,
353
then it's OK to come in. Any of the core developers can bring it into the
354
bzr.dev trunk and backport it to maintenance branches if required. The
355
Release Manager will merge the change into the branch for a pending
356
release, if any. As a guideline, core developers usually merge their own
357
changes and volunteer to merge other contributions if they were the second
358
reviewer to agree to a change.
359
360
To track the progress of proposed changes, use Bundle Buggy. See
361
http://bundlebuggy.aaronbentley.com/help for a link to all the
362
outstanding merge requests together with an explanation of the columns.
363
Bundle Buggy will also mail you a link to track just your change.
339
364
340
365
Coding Style Guidelines
341
366
#######################
1206
1231
how to set it up and configure it.
1207
1232
1208
1233
1209
Reviewing Changes
1210
=================
1211
1212
Setting Up Your Workspace for Reviews
1213
-------------------------------------
1214
1215
TODO: Incorporate John Arbash Meinel's detailed email to Ian C on the
1216
numerous ways of setting up integration branches.
1217
1218
1219
The Review Checklist
1220
--------------------
1221
1222
See `A Closer Look at the Merge & Review Process`_
1223
for information on the gates used to decide whether code can be merged
1224
or not and details on how review results are recorded and communicated.
1225
1226
1227
The Importance of Timely Reviews
1228
--------------------------------
1229
1230
Good reviews do take time. They also regularly require a solid
1231
understanding of the overall code base. In practice, this means a small
1232
number of people often have a large review burden - with knowledge comes
1233
responsibility. No one like their merge requests sitting in a queue going
1234
nowhere, so reviewing sooner rather than later is strongly encouraged.
1235
1236
1237
1234
Submitting Changes
1238
1235
==================
1239
1236
Loggerhead is a web-based interface for Breezy
Version: 2.0.1