| 1 | ---------------------------------------------------------------- |
| 2 | [the structure of a log message] |
| 3 | |
| 4 | The usual way to compose a log message of this project is to |
| 5 | |
| 6 | - Give an observation on how the current system works in the |
| 7 | present tense (so no need to say "Currently X is Y", or |
| 8 | "Previously X was Y" to describe the state before your change; |
| 9 | just "X is Y" is enough), and discuss what you perceive as a |
| 10 | problem in it. |
| 11 | |
| 12 | - Propose a solution (optional---often, problem description |
| 13 | trivially leads to an obvious solution in reader's minds). |
| 14 | |
| 15 | - Give commands to somebody editing the codebase to "make it so", |
| 16 | instead of saying "This commit does X". |
| 17 | |
| 18 | in this order. |
| 19 | |
| 20 | To those who have been intimately following the discussion, it often |
| 21 | is understandable without some of the above, but we are not writing |
| 22 | for those who review the patches. We are primarily writing for future |
| 23 | readers of "git log" who are not aware of the review discussion we |
| 24 | have on list, so we should give something to prepare them by setting |
| 25 | the stage and stating the objective first, before going into how the |
| 26 | patch solved it. |
| 27 | |
| 28 | |
| 29 | ------------------------------------------------------------------------ |
| 30 | [trim your quote] |
| 31 | |
| 32 | Please do not force your readers scroll through huge quoted material |
| 33 | to find your comments, when your comments do not address anything |
| 34 | there. Trim your quote and leave enough context to understand the |
| 35 | discussion. See Documentation/ReviewingGuidelines.adoc, which says |
| 36 | |
| 37 | - If a patch is long, you are encouraged to delete parts of it that are |
| 38 | unrelated to your review from the email reply. Make sure to leave enough |
| 39 | context for readers to understand your comments! |
| 40 | |
| 41 | ------------------------------------------------------------------------ |
| 42 | [polish your history before sending] |
| 43 | |
| 44 | We frown upon a patch series that makes mistakes in an earlier step, |
| 45 | only to fix them in a later step. The "git rebase -i" command helps |
| 46 | us pretend to be more perfect developers than we actually are, |
| 47 | whipping your patch series into a shape that builds one small step |
| 48 | on top of another in a logical succession. Such a patch series is |
| 49 | easier to understand than a history that faithfully records all the |
| 50 | stumbles the developer made until they reached the final solution. |
| 51 | |
| 52 | ------------------------------------------------------------------------ |
| 53 | [not just respond, update the patches] |
| 54 | |
| 55 | Not limited to this one, but when a reviewer says "this is not |
| 56 | clear", it is often not a request to only clarify something, which |
| 57 | is clear to any intelligent user of the end product, to a clueless |
| 58 | reviewer, whose intelligence is below the target audience, in an |
| 59 | e-mail response. It is pointing out that the end product, either the |
| 60 | patch text or the proposed log message, is not clear to target |
| 61 | audience and needs update. |
| 62 | |
| 63 | We would expect a review comment to be at least responded to either |
| 64 | rebut or admit the issues raised. It may be that a reviewer's point |
| 65 | were missing the mark and the patches themselves were perfectly |
| 66 | fine. |
| 67 | |
| 68 | But all other cases, even when the reviewer's comment were missing |
| 69 | the mark, such a confusion may have been the result of the patch |
| 70 | text or the proposed log message being unclear. Of course, the |
| 71 | review comments may have been pointing out an actionable issue. |
| 72 | They would hopefully lead to an improved version of the patches |
| 73 | posted sometime later, so that we can conclude a topic and move |
| 74 | ahead. |
| 75 | ---------------------------------------------------------------- |
| 76 | [not just reroll, but respond to reviews] |
| 77 | |
| 78 | After getting review comments but before sending a new iteration of |
| 79 | your patches, do respond to the review e-mails directly to develop a |
| 80 | dialog between you as the author and your reviewers. |
| 81 | |
| 82 | A new iteration of a patch is harder to read without such a dialog |
| 83 | telling reviewers what the author thought after getting suggestions in |
| 84 | earlier reviews. |
| 85 | |
| 86 | Did they agree and took the suggestion? Did they disagree but took |
| 87 | the suggestion anyway, and if so why did they think the suggested |
| 88 | changes are not good? Did they disagree and did not take the |
| 89 | suggestion and if so why? They changed the code but not in the way |
| 90 | suggested in the review, but why the new way was thought to be better |
| 91 | than both the original and the reviewer input? |
| 92 | |
| 93 | These are natural questions reviewers want to get answered, and some |
| 94 | of them can be answered by reading a new iteration of patch (e.g., did |
| 95 | they or did they not update the patch?) but not others (e.g., why did |
| 96 | they or did they not do so?). |
| 97 | ---------------------------------------------------------------- |
| 98 | [make us come to you, begging] |
| 99 | |
| 100 | I've seen from time to time people ask "I am thinking of doing this; |
| 101 | will a patch be accepted? If so, I'll work on it." before showing |
| 102 | any work, and my response always has been: |
| 103 | |
| 104 | (1) We don't know how useful and interesting your contribution would |
| 105 | be for our audience, until we see it; and |
| 106 | |
| 107 | (2) If you truly believe in your work (find it useful, find writing |
| 108 | it fun, etc.), that would be incentive enough for you to work |
| 109 | on it, whether or not the result will land in my tree. You |
| 110 | should instead aim for something so brilliant that we would |
| 111 | come to you begging for your permission to include it in our |
| 112 | project. |
| 113 | |
| 114 | ---------------------------------------------------------------- |
| 115 | [mailing list is the primary place] |
| 116 | |
| 117 | One thing to note is that I do not respond to a pull request in private, |
| 118 | and Github pull request, as I understand it, is very private in nature. |
| 119 | The patches are to be reviewed on the main mailing list first. |
| 120 | |
| 121 | It's OK to say "these patches are also available in my repository at |
| 122 | Github whose URL is this" in the commentary part of the final submission |
| 123 | message after the list reaches consensus that your change is a good thing, |
| 124 | and that may reduce the chance of mistakes when I accept the patches |
| 125 | especially if the series is large, so I am not saying that repositories |
| 126 | people have Github have no value to my workflow. But it will not come |
| 127 | into the picture before the final submission phase. |
| 128 | |
| 129 | ---------------------------------------------------------------- |
| 130 | [going incremental after hitting next] |
| 131 | |
| 132 | Once a commit hits 'next', it gets improved only by piling incremental |
| 133 | updates on top with explanation. The idea is: if all of us thought it |
| 134 | has seen enough eyeballs and is good enough for 'next', yet we later |
| 135 | find there was something we all missed, that is worth a separate |
| 136 | explanation, e.g., "The primary motivation behind the series is still |
| 137 | good, but for such and such reasons we missed this case we are |
| 138 | fixing." |
| 139 | |
| 140 | Unless it turns out that the approach was fundamentally wrong and such |
| 141 | an incremental update boils down to almost reverting the earlier one |
| 142 | entirely and replacing it with the newer one. In such a case, we do |
| 143 | revert the earlier and replace it with the newer, in 'next'. |
| 144 | |
| 145 | ---------------------------------------------------------------- |
| 146 | [do not iterate too quickly] |
| 147 | |
| 148 | After sending your patches out, it would be nice to give potential |
| 149 | reviewers at least 24 hours to ensure people anywhere on the globe |
| 150 | have a chance to comment, and a chance for you to respond to them, |
| 151 | before sending your next iteration. |
| 152 | |
| 153 | When responding to a review comment that causes you to drastically |
| 154 | change the course of the series, it is nice to give potential |
| 155 | reviewers at least 24 hours to ensure they have a chance to voice |
| 156 | their opinions, before sending an updated series based on that |
| 157 | comment, because suggested changes in such a comment may be |
| 158 | controversial and deserve discussion. After seeing you spend some |
| 159 | time already to adjust to one opinion on such a discussion, others may |
| 160 | feel discouraged to make you redo your series again even whey they |
| 161 | think the suggested changes are not taking us in the right direction. |