|
| 1 | +Contributing to TittieCoin Core |
| 2 | +============================ |
| 3 | + |
| 4 | +The TittieCoin Core project operates an open contributor model where anyone is |
| 5 | +welcome to contribute towards development in the form of peer review, testing |
| 6 | +and patches. This document explains the practical process and guidelines for |
| 7 | +contributing. |
| 8 | + |
| 9 | +Firstly in terms of structure, there is no particular concept of "Core |
| 10 | +developers" in the sense of privileged people. Open source often naturally |
| 11 | +revolves around meritocracy where longer term contributors gain more trust from |
| 12 | +the developer community. However, some hierarchy is necessary for practical |
| 13 | +purposes. As such there are repository "maintainers" who are responsible for |
| 14 | +merging pull requests as well as a "lead maintainer" who is responsible for the |
| 15 | +release cycle, overall merging, moderation and appointment of maintainers. |
| 16 | + |
| 17 | + |
| 18 | +Contributor Workflow |
| 19 | +-------------------- |
| 20 | + |
| 21 | +The codebase is maintained using the "contributor workflow" where everyone |
| 22 | +without exception contributes patch proposals using "pull requests". This |
| 23 | +facilitates social contribution, easy testing and peer review. |
| 24 | + |
| 25 | +To contribute a patch, the workflow is as follows: |
| 26 | + |
| 27 | + - Fork repository |
| 28 | + - Create topic branch |
| 29 | + - Commit patches |
| 30 | + |
| 31 | +The project coding conventions in the [developer notes](doc/developer-notes.md) |
| 32 | +must be adhered to. |
| 33 | + |
| 34 | +In general [commits should be atomic](https://en.wikipedia.org/wiki/Atomic_commit#Atomic_commit_convention) |
| 35 | +and diffs should be easy to read. For this reason do not mix any formatting |
| 36 | +fixes or code moves with actual code changes. |
| 37 | + |
| 38 | +Commit messages should be verbose by default consisting of a short subject line |
| 39 | +(50 chars max), a blank line and detailed explanatory text as separate |
| 40 | +paragraph(s), unless the title alone is self-explanatory (like "Corrected typo |
| 41 | +in init.cpp") in which case a single title line is sufficient. Commit messages should be |
| 42 | +helpful to people reading your code in the future, so explain the reasoning for |
| 43 | +your decisions. Further explanation [here](http://chris.beams.io/posts/git-commit/). |
| 44 | + |
| 45 | +If a particular commit references another issue, please add the reference, for |
| 46 | +example `refs #1234`, or `fixes #4321`. Using the `fixes` or `closes` keywords |
| 47 | +will cause the corresponding issue to be closed when the pull request is merged. |
| 48 | + |
| 49 | +Please refer to the [Git manual](https://git-scm.com/doc) for more information |
| 50 | +about Git. |
| 51 | + |
| 52 | + - Push changes to your fork |
| 53 | + - Create pull request |
| 54 | + |
| 55 | +The title of the pull request should be prefixed by the component or area that |
| 56 | +the pull request affects. Valid areas as: |
| 57 | + |
| 58 | + - *Consensus* for changes to consensus critical code |
| 59 | + - *Docs* for changes to the documentation |
| 60 | + - *Qt* for changes to tittiecoin-qt |
| 61 | + - *Minting* for changes to the minting code |
| 62 | + - *Net* or *P2P* for changes to the peer-to-peer network code |
| 63 | + - *RPC/REST* for changes to the RPC or REST APIs |
| 64 | + - *Scripts and tools* for changes to the scripts and tools |
| 65 | + - *Tests* for changes to the tittiecoin unit tests or QA tests |
| 66 | + - *Trivial* should **only** be used for PRs that do not change generated |
| 67 | + executable code. Notably, refactors (change of function arguments and code |
| 68 | + reorganization) and changes in behavior should **not** be marked as trivial. |
| 69 | + Examples of trivial PRs are changes to: |
| 70 | + - comments |
| 71 | + - whitespace |
| 72 | + - variable names |
| 73 | + - logging and messages |
| 74 | + - *Utils and libraries* for changes to the utils and libraries |
| 75 | + - *Wallet* for changes to the wallet code |
| 76 | + |
| 77 | +Examples: |
| 78 | + |
| 79 | + Consensus: Add new opcode for BIP-XXXX OP_CHECKAWESOMESIG |
| 80 | + Net: Automatically create hidden service, listen on Tor |
| 81 | + Qt: Add feed bump button |
| 82 | + Trivial: Fix typo in init.cpp |
| 83 | + |
| 84 | +If a pull request is specifically not to be considered for merging (yet) please |
| 85 | +prefix the title with [WIP] or use [Tasks Lists](https://help.github.com/articles/basic-writing-and-formatting-syntax/#task-lists) |
| 86 | +in the body of the pull request to indicate tasks are pending. |
| 87 | + |
| 88 | +The body of the pull request should contain enough description about what the |
| 89 | +patch does together with any justification/reasoning. You should include |
| 90 | +references to any discussions (for example other tickets or mailing list |
| 91 | +discussions). |
| 92 | + |
| 93 | +At this stage one should expect comments and review from other contributors. You |
| 94 | +can add more commits to your pull request by committing them locally and pushing |
| 95 | +to your fork until you have satisfied all feedback. |
| 96 | + |
| 97 | +Squashing Commits |
| 98 | +--------------------------- |
| 99 | +If your pull request is accepted for merging, you may be asked by a maintainer |
| 100 | +to squash and or [rebase](https://git-scm.com/docs/git-rebase) your commits |
| 101 | +before it will be merged. The basic squashing workflow is shown below. |
| 102 | + |
| 103 | + git checkout your_branch_name |
| 104 | + git rebase -i HEAD~n |
| 105 | + # n is normally the number of commits in the pull |
| 106 | + # set commits from 'pick' to 'squash', save and quit |
| 107 | + # on the next screen, edit/refine commit messages |
| 108 | + # save and quit |
| 109 | + git push -f # (force push to GitHub) |
| 110 | + |
| 111 | +If you have problems with squashing (or other workflows with `git`), you can |
| 112 | +alternatively enable "Allow edits from maintainers" in the right GitHub |
| 113 | +sidebar and ask for help in the pull request. |
| 114 | + |
| 115 | +Please refrain from creating several pull requests for the same change. |
| 116 | +Use the pull request that is already open (or was created earlier) to amend |
| 117 | +changes. This preserves the discussion and review that happened earlier for |
| 118 | +the respective change set. |
| 119 | + |
| 120 | +The length of time required for peer review is unpredictable and will vary from |
| 121 | +pull request to pull request. |
| 122 | + |
| 123 | + |
| 124 | +Pull Request Philosophy |
| 125 | +----------------------- |
| 126 | + |
| 127 | +Patch sets should always be focused. For example, a pull request could add a |
| 128 | +feature, fix a bug, or refactor code; but not a mixture. Please also avoid super |
| 129 | +pull requests which attempt to do too much, are overly large, or overly complex |
| 130 | +as this makes review difficult. |
| 131 | + |
| 132 | + |
| 133 | +### Features |
| 134 | + |
| 135 | +When adding a new feature, thought must be given to the long term technical debt |
| 136 | +and maintenance that feature may require after inclusion. Before proposing a new |
| 137 | +feature that will require maintenance, please consider if you are willing to |
| 138 | +maintain it (including bug fixing). If features get orphaned with no maintainer |
| 139 | +in the future, they may be removed by the Repository Maintainer. |
| 140 | + |
| 141 | + |
| 142 | +### Refactoring |
| 143 | + |
| 144 | +Refactoring is a necessary part of any software project's evolution. The |
| 145 | +following guidelines cover refactoring pull requests for the project. |
| 146 | + |
| 147 | +There are three categories of refactoring, code only moves, code style fixes, |
| 148 | +code refactoring. In general refactoring pull requests should not mix these |
| 149 | +three kinds of activity in order to make refactoring pull requests easy to |
| 150 | +review and uncontroversial. In all cases, refactoring PRs must not change the |
| 151 | +behavior of code within the pull request (bugs must be preserved as is). |
| 152 | + |
| 153 | +Project maintainers aim for a quick turnaround on refactoring pull requests, so |
| 154 | +where possible keep them short, un-complex and easy to verify. |
| 155 | + |
| 156 | + |
| 157 | +"Decision Making" Process |
| 158 | +------------------------- |
| 159 | + |
| 160 | +The following applies to code changes to the TittieCoin Core project, and is not to be |
| 161 | +confused with overall TittieCoin Network Protocol consensus changes. |
| 162 | + |
| 163 | +Whether a pull request is merged into TittieCoin Core rests with the project merge |
| 164 | +maintainers and ultimately the project lead. |
| 165 | + |
| 166 | +Maintainers will take into consideration if a patch is in line with the general |
| 167 | +principles of the project; meets the minimum standards for inclusion; and will |
| 168 | +judge the general consensus of contributors. |
| 169 | + |
| 170 | +In general, all pull requests must: |
| 171 | + |
| 172 | + - have a clear use case, fix a demonstrable bug or serve the greater good of |
| 173 | + the project (for example refactoring for modularisation); |
| 174 | + - be well peer reviewed; |
| 175 | + - follow code style guidelines; |
| 176 | + |
| 177 | +Patches that change TittieCoin consensus rules are considerably more involved than |
| 178 | +normal because they affect the entire ecosystem and so must be preceded by |
| 179 | +extensive discussions and clear detailing. While each case will be different, |
| 180 | +one should be prepared to expend more time and effort than for other kinds of |
| 181 | +patches because of increased peer review and consensus building requirements. |
| 182 | + |
| 183 | + |
| 184 | +### Peer Review |
| 185 | + |
| 186 | +Anyone may participate in peer review which is expressed by comments in the pull |
| 187 | +request. Typically reviewers will review the code for obvious errors, as well as |
| 188 | +test out the patch set and opine on the technical merits of the patch. Project |
| 189 | +maintainers take into account the peer review when determining if there is |
| 190 | +consensus to merge a pull request (remember that discussions may have been |
| 191 | +spread out over GitHub, forums, email, and Slack discussions). The following |
| 192 | +language is used within pull-request comments: |
| 193 | + |
| 194 | + - ACK means "I have tested the code and I agree it should be merged"; |
| 195 | + - NACK means "I disagree this should be merged", and must be accompanied by |
| 196 | + sound technical justification (or in certain cases of copyright/patent/licensing |
| 197 | + issues, legal justification). NACKs without accompanying reasoning may be |
| 198 | + disregarded; |
| 199 | + - utACK means "I have not tested the code, but I have reviewed it and it looks |
| 200 | + OK, I agree it can be merged"; |
| 201 | + - Concept ACK means "I agree in the general principle of this pull request"; |
| 202 | + - Nit refers to trivial, often non-blocking issues. |
| 203 | + |
| 204 | +Reviewers should include the commit hash which they reviewed in their comments. |
| 205 | + |
| 206 | +Project maintainers reserve the right to weigh the opinions of peer reviewers |
| 207 | +using common sense judgement and also may weight based on meritocracy: Those |
| 208 | +that have demonstrated a deeper commitment and understanding towards the project |
| 209 | +(over time) or have clear domain expertise may naturally have more weight, as |
| 210 | +one would expect in all walks of life. |
| 211 | + |
| 212 | +Where a patch set affects consensus critical code, the bar will be set much |
| 213 | +higher in terms of discussion and peer review requirements, keeping in mind that |
| 214 | +mistakes could be very costly to the wider community. This includes refactoring |
| 215 | +of consensus critical code. |
| 216 | + |
| 217 | +Where a patch set proposes to change the TittieCoin consensus, it must have been |
| 218 | +discussed extensively on the forums and Slack, be accompanied by a widely |
| 219 | +discussed Proposal and have a generally widely perceived technical consensus of being |
| 220 | +a worthwhile change based on the judgement of the maintainers. |
| 221 | + |
| 222 | +### Finding Reviewers |
| 223 | + |
| 224 | +As most reviewers are themselves developers with their own projects, the review |
| 225 | +process can be quite lengthy, and some amount of patience is required. If you find |
| 226 | +that you've been waiting for a pull request to be given attention for several |
| 227 | +months, there may be a number of reasons for this, some of which you can do something |
| 228 | +about: |
| 229 | + |
| 230 | + - It may be because of a feature freeze due to an upcoming release. During this time, |
| 231 | + only bug fixes are taken into consideration. If your pull request is a new feature, |
| 232 | + it will not be prioritized until the release is over. Wait for release. |
| 233 | + - It may be because the changes you are suggesting do not appeal to people. Rather than |
| 234 | + nits and critique, which require effort and means they care enough to spend time on your |
| 235 | + contribution, thundering silence is a good sign of widespread (mild) dislike of a given change |
| 236 | + (because people don't assume *others* won't actually like the proposal). Don't take |
| 237 | + that personally, though! Instead, take another critical look at what you are suggesting |
| 238 | + and see if it: changes too much, is too broad, doesn't adhere to the |
| 239 | + [developer notes](doc/developer-notes.md), is dangerous or insecure, is messily written, etc. |
| 240 | + Identify and address any of the issues you find. Then ask e.g. on Slack if someone could give |
| 241 | + their opinion on the concept itself. |
| 242 | + - It may be because your code is too complex for all but a few people. And those people |
| 243 | + may not have realized your pull request even exists. A great way to find people who |
| 244 | + are qualified and care about the code you are touching is the |
| 245 | + [Git Blame feature](https://help.github.com/articles/tracing-changes-in-a-file/). Simply |
| 246 | + find the person touching the code you are touching before you and see if you can find |
| 247 | + them and give them a nudge. Don't be incessant about the nudging though. |
| 248 | + - Finally, if all else fails, ask on Slack or elsewhere for someone to give your pull request |
| 249 | + a look. If you think you've been waiting an unreasonably long amount of time (month+) for |
| 250 | + no particular reason (few lines changed, etc), this is totally fine. Try to return the favor |
| 251 | + when someone else is asking for feedback on their code, and universe balances out. |
| 252 | + |
| 253 | + |
| 254 | +Release Policy |
| 255 | +-------------- |
| 256 | + |
| 257 | +The project leader is the release manager for each TittieCoin Core release. |
| 258 | + |
| 259 | +Copyright |
| 260 | +--------- |
| 261 | + |
| 262 | +By contributing to this repository, you agree to license your work under the |
| 263 | +MIT license unless specified otherwise in `contrib/debian/copyright` or at |
| 264 | +the top of the file itself. Any work contributed where you are not the original |
| 265 | +author must contain its license header with the original author(s) and source. |
0 commit comments