[llvm-dev] [RFC] High-Level Code-Review Documentation Update

[Nicolai Hähnle via llvm-dev]

On Wed, Nov 20, 2019 at 6:53 PM David Blaikie <dblaikie at gmail.com> wrote:
> On Wed, Nov 20, 2019 at 2:28 AM Nicolai Hähnle via llvm-dev <llvm-dev at lists.llvm.org> wrote:
>> On a somewhat related note: Due to timezones, I think good etiquette
>> is to wait a reasonable time before committing a patch if you receive
>> a LGTM very quickly. I think non-trivial patches should generally be
>> up for at least 24 hours -- maybe even extend that to 48 hours --
>> before committing.
>
>
> I'd disagree here - and perhaps my motivation should be enshrined in the documentation too: I believe a review approval is roughly equivalent to "I would be comfortable committing this code myself with post-commit review", so a single approval under those constraints should be able to be committed immediately & if other reviewers chime in later it can be handled as it would be with any other post-commit review feedback (which can include reverting).

Post-commit review should emphatically not be the default. Also, the
social barrier to just unilaterally reverting somebody else's commit
are high, and for a good reason.

Just because two people are comfortable committing code (the author
plus a single reviewer), that does *not* necessarily mean that the
code is good to go in as-is. Consider that both people may be from the
same team, for example. Having a cool-down period is a necessary
antidote against toxic clique behavior.

(This argument may not apply to specific subsystems. For example, if
you're maintaining a small backend that is genuinely only worked on by
a closely-knit team, then it is of course reasonable to work under
more relaxed rules.)

Cheers,
Nicolai

-- 
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.