On Monday, March 13, 2017 at 7:45:44 AM UTC-7, Byron Jones wrote: > David Burns wrote: > > We should try mitigate the security problem and fix our nit problem > > instead of bashing that we can't handle re-reviews because of nits. > one way tooling could help here is to allow the reviewer to make minor > changes to the patch before it lands. > ie. "r+, fix typo in comment before landing" would become "r+, i fixed > the comment typo"
I don't think it's realistic to expect already-overloaded reviewers to do even more. In my experience, reviewer's time is worth ~4 times more than patch author's time. That's a completely arbitrary number, but represents how in my experience the load balance work. So, I'd actually say we should do everything possible to *minimize* the amount of time required from the reviewer, rather than increasing it. And I also don't think that increasing the number of reviewers would fix it. Reviewer by nature is often a senior engineer trying to balance out writing patches that very few people can, and review patches that less experienced engineers wrote. Their time is insanely valuable because neither of those tasks can be easily done by the person requesting the review. Of course there are exceptions like peer-reviews and rubber-stamping of a patch, but in general, I'd like us to think about shifting the burden onto automation / patch author to do as much work as possible before the reviewer commits their time. And once their done, once again we should imho limit the time we expect from the reviewer for any follow-up reviews. For that reason, the lack of interdiff in rebase scenario in MR is a major hassle in my experience. And the idea that the reviewer has to re-review multiple times or edit the patch themselves, as a step in the wrong direction. Also, the idea that "anyone can re-review the patch" is very shaky. It would not work in the most crucial and delicate areas where the number of people familiar with the area is just low. Say, accessibility, graphics, internationalization, security etc. In those lines, there's often a single person in the organization who can comfortably review the patch, and if they're in a different timezone, then asking a random reviewer on IRC for a review on nits is an illusion if the nits are anything beyond "update the comment". On top of that, the idea also taps into the concern I raised above. Cognitive load required for a reviewer to step into a bug, skim through all comments, patch history and latest review with request for nits to understand if the nits represent the original reviewer request is also non trivial. The way it's presented in this thread feels like a utopian vision where anyone can just take a quick glance and stamp an r+, but in reality it'll either add significantly to the load of already overloaded group in our project, or become an illusion of security with people just accepting everything from people they know. I'm actually concerned that in the era where most projects go in the direction of streamlining the development and reducing the bureaucracy as much as possible (post-landing reviews, peer-reviews etc.), we're talking about adding another hoop to jump through. I'm all for increased security (2FA etc.), but unless there's an unspoken set of cases where security of our project has been compromised by a change in the patch that was added after r+, I'd like to question if we're really at the point where we need such tradeoff. zb. _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
