blob: 968da4fc41d62230b5594bd0be44f9ee9fe80138 [file] [log] [blame]
Willy Tarreau50529282015-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
72If your work is very confidential and you can't publicly discuss it, you can
73also mail willy@haproxy.org directly about it, but your mail may be waiting
74several days in the queue before you get a response. Retransmit if you don't
75get a response by one week.
76
77If you'd like a feature to be added but you think you don't have the skills to
78implement it yourself, you should follow these steps :
79
80 1. discuss the feature on the mailing list. It is possible that someone
81 else has already implemented it, or that someone will tell you how to
82 proceed without it, or even why not to do it. It is also possible that
83 in fact it's quite easy to implement and people will guide you through
84 the process. That way you'll finally have YOUR patch merged, providing
85 the feature YOU need.
86
87 2. if you really can't code it yourself after discussing it, then you may
88 consider contacting someone to do the job for you. Some people on the
89 list might sometimes be OK with trying to do it.
90
91
92Rules : the 12 laws of patch contribution
93-----------------------------------------
94
95People contributing patches must apply the following rules. That may sound heavy
96at the beginning but it's common sense more than anything else and contributors
97do not think about them anymore after a few patches.
98
991) Before modifying some code, you have read the LICENSE file ("main license")
100 coming with the sources, and all the files this file references. Certain
101 files may be covered by different licenses, in which case it will be
102 indicated in the files themselves. In any case, you agree to respect these
103 licenses and to contribute your changes under the same licenses. If you want
104 to create new files, they will be under the main license, or any license of
105 your choice that you have verified to be compatible with the main license,
106 and that will be explicitly mentionned in the affected files. The project's
107 maintainers are free to reject contributions proposing license changes they
108 feel are not appropriate or could cause future trouble.
109
1102) Your work may only be based on the latest development version. No development
111 is made on a stable branch. If your work needs to be applied to a stable
112 branch, it will first be applied to the development branch and only then will
113 be backported to the stable branch. You are responsible for ensuring that
114 your work correctly applies to the development version. If at any moment you
115 are going to work on restructuring something important which may impact other
116 contributors, the rule that applies is that the first sent is the first
117 served. However it is considered good practice and politeness to warn others
118 in advance if you know you're going to make changes that may force them to
119 re-adapt their code, because they did probably not expect to have to spend
120 more time discovering your changes and rebasing their work.
121
1223) You have read and understood "doc/codingstyle.txt", and you're actively
123 determined to respect it and to enforce it on your coworkers if you're going
124 to submit a team's work. We don't care what text editor you use, whether it's
125 an hex editor, cat, vi, emacs, Notepad, Word, or even Eclipse. The editor is
126 only the interface between you and the text file. What matters is what is in
127 the text file in the end. The editor is not an excuse for submitting poorly
128 indented code, which only proves that the person has no consideration for
129 quality and/or has done it in a hurry (probably worse). Please note that most
130 bugs were found in low-quality code. Reviewers know this and tend to be much
131 more reluctant to accept poorly formated code because by experience they
132 won't trust their author's ability to write correct code. It is also worth
133 noting that poor quality code is painful to read and may result in nobody
134 willing to waste their time even reviewing your work.
135
1364) The time it takes for you to polish your code is always much smaller than the
137 time it takes others to do it for you, because they always have to wonder if
138 what they see is intended (meaning they didn't understand something) or if it
139 is a mistake that needs to be fixed. And since there are less reviewers than
140 submitters, it is vital to spread the effort closer to where the code is
141 written and not closer to where it gets merged. For example if you have to
142 write a report for a customer that your boss wants to review before you send
143 it to the customer, will you throw on his desk a pile of paper with stains,
144 typos and copy-pastes everywhere ? Will you say "come on, OK I made a mistake
145 in the company's name but they will find it by themselves, it's obvious it
146 comes from us" ? No. When in doubt, simply ask for help on the mailing list.
147
1485) There are four levels of importance of quality in the project :
149
150 - The most important one, and by far, is the quality of the user-facing
151 documentation. This is the first contact for most users and it immediately
152 gives them an accurate idea of how the project is maintained. Dirty docs
153 necessarily belong to a dirty project. Be careful to the way the text you
154 add is presented and indented. Be very careful about typos, usual mistakes
155 such as double consonants when only one is needed or "it's" instead of
156 "its", don't mix US english and UK english in the same paragraph, etc.
157 When in doubt, check in a dictionary. Fixes for existing typos in the doc
158 are always welcome and chasing them is a good way to become familiar with
159 the project and to get other participants' respect and consideration.
160
161 - The second most important level is user-facing messages emitted by the
162 code. You must try to see all the messages your code produces to ensure
163 they are understandable outside of the context where you wrote them,
164 because the user often doesn't expect them. That's true for warnings, and
165 that's even more important for errors which prevent the program from
166 working and which require an immediate and well understood fix in the
167 configuration. It's much better to say "line 35: compression level must be
168 an integer between 1 and 9" than "invalid argument at line 35". In HAProxy,
169 error handling roughly represents half of the code, and that's about 3/4 of
170 the configuration parser. Take the time to do something you're proud of. A
171 good rule of thumb is to keep in mind that your code talks to a human and
172 tries to teach him/her how to proceed. It must then speak like a human.
173
174 - The third most important level is the code and its accompanying comments,
175 including the commit message which is a complement to your code and
176 comments. It's important for all other contributors that the code is
177 readable, fluid, understandable and that the commit message describes what
178 was done, the choices made, the possible alternatives you thought about,
179 the reason for picking this one and its limits if any. Comments should be
180 written where it's easy to have a doubt or after some error cases have been
181 wiped out and you want to explain what possibilities remain. All functions
182 must have a comment indicating what they take on input and what they
183 provide on output. Please adjust the comments when you copy-paste a
184 function or change its prototype, this type of lazy mistake is too common
185 and very confusing when reading code later to debug an issue. Do not forget
186 that others will feel really angry at you when they have to dig into your
187 code for a bug that your code caused and they feel like this code is dirty
188 or confusing, that the commit message doesn't explain anything useful and
189 that the patch should never have been accepted in the first place. That
190 will strongly impact your reputation and will definitely affect your
191 chances to contribute again!
192
193 - The fourth level of importance is in the technical documentation that you
194 may want to add with your code. Technical documentation is always welcome
195 as it helps others make the best use of your work and to go exactly in the
196 direction you thought about during the design. This is also what reduces
197 the risk that your design gets changed in the near future due to a misuse
198 and/or a poor understanding. All such documentation is actually considered
199 as a bonus. It is more important that this documentation exists than that
200 it looks clean. Sometimes just copy-pasting your draft notes in a file to
201 keep a record of design ideas is better than losing them. Please do your
202 best so that other ones can read your doc. If these docs require a special
203 tool such as a graphics utility, ensure that the file name makes it
204 unambiguous how to process it. So there are no rules here for the contents,
205 except one. Please write the date in your file. Design docs tend to stay
206 forever and to remain long after they become obsolete. At this point that
207 can cause harm more than it can help. Writing the date in the document
208 helps developers guess the degree of validity and/or compare them with the
209 date of certain commits touching the same area.
210
2116) All text files and commit messages are written using the US-ASCII charset.
212 Please be careful that your contributions do not contain any character not
213 printable using this charset, as they will render differently in different
214 editors and/or terminals. Avoid latin1 and more importantly UTF-8 which some
215 editors tend to abuse to replace some US-ASCII characters with their
216 typographic equivalent which aren't readable anymore in other editors. The
217 only place where alternative charsets are tolerated is in your name in the
218 commit message, but it's at your own risk as it can be mangled during the
219 merge. Anyway if you have an e-mail address, you probably have a valid
220 US-ASCII representation for it as well.
221
2227) Be careful about comments when you move code around. It's not acceptable that
223 a block of code is moved to another place leaving irrelevant comments at the
224 old place, just like it's not acceptable that a function is duplicated without
225 the comments being adjusted. The example below started to become quite common
226 during the 1.6 cycle, it is not acceptable and wastes everyone's time :
227
228 /* Parse switching <str> to build rule <rule>. Returns 0 on error. */
229 int parse_switching_rule(const char *str, struct rule *rule)
230 {
231 ...
232 }
233
234 /* Parse switching <str> to build rule <rule>. Returns 0 on error. */
235 void execute_switching_rule(struct rule *rule)
236 {
237 ...
238 }
239
240 This patch is not acceptable either (and it's unfortunately not that rare) :
241
242 + if (!session || !arg || list_is_empty(&session->rules->head))
243 + return 0;
244 +
245 /* Check if session->rules is valid before dereferencing it */
246 if (!session->rules_allocated)
247 return 0;
248
249 - if (!arg || list_is_empty(&session->rules->head))
250 - return 0;
251 -
252
2538) Limit the length of your identifiers in the code. When your identifiers start
254 to sound like sentences, it's very hard for the reader to keep on track with
255 what operation they are observing. Also long names force expressions to fit
256 on several lines which also cause some difficulties to the reader. See the
257 example below :
258
259 int file_name_len_including_global_path;
260 int file_name_len_without_global_path;
261 int global_path_len_or_zero_if_default;
262
263 if (global_path)
264 global_path_len_or_zero_if_default = strlen(global_path);
265 else
266 global_path_len_or_zero_if_default = 0;
267
268 file_name_len_without_global_path = strlen(file_name);
269 file_name_len_including_global_path =
270 file_name_len_without_global_path + 1 + /* for '/' */
271 global_path_len_or_zero_if_default ?
272 global_path_len_or_zero_if_default : default_path_len;
273
274 Compare it to this one :
275
276 int f, p;
277
278 p = global_path ? strlen(global_path) : default_path_len;
279 f = p + 1 + strlen(file_name); /* 1 for '/' */
280
281 A good rule of thumb is that if your identifiers start to contain more than
282 3 words or more than 15 characters, they can become confusing. For function
283 names it's less important especially if these functions are rarely used or
284 are used in a complex context where it is important to differenciate between
285 their multiple variants.
286
2879) Your patches should be sent in "diff -up" format, which is also the format
288 used by Git. This means the "unified" diff format must be used exclusively,
289 and with the function name printed in the diff header of each block. That
290 significantly helps during reviews. Keep in mind that most reviews are done
291 on the patch and not on the code after applying the patch. Your diff must
292 keep some context (3 lines above and 3 lines below) so that there's no doubt
293 where the code has to be applied. Don't change code outside of the context of
294 your patch (eg: take care of not adding/removing empty lines once you remove
295 your debugging code). If you are using Git (which is strongly recommended),
296 please produce your patches using "git format-patch" and not "git diff", and
297 always use "git show" after doing a commit to ensure it looks good, and
298 enable syntax coloring that will automatically report in red the trailing
299 spaces or tabs that your patch added to the code and that must absolutely be
300 removed. These ones cause a real pain to apply patches later because they
301 mangle the context in an invisible way. Such patches with trailing spaces at
302 end of lines will be rejected.
303
30410) Please cut your work in series of patches that can be independantly reviewed
305 and merged. Each patch must do something on its own that you can explain to
306 someone without being ashamed of what you did. For example, you must not say
307 "This is the patch that implements SSL, it was tricky". There's clearly
308 something wrong there, your patch will be huge, will definitely break things
309 and nobody will be able to figure what exactly introduced the bug. However
310 it's much better to say "I needed to add some fields in the session to store
311 the SSL context so this patch does this and doesn't touch anything else, so
312 it's safe". Also when dealing with series, you will sometimes fix a bug that
313 one of your patches introduced. Please do merge these fixes (eg: using git
314 rebase -i and squash or fixup), as it is not acceptable to see patches which
315 introduce known bugs even if they're fixed later. Another benefit of cleanly
316 splitting patches is that if some of your patches need to be reworked after
317 a review, the other ones can still be merged so that you don't need to care
318 about them anymore. When sending multiple patches for review, prefer to send
319 one e-mail per patch than all patches in a single e-mail. The reason is that
320 not everyone is skilled in all areas nor has the time to review everything
321 at once. With one patch per e-mail, it's easy to comment on a single patch
322 without giving an opinion on the other ones, especially if a long thread
323 starts about one specific patch on the mailing list. "git send-email" does
324 that for you though it requires a few trials before getting it right.
325
32611) Please properly format your commit messages. While it's not strictly
327 required to use Git, it is strongly recommended because it helps you do the
328 cleanest job with the least effort. Patches always have the format of an
329 e-mail made of a subject, a description and the actual patch. If you're
330 sending a patch as an e-mail formated this way, it can quickly be applied
331 with limited effort so that's acceptable. But in any case, it is important
332 that there is a clean description of what the patch does, the motivation for
333 what it does, why it's the best way to do it, its impacts, and what it does
334 not yet cover. Also, in HAProxy, like many projects which take a great care
335 of maintaining stable branches, patches are reviewed later so that some of
336 them can be backported to stable releases. While reviewing hundreds of
337 patches can seem cumbersome, with a proper formating of the subject line it
338 actually becomes very easy. For example, here's how one can find patches
339 that need to be reviewed for backports (bugs and doc) between since commit
340 ID 827752e :
341
342 $ git log --oneline 827752e.. | grep 'BUG\|DOC'
343 0d79cf6 DOC: fix function name
344 bc96534 DOC: ssl: missing LF
345 10ec214 BUG/MEDIUM: lua: the lua fucntion Channel:close() causes a segf
346 bdc97a8 BUG/MEDIUM: lua: outgoing connection was broken since 1.6-dev2
347 ba56d9c DOC: mention support for RFC 5077 TLS Ticket extension in start
348 f1650a8 DOC: clarify some points about SSL and the proxy protocol
349 b157d73 BUG/MAJOR: peers: fix current table pointer not re-initialized
350 e1ab808 BUG/MEDIUM: peers: fix wrong message id on stick table updates
351 cc79b00 BUG/MINOR: ssl: TLS Ticket Key rotation broken via socket comma
352 d8e42b6 DOC: add new file intro.txt
353 c7d7607 BUG/MEDIUM: lua: bad error processing
354 386a127 DOC: match several lua configuration option names to those impl
355 0f4eadd BUG/MEDIUM: counters: ensure that src_{inc,clr}_gpc0 creates a
356
357 It is made possible by the fact that subject lines are properly formated and
358 always respect the same principle : one part indicating the nature and
359 severity of the patch, another one to indicate which subsystem is affected,
360 and the last one is a succint description of the change, with the important
361 part at the beginning so that it's obvious what it does even when lines are
362 truncated like above. The whole stable maintenance process relies on this.
363 For this reason, it is mandatory to respect some easy rules regarding the
364 way the subject is built. Please see the section below for more information
365 regarding this formating.
366
36712) When submitting changes, please always CC the mailing list address so that
368 everyone gets a chance to spot any issue in your code. It will also serve
369 as an advertisement for your work, you'll get more testers quicker and
370 you'll feel better knowing that people really use your work. It is also
371 important to CC any author mentionned in the file you change, or a subsystem
372 maintainers whose address is mentionned in a MAINTAINERS file. Not everyone
373 reads the list on a daily basis so it's very easy to miss some changes.
374 Don't consider it as a failure when a reviewer tells you you have to modify
375 your patch, actually it's a success because now you know what is missing for
376 your work to get accepted. That's why you should not hesitate to CC enough
377 people. Don't copy people who have no deal with your work area just because
378 you found their address on the list. That's the best way to appear careless
379 about their time and make them reject your changes in the future.
380
381
382Patch classifying rules
383-----------------------
384
385There are 3 criteria of particular importance in any patch :
386 - its nature (is it a fix for a bug, a new feature, an optimization, ...)
387 - its importance, which generally reflects the risk of merging/not merging it
388 - what area it applies to (eg: http, stats, startup, config, doc, ...)
389
390It's important to make these 3 criteria easy to spot in the patch's subject,
391because it's the first (and sometimes the only) thing which is read when
392reviewing patches to find which ones need to be backported to older versions.
393It also helps when trying to find which patch is the most likely to have caused
394a regression.
395
396Specifically, bugs must be clearly easy to spot so that they're never missed.
397Any patch fixing a bug must have the "BUG" tag in its subject. Most common
398patch types include :
399
400 - BUG fix for a bug. The severity of the bug should also be indicated
401 when known. Similarly, if a backport is needed to older versions,
402 it should be indicated on the last line of the commit message. If
403 the bug has been identified as a regression brought by a specific
404 patch or version, this indication will be appreciated too. New
405 maintenance releases are generally emitted when a few of these
406 patches are merged. If the bug is a vulnerability for which a CVE
407 identifier was assigned before you publish the fix, you can mention
408 it in the commit message, it will help distro maintainers.
409
410 - CLEANUP code cleanup, silence of warnings, etc... theorically no impact.
411 These patches will rarely be seen in stable branches, though they
412 may appear when they remove some annoyance or when they make
413 backporting easier. By nature, a cleanup is always of minor
414 importance and it's not needed to mention it.
415
416 - DOC updates to any of the documentation files, including README. Many
417 documentation updates are backported since they don't impact the
418 product's stability and may help users avoid bugs. So please
419 indicate in the commit message if a backport is desired. When a
420 feature gets documented, it's preferred that the doc patch appears
421 in the same patch or after the feature patch, but not before, as it
422 becomes confusing when someone working on a code base including
423 only the doc patch won't understand why a documented feature does
424 not work as documented.
425
426 - REORG code reorganization. Some blocks may be moved to other places,
427 some important checks might be swapped, etc... These changes
428 always present a risk of regression. For this reason, they should
429 never be mixed with any bug fix nor functional change. Code is
430 only moved as-is. Indicating the risk of breakage is highly
431 recommended. Minor breakage is tolerated in such patches if trying
432 to fix it at once makes the whole change even more confusing. That
433 may happen for example when some #ifdefs need to be propagated in
434 every file consecutive to the change.
435
436 - BUILD updates or fixes for build issues. Changes to makefiles also fall
437 into this category. The risk of breakage should be indicated if
438 known. It is also appreciated to indicate what platforms and/or
439 configurations were tested after the change.
440
441 - OPTIM some code was optimised. Sometimes if the regression risk is very
442 low and the gains significant, such patches may be merged in the
443 stable branch. Depending on the amount of code changed or replaced
444 and the level of trust the author has in the change, the risk of
445 regression should be indicated.
446
447 - RELEASE release of a new version (development or stable).
448
449 - LICENSE licensing updates (may impact distro packagers).
450
451
452When the patch cannot be categorized, it's best not to put any type tag. This is
453commonly the case for new features, which development versions are mostly made
454of.
455
456Additionally, the importance of the patch or severity of the bug it fixes must
457be indicated when relevant. A single upper-case word is preferred, among :
458
459 - MINOR minor change, very low risk of impact. It is often the case for
460 code additions that don't touch live code. As a rule of thumb, a
461 patch tagged "MINOR" is safe enough to be backported to stable
462 branches. For a bug, it generally indicates an annoyance, nothing
463 more.
464
465 - MEDIUM medium risk, may cause unexpected regressions of low importance or
466 which may quickly be discovered. In short, the patch is safe but
467 touches working areas and it is always possible that you missed
468 something you didn't know existed (eg: adding a "case" entry or
469 an error message after adding an error code to an enum). For a bug,
470 it generally indicates something odd which requires changing the
471 configuration in an undesired way to work around the issue.
472
473 - MAJOR major risk of hidden regression. This happens when large parts of
474 the code are rearranged, when new timeouts are introduced, when
475 sensitive parts of the session scheduling are touched, etc... We
476 should only exceptionally find such patches in stable branches when
477 there is no other option to fix a design issue. For a bug, it
478 indicates severe reliability issues for which workarounds are
479 identified with or without performance impacts.
480
481 - CRITICAL medium-term reliability or security is at risk and workarounds,
482 if they exist, might not always be acceptable. An upgrade is
483 absolutely required. A maintenance release may be emitted even if
484 only one of these bugs are fixed. Note that this tag is only used
485 with bugs. Such patches must indicate what is the first version
486 affected, and if known, the commit ID which introduced the issue.
487
488The expected length of the commit message grows with the importance of the
489change. While a MINOR patch may sometimes be described in 1 or 2 lines, MAJOR
490or CRITICAL patches cannot have less than 10-15 lines to describe exactly the
491impacts otherwise the submitter's work will be considered as rough sabotage.
492
493For BUILD, DOC and CLEANUP types, this tag is not always relevant and may be
494omitted.
495
496The area the patch applies to is quite important, because some areas are known
497to be similar in older versions, suggesting a backport might be desirable, and
498conversely, some areas are known to be specific to one version. The area is a
499single-word lowercase name the contributor find clear enough to describe what
500part is being touched. The following tags are suggested but not limitative :
501
502 - examples example files. Be careful, sometimes these files are packaged.
503
504 - tests regression test files. No code is affected, no need to upgrade.
505
506 - init initialization code, arguments parsing, etc...
507
508 - config configuration parser, mostly used when adding new config keywords
509
510 - http the HTTP engine
511
512 - stats the stats reporting engine
513
514 - cli the stats socket CLI
515
516 - checks the health checks engine (eg: when adding new checks)
517
518 - sample the sample fetch system (new fetch or converter functions)
519
520 - acl the ACL processing core or some ACLs from other areas
521
522 - peers the peer synchronization engine
523
524 - lua the Lua scripting engine
525
526 - listeners everything related to incoming connection settings
527
528 - frontend everything related to incoming connection processing
529
530 - backend everything related to LB algorithms and server farm
531
532 - session session processing and flags (very sensible, be careful)
533
534 - server server connection management, queueing
535
536 - ssl the SSL/TLS interface
537
538 - proxy proxy maintenance (start/stop)
539
540 - log log management
541
542 - poll any of the pollers
543
544 - halog the halog sub-component in the contrib directory
545
546 - contrib any addition to the contrib directory
547
548Other names may be invented when more precise indications are meaningful, for
549instance : "cookie" which indicates cookie processing in the HTTP core. Last,
550indicating the name of the affected file is also a good way to quickly spot
551changes. Many commits were already tagged with "stream_sock" or "cfgparse" for
552instance.
553
554It is required that the type of change and the severity when relevant are
555indicated, as well as the touched area when relevant as well in the patch
556subject. Normally, we would have the 3 most often. The two first criteria should
557be present before a first colon (':'). If both are present, then they should be
558delimited with a slash ('/'). The 3rd criterion (area) should appear next, also
559followed by a colon. Thus, all of the following messages are valid :
560
561Examples of messages :
562 - DOC: document options forwardfor to logasap
563 - DOC/MAJOR: reorganize the whole document and change indenting
564 - BUG: stats: connection reset counters must be plain ascii, not HTML
565 - BUG/MINOR: stats: connection reset counters must be plain ascii, not HTML
566 - MEDIUM: checks: support multi-packet health check responses
567 - RELEASE: Released version 1.4.2
568 - BUILD: stats: stdint is not present on solaris
569 - OPTIM/MINOR: halog: make fgets parse more bytes by blocks
570 - REORG/MEDIUM: move syscall redefinition to specific places
571
572Please do not use square brackets anymore around the tags, because they induce
573more work when merging patches, which need to be hand-edited not to lose the
574enclosed part.
575
576In fact, one of the only square bracket tags that still makes sense is '[RFC]'
577at the beginning of the subject, when you're asking for someone to review your
578change before getting it merged. If the patch is OK to be merged, then it can
579be merge as-is and the '[RFC]' tag will automatically be removed. If you don't
580want it to be merged at all, you can simply state it in the message, or use an
581alternate 'WIP/' prefix in front of your tag tag ("work in progress").
582
583The tags are not rigid, follow your intuition first, and they may be readjusted
584when your patch is merged. It may happen that a same patch has a different tag
585in two distinct branches. The reason is that a bug in one branch may just be a
586cleanup or safety measure in the other one because the code cannot be triggered.
587
588
589Working with Git
590----------------
591
592For a more efficient interaction between the mainline code and your code, you
593are strongly encouraged to try the Git version control system :
594
595 http://git-scm.com/
596
597It's very fast, lightweight and lets you undo/redo your work as often as you
598want, without making your mistakes visible to the rest of the world. It will
599definitely help you contribute quality code and take other people's feedback
600in consideration. In order to clone the HAProxy Git repository :
601
602 $ git clone http://git.haproxy.org/git/haproxy.git/ (development)
603
604If you decide to use Git for your developments, then your commit messages will
605have the subject line in the format described above, then the whole description
606of your work (mainly why you did it) will be in the body. You can directly send
607your commits to the mailing list, the format is convenient to read and process.
608
609It is recommended to create a branch for your work that is based on the master
610branch :
611
612 $ git checkout -b 20150920-fix-stats master
613
614You can then do your work and even experiment with multiple alternatives if you
615are not completely sure that your solution is the best one :
616
617 $ git checkout -b 20150920-fix-stats-v2
618
619Then reorder/merge/edit your patches :
620
621 $ git rebase -i master
622
623When you think you're ready, reread your whole patchset to ensure there is no
624formating or style issue :
625
626 $ git show master..
627
628And once you're satisfied, you should update your master branch to be sure that
Thiago Farinaee6ec8c2016-04-01 16:43:50 -0300629nothing changed during your work (only needed if you left it unattended for days
Willy Tarreau50529282015-09-20 22:31:42 +0200630or weeks) :
631
632 $ git checkout -b 20150920-fix-stats-rebased
633 $ git fetch origin master:master
634 $ git rebase master
635
Thiago Farinaee6ec8c2016-04-01 16:43:50 -0300636You can build a list of patches ready for submission like this :
Willy Tarreau50529282015-09-20 22:31:42 +0200637
638 $ git format-patch master
639
640The output files are the patches ready to be sent over e-mail, either via a
641regular e-mail or via git send-email (carefully check the man page). Don't
642destroy your other work branches until your patches get merged, it may happen
643that earlier designs will be preferred for various reasons. Patches should be
644sent to the mailing list : haproxy@formilux.org and CCed to relevant subsystem
645maintainers or authors of the modified files if their address appears at the
646top of the file.
647
648Please don't send pull-requests, they are really unconvenient. First, a pull
649implies a merge operation and the code doesn't move fast enough to justify the
650use of merges. Second, pull requests are not easily commented on by the
651project's participants, contrary to e-mails where anyone is allowed to have an
652opinion and to express it.
653
654-- end