Also sprach Mark Côté: > On Thu, Jul 5, 2018 at 11:37 AM, Andreas Tolfsen <a...@sny.no> wrote: > >> Also sprach Andrew Halberstadt: >> >>> It might be worth investigating whether we can switch Phabricator's >>> default (so that multiple reviews are all blocking, and to make >>> them non-blocking would require the extra step). >> >> I agree with Andrew. >> >> Making the default “everyone explicitly marked as reviewers must >> accept the change” would in my opinion be much less surprising. >> >> It is then easier to use the UI to add other non-blocking reviewers >> who might or might not be interested using the web UI. > > I sympathize with the concerns here; however, changing the default > would be a very invasive change to Phabricator, which would not > only be complex to implement but troublesome to maintain, as we > upgrade Phabricator every week or two. > > This is, however, something we can address with our new custom > commit-series-friendly command-line tool. We are also working towards > the superior solution of automatically selecting reviewers based > on module owners and peers and enforcing this in Lando.
If the program you use to upload patches would interpret "r?foo,bar" as making both foo and bar blocking reviewers when uploading your patches, that would be a very fine solution. Whenever I write this in a commit message I expect both reviewers to have to give their approval before landing the patch, but the reality with mozreview has been that the tooling would allow you to land something with only one r+. It is only when someone explicitly gives an r- that the patch is blocked. It is this behaviour that I earlier characterised as surprising. To underline why I think this is important, I’d like to say that there have been real cases in recent past where committers have landed module-transgressing patches where only one or a couple of the total set of reviewers have given their r+, effectively leaving parts of the patch unreviewed. In retrospect I regret that I didn’t r- the patch on the grounds that it ought to be split in multiple patches with clear review responsibilities. Echoing Andrew from before, it was probably disrespectful to the set of reviewers on this particular patch to have to wade through code from foreign modules that they were not supposed to review. But given that mozreview treats a single r+ as a go-ahead for landing, this default behaviour of mozreview is outright dangerous when we know that patches occasionally end up landing when not all reviewers have yet had time to look at the review. _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
