blob: f217e9d0443e2dd9ea88e4810d1118a14b160a68 [file] [log] [blame]
Willy Tarreau11e334d92015-09-20 22:31:42 +02001 HOW TO GET YOUR CODE ACCEPTED IN HAPROXY
2 READ THIS CAREFULLY BEFORE SUBMITTING CODE
3
4THIS DOCUMENT PROVIDES SOME RULES TO FOLLOW WHEN SENDING CONTRIBUTIONS. PATCHES
5NOT FOLLOWING THESE RULES WILL SIMPLY BE REJECTED IN ORDER TO PROTECT ALL OTHER
6RESPECTFUL CONTRIBUTORS' VALUABLE TIME.
7
8
9Background
10----------
11
12During the development cycle of version 1.6, much more time was spent reviewing
13poor quality submissions, fixing them and troubleshooting the bugs they
14introduced than doing any development work. This is not acceptable as it ends
15up with people actually slowing down the project for the features they're the
16only ones interested in. On the other end of the scale, there are people who
17make the effort of polishing their work to contribute excellent quality work
18which doesn't even require a review. Contrary to what newcomers may think, it's
19very easy to reach that level of quality and get your changes accepted quickly,
20even late in the development cycle. It only requires that you make your homework
21and not rely on others to do it for you. The most important point is that
22HAProxy is a community-driven project, all involved participants must respect
23all other ones' time and work.
24
25
26Preparation
27-----------
28
29It is possible that you'll want to add a specific feature to satisfy your needs
30or one of your customers'. Contributions are welcome, however maintainers are
31often very picky about changes. Patches that change massive parts of the code,
32or that touch the core parts without any good reason will generally be rejected
33if those changes have not been discussed first.
34
35The proper place to discuss your changes is the HAProxy Mailing List. There are
36enough skilled readers to catch hazardous mistakes and to suggest improvements.
37There is no other place where you'll find as many skilled people on the project,
38and these people can help you get your code integrated quickly. You can
39subscribe to it by sending an empty e-mail at the following address :
40
41 haproxy+subscribe@formilux.org
42
43If you have an idea about something to implement, *please* discuss it on the
44list first. It has already happened several times that two persons did the same
45thing simultaneously. This is a waste of time for both of them. It's also very
46common to see some changes rejected because they're done in a way that will
47conflict with future evolutions, or that does not leave a good feeling. It's
48always unpleasant for the person who did the work, and it is unpleasant in
49general because value people's time and efforts are valuable and would be better
50spent working on something else. That would not happen if these were discussed
51first. There is no problem posting work in progress to the list, it happens
52quite often in fact. Also, don't waste your time with the doc when submitting
53patches for review, only add the doc with the patch you consider ready to merge.
54
55Another important point concerns code portability. Haproxy requires gcc as the
56C compiler, and may or may not work with other compilers. However it's known to
57build using gcc 2.95 or any later version. As such, it is important to keep in
58mind that certain facilities offered by recent versions must not be used in the
59code :
60
61 - declarations mixed in the code (requires gcc >= 3.x and is a bad practice)
62 - GCC builtins without checking for their availability based on version and
63 architecture ;
64 - assembly code without any alternate portable form for other platforms
65 - use of stdbool.h, "bool", "false", "true" : simply use "int", "0", "1"
66 - in general, anything which requires C99 (such as declaring variables in
67 "for" statements)
68
69Since most of these restrictions are just a matter of coding style, it is
70normally not a problem to comply.
71
Willy Tarreau9d84cd62017-07-18 06:56:40 +020072When modifying some optional subsystem (SSL, Lua, compression, device detection
73engines), please make sure the code continues to build (and to work) when these
74features are disabled. Similarly, when modifying the SSL stack, please always
75ensure that supported OpenSSL versions continue to build and to work, especially
76if you modify support for alternate libraries. Clean support for the legacy
77OpenSSL libraries is mandatory, support for its derivatives is a bonus and may
78occasionally break eventhough a great care is taken. In other words, if you
79provide a patch for OpenSSL you don't need to test its derivatives, but if you
80provide a patch for a derivative you also need to test with OpenSSL.
81
Willy Tarreau11e334d92015-09-20 22:31:42 +020082If your work is very confidential and you can't publicly discuss it, you can
83also mail willy@haproxy.org directly about it, but your mail may be waiting
Willy Tarreau138544f2017-03-31 16:24:44 +020084several days in the queue before you get a response, if you get a response at
85all. Retransmit if you don't get a response by one week. Please note that
86direct sent e-mails to this address for non-confidential subjects may simply
87be forwarded to the list or be deleted without notification.
Willy Tarreau11e334d92015-09-20 22:31:42 +020088
89If you'd like a feature to be added but you think you don't have the skills to
90implement it yourself, you should follow these steps :
91
92 1. discuss the feature on the mailing list. It is possible that someone
93 else has already implemented it, or that someone will tell you how to
94 proceed without it, or even why not to do it. It is also possible that
95 in fact it's quite easy to implement and people will guide you through
96 the process. That way you'll finally have YOUR patch merged, providing
97 the feature YOU need.
98
99 2. if you really can't code it yourself after discussing it, then you may
100 consider contacting someone to do the job for you. Some people on the
101 list might sometimes be OK with trying to do it.
102
103
104Rules : the 12 laws of patch contribution
105-----------------------------------------
106
107People contributing patches must apply the following rules. That may sound heavy
108at the beginning but it's common sense more than anything else and contributors
109do not think about them anymore after a few patches.
110
Willy Tarreau138544f2017-03-31 16:24:44 +02001111) Comply with the license
112
113 Before modifying some code, you have read the LICENSE file ("main license")
Willy Tarreau11e334d92015-09-20 22:31:42 +0200114 coming with the sources, and all the files this file references. Certain
115 files may be covered by different licenses, in which case it will be
116 indicated in the files themselves. In any case, you agree to respect these
117 licenses and to contribute your changes under the same licenses. If you want
118 to create new files, they will be under the main license, or any license of
119 your choice that you have verified to be compatible with the main license,
Tim Düsterhus4896c442016-11-29 02:15:19 +0100120 and that will be explicitly mentioned in the affected files. The project's
Willy Tarreau11e334d92015-09-20 22:31:42 +0200121 maintainers are free to reject contributions proposing license changes they
122 feel are not appropriate or could cause future trouble.
123
Willy Tarreau138544f2017-03-31 16:24:44 +02001242) Develop on development branch, not stable ones
125
126 Your work may only be based on the latest development version. No development
Willy Tarreau11e334d92015-09-20 22:31:42 +0200127 is made on a stable branch. If your work needs to be applied to a stable
128 branch, it will first be applied to the development branch and only then will
129 be backported to the stable branch. You are responsible for ensuring that
130 your work correctly applies to the development version. If at any moment you
131 are going to work on restructuring something important which may impact other
132 contributors, the rule that applies is that the first sent is the first
133 served. However it is considered good practice and politeness to warn others
134 in advance if you know you're going to make changes that may force them to
135 re-adapt their code, because they did probably not expect to have to spend
136 more time discovering your changes and rebasing their work.
137
Willy Tarreau138544f2017-03-31 16:24:44 +02001383) Read and respect the coding style
139
140 You have read and understood "doc/coding-style.txt", and you're actively
Willy Tarreau11e334d92015-09-20 22:31:42 +0200141 determined to respect it and to enforce it on your coworkers if you're going
142 to submit a team's work. We don't care what text editor you use, whether it's
143 an hex editor, cat, vi, emacs, Notepad, Word, or even Eclipse. The editor is
144 only the interface between you and the text file. What matters is what is in
145 the text file in the end. The editor is not an excuse for submitting poorly
146 indented code, which only proves that the person has no consideration for
147 quality and/or has done it in a hurry (probably worse). Please note that most
148 bugs were found in low-quality code. Reviewers know this and tend to be much
149 more reluctant to accept poorly formated code because by experience they
150 won't trust their author's ability to write correct code. It is also worth
151 noting that poor quality code is painful to read and may result in nobody
152 willing to waste their time even reviewing your work.
153
Willy Tarreau138544f2017-03-31 16:24:44 +02001544) Present clean work
155
156 The time it takes for you to polish your code is always much smaller than the
Willy Tarreau11e334d92015-09-20 22:31:42 +0200157 time it takes others to do it for you, because they always have to wonder if
158 what they see is intended (meaning they didn't understand something) or if it
159 is a mistake that needs to be fixed. And since there are less reviewers than
160 submitters, it is vital to spread the effort closer to where the code is
161 written and not closer to where it gets merged. For example if you have to
162 write a report for a customer that your boss wants to review before you send
163 it to the customer, will you throw on his desk a pile of paper with stains,
164 typos and copy-pastes everywhere ? Will you say "come on, OK I made a mistake
165 in the company's name but they will find it by themselves, it's obvious it
166 comes from us" ? No. When in doubt, simply ask for help on the mailing list.
167
Willy Tarreau138544f2017-03-31 16:24:44 +02001685) Documentation is very important
169
170 There are four levels of importance of quality in the project :
Willy Tarreau11e334d92015-09-20 22:31:42 +0200171
172 - The most important one, and by far, is the quality of the user-facing
173 documentation. This is the first contact for most users and it immediately
174 gives them an accurate idea of how the project is maintained. Dirty docs
175 necessarily belong to a dirty project. Be careful to the way the text you
176 add is presented and indented. Be very careful about typos, usual mistakes
177 such as double consonants when only one is needed or "it's" instead of
178 "its", don't mix US english and UK english in the same paragraph, etc.
179 When in doubt, check in a dictionary. Fixes for existing typos in the doc
180 are always welcome and chasing them is a good way to become familiar with
181 the project and to get other participants' respect and consideration.
182
183 - The second most important level is user-facing messages emitted by the
184 code. You must try to see all the messages your code produces to ensure
185 they are understandable outside of the context where you wrote them,
186 because the user often doesn't expect them. That's true for warnings, and
187 that's even more important for errors which prevent the program from
188 working and which require an immediate and well understood fix in the
189 configuration. It's much better to say "line 35: compression level must be
190 an integer between 1 and 9" than "invalid argument at line 35". In HAProxy,
191 error handling roughly represents half of the code, and that's about 3/4 of
192 the configuration parser. Take the time to do something you're proud of. A
193 good rule of thumb is to keep in mind that your code talks to a human and
194 tries to teach him/her how to proceed. It must then speak like a human.
195
196 - The third most important level is the code and its accompanying comments,
197 including the commit message which is a complement to your code and
198 comments. It's important for all other contributors that the code is
199 readable, fluid, understandable and that the commit message describes what
200 was done, the choices made, the possible alternatives you thought about,
201 the reason for picking this one and its limits if any. Comments should be
202 written where it's easy to have a doubt or after some error cases have been
203 wiped out and you want to explain what possibilities remain. All functions
204 must have a comment indicating what they take on input and what they
205 provide on output. Please adjust the comments when you copy-paste a
206 function or change its prototype, this type of lazy mistake is too common
207 and very confusing when reading code later to debug an issue. Do not forget
208 that others will feel really angry at you when they have to dig into your
209 code for a bug that your code caused and they feel like this code is dirty
210 or confusing, that the commit message doesn't explain anything useful and
211 that the patch should never have been accepted in the first place. That
212 will strongly impact your reputation and will definitely affect your
213 chances to contribute again!
214
215 - The fourth level of importance is in the technical documentation that you
216 may want to add with your code. Technical documentation is always welcome
217 as it helps others make the best use of your work and to go exactly in the
218 direction you thought about during the design. This is also what reduces
219 the risk that your design gets changed in the near future due to a misuse
220 and/or a poor understanding. All such documentation is actually considered
221 as a bonus. It is more important that this documentation exists than that
222 it looks clean. Sometimes just copy-pasting your draft notes in a file to
223 keep a record of design ideas is better than losing them. Please do your
224 best so that other ones can read your doc. If these docs require a special
225 tool such as a graphics utility, ensure that the file name makes it
226 unambiguous how to process it. So there are no rules here for the contents,
227 except one. Please write the date in your file. Design docs tend to stay
228 forever and to remain long after they become obsolete. At this point that
229 can cause harm more than it can help. Writing the date in the document
230 helps developers guess the degree of validity and/or compare them with the
231 date of certain commits touching the same area.
232
Willy Tarreau138544f2017-03-31 16:24:44 +02002336) US-ASCII only!
234
235 All text files and commit messages are written using the US-ASCII charset.
Willy Tarreau11e334d92015-09-20 22:31:42 +0200236 Please be careful that your contributions do not contain any character not
237 printable using this charset, as they will render differently in different
238 editors and/or terminals. Avoid latin1 and more importantly UTF-8 which some
239 editors tend to abuse to replace some US-ASCII characters with their
240 typographic equivalent which aren't readable anymore in other editors. The
241 only place where alternative charsets are tolerated is in your name in the
242 commit message, but it's at your own risk as it can be mangled during the
243 merge. Anyway if you have an e-mail address, you probably have a valid
244 US-ASCII representation for it as well.
245
Willy Tarreau138544f2017-03-31 16:24:44 +02002467) Comments
247
248 Be careful about comments when you move code around. It's not acceptable that
Willy Tarreau11e334d92015-09-20 22:31:42 +0200249 a block of code is moved to another place leaving irrelevant comments at the
250 old place, just like it's not acceptable that a function is duplicated without
251 the comments being adjusted. The example below started to become quite common
252 during the 1.6 cycle, it is not acceptable and wastes everyone's time :
253
254 /* Parse switching <str> to build rule <rule>. Returns 0 on error. */
255 int parse_switching_rule(const char *str, struct rule *rule)
256 {
257 ...
258 }
259
260 /* Parse switching <str> to build rule <rule>. Returns 0 on error. */
261 void execute_switching_rule(struct rule *rule)
262 {
263 ...
264 }
265
266 This patch is not acceptable either (and it's unfortunately not that rare) :
267
268 + if (!session || !arg || list_is_empty(&session->rules->head))
269 + return 0;
270 +
271 /* Check if session->rules is valid before dereferencing it */
272 if (!session->rules_allocated)
273 return 0;
274
275 - if (!arg || list_is_empty(&session->rules->head))
276 - return 0;
277 -
278
Willy Tarreau138544f2017-03-31 16:24:44 +02002798) Short, readable identifiers
280
281 Limit the length of your identifiers in the code. When your identifiers start
Willy Tarreau11e334d92015-09-20 22:31:42 +0200282 to sound like sentences, it's very hard for the reader to keep on track with
283 what operation they are observing. Also long names force expressions to fit
284 on several lines which also cause some difficulties to the reader. See the
285 example below :
286
287 int file_name_len_including_global_path;
288 int file_name_len_without_global_path;
289 int global_path_len_or_zero_if_default;
290
291 if (global_path)
292 global_path_len_or_zero_if_default = strlen(global_path);
293 else
294 global_path_len_or_zero_if_default = 0;
295
296 file_name_len_without_global_path = strlen(file_name);
297 file_name_len_including_global_path =
298 file_name_len_without_global_path + 1 + /* for '/' */
299 global_path_len_or_zero_if_default ?
300 global_path_len_or_zero_if_default : default_path_len;
301
302 Compare it to this one :
303
304 int f, p;
305
306 p = global_path ? strlen(global_path) : default_path_len;
307 f = p + 1 + strlen(file_name); /* 1 for '/' */
308
309 A good rule of thumb is that if your identifiers start to contain more than
310 3 words or more than 15 characters, they can become confusing. For function
311 names it's less important especially if these functions are rarely used or
Bertrand Jacquind5e4de82018-10-13 16:06:18 +0100312 are used in a complex context where it is important to differentiate between
Willy Tarreau11e334d92015-09-20 22:31:42 +0200313 their multiple variants.
314
Willy Tarreau138544f2017-03-31 16:24:44 +02003159) Unified diff only
316
317 The best way to build your patches is to use "git format-patch". This means
318 that you have committed your patch to a local branch, with an appropriate
319 subject line and a useful commit message explaining what the patch attempts
320 to do. It is not strictly required to use git, but what is strictly required
Bertrand Jacquind5e4de82018-10-13 16:06:18 +0100321 is to have all these elements in the same mail, easily distinguishable, and
Willy Tarreau138544f2017-03-31 16:24:44 +0200322 a patch in "diff -up" format (which is also the format used by Git). This
323 means the "unified" diff format must be used exclusively, and with the
324 function name printed in the diff header of each block. That significantly
325 helps during reviews. Keep in mind that most reviews are done on the patch
326 and not on the code after applying the patch. Your diff must keep some
327 context (3 lines above and 3 lines below) so that there's no doubt where the
328 code has to be applied. Don't change code outside of the context of your
329 patch (eg: take care of not adding/removing empty lines once you remove
Willy Tarreau11e334d92015-09-20 22:31:42 +0200330 your debugging code). If you are using Git (which is strongly recommended),
Willy Tarreau11e334d92015-09-20 22:31:42 +0200331 always use "git show" after doing a commit to ensure it looks good, and
332 enable syntax coloring that will automatically report in red the trailing
333 spaces or tabs that your patch added to the code and that must absolutely be
334 removed. These ones cause a real pain to apply patches later because they
335 mangle the context in an invisible way. Such patches with trailing spaces at
336 end of lines will be rejected.
337
Willy Tarreau138544f2017-03-31 16:24:44 +020033810) One patch per feature
339
340 Please cut your work in series of patches that can be independently reviewed
341 and merged. Each patch must do something on its own that you can explain to
342 someone without being ashamed of what you did. For example, you must not say
343 "This is the patch that implements SSL, it was tricky". There's clearly
344 something wrong there, your patch will be huge, will definitely break things
345 and nobody will be able to figure what exactly introduced the bug. However
346 it's much better to say "I needed to add some fields in the session to store
347 the SSL context so this patch does this and doesn't touch anything else, so
348 it's safe". Also when dealing with series, you will sometimes fix a bug that
349 one of your patches introduced. Please do merge these fixes (eg: using git
350 rebase -i and squash or fixup), as it is not acceptable to see patches which
351 introduce known bugs even if they're fixed later. Another benefit of cleanly
352 splitting patches is that if some of your patches need to be reworked after
353 a review, the other ones can still be merged so that you don't need to care
354 about them anymore. When sending multiple patches for review, prefer to send
355 one e-mail per patch than all patches in a single e-mail. The reason is that
356 not everyone is skilled in all areas nor has the time to review everything
357 at once. With one patch per e-mail, it's easy to comment on a single patch
358 without giving an opinion on the other ones, especially if a long thread
359 starts about one specific patch on the mailing list. "git send-email" does
360 that for you though it requires a few trials before getting it right.
361
362 If you can, please always put all the bug fixes at the beginning of the
363 series. This often makes it easier to backport them because they will not
364 depend on context that your other patches changed.
Willy Tarreau11e334d92015-09-20 22:31:42 +0200365
Willy Tarreau138544f2017-03-31 16:24:44 +020036611) Real commit messages please!
Willy Tarreau11e334d92015-09-20 22:31:42 +0200367
Willy Tarreau138544f2017-03-31 16:24:44 +0200368 Please properly format your commit messages. To get an idea, just run
369 "git log" on the file you've just modified. Patches always have the format
370 of an e-mail made of a subject, a description and the actual patch. If you
371 are sending a patch as an e-mail formatted this way, it can quickly be
372 applied with limited effort so that's acceptable :
Willy Tarreau11e334d92015-09-20 22:31:42 +0200373
Willy Tarreau138544f2017-03-31 16:24:44 +0200374 - A subject line (may wrap to the next line, but please read below)
375 - an empty line (subject delimiter)
376 - a non-empty description (the body of the e-mail)
377 - the patch itself
Willy Tarreau11e334d92015-09-20 22:31:42 +0200378
Willy Tarreau138544f2017-03-31 16:24:44 +0200379 The subject describes the "What" of the change ; the description explains
380 the "why", the "how" and sometimes "what next". For example a commit message
381 looking like this will be rejected :
Willy Tarreau11e334d92015-09-20 22:31:42 +0200382
Willy Tarreau138544f2017-03-31 16:24:44 +0200383 | From: Mr Foobar <foobar@example.com>
384 | Subject: BUG: fix typo in ssl_sock
385 |
386
387 This one as well (too long subject, not the right place for the details) :
388
389 | From: Mr Foobar <foobar@example.com>
390 | Subject: BUG/MEDIUM: ssl: use an error flag to prevent ssl_read() from
391 | returning 0 when dealing with large buffers because that can cause
392 | an infinite loop
393 |
394
395 This one ought to be used instead :
396
397 | From: Mr Foobar <foobar@example.com>
398 | Subject: BUG/MEDIUM: ssl: fix risk of infinite loop in ssl_sock
399 |
400 | ssl_read() must not return 0 on error or the caller may loop forever.
401 | Instead we add a flag to the connection to notify about the error and
402 | check it at all call places. This situation can only happen with large
403 | buffers so a workaround is to limit buffer sizes. Another option would
404 | have been to return -1 but it required to use signed ints everywhere
405 | and would have made the patch larger and riskier. This fix should be
406 | backported to versions 1.2 and upper.
407
408 It is important to understand that for any reader to guess the text above
409 when it's absent, it will take a huge amount of time. If you made the
410 analysis leading to your patch, you must explain it, including the ideas
411 you dropped if you had a good reason for this.
412
413 While it's not strictly required to use Git, it is strongly recommended
414 because it helps you do the cleanest job with the least effort. But if you
415 are comfortable with writing clean e-mails and inserting your patches, you
416 don't need to use Git.
417
418 But in any case, it is important that there is a clean description of what
419 the patch does, the motivation for what it does, why it's the best way to do
420 it, its impacts, and what it does not yet cover. Also, in HAProxy, like many
421 projects which take a great care of maintaining stable branches, patches are
422 reviewed later so that some of them can be backported to stable releases.
423
424 While reviewing hundreds of patches can seem cumbersome, with a proper
425 formatting of the subject line it actually becomes very easy. For example,
426 here's how one can find patches that need to be reviewed for backports (bugs
427 and doc) between since commit ID 827752e :
428
429 $ git log --oneline 827752e.. | grep 'BUG\|DOC'
430 0d79cf6 DOC: fix function name
431 bc96534 DOC: ssl: missing LF
Joseph Herlante07bc142018-11-09 17:44:10 -0800432 10ec214 BUG/MEDIUM: lua: the lua function Channel:close() causes a segf
Willy Tarreau138544f2017-03-31 16:24:44 +0200433 bdc97a8 BUG/MEDIUM: lua: outgoing connection was broken since 1.6-dev2
434 ba56d9c DOC: mention support for RFC 5077 TLS Ticket extension in start
435 f1650a8 DOC: clarify some points about SSL and the proxy protocol
436 b157d73 BUG/MAJOR: peers: fix current table pointer not re-initialized
437 e1ab808 BUG/MEDIUM: peers: fix wrong message id on stick table updates
438 cc79b00 BUG/MINOR: ssl: TLS Ticket Key rotation broken via socket comma
439 d8e42b6 DOC: add new file intro.txt
440 c7d7607 BUG/MEDIUM: lua: bad error processing
441 386a127 DOC: match several lua configuration option names to those impl
442 0f4eadd BUG/MEDIUM: counters: ensure that src_{inc,clr}_gpc0 creates a
443
444 It is made possible by the fact that subject lines are properly formatted and
445 always respect the same principle : one part indicating the nature and
446 severity of the patch, another one to indicate which subsystem is affected,
447 and the last one is a succinct description of the change, with the important
448 part at the beginning so that it's obvious what it does even when lines are
449 truncated like above. The whole stable maintenance process relies on this.
450 For this reason, it is mandatory to respect some easy rules regarding the
451 way the subject is built. Please see the section below for more information
452 regarding this formatting.
453
Willy Tarreau9d84cd62017-07-18 06:56:40 +0200454 As a rule of thumb, your patch MUST NEVER be made only of a subject line,
Willy Tarreau138544f2017-03-31 16:24:44 +0200455 it *must* contain a description. Even one or two lines, or indicating
456 whether a backport is desired or not. It turns out that single-line commits
457 are so rare in the Git world that they require special manual (hence
458 painful) handling when they are backported, and at least for this reason
459 it's important to keep this in mind.
460
Willy Tarreau9d84cd62017-07-18 06:56:40 +0200461 Each patch fixing a bug MUST be tagged with "BUG", a severity level, an
462 indication of the affected subsystem and a brief description of the nature
463 of the issue in the subject line, and a detailed analysis in the message
464 body. The explanation of the user-visible impact and the need for
465 backporting to stable branches or not are MANDATORY. Bug fixes with no
466 indication will simply be rejected as they are very likely to cause more
467 harm when nobody is able to tell whether or not the patch needs to be
468 backported or can be reverted in case of regression.
469
Frédéric Lécaille4891e402018-06-19 14:06:07 +0200470 When fixing a bug which is reproducible, if possible, the contributors are
471 strongly encouraged to write a regression testing VTC file for varnishtest
472 to add to reg-tests directory. More information about varnishtest may be
473 found in README file of reg-tests directory and in doc/regression-testing.txt
474 file.
475
Willy Tarreau138544f2017-03-31 16:24:44 +020047612) Discuss on the mailing list
477
478 When submitting changes, please always CC the mailing list address so that
479 everyone gets a chance to spot any issue in your code. It will also serve
480 as an advertisement for your work, you'll get more testers quicker and
481 you'll feel better knowing that people really use your work. It's often
482 convenient to prepend "[PATCH]" in front of your mail's subject to mention
483 that this e-mail contains a patch (or a series of patches), because it will
484 easily catch reviewer's attention. It's automatically done by tools such as
485 "git format-patch" and "git send-email". If you don't want your patch to be
486 merged yet and prefer to show it for discussion, better tag it as "[RFC]"
487 (stands for "Request For Comments") and it will be reviewed but not merged
488 without your approval. It is also important to CC any author mentioned in
489 the file you change, or a subsystem maintainers whose address is mentioned
490 in a MAINTAINERS file. Not everyone reads the list on a daily basis so it's
491 very easy to miss some changes. Don't consider it as a failure when a
492 reviewer tells you you have to modify your patch, actually it's a success
493 because now you know what is missing for your work to get accepted. That's
494 why you should not hesitate to CC enough people. Don't copy people who have
495 no deal with your work area just because you found their address on the
496 list. That's the best way to appear careless about their time and make them
497 reject your changes in the future.
498
Willy Tarreau11e334d92015-09-20 22:31:42 +0200499
500Patch classifying rules
501-----------------------
502
503There are 3 criteria of particular importance in any patch :
504 - its nature (is it a fix for a bug, a new feature, an optimization, ...)
505 - its importance, which generally reflects the risk of merging/not merging it
506 - what area it applies to (eg: http, stats, startup, config, doc, ...)
507
508It's important to make these 3 criteria easy to spot in the patch's subject,
509because it's the first (and sometimes the only) thing which is read when
510reviewing patches to find which ones need to be backported to older versions.
511It also helps when trying to find which patch is the most likely to have caused
512a regression.
513
514Specifically, bugs must be clearly easy to spot so that they're never missed.
515Any patch fixing a bug must have the "BUG" tag in its subject. Most common
516patch types include :
517
518 - BUG fix for a bug. The severity of the bug should also be indicated
519 when known. Similarly, if a backport is needed to older versions,
520 it should be indicated on the last line of the commit message. If
521 the bug has been identified as a regression brought by a specific
522 patch or version, this indication will be appreciated too. New
523 maintenance releases are generally emitted when a few of these
524 patches are merged. If the bug is a vulnerability for which a CVE
525 identifier was assigned before you publish the fix, you can mention
526 it in the commit message, it will help distro maintainers.
527
Tim Düsterhus4896c442016-11-29 02:15:19 +0100528 - CLEANUP code cleanup, silence of warnings, etc... theoretically no impact.
Willy Tarreau11e334d92015-09-20 22:31:42 +0200529 These patches will rarely be seen in stable branches, though they
530 may appear when they remove some annoyance or when they make
531 backporting easier. By nature, a cleanup is always of minor
532 importance and it's not needed to mention it.
533
534 - DOC updates to any of the documentation files, including README. Many
535 documentation updates are backported since they don't impact the
536 product's stability and may help users avoid bugs. So please
537 indicate in the commit message if a backport is desired. When a
538 feature gets documented, it's preferred that the doc patch appears
539 in the same patch or after the feature patch, but not before, as it
540 becomes confusing when someone working on a code base including
541 only the doc patch won't understand why a documented feature does
542 not work as documented.
543
544 - REORG code reorganization. Some blocks may be moved to other places,
545 some important checks might be swapped, etc... These changes
546 always present a risk of regression. For this reason, they should
547 never be mixed with any bug fix nor functional change. Code is
548 only moved as-is. Indicating the risk of breakage is highly
549 recommended. Minor breakage is tolerated in such patches if trying
550 to fix it at once makes the whole change even more confusing. That
551 may happen for example when some #ifdefs need to be propagated in
552 every file consecutive to the change.
553
554 - BUILD updates or fixes for build issues. Changes to makefiles also fall
555 into this category. The risk of breakage should be indicated if
556 known. It is also appreciated to indicate what platforms and/or
557 configurations were tested after the change.
558
559 - OPTIM some code was optimised. Sometimes if the regression risk is very
560 low and the gains significant, such patches may be merged in the
561 stable branch. Depending on the amount of code changed or replaced
562 and the level of trust the author has in the change, the risk of
563 regression should be indicated.
564
565 - RELEASE release of a new version (development or stable).
566
567 - LICENSE licensing updates (may impact distro packagers).
568
Frédéric Lécaillea8cf95d2018-06-20 10:14:01 +0200569 - REGTEST updates to any of the regression testing files found in reg-tests
570 directory, including README or any documentation file.
571
Willy Tarreau11e334d92015-09-20 22:31:42 +0200572
Willy Tarreau138544f2017-03-31 16:24:44 +0200573When the patch cannot be categorized, it's best not to put any type tag, and to
574only use a risk or complexity information only as below. This is commonly the
575case for new features, which development versions are mostly made of.
Willy Tarreau11e334d92015-09-20 22:31:42 +0200576
Willy Tarreau138544f2017-03-31 16:24:44 +0200577The importance, complexity of the patch, or severity of the bug it fixes must
Willy Tarreau11e334d92015-09-20 22:31:42 +0200578be indicated when relevant. A single upper-case word is preferred, among :
579
580 - MINOR minor change, very low risk of impact. It is often the case for
581 code additions that don't touch live code. As a rule of thumb, a
582 patch tagged "MINOR" is safe enough to be backported to stable
583 branches. For a bug, it generally indicates an annoyance, nothing
584 more.
585
586 - MEDIUM medium risk, may cause unexpected regressions of low importance or
587 which may quickly be discovered. In short, the patch is safe but
588 touches working areas and it is always possible that you missed
589 something you didn't know existed (eg: adding a "case" entry or
590 an error message after adding an error code to an enum). For a bug,
591 it generally indicates something odd which requires changing the
592 configuration in an undesired way to work around the issue.
593
594 - MAJOR major risk of hidden regression. This happens when large parts of
595 the code are rearranged, when new timeouts are introduced, when
596 sensitive parts of the session scheduling are touched, etc... We
597 should only exceptionally find such patches in stable branches when
598 there is no other option to fix a design issue. For a bug, it
599 indicates severe reliability issues for which workarounds are
600 identified with or without performance impacts.
601
602 - CRITICAL medium-term reliability or security is at risk and workarounds,
603 if they exist, might not always be acceptable. An upgrade is
604 absolutely required. A maintenance release may be emitted even if
605 only one of these bugs are fixed. Note that this tag is only used
606 with bugs. Such patches must indicate what is the first version
607 affected, and if known, the commit ID which introduced the issue.
608
609The expected length of the commit message grows with the importance of the
610change. While a MINOR patch may sometimes be described in 1 or 2 lines, MAJOR
611or CRITICAL patches cannot have less than 10-15 lines to describe exactly the
612impacts otherwise the submitter's work will be considered as rough sabotage.
613
614For BUILD, DOC and CLEANUP types, this tag is not always relevant and may be
615omitted.
616
617The area the patch applies to is quite important, because some areas are known
618to be similar in older versions, suggesting a backport might be desirable, and
619conversely, some areas are known to be specific to one version. The area is a
620single-word lowercase name the contributor find clear enough to describe what
621part is being touched. The following tags are suggested but not limitative :
622
623 - examples example files. Be careful, sometimes these files are packaged.
624
625 - tests regression test files. No code is affected, no need to upgrade.
626
Frédéric Lécaille4891e402018-06-19 14:06:07 +0200627 - reg-tests regression test files for varnishtest. No code is affected, no
628 need to upgrade.
629
Willy Tarreau11e334d92015-09-20 22:31:42 +0200630 - init initialization code, arguments parsing, etc...
631
632 - config configuration parser, mostly used when adding new config keywords
633
634 - http the HTTP engine
635
636 - stats the stats reporting engine
637
638 - cli the stats socket CLI
639
640 - checks the health checks engine (eg: when adding new checks)
641
642 - sample the sample fetch system (new fetch or converter functions)
643
644 - acl the ACL processing core or some ACLs from other areas
645
Willy Tarreau138544f2017-03-31 16:24:44 +0200646 - filters everything related to the filters core
647
Willy Tarreau11e334d92015-09-20 22:31:42 +0200648 - peers the peer synchronization engine
649
650 - lua the Lua scripting engine
651
652 - listeners everything related to incoming connection settings
653
654 - frontend everything related to incoming connection processing
655
656 - backend everything related to LB algorithms and server farm
657
658 - session session processing and flags (very sensible, be careful)
659
660 - server server connection management, queueing
661
Willy Tarreau138544f2017-03-31 16:24:44 +0200662 - spoe SPOE code
663
Willy Tarreau11e334d92015-09-20 22:31:42 +0200664 - ssl the SSL/TLS interface
665
666 - proxy proxy maintenance (start/stop)
667
668 - log log management
669
670 - poll any of the pollers
671
672 - halog the halog sub-component in the contrib directory
673
674 - contrib any addition to the contrib directory
675
676Other names may be invented when more precise indications are meaningful, for
677instance : "cookie" which indicates cookie processing in the HTTP core. Last,
678indicating the name of the affected file is also a good way to quickly spot
679changes. Many commits were already tagged with "stream_sock" or "cfgparse" for
680instance.
681
682It is required that the type of change and the severity when relevant are
683indicated, as well as the touched area when relevant as well in the patch
684subject. Normally, we would have the 3 most often. The two first criteria should
685be present before a first colon (':'). If both are present, then they should be
686delimited with a slash ('/'). The 3rd criterion (area) should appear next, also
Willy Tarreau138544f2017-03-31 16:24:44 +0200687followed by a colon. Thus, all of the following subject lines are valid :
Willy Tarreau11e334d92015-09-20 22:31:42 +0200688
Willy Tarreau138544f2017-03-31 16:24:44 +0200689Examples of subject lines :
Willy Tarreau11e334d92015-09-20 22:31:42 +0200690 - DOC: document options forwardfor to logasap
691 - DOC/MAJOR: reorganize the whole document and change indenting
692 - BUG: stats: connection reset counters must be plain ascii, not HTML
693 - BUG/MINOR: stats: connection reset counters must be plain ascii, not HTML
694 - MEDIUM: checks: support multi-packet health check responses
695 - RELEASE: Released version 1.4.2
696 - BUILD: stats: stdint is not present on solaris
697 - OPTIM/MINOR: halog: make fgets parse more bytes by blocks
698 - REORG/MEDIUM: move syscall redefinition to specific places
699
700Please do not use square brackets anymore around the tags, because they induce
701more work when merging patches, which need to be hand-edited not to lose the
702enclosed part.
703
704In fact, one of the only square bracket tags that still makes sense is '[RFC]'
705at the beginning of the subject, when you're asking for someone to review your
706change before getting it merged. If the patch is OK to be merged, then it can
707be merge as-is and the '[RFC]' tag will automatically be removed. If you don't
708want it to be merged at all, you can simply state it in the message, or use an
709alternate 'WIP/' prefix in front of your tag tag ("work in progress").
710
711The tags are not rigid, follow your intuition first, and they may be readjusted
712when your patch is merged. It may happen that a same patch has a different tag
713in two distinct branches. The reason is that a bug in one branch may just be a
714cleanup or safety measure in the other one because the code cannot be triggered.
715
716
717Working with Git
718----------------
719
720For a more efficient interaction between the mainline code and your code, you
721are strongly encouraged to try the Git version control system :
722
723 http://git-scm.com/
724
725It's very fast, lightweight and lets you undo/redo your work as often as you
726want, without making your mistakes visible to the rest of the world. It will
727definitely help you contribute quality code and take other people's feedback
728in consideration. In order to clone the HAProxy Git repository :
729
730 $ git clone http://git.haproxy.org/git/haproxy.git/ (development)
731
732If you decide to use Git for your developments, then your commit messages will
733have the subject line in the format described above, then the whole description
734of your work (mainly why you did it) will be in the body. You can directly send
735your commits to the mailing list, the format is convenient to read and process.
736
737It is recommended to create a branch for your work that is based on the master
738branch :
739
740 $ git checkout -b 20150920-fix-stats master
741
742You can then do your work and even experiment with multiple alternatives if you
743are not completely sure that your solution is the best one :
744
745 $ git checkout -b 20150920-fix-stats-v2
746
747Then reorder/merge/edit your patches :
748
749 $ git rebase -i master
750
751When you think you're ready, reread your whole patchset to ensure there is no
Tim Düsterhus4896c442016-11-29 02:15:19 +0100752formatting or style issue :
Willy Tarreau11e334d92015-09-20 22:31:42 +0200753
754 $ git show master..
755
756And once you're satisfied, you should update your master branch to be sure that
Thiago Farina9f72a392016-04-01 16:43:50 -0300757nothing changed during your work (only needed if you left it unattended for days
Willy Tarreau11e334d92015-09-20 22:31:42 +0200758or weeks) :
759
760 $ git checkout -b 20150920-fix-stats-rebased
761 $ git fetch origin master:master
762 $ git rebase master
763
Thiago Farina9f72a392016-04-01 16:43:50 -0300764You can build a list of patches ready for submission like this :
Willy Tarreau11e334d92015-09-20 22:31:42 +0200765
766 $ git format-patch master
767
768The output files are the patches ready to be sent over e-mail, either via a
769regular e-mail or via git send-email (carefully check the man page). Don't
770destroy your other work branches until your patches get merged, it may happen
771that earlier designs will be preferred for various reasons. Patches should be
772sent to the mailing list : haproxy@formilux.org and CCed to relevant subsystem
773maintainers or authors of the modified files if their address appears at the
774top of the file.
775
Bertrand Jacquind5e4de82018-10-13 16:06:18 +0100776Please don't send pull-requests, they are really inconvenient. First, a pull
Willy Tarreau11e334d92015-09-20 22:31:42 +0200777implies a merge operation and the code doesn't move fast enough to justify the
778use of merges. Second, pull requests are not easily commented on by the
779project's participants, contrary to e-mails where anyone is allowed to have an
780opinion and to express it.
781
782-- end