Wait for PRs to be at least 24 hours old before merging

[CyberShadow]

Inspired by dlang/phobos#5529 (comment) and previous discussions.

To allow everyone from the D community to review PRs they are interested in, it's good to allow them to be open for at least 24 hours. This gives everyone from around the globe and any timezone a chance to have a look at it before it's merged.

PRs labeled as "trivial" could be exempted from this rule; if we find it necessary, a new "urgent" label could be added as well. However, manual merging once required CIs are green is still an option to all users with push access.

[PetarKirov]

So, in summary:

• We add handling for two additional labels: urgent and trivial
• PRs tagged with auto-merge, but not with one of the labels above are merged 24 hours after they are tagged (plus the time needed for the tests to pass)
• auto-merge + trivial = current meaning of auto-merge
• auto-merge + urgent = merge now, whatever the CI status is? (may be useful when we need to do an urgent revert or the CIs are broken at the moment)

(BTW, @CyberShadow you have a pending invitation to the dlang-bots team).

[CyberShadow]

auto-merge + urgent = merge now, whatever the CI status is? (may be useful when we need to do an urgent revert or the CIs are broken at the moment)

Effectively synonymous with "Trivial" but not the same semantic meaning for reviewers.

I say that we shouldn't implement this or add the label until it becomes clear that it's necessary, as anyone with push access can merge commits manually in urgent situations.

Also, I don't think we should make it easy to merge PRs bypassing or in spite of broken CI. Restricting it to users with administrator access has been working OK for us (occasionally there even were situations where administrators abused their privileges to merge broken PRs which in turn broke master).

(BTW, @CyberShadow you have a pending invitation to the dlang-bots team).

Thanks for the reminder, I've accepted it now.

[wilzbach]

A couple of minor comments (currently on the go):

• issues labelled with auto-merge have priority on the auto-tester, so they will be constantly rebuilt until merged
• we should add add something like "this PR has been flagged for auto-merge and will be automatically merged after the 24h review time frame has passed" to the comment (or make a new one), so that it's not confusing for users

[andralex]

Nice. Playing devil's advocate for a bit - I fear that adding increasingly complex rules and regulations put more strain on all involved without solving the real underlying problem: we should improve the quality of reviews and number of solid reviewers.

At Facebook quick good reviews followed by pulls do occur frequently. Complex code naturally takes longer. Putting a number on that just doesn't sound like the right way to go about it.

Code reviews should not ask for overengineering the trivial. They should not hold code hostage on a matter of preference of the reviewer. The reviewers should exercise good judgment instead of rigidizing guidelines and precedents into ironclad rules. @WalterBright and myself found ourselves afraid to recommend things to do in code reviews lest things we said be taken out of context and used without proportion. (So please take it all with a grain of salt!) PRs that degrade quality of code are worse than PRs that introduce bugs because the latter are dustmite-able and easy to undo; the kind of slow degradation in quality that does not introduce bugs is much worse.

I don't see many of the above getting improved by a 24-hour latency.

[CyberShadow]

@andralex At Facebook how often did you have submitters and reviewers in drastically different time zones?

[CyberShadow]

Code reviews should not ask for overengineering the trivial. They should not hold code hostage on a matter of preference of the reviewer. The reviewers should exercise good judgment instead of rigidizing guidelines and precedents into ironclad rules. @WalterBright and myself found ourselves afraid to recommend things to do in code reviews lest things we said be taken out of context and used without proportion. (So please take it all with a grain of salt!) PRs that degrade quality of code are worse than PRs that introduce bugs because the latter are dustmite-able and easy to undo; the kind of slow degradation in quality that does not introduce bugs is much worse.

I don't understand what any of that has to do with this issue.

[andralex]

@andralex At Facebook how often did you have submitters and reviewers in drastically different time zones?

Probably not often.

[wilzbach]

PRs that degrade quality of code are worse than PRs that introduce bugs because the latter are dustmite-able and easy to undo;

FWIW at least from my feeling, this isn't really true. I don't think we revert many of the regression bugs that @CyberShadow finds.
@CyberShadow do you keep track of this or have a really easy way to summarize stats?

[CyberShadow]

Mmm, don't think so, sorry. Missing a way to find a full list of PRs which included revert commits.

[MartinNowak]

I'm also on the side of leaving this to common sense, hardly had an issues with this except for the few occasions where Andrei merged huge disputable changes of his best friend Walter ;).
Overall anything that's reasonable complex intrinsically requires enough time to be reviewed anyhow, probably including a second look from the reviewer on another day.

[CyberShadow]

Overall anything that's reasonable complex intrinsically requires enough time to be reviewed anyhow,

Yep, but this idea is not about those cases.

We have:

• really trivial PRs that fix typos or improve documentation
• complex PRs that require approval, or multiple review passes, or reviews from multiple people
• everything else in between, which is the vast majority.

Right now for the last category anyone can do the review, but then they have to decide whether to merge it or leave it open in case someone else may want to look at it as well. This idea will remove the need to make that decision - anyone will be able to boldly add the "auto-merge" tag on PRs that look good overall, even if it's something that other people might have additional feedback on.