| HOW TO GET YOUR CODE ACCEPTED IN HAPROXY |
| READ THIS CAREFULLY BEFORE SUBMITTING CODE |
| |
| THIS DOCUMENT PROVIDES SOME RULES TO FOLLOW WHEN SENDING CONTRIBUTIONS. PATCHES |
| NOT FOLLOWING THESE RULES WILL SIMPLY BE IGNORED IN ORDER TO PROTECT ALL OTHER |
| RESPECTFUL CONTRIBUTORS' VALUABLE TIME. |
| |
| |
| Abstract |
| -------- |
| |
| If you have never contributed to HAProxy before, or if you did so and noticed |
| that nobody seems to be interested in reviewing your submission, please do read |
| this long document carefully. HAProxy maintainers are particularly demanding on |
| respecting certain simple rules related to general code and documentation style |
| as well as splitting your patches and providing high quality commit messages. |
| The reason behind this is that your patch will be met multiple times in the |
| future, when doing some backporting work or when bisecting a bug, and it is |
| critical that anyone can quickly decide if the patch is right, wrong, if it |
| misses something, if it must be reverted or needs to be backported. Maintainers |
| are generally benevolent with newcomers and will help them provided their work |
| indicates they have at least read this document. Some have improved over time, |
| to the point of being totally trusted and gaining commit access so they don't |
| need to depend on anyone to pick their code. On the opposite, those who insist |
| not making minimal efforts however will simply be ignored. |
| |
| |
| Background |
| ---------- |
| |
| HAProxy is a community-driven project. But like most highly technical projects |
| it takes a lot of time to develop the skills necessary to be autonomous in the |
| project, and there is a very small core team helped by a small set of very |
| active participants. While most of the core team members work on the code as |
| part of their day job, most participants do it on a voluntary basis during |
| their spare time. The ideal model for developers is to spend their time : |
| 1) developing new features |
| 2) fixing bugs |
| 3) doing maintenance backports |
| 4) reviewing other people's code |
| |
| It turns out that on a project like HAProxy, like many other similarly complex |
| projects, the time spent is exactly the opposite : |
| 1) reviewing other peopel's code |
| 2) doing maintenance backports |
| 3) fixing bugs |
| 4) developing new features |
| |
| A large part of the time spent reviewing code often consists in giving basic |
| recommendations that are already explained in this file. In addition to taking |
| time, it is not appealing for people willing to spend one hour helping others |
| to do the same thing over and over instead of discussing the code design, and |
| it tends to delay the start of code reviews. |
| |
| Regarding backports, they are necessary to provide a set of stable branches |
| that are deployed in production at many places. Load balancers are complex and |
| new features often induce undesired side effects in other areas, which we will |
| call bugs. Thus it's common for users to stick to a branch featuring everything |
| they need and not to upgrade too often. This backporting job is critical to the |
| ecosystem's health and must be done regularly. Very often the person devoting |
| some time on backports has little to no information about the relevance (let |
| alone importance) of a patch and is unlikely to be an expert in the area |
| affected by the patch. It's the role of the commit message to explain WHAT |
| problem the patch tries to solve, WHY it is estimated that it is a problem, and |
| HOW it tries to address it. With these elements, the person in charge of the |
| backports can decide whether or not to pick the patch. And if the patch does |
| not apply (which is common for older versions) they have information in the |
| commit message about the principle and choices that the initial developer made |
| and will try to adapt the patch sticking to these principles. Thus, the time |
| spent backporting patches solely depends on the code quality and the commit |
| message details and accuracy. |
| |
| When it turns to fixing bugs, before declaring a bug, there is an analysis |
| phase. It starts with "is this behaviour expected", "is it normal", "under what |
| circumstances does it happen", "when did it start to happen", "was it intended", |
| "was it just overlooked", and "how to fix it without breaking the initial |
| intent". A utility called "git bisect" is usually involved in determining when |
| the behaviour started to happen. It determines the first patch which introduced |
| the new behaviour. If the patch is huge, touches many areas, is really difficult |
| to read because it needlessly reindents code or adds/removes line breaks out of |
| context, it will be very difficult to figure what part of this patch broke the |
| behaviour. Then once the part is figured, if the commit message doesn't provide |
| a detailed description about the intent of the patch, i.e. the problem it was |
| trying to solve, why and how, the developer landing on that patch will really |
| feel powerless. And very often in this case, the fix for the problem will break |
| something else or something that depended on the original patch. |
| |
| But contrary to what it could look like, providing great quality patches is not |
| difficult, and developers will always help contributors improve their patches |
| quality because it's in their interest as well. History has shown that first |
| time contributors can provide an excellent work when they have carefully read |
| this document, and that people coming from projects with different practices |
| can grow from first-time contributor to trusted committer in about 6 months. |
| |
| |
| Preparation |
| ----------- |
| |
| It is possible that you'll want to add a specific feature to satisfy your needs |
| or one of your customers'. Contributions are welcome, however maintainers are |
| often very picky about changes. Patches that change massive parts of the code, |
| or that touch the core parts without any good reason will generally be rejected |
| if those changes have not been discussed first. |
| |
| The proper place to discuss your changes is the HAProxy Mailing List. There are |
| enough skilled readers to catch hazardous mistakes and to suggest improvements. |
| There is no other place where you'll find as many skilled people on the project, |
| and these people can help you get your code integrated quickly. You can |
| subscribe to it by sending an empty e-mail at the following address : |
| |
| haproxy+subscribe@formilux.org |
| |
| It is not even necessary to subscribe, you can post there and verify via the |
| public list archives that your message was properly delivered. In this case you |
| should indicate in your message that you'd like responders to keep you CCed. |
| Please visit http://haproxy.org/ to figure available options to join the list. |
| |
| If you have an idea about something to implement, *please* discuss it on the |
| list first. It has already happened several times that two persons did the same |
| thing simultaneously. This is a waste of time for both of them. It's also very |
| common to see some changes rejected because they're done in a way that will |
| conflict with future evolutions, or that does not leave a good feeling. It's |
| always unpleasant for the person who did the work, and it is unpleasant in |
| general because people's time and efforts are valuable and would be better |
| spent working on something else. That would not happen if these were discussed |
| first. There is no problem posting work in progress to the list, it happens |
| quite often in fact. Just prefix your mail subject with "RFC" (it stands for |
| "request for comments") and everyone will understand you'd like some opinion |
| on your work in progress. Also, don't waste your time with the doc when |
| submitting patches for review, only add the doc with the patch you consider |
| ready to merge (unless you need some help on the doc itself, of course). |
| |
| Another important point concerns code portability. Haproxy requires gcc as the |
| C compiler, and may or may not work with other compilers. However it's known to |
| build using gcc 2.95 or any later version. As such, it is important to keep in |
| mind that certain facilities offered by recent versions must not be used in the |
| code : |
| |
| - declarations mixed in the code (requires gcc >= 3.x and is a bad practice) |
| - GCC builtins without checking for their availability based on version and |
| architecture ; |
| - assembly code without any alternate portable form for other platforms |
| - use of stdbool.h, "bool", "false", "true" : simply use "int", "0", "1" |
| - in general, anything which requires C99 (such as declaring variables in |
| "for" statements) |
| |
| Since most of these restrictions are just a matter of coding style, it is |
| normally not a problem to comply. Please read doc/coding-style.txt for all the |
| details. |
| |
| When modifying some optional subsystem (SSL, Lua, compression, device detection |
| engines), please make sure the code continues to build (and to work) when these |
| features are disabled. Similarly, when modifying the SSL stack, please always |
| ensure that supported OpenSSL versions continue to build and to work, especially |
| if you modify support for alternate libraries. Clean support for the legacy |
| OpenSSL libraries is mandatory, support for its derivatives is a bonus and may |
| occasionally break eventhough a great care is taken. In other words, if you |
| provide a patch for OpenSSL you don't need to test its derivatives, but if you |
| provide a patch for a derivative you also need to test with OpenSSL. |
| |
| If your work is very confidential and you can't publicly discuss it, you can |
| also mail willy@haproxy.org directly about it, but your mail may be waiting |
| several days in the queue before you get a response, if you get a response at |
| all. Retransmit if you don't get a response by one week. Please note that |
| direct sent e-mails to this address for non-confidential subjects may simply |
| be forwarded to the list or be deleted without notification. An auto-responder |
| bot is in place to try to detect e-mails from people asking for help and to |
| redirect them to the mailing list. Do not be surprised if this happens to you. |
| |
| If you'd like a feature to be added but you think you don't have the skills to |
| implement it yourself, you should follow these steps : |
| |
| 1. discuss the feature on the mailing list. It is possible that someone |
| else has already implemented it, or that someone will tell you how to |
| proceed without it, or even why not to do it. It is also possible that |
| in fact it's quite easy to implement and people will guide you through |
| the process. That way you'll finally have YOUR patch merged, providing |
| the feature YOU need. |
| |
| 2. if you really can't code it yourself after discussing it, then you may |
| consider contacting someone to do the job for you. Some people on the |
| list might sometimes be OK with trying to do it. |
| |
| The version control system used by the project (Git) keeps authorship |
| information in the form of the patch author's e-mail address. This way you will |
| be credited for your work in the project's history. If you contract with |
| someone to implement your idea you may have to discuss such modalities with |
| the person doing the work as by default this person will be mentioned as the |
| work's author. |
| |
| |
| Rules : the 12 laws of patch contribution |
| ----------------------------------------- |
| |
| People contributing patches must apply the following rules. That may sound heavy |
| at the beginning but it's common sense more than anything else and contributors |
| do not think about them anymore after a few patches. |
| |
| 1) Comply with the license |
| |
| Before modifying some code, you have read the LICENSE file ("main license") |
| coming with the sources, and all the files this file references. Certain |
| files may be covered by different licenses, in which case it will be |
| indicated in the files themselves. In any case, you agree to respect these |
| licenses and to contribute your changes under the same licenses. If you want |
| to create new files, they will be under the main license, or any license of |
| your choice that you have verified to be compatible with the main license, |
| and that will be explicitly mentioned in the affected files. The project's |
| maintainers are free to reject contributions proposing license changes they |
| feel are not appropriate or could cause future trouble. |
| |
| 2) Develop on development branch, not stable ones |
| |
| Your work may only be based on the latest development version. No development |
| is made on a stable branch. If your work needs to be applied to a stable |
| branch, it will first be applied to the development branch and only then will |
| be backported to the stable branch. You are responsible for ensuring that |
| your work correctly applies to the development version. If at any moment you |
| are going to work on restructuring something important which may impact other |
| contributors, the rule that applies is that the first sent is the first |
| served. However it is considered good practice and politeness to warn others |
| in advance if you know you're going to make changes that may force them to |
| re-adapt their code, because they did probably not expect to have to spend |
| more time discovering your changes and rebasing their work. |
| |
| 3) Read and respect the coding style |
| |
| You have read and understood "doc/coding-style.txt", and you're actively |
| determined to respect it and to enforce it on your coworkers if you're going |
| to submit a team's work. We don't care what text editor you use, whether it's |
| an hex editor, cat, vi, emacs, Notepad, Word, or even Eclipse. The editor is |
| only the interface between you and the text file. What matters is what is in |
| the text file in the end. The editor is not an excuse for submitting poorly |
| indented code, which only proves that the person has no consideration for |
| quality and/or has done it in a hurry (probably worse). Please note that most |
| bugs were found in low-quality code. Reviewers know this and tend to be much |
| more reluctant to accept poorly formated code because by experience they |
| won't trust their author's ability to write correct code. It is also worth |
| noting that poor quality code is painful to read and may result in nobody |
| willing to waste their time even reviewing your work. |
| |
| 4) Present clean work |
| |
| The time it takes for you to polish your code is always much smaller than the |
| time it takes others to do it for you, because they always have to wonder if |
| what they see is intended (meaning they didn't understand something) or if it |
| is a mistake that needs to be fixed. And since there are less reviewers than |
| submitters, it is vital to spread the effort closer to where the code is |
| written and not closer to where it gets merged. For example if you have to |
| write a report for a customer that your boss wants to review before you send |
| it to the customer, will you throw on his desk a pile of paper with stains, |
| typos and copy-pastes everywhere ? Will you say "come on, OK I made a mistake |
| in the company's name but they will find it by themselves, it's obvious it |
| comes from us" ? No. When in doubt, simply ask for help on the mailing list. |
| |
| 5) Documentation is very important |
| |
| There are four levels of importance of quality in the project : |
| |
| - The most important one, and by far, is the quality of the user-facing |
| documentation. This is the first contact for most users and it immediately |
| gives them an accurate idea of how the project is maintained. Dirty docs |
| necessarily belong to a dirty project. Be careful to the way the text you |
| add is presented and indented. Be very careful about typos, usual mistakes |
| such as double consonants when only one is needed or "it's" instead of |
| "its", don't mix US english and UK english in the same paragraph, etc. |
| When in doubt, check in a dictionary. Fixes for existing typos in the doc |
| are always welcome and chasing them is a good way to become familiar with |
| the project and to get other participants' respect and consideration. |
| |
| - The second most important level is user-facing messages emitted by the |
| code. You must try to see all the messages your code produces to ensure |
| they are understandable outside of the context where you wrote them, |
| because the user often doesn't expect them. That's true for warnings, and |
| that's even more important for errors which prevent the program from |
| working and which require an immediate and well understood fix in the |
| configuration. It's much better to say "line 35: compression level must be |
| an integer between 1 and 9" than "invalid argument at line 35". In HAProxy, |
| error handling roughly represents half of the code, and that's about 3/4 of |
| the configuration parser. Take the time to do something you're proud of. A |
| good rule of thumb is to keep in mind that your code talks to a human and |
| tries to teach him/her how to proceed. It must then speak like a human. |
| |
| - The third most important level is the code and its accompanying comments, |
| including the commit message which is a complement to your code and |
| comments. It's important for all other contributors that the code is |
| readable, fluid, understandable and that the commit message describes what |
| was done, the choices made, the possible alternatives you thought about, |
| the reason for picking this one and its limits if any. Comments should be |
| written where it's easy to have a doubt or after some error cases have been |
| wiped out and you want to explain what possibilities remain. All functions |
| must have a comment indicating what they take on input and what they |
| provide on output. Please adjust the comments when you copy-paste a |
| function or change its prototype, this type of lazy mistake is too common |
| and very confusing when reading code later to debug an issue. Do not forget |
| that others will feel really angry at you when they have to dig into your |
| code for a bug that your code caused and they feel like this code is dirty |
| or confusing, that the commit message doesn't explain anything useful and |
| that the patch should never have been accepted in the first place. That |
| will strongly impact your reputation and will definitely affect your |
| chances to contribute again! |
| |
| - The fourth level of importance is in the technical documentation that you |
| may want to add with your code. Technical documentation is always welcome |
| as it helps others make the best use of your work and to go exactly in the |
| direction you thought about during the design. This is also what reduces |
| the risk that your design gets changed in the near future due to a misuse |
| and/or a poor understanding. All such documentation is actually considered |
| as a bonus. It is more important that this documentation exists than that |
| it looks clean. Sometimes just copy-pasting your draft notes in a file to |
| keep a record of design ideas is better than losing them. Please do your |
| best so that other ones can read your doc. If these docs require a special |
| tool such as a graphics utility, ensure that the file name makes it |
| unambiguous how to process it. So there are no rules here for the contents, |
| except one. Please write the date in your file. Design docs tend to stay |
| forever and to remain long after they become obsolete. At this point that |
| can cause harm more than it can help. Writing the date in the document |
| helps developers guess the degree of validity and/or compare them with the |
| date of certain commits touching the same area. |
| |
| 6) US-ASCII only! |
| |
| All text files and commit messages are written using the US-ASCII charset. |
| Please be careful that your contributions do not contain any character not |
| printable using this charset, as they will render differently in different |
| editors and/or terminals. Avoid latin1 and more importantly UTF-8 which some |
| editors tend to abuse to replace some US-ASCII characters with their |
| typographic equivalent which aren't readable anymore in other editors. The |
| only place where alternative charsets are tolerated is in your name in the |
| commit message, but it's at your own risk as it can be mangled during the |
| merge. Anyway if you have an e-mail address, you probably have a valid |
| US-ASCII representation for it as well. |
| |
| 7) Comments |
| |
| Be careful about comments when you move code around. It's not acceptable that |
| a block of code is moved to another place leaving irrelevant comments at the |
| old place, just like it's not acceptable that a function is duplicated without |
| the comments being adjusted. The example below started to become quite common |
| during the 1.6 cycle, it is not acceptable and wastes everyone's time : |
| |
| /* Parse switching <str> to build rule <rule>. Returns 0 on error. */ |
| int parse_switching_rule(const char *str, struct rule *rule) |
| { |
| ... |
| } |
| |
| /* Parse switching <str> to build rule <rule>. Returns 0 on error. */ |
| void execute_switching_rule(struct rule *rule) |
| { |
| ... |
| } |
| |
| This patch is not acceptable either (and it's unfortunately not that rare) : |
| |
| + if (!session || !arg || list_is_empty(&session->rules->head)) |
| + return 0; |
| + |
| /* Check if session->rules is valid before dereferencing it */ |
| if (!session->rules_allocated) |
| return 0; |
| |
| - if (!arg || list_is_empty(&session->rules->head)) |
| - return 0; |
| - |
| |
| 8) Short, readable identifiers |
| |
| Limit the length of your identifiers in the code. When your identifiers start |
| to sound like sentences, it's very hard for the reader to keep on track with |
| what operation they are observing. Also long names force expressions to fit |
| on several lines which also cause some difficulties to the reader. See the |
| example below : |
| |
| int file_name_len_including_global_path; |
| int file_name_len_without_global_path; |
| int global_path_len_or_zero_if_default; |
| |
| if (global_path) |
| global_path_len_or_zero_if_default = strlen(global_path); |
| else |
| global_path_len_or_zero_if_default = 0; |
| |
| file_name_len_without_global_path = strlen(file_name); |
| file_name_len_including_global_path = |
| file_name_len_without_global_path + 1 + /* for '/' */ |
| global_path_len_or_zero_if_default ? |
| global_path_len_or_zero_if_default : default_path_len; |
| |
| Compare it to this one : |
| |
| int f, p; |
| |
| p = global_path ? strlen(global_path) : default_path_len; |
| f = p + 1 + strlen(file_name); /* 1 for '/' */ |
| |
| A good rule of thumb is that if your identifiers start to contain more than |
| 3 words or more than 15 characters, they can become confusing. For function |
| names it's less important especially if these functions are rarely used or |
| are used in a complex context where it is important to differentiate between |
| their multiple variants. |
| |
| 9) Unified diff only |
| |
| The best way to build your patches is to use "git format-patch". This means |
| that you have committed your patch to a local branch, with an appropriate |
| subject line and a useful commit message explaining what the patch attempts |
| to do. It is not strictly required to use git, but what is strictly required |
| is to have all these elements in the same mail, easily distinguishable, and |
| a patch in "diff -up" format (which is also the format used by Git). This |
| means the "unified" diff format must be used exclusively, and with the |
| function name printed in the diff header of each block. That significantly |
| helps during reviews. Keep in mind that most reviews are done on the patch |
| and not on the code after applying the patch. Your diff must keep some |
| context (3 lines above and 3 lines below) so that there's no doubt where the |
| code has to be applied. Don't change code outside of the context of your |
| patch (eg: take care of not adding/removing empty lines once you remove |
| your debugging code). If you are using Git (which is strongly recommended), |
| always use "git show" after doing a commit to ensure it looks good, and |
| enable syntax coloring that will automatically report in red the trailing |
| spaces or tabs that your patch added to the code and that must absolutely be |
| removed. These ones cause a real pain to apply patches later because they |
| mangle the context in an invisible way. Such patches with trailing spaces at |
| end of lines will be rejected. |
| |
| 10) One patch per feature |
| |
| Please cut your work in series of patches that can be independently reviewed |
| and merged. Each patch must do something on its own that you can explain to |
| someone without being ashamed of what you did. For example, you must not say |
| "This is the patch that implements SSL, it was tricky". There's clearly |
| something wrong there, your patch will be huge, will definitely break things |
| and nobody will be able to figure what exactly introduced the bug. However |
| it's much better to say "I needed to add some fields in the session to store |
| the SSL context so this patch does this and doesn't touch anything else, so |
| it's safe". Also when dealing with series, you will sometimes fix a bug that |
| one of your patches introduced. Please do merge these fixes (eg: using git |
| rebase -i and squash or fixup), as it is not acceptable to see patches which |
| introduce known bugs even if they're fixed later. Another benefit of cleanly |
| splitting patches is that if some of your patches need to be reworked after |
| a review, the other ones can still be merged so that you don't need to care |
| about them anymore. When sending multiple patches for review, prefer to send |
| one e-mail per patch than all patches in a single e-mail. The reason is that |
| not everyone is skilled in all areas nor has the time to review everything |
| at once. With one patch per e-mail, it's easy to comment on a single patch |
| without giving an opinion on the other ones, especially if a long thread |
| starts about one specific patch on the mailing list. "git send-email" does |
| that for you though it requires a few trials before getting it right. |
| |
| If you can, please always put all the bug fixes at the beginning of the |
| series. This often makes it easier to backport them because they will not |
| depend on context that your other patches changed. As a hint, if you can't |
| do this, there are little chances that your bug fix can be backported. |
| |
| 11) Real commit messages please! |
| |
| Please properly format your commit messages. To get an idea, just run |
| "git log" on the file you've just modified. Patches always have the format |
| of an e-mail made of a subject, a description and the actual patch. If you |
| are sending a patch as an e-mail formatted this way, it can quickly be |
| applied with limited effort so that's acceptable : |
| |
| - A subject line (may wrap to the next line, but please read below) |
| - an empty line (subject delimiter) |
| - a non-empty description (the body of the e-mail) |
| - the patch itself |
| |
| The subject describes the "What" of the change ; the description explains |
| the "why", the "how" and sometimes "what next". For example a commit message |
| looking like this will be rejected : |
| |
| | From: Mr Foobar <foobar@example.com> |
| | Subject: BUG: fix typo in ssl_sock |
| | |
| |
| This one as well (too long subject, not the right place for the details) : |
| |
| | From: Mr Foobar <foobar@example.com> |
| | Subject: BUG/MEDIUM: ssl: use an error flag to prevent ssl_read() from |
| | returning 0 when dealing with large buffers because that can cause |
| | an infinite loop |
| | |
| |
| This one ought to be used instead : |
| |
| | From: Mr Foobar <foobar@example.com> |
| | Subject: BUG/MEDIUM: ssl: fix risk of infinite loop in ssl_sock |
| | |
| | ssl_read() must not return 0 on error or the caller may loop forever. |
| | Instead we add a flag to the connection to notify about the error and |
| | check it at all call places. This situation can only happen with large |
| | buffers so a workaround is to limit buffer sizes. Another option would |
| | have been to return -1 but it required to use signed ints everywhere |
| | and would have made the patch larger and riskier. This fix should be |
| | backported to versions 1.2 and upper. |
| |
| It is important to understand that for any reader to guess the text above |
| when it's absent, it will take a huge amount of time. If you made the |
| analysis leading to your patch, you must explain it, including the ideas |
| you dropped if you had a good reason for this. |
| |
| While it's not strictly required to use Git, it is strongly recommended |
| because it helps you do the cleanest job with the least effort. But if you |
| are comfortable with writing clean e-mails and inserting your patches, you |
| don't need to use Git. |
| |
| But in any case, it is important that there is a clean description of what |
| the patch does, the motivation for what it does, why it's the best way to do |
| it, its impacts, and what it does not yet cover. Also, in HAProxy, like many |
| projects which take a great care of maintaining stable branches, patches are |
| reviewed later so that some of them can be backported to stable releases. |
| |
| While reviewing hundreds of patches can seem cumbersome, with a proper |
| formatting of the subject line it actually becomes very easy. For example, |
| here's how one can find patches that need to be reviewed for backports (bugs |
| and doc) between since commit ID 827752e : |
| |
| $ git log --oneline 827752e.. | grep 'BUG\|DOC' |
| 0d79cf6 DOC: fix function name |
| bc96534 DOC: ssl: missing LF |
| 10ec214 BUG/MEDIUM: lua: the lua function Channel:close() causes a segf |
| bdc97a8 BUG/MEDIUM: lua: outgoing connection was broken since 1.6-dev2 |
| ba56d9c DOC: mention support for RFC 5077 TLS Ticket extension in start |
| f1650a8 DOC: clarify some points about SSL and the proxy protocol |
| b157d73 BUG/MAJOR: peers: fix current table pointer not re-initialized |
| e1ab808 BUG/MEDIUM: peers: fix wrong message id on stick table updates |
| cc79b00 BUG/MINOR: ssl: TLS Ticket Key rotation broken via socket comma |
| d8e42b6 DOC: add new file intro.txt |
| c7d7607 BUG/MEDIUM: lua: bad error processing |
| 386a127 DOC: match several lua configuration option names to those impl |
| 0f4eadd BUG/MEDIUM: counters: ensure that src_{inc,clr}_gpc0 creates a |
| |
| It is made possible by the fact that subject lines are properly formatted and |
| always respect the same principle : one part indicating the nature and |
| severity of the patch, another one to indicate which subsystem is affected, |
| and the last one is a succinct description of the change, with the important |
| part at the beginning so that it's obvious what it does even when lines are |
| truncated like above. The whole stable maintenance process relies on this. |
| For this reason, it is mandatory to respect some easy rules regarding the |
| way the subject is built. Please see the section below for more information |
| regarding this formatting. |
| |
| As a rule of thumb, your patch MUST NEVER be made only of a subject line, |
| it *must* contain a description. Even one or two lines, or indicating |
| whether a backport is desired or not. It turns out that single-line commits |
| are so rare in the Git world that they require special manual (hence |
| painful) handling when they are backported, and at least for this reason |
| it's important to keep this in mind. |
| |
| Maintainers who pick your patch may slightly adjust the description as they |
| see fit. Do not see this as a failure to do a clean job, it just means they |
| think it will help them do their daily job this way. The code may also be |
| slightly adjusted before being merged (non-functional changes only, fix for |
| typos, tabs vs spaces for example), unless your patch contains a |
| Signed-off-By tag, in which case they will either modify it and mention the |
| changes after your Signed-off-By line, or (more likely) ask you to perform |
| these changes yourself. This ability to slightly adjust a patch before |
| merging is is the main reason for not using pull requests which do not |
| provide this facility and will require to iterate back and forth with the |
| submitter and significantly delay the patch inclusion. |
| |
| Each patch fixing a bug MUST be tagged with "BUG", a severity level, an |
| indication of the affected subsystem and a brief description of the nature |
| of the issue in the subject line, and a detailed analysis in the message |
| body. The explanation of the user-visible impact and the need for |
| backporting to stable branches or not are MANDATORY. Bug fixes with no |
| indication will simply be rejected as they are very likely to cause more |
| harm when nobody is able to tell whether or not the patch needs to be |
| backported or can be reverted in case of regression. |
| |
| When fixing a bug which is reproducible, if possible, the contributors are |
| strongly encouraged to write a regression testing VTC file for varnishtest |
| to add to reg-tests directory. More information about varnishtest may be |
| found in README file of reg-tests directory and in doc/regression-testing.txt |
| file. |
| |
| 12) Discuss on the mailing list |
| |
| Note, some first-time contributors might feel impressed or scared by posting |
| to a list. This list is frequented only by nice people who are willing to |
| help you polish your work so that it is perfect and can last long. What you |
| think could be perceived as a proof of incompetence or lack of care will |
| instead be a proof of your ability to work with a community. You will not be |
| judged nor blamed for making mistakes. The project maintainers are the ones |
| creating the most bugs and mistakes anyway, and nobody knows the project in |
| its entirety anymore so you're just like anyone else. And people who have no |
| consideration for other's work are quickly ejected from the list so the |
| place is as safe and welcoming to new contributors as it is to long time |
| ones. |
| |
| When submitting changes, please always CC the mailing list address so that |
| everyone gets a chance to spot any issue in your code. It will also serve |
| as an advertisement for your work, you'll get more testers quicker and |
| you'll feel better knowing that people really use your work. It's often |
| convenient to prepend "[PATCH]" in front of your mail's subject to mention |
| that this e-mail contains a patch (or a series of patches), because it will |
| easily catch reviewer's attention. It's automatically done by tools such as |
| "git format-patch" and "git send-email". If you don't want your patch to be |
| merged yet and prefer to show it for discussion, better tag it as "[RFC]" |
| (stands for "Request For Comments") and it will be reviewed but not merged |
| without your approval. It is also important to CC any author mentioned in |
| the file you change, or a subsystem maintainers whose address is mentioned |
| in a MAINTAINERS file. Not everyone reads the list on a daily basis so it's |
| very easy to miss some changes. Don't consider it as a failure when a |
| reviewer tells you you have to modify your patch, actually it's a success |
| because now you know what is missing for your work to get accepted. That's |
| why you should not hesitate to CC enough people. Don't copy people who have |
| no deal with your work area just because you found their address on the |
| list. That's the best way to appear careless about their time and make them |
| reject your changes in the future. |
| |
| |
| Patch classifying rules |
| ----------------------- |
| |
| There are 3 criteria of particular importance in any patch : |
| - its nature (is it a fix for a bug, a new feature, an optimization, ...) |
| - its importance, which generally reflects the risk of merging/not merging it |
| - what area it applies to (eg: http, stats, startup, config, doc, ...) |
| |
| It's important to make these 3 criteria easy to spot in the patch's subject, |
| because it's the first (and sometimes the only) thing which is read when |
| reviewing patches to find which ones need to be backported to older versions. |
| It also helps when trying to find which patch is the most likely to have caused |
| a regression. |
| |
| Specifically, bugs must be clearly easy to spot so that they're never missed. |
| Any patch fixing a bug must have the "BUG" tag in its subject. Most common |
| patch types include : |
| |
| - BUG fix for a bug. The severity of the bug should also be indicated |
| when known. Similarly, if a backport is needed to older versions, |
| it should be indicated on the last line of the commit message. If |
| the bug has been identified as a regression brought by a specific |
| patch or version, this indication will be appreciated too. New |
| maintenance releases are generally emitted when a few of these |
| patches are merged. If the bug is a vulnerability for which a CVE |
| identifier was assigned before you publish the fix, you can mention |
| it in the commit message, it will help distro maintainers. |
| |
| - CLEANUP code cleanup, silence of warnings, etc... theoretically no impact. |
| These patches will rarely be seen in stable branches, though they |
| may appear when they remove some annoyance or when they make |
| backporting easier. By nature, a cleanup is always of minor |
| importance and it's not needed to mention it. |
| |
| - DOC updates to any of the documentation files, including README. Many |
| documentation updates are backported since they don't impact the |
| product's stability and may help users avoid bugs. So please |
| indicate in the commit message if a backport is desired. When a |
| feature gets documented, it's preferred that the doc patch appears |
| in the same patch or after the feature patch, but not before, as it |
| becomes confusing when someone working on a code base including |
| only the doc patch won't understand why a documented feature does |
| not work as documented. |
| |
| - REORG code reorganization. Some blocks may be moved to other places, |
| some important checks might be swapped, etc... These changes |
| always present a risk of regression. For this reason, they should |
| never be mixed with any bug fix nor functional change. Code is |
| only moved as-is. Indicating the risk of breakage is highly |
| recommended. Minor breakage is tolerated in such patches if trying |
| to fix it at once makes the whole change even more confusing. That |
| may happen for example when some #ifdefs need to be propagated in |
| every file consecutive to the change. |
| |
| - BUILD updates or fixes for build issues. Changes to makefiles also fall |
| into this category. The risk of breakage should be indicated if |
| known. It is also appreciated to indicate what platforms and/or |
| configurations were tested after the change. |
| |
| - OPTIM some code was optimised. Sometimes if the regression risk is very |
| low and the gains significant, such patches may be merged in the |
| stable branch. Depending on the amount of code changed or replaced |
| and the level of trust the author has in the change, the risk of |
| regression should be indicated. If the optimization depends on the |
| architecture or on build options, it is important to verify that |
| the code continues to work without it. |
| |
| - RELEASE release of a new version (development or stable). |
| |
| - LICENSE licensing updates (may impact distro packagers). |
| |
| - REGTEST updates to any of the regression testing files found in reg-tests |
| directory, including README or any documentation file. |
| |
| |
| When the patch cannot be categorized, it's best not to put any type tag, and to |
| only use a risk or complexity information only as below. This is commonly the |
| case for new features, which development versions are mostly made of. |
| |
| The importance, complexity of the patch, or severity of the bug it fixes must |
| be indicated when relevant. A single upper-case word is preferred, among : |
| |
| - MINOR minor change, very low risk of impact. It is often the case for |
| code additions that don't touch live code. As a rule of thumb, a |
| patch tagged "MINOR" is safe enough to be backported to stable |
| branches. For a bug, it generally indicates an annoyance, nothing |
| more. |
| |
| - MEDIUM medium risk, may cause unexpected regressions of low importance or |
| which may quickly be discovered. In short, the patch is safe but |
| touches working areas and it is always possible that you missed |
| something you didn't know existed (eg: adding a "case" entry or |
| an error message after adding an error code to an enum). For a bug, |
| it generally indicates something odd which requires changing the |
| configuration in an undesired way to work around the issue. |
| |
| - MAJOR major risk of hidden regression. This happens when large parts of |
| the code are rearranged, when new timeouts are introduced, when |
| sensitive parts of the session scheduling are touched, etc... We |
| should only exceptionally find such patches in stable branches when |
| there is no other option to fix a design issue. For a bug, it |
| indicates severe reliability issues for which workarounds are |
| identified with or without performance impacts. |
| |
| - CRITICAL medium-term reliability or security is at risk and workarounds, |
| if they exist, might not always be acceptable. An upgrade is |
| absolutely required. A maintenance release may be emitted even if |
| only one of these bugs are fixed. Note that this tag is only used |
| with bugs. Such patches must indicate what is the first version |
| affected, and if known, the commit ID which introduced the issue. |
| |
| The expected length of the commit message grows with the importance of the |
| change. While a MINOR patch may sometimes be described in 1 or 2 lines, MAJOR |
| or CRITICAL patches cannot have less than 10-15 lines to describe exactly the |
| impacts otherwise the submitter's work will be considered as rough sabotage. |
| If you are sending a new patch series after a review, it is generally good to |
| enumerate at the end of the commit description what changed from the previous |
| one as it helps reviewers quickly glance over such changes and not re-read the |
| rest. |
| |
| For BUILD, DOC and CLEANUP types, this tag is not always relevant and may be |
| omitted. |
| |
| The area the patch applies to is quite important, because some areas are known |
| to be similar in older versions, suggesting a backport might be desirable, and |
| conversely, some areas are known to be specific to one version. The area is a |
| single-word lowercase name the contributor find clear enough to describe what |
| part is being touched. The following tags are suggested but not limitative : |
| |
| - examples example files. Be careful, sometimes these files are packaged. |
| |
| - tests regression test files. No code is affected, no need to upgrade. |
| |
| - reg-tests regression test files for varnishtest. No code is affected, no |
| need to upgrade. |
| |
| - init initialization code, arguments parsing, etc... |
| |
| - config configuration parser, mostly used when adding new config keywords |
| |
| - http the HTTP engine |
| |
| - stats the stats reporting engine |
| |
| - cli the stats socket CLI |
| |
| - checks the health checks engine (eg: when adding new checks) |
| |
| - sample the sample fetch system (new fetch or converter functions) |
| |
| - acl the ACL processing core or some ACLs from other areas |
| |
| - filters everything related to the filters core |
| |
| - peers the peer synchronization engine |
| |
| - lua the Lua scripting engine |
| |
| - listeners everything related to incoming connection settings |
| |
| - frontend everything related to incoming connection processing |
| |
| - backend everything related to LB algorithms and server farm |
| |
| - session session processing and flags (very sensible, be careful) |
| |
| - server server connection management, queueing |
| |
| - spoe SPOE code |
| |
| - ssl the SSL/TLS interface |
| |
| - proxy proxy maintenance (start/stop) |
| |
| - log log management |
| |
| - poll any of the pollers |
| |
| - halog the halog sub-component in the contrib directory |
| |
| - contrib any addition to the contrib directory |
| |
| - htx general HTX subsystem |
| |
| - mux-h1 HTTP/1.x multiplexer/demultiplexer |
| |
| - mux-h2 HTTP/2 multiplexer/demultiplexer |
| |
| - h1 general HTTP/1.x protocol parser |
| |
| - h2 general HTTP/2 protocol parser |
| |
| Other names may be invented when more precise indications are meaningful, for |
| instance : "cookie" which indicates cookie processing in the HTTP core. Last, |
| indicating the name of the affected file is also a good way to quickly spot |
| changes. Many commits were already tagged with "stream_sock" or "cfgparse" for |
| instance. |
| |
| It is required that the type of change and the severity when relevant are |
| indicated, as well as the touched area when relevant as well in the patch |
| subject. Normally, we would have the 3 most often. The two first criteria should |
| be present before a first colon (':'). If both are present, then they should be |
| delimited with a slash ('/'). The 3rd criterion (area) should appear next, also |
| followed by a colon. Thus, all of the following subject lines are valid : |
| |
| Examples of subject lines : |
| - DOC: document options forwardfor to logasap |
| - DOC/MAJOR: reorganize the whole document and change indenting |
| - BUG: stats: connection reset counters must be plain ascii, not HTML |
| - BUG/MINOR: stats: connection reset counters must be plain ascii, not HTML |
| - MEDIUM: checks: support multi-packet health check responses |
| - RELEASE: Released version 1.4.2 |
| - BUILD: stats: stdint is not present on solaris |
| - OPTIM/MINOR: halog: make fgets parse more bytes by blocks |
| - REORG/MEDIUM: move syscall redefinition to specific places |
| |
| Please do not use square brackets anymore around the tags, because they induce |
| more work when merging patches, which need to be hand-edited not to lose the |
| enclosed part. |
| |
| In fact, one of the only square bracket tags that still makes sense is '[RFC]' |
| at the beginning of the subject, when you're asking for someone to review your |
| change before getting it merged. If the patch is OK to be merged, then it can |
| be merge as-is and the '[RFC]' tag will automatically be removed. If you don't |
| want it to be merged at all, you can simply state it in the message, or use an |
| alternate 'WIP/' prefix in front of your tag tag ("work in progress"). |
| |
| The tags are not rigid, follow your intuition first, and they may be readjusted |
| when your patch is merged. It may happen that a same patch has a different tag |
| in two distinct branches. The reason is that a bug in one branch may just be a |
| cleanup or safety measure in the other one because the code cannot be triggered. |
| |
| |
| Working with Git |
| ---------------- |
| |
| For a more efficient interaction between the mainline code and your code, you |
| are strongly encouraged to try the Git version control system : |
| |
| http://git-scm.com/ |
| |
| It's very fast, lightweight and lets you undo/redo your work as often as you |
| want, without making your mistakes visible to the rest of the world. It will |
| definitely help you contribute quality code and take other people's feedback |
| in consideration. In order to clone the HAProxy Git repository : |
| |
| $ git clone http://git.haproxy.org/git/haproxy.git/ (development) |
| |
| If you decide to use Git for your developments, then your commit messages will |
| have the subject line in the format described above, then the whole description |
| of your work (mainly why you did it) will be in the body. You can directly send |
| your commits to the mailing list, the format is convenient to read and process. |
| |
| It is recommended to create a branch for your work that is based on the master |
| branch : |
| |
| $ git checkout -b 20150920-fix-stats master |
| |
| You can then do your work and even experiment with multiple alternatives if you |
| are not completely sure that your solution is the best one : |
| |
| $ git checkout -b 20150920-fix-stats-v2 |
| |
| Then reorder/merge/edit your patches : |
| |
| $ git rebase -i master |
| |
| When you think you're ready, reread your whole patchset to ensure there is no |
| formatting or style issue : |
| |
| $ git show master.. |
| |
| And once you're satisfied, you should update your master branch to be sure that |
| nothing changed during your work (only needed if you left it unattended for days |
| or weeks) : |
| |
| $ git checkout -b 20150920-fix-stats-rebased |
| $ git fetch origin master:master |
| $ git rebase master |
| |
| You can build a list of patches ready for submission like this : |
| |
| $ git format-patch master |
| |
| The output files are the patches ready to be sent over e-mail, either via a |
| regular e-mail or via git send-email (carefully check the man page). Don't |
| destroy your other work branches until your patches get merged, it may happen |
| that earlier designs will be preferred for various reasons. Patches should be |
| sent to the mailing list : haproxy@formilux.org and CCed to relevant subsystem |
| maintainers or authors of the modified files if their address appears at the |
| top of the file. |
| |
| Please don't send pull requests, they are really inconvenient as they make it |
| much more complicate to perform minor adjustments, and nobody benefits from |
| any comment on the code while on a list all subscribers learn a little bit on |
| each review of anyone else's code. |
| |
| |
| What to do if your patch is ignored |
| ----------------------------------- |
| |
| All patches merged are acknowledged by the maintainer who picked it. If you |
| didn't get an acknowledgement, check the mailing list archives to see if your |
| mail was propely delivered there and possibly if anyone responded and you did |
| not get their response (please look at http://haproxy.org/ for the mailing list |
| archive's address). |
| |
| If you see that your mail is there but nobody responded, please recheck : |
| - was the subject clearly indicating that it was a patch and/or that you were |
| seeking some review ? |
| |
| - was your e-mail mangled by your mail agent ? If so it's possible that |
| nobody had the willingness yet to mention it. |
| |
| - was your e-mail sent as HTML ? If so it definitely ended in spam boxes |
| regardless of the archives |
| |
| - did the patch violate some of the principles explained in this document ? |
| |
| If none of these cases matches, it might simply be that everyone was busy when |
| your patch was sent and that it was overlooked. In this case it's fine to |
| either resubmit it or respond to your own e-mail asking if anything's wrong |
| about it. In general don't expect a response after one week of silence, just |
| because your e-mail will not appear in anyone else's current window. So after |
| one week it's time to resubmit. |
| |
| Among the mistakes that tend to make reviewers not respond are those who send |
| multiple versions of a patch in a row. It's natural for others then to wait for |
| the series to stabilize. And once it doesn't move anymore everyone forgot about |
| it. As a rule of thumb, if you have to update your original e-mail more than |
| twice, first double-check that your series is really ready for submission, and |
| second, start a new thread and stop responding to the previous one. In this |
| case it is well appreciated to mention a version of your patch set in the |
| subject such as "[PATCH v2]", so that reviewers can immediately spot the new |
| version and not waste their time on the old one. |
| |
| If you still do not receive any response, it is possible that you've already |
| played your last card by not respecting the basic principles multiple times |
| despite being told about it several times, and that nobody is willing to spend |
| more of their time than normally needed with your work anymore. Your best |
| option at this point probably is to ask "did I do something wrong" than to |
| resend the same patches. |
| |
| |
| How to be sure to irritate everyone |
| ----------------------------------- |
| |
| Among the best ways to quickly lose everyone's respect, there is this small |
| selection, which should help you improve the way you work with others, if |
| you notice you're already practising some of them : |
| - repeatedly send improperly formated commit messages, with no type or |
| severity, or with no commit message body. These ones require manual |
| edition, maintainers will quickly learn to recognize your name. |
| |
| - repeatedly send patches which break something, and disappear or take a long |
| time to provide a fix. |
| |
| - fail to respond to questions related to features you have contributed in |
| the past, which can further lead to the feature being declared unmaintained |
| and removed in a future version. |
| |
| - send a new patch iteration without taking *all* comments from previous |
| review into consideration, so that the reviewer discovers he/she has to do |
| the exact same work again. |
| |
| - "hijack" an existing thread to discuss something different or promote your |
| work. This will generally make you look like a fool so that everyone wants |
| to stay away from your e-mails. |
| |
| - continue to send pull requests after having been explained why they are not |
| welcome. |
| |
| - give wrong advices to people asking for help, or sending them patches to |
| try which make no sense, waste their time, and give them a bad impression |
| of the people working on the project. |
| |
| - be disrespectful to anyone asking for help or contributing some work. This |
| may actually even get you kicked out of the list and banned from it. |
| |
| -- end |