humane practices for review requests

I dislike seeing “review not granted” or “review cancelled” in my bugmail.

Even if the reviewer provides helpful comments and points out things that you missed, those few words are the first thing you see about your patch.  Part of me understands that these headlines are informative messages, that they are not judgements on me as a person, merely some sort of judgement on the patch that I have written.  But the language doesn’t encourage that perspective.  Permissions are not granted, entrance is not granted, your favorite show on television is cancelled, and so forth.  These are not heartening words: they are words that shut you out, that indicate your contribution was not desirable.

I took a mathematics class in college where individual problems on homework sets were scored out of a maximum of three points.  Half points were virtually unheard of.  In the usual grade/percentage terms, the scores for a problem looked roughly like this: A, D, F, F.  Not encouraging.  One day, the professor announced that there had been some grumbling from students about this scale.  He then described a colleague’s grading scale for problems in a class of similar difficulty: check-plus, check, check-minus, and zero.  “Now it is trivial to show that these two scales are isomorphic to one another,” he said, smiling.  “But I don’t hear any [grumbling] from Dr. L’s students about his grading scale!”  The message was clear: the scale isn’t that bad, just get over it.[1][2]

However, it matters how you say things, not just what you say.  Some organizations use a plus/delta feedback framework instead of a plus/minus framework.  That is, instead of dividing things into good and bad, you divide things into good and “changes needed”.  Phrasing things as “deltas” nudges people to provide actionable items; it’s the difference between saying “it sucked that the meeting ran half an hour over” versus “the meeting should stay within the time allotted”.

With all this in mind, if I get r?‘d on a patch, I will provide one of three responses (within 24 hours, natch):

  • r+ the patch.  Minor changes may be requested; whether I ask to see the patch again depends on how confident I am in the author and/or the triviality of the changes.  For instance, syntax changes and so forth don’t usually require a re-review.  Documentation changes often can.
  • f+ the patch.  As I understand it, this shows up in bugmail as “feedback granted”, not “review cancelled” (props to the Bugzilla team for doing this!).  Doing this lets the patch author know that they are making progress and that their contribution is appreciated (you took the time to provide feedback), but there are still some changes to be made.
    If at all possible, once the author makes the changes you have requested, r+ the patch.  Your feedback can be thought of as a contract with the patch author: you do these things and the patch is good enough.  Don’t change the terms of the contract.  File a followup bug, if need be.
  • Cancel the review.  I don’t like doing this due to reasons given above, but sometimes you have to, e.g. the wrong patch is provided.  The following scenario is also, I think, reasonable grounds for cancellation, provided you make it clear:
    • A patch depends on prior patches in the same bug;
    • You have reviewed those prior patches and requested significant changes;
    • Those changes would require equally significant changes to the patch in question, thereby rendering any review that you would do moot.

[1] This class was Real Analysis.  Real Analysis is often graded on a very…generous curve, owing to the material and that the course is typically students’ first introduction to rigorous mathematical proof.  So your mental model of what a “good” grade is requires some adjustment anyway.  But still.

[2] I was taking Dr. L’s course simultaneously with Reals.  I can attest that his grading scale did not bother me; indeed, I don’t think I recognized the isomorphism until it was pointed out.


  1. Wes Johnston

    We were talking about this at the last work week. I’ve had people switch an r? to an f+ before. I always find it condescending. As if they’re worried that my frail ego can’t take an r-. r- is good for people who want to track things as well. It says this bug WAS reviewed and I need to change it. For people with a lot of work going on in a lot of patches, its very helpful.

    But there’s some element of knowing who your audience is when you’re reviewing. If its the guy who sits next to you every day and constantly doles out snarky comments, you’re probably fine with r-. If its someone’s first bug, a gentle hand is a good thing.

  2. I don’t like turning r? into f+. Feedback is different, and independently useful, so I’d rather not munge them together. (I’ve also never seen that done, and would probably get very confused if it happened.)

    I’d rather just change the bugmail subject lines. Make cancelling a review request say “review request removed”.

    I see the valid responses as:

    r-: this approach isn’t going to work and/or I don’t want this change

    cancelled with comments: I don’t want the patch to land in its current form but I have reviewed it so am removing it from my review queue; please make the changes and try again

    cancelled without comments: I’m having trouble using bugzilla, or this request is ancient please stop pretending it’ll ever happen

    r? somebody else: I’m the wrong reviewer, try this dev instead

    r+ with comments: Make the changes requested and land it without bothering me again

    r+ with no comment: this is trivial, or I didn’t really read it please leave me alone just land the damn thing