[llvm-dev] [RFC] High-Level Code-Review Documentation Update
Nicolai Hähnle via llvm-dev
llvm-dev at lists.llvm.org
Mon Nov 25 05:15:30 PST 2019
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.)
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
More information about the llvm-dev