{"thread":{"id":"45046","subject":"How I review patches","startedAt":"2017-02-04T07:27:14Z","lastAt":"2017-02-04T07:27:14Z","messageCount":1,"participants":["Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"310841","messageId":"xmqq37fu5sc4.fsf@gitster.mtv.corp.google.com","threadId":"45046","inReplyTo":null,"subject":"How I review patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-04T07:27:07Z","receivedAt":"2017-02-04T07:27:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here is a write-up on how I review patches posted on the list, in\nthe hope that knowing what to expect may help contributors [*0*].\n\nBut before going into exactly how I do my reviews, here is a short\nlist of the goals of doing reviews.  I review a patch (or a set of\npatches) to ensure that including it to our codebase will NOT:\n\n * hurt users of existing versions of Git. [*1*]\n\n * hurt users of the version of Git the change appears in. [*2*]\n\n * make life harder for developers [*3*] of Git in the future. [*4*]\n\nThe remaining paragraphs of this message are not to be taken as me\ntelling other reviewers how they must conduct their reviews.  I will\nqueue, and I have queued, patches reviewed by other reviewers\nwithout doing detailed reviews myself, as long as I trust that the\nreviewers share the same goals in their reviews, and as long as I\ntrust their competence and taste.  How they exactly review may be\ndifferent from how I would have reviewed the patches, and that is\nperfectly fine.\n\n1. Description of the problem being solved.\n\nWhen I see a patch (or a set of patches), I first read the proposed\nlog message, documentation update and new in-code comments.\n\nThese are places where the contributor can (and is expected to)\nexplain the motivation behind the change.  I read them to make sure\nthat they clearly state what problem is being solved, why a\nparticular solution was chosen, what exactly that solution is, and\nwhat other solutions were considered but discarded.\n\nJust like \"X is broken\" in a bug report is not clear enough, \"Fix X\"\nis often not enough in the proposed log message, as it is not clear\nwhich part of what X does is wrong in the contributor's mind, and\nwhy the contributor thinks it is broken.  Saying \"X currently does Y\nbut it should do Z instead to help such and such use case.\" would\nhelp reviewers (and future developers who will read it) understand\nthe motivation behind the change.\n\nA new feature or enhancement that is worth adding by default needs\nto be explained how that new thing works and why it is there to the\nend users, so a lack of documentation update is noticed at this\nstage as well.\n\nWhen I find that the explanation is lacking after reading the cover\nletter, proposed log message, and documentation updates, I often ask\nthe contributor to elaborate, before going into the actual diff.  I\noften suggest \"perhaps you meant this?\", and I end up reading the\nactual diff to base my best guess on in order to do so.  This is\nalso where I notice a tricky code whose \"why\" is under-explained in\nin-code comments [*5*].\n\nTo such review comments, I do not want the contributor to just say\n\"yes, you now understand what I wanted to do with this change\".  I\nwant to see the log message, in-code comments and documentation\nupdated so that other reviewers and future developers will not have\nto ask the same question as I asked again.\n\n2. Design of the solution.\n\nAfter clarifying the original motivation of the contributor, it\nsometimes becomes apparent that the patch aims too low and attempts\nto solve too narrow an issue.  I would point out that, within the\ncontext of the patches, they can and should solve a wider range of\nproblems of the same class [*6*].  Or the patch may hurt users in\nuse cases that the contributor did not consider, and the solution\nmay need to be designed to cover these cases [*7*].  This design\nreview may cause us to iterate until we have a good description of\nthe problem and design of the solution.\n\n\n3. \"Code\" review.\n\nOnce we made sure that the motivation is made clear, the scope of\nthe change is refined, and the design of the solution described\nclearly, I dive into the code changes.  What I look for primarily\nduring this phase is to see what the code does matches the desired\nbehaviour we established before this phase for correctness.  This is\nwhat many people think of as \"code review\", and it ranges from\nspotting style issues and typoes, finding and fixing stupid\noff-by-one errors, to noticing future maintainance issues caused by\nusing a wrong abstration or a misdesigned API.\n\nDuring the review of the actual code change, I may discover that\nsome common corner cases are not handled properly, which I would\npoint out.  Or the contributor may have thought about tricky corner\ncases and handled them correctly in the patches, but did not explain\nthe \"what\" and \"why\" in the log message.  Recording what cases were\nconsidered and decided based on what reasoning in the log message is\nimportant to help future developers and sets the course of evolution\nof the codebase, and this may result in updates in the \"explanation\nof the changes\" reviewed early in the cycle.\n\nIt is not like I never look at the code until log message and\nexplanation is perfect; to reduce the number of back-and-forth, I do\ncomment on the code even before it becomes clear if the design is\nsound and clearly described.  But at the conceptual level, because\nthe motivation guides the design and the design guides the\nimplementation, I tend to review the patches in this order.\n\nHope the above helps current and future contributors when they are\npreparing and reviewing their patch series before submitting.\n\n\n[Footnotes]\n\n*0* If disclosing this to contributors turns out to be a good idea,\n    we may want to add a polished version of this to somewhere in\n    Documentation/ next to SubmittingPatches.\n\n*1* For example, we need to be very careful when changing the\n    on-disk or on-wire data.  We as developers may always run the\n    bleeding-edge version of Git, but how well do users with older\n    version of Git interact with our new shiny toys?  Backward\n    compatibility issues can hurt users of existing versions Git\n    that do not have the change the patch introduces.\n\n*2* That's called a \"bug\" in general, but can take different forms\n    (i.e. implementation bug, documentation bug, design bug, etc.).\n    Maybe the documentation promises to (or it can be misread to\n    promise to) do A when the actual code does B instead.  Maybe the\n    code does A most of the time but silently does something else in\n    a corner case, without documenting it.  Maybe the problem the\n    patches wanted to solve have two different ways to solve,\n    and the patches chose one way to solve and implemented it\n    correctly as it documented the feature, but the approach taken\n    may be a way that forces users to use Git in an awkward way or\n    encourages them to employ a bad workflow, when there is another\n    way that helps users better.\n\n*3* Future users of Git cannot be hurt more than the patches\n    themselves hurts them immediately.  Patches may make a design\n    mistake that makes it hard or impossible to extend a part of the\n    system further, and users can be robbed their opportunity to use\n    even better Git in the future---but the \"even better Git\" does\n    not exist yet when the patches are accepted anyway, and their\n    user experience at least does not get worse, at least.  The\n    developers will find a way to work it around and transition\n    existing users to allow such an extension into existence and\n    their work may be made harder by such a design mistake, though.\n\n*4* A set of patches may add a helper function that is useful for any\n    NUL-terminated string, but may make the helper take a pointer to\n    a strbuf because the callers to it in the patches happen to all\n    have the string in a strbuf.  That forces future developers who\n    want to call the helper to either wrap their string in a strbuf\n    or to fix the misdesigned API to the helper function to take a\n    simple \"char *\".\n\n    A set of patches may introduce a new helper function to split\n    fields in a string whose format is well known throughout the\n    system, for which there already exists a helper function to do\n    the splitting.  This doubles the amount of work required when\n    the format of the string needs to be extended in the future.\n\n    A set of patches may conflate two semantically distinct sets of\n    things and try to parse elements of both set with a single\n    helper function, only because the elements in these two sets\n    happen to be the same in the version immediately after applying\n    the patches. This forces future developers who need to add\n    more elements to one set without affecting others to split the\n    helper into two.\n\n    All of the above make it harder for developers that need to\n    enhance the system in the future, but they will NOT hurt users\n    of existing version or the version immediately after the patches\n    land.  Such a patch series may happen to work correctly\n    at that version, and no amount of tests or field trials will\n    reveal these maintainance issues.\n\n*5* In-code comments that do not explain why the code does things in\n    a particular way but just translates what it does from C to\n    English add to maintenance burden of having to keep it in sync\n    with the code, without adding much value to the readers of the\n    code.  They instead should explain why the code does what it\n    does.\n\n*6* For example, a contributor may originally wants to solve\n    something only for tags, but it is not uncommon that the issue\n    that exists for tags are shared by other kinds of refs, and it\n    may be better to solve it uniformly across tags, branches, etc.\n\n*7* \"cover\"ing other uses cases does not necessarily have to be done\n    perfectly.  In less likely situations, it may be OK to punt and\n    say \"my code cannot handle it\" and die() with a message, and\n    that is much much better than not considering these situations\n    and doing wrong things.\n    \n"}]}