2

Should the author of a changeset be required to obtain sign off from all reviewers who have reviewed the code or should code be allowed to check in as long as there is one sign off? I think giving veto power to reviewers does more harm than good as then they use it to blackmail (asking for unnecessary changes) and block checkins but would be curious to hear people's opinion on this matter as to what is the best practice to follow.

EDIT: Being more specific and to the point, Should a checkin be blocked if:

  1. there are some people (one or more) who have signed off
  2. but there are other(s) who have marked the diff as needing revision
  3. and the author has made a good faith effort addressing comments of reviewers who have marked the diff as needing revision but cannot come to agreement with them?

what do people think?

  • 3
    This is primarily an opinion poll. It may not be on topic here. – Adam Zuckerman Mar 29 '16 at 23:35
  • 5
    I expect this to be closed as an opinion poll. If you are at the point, though, where you are worried about reviewers "blackmailing" other developers rather than being useful collaborators (i.e. "I would prefer you do X but I see that the other two reviewers disagree with me and have a reasonable argument so I'll live with it."), you've got deeper problems. – Justin Cave Mar 29 '16 at 23:40
  • 2
    I think this should be left open. It's not asking if each of us has such a veto power in our teams, but rather whether it's a good idea, and although that depends on the environment to some degree, there is a useful "correct answer" to this which is basically what Telastyn already wrote. – Ixrec Mar 29 '16 at 23:45
  • 1
    Your question reminds me of the warning on JSLint, if I may rephrase. Warning: code reviews may hurt your feelings. – Karl Bielefeldt Mar 30 '16 at 5:33
  • 2
6

It depends on your environment. In a few environments, I've seen reviewers have veto power, and that was important since the skill level of the developers (and the impact if a bug made it through) necessitated it. For most environments, I don't believe that code reviews are necessary at all for 80%+ of check-ins. For some environments, I've seen team leads have veto power since in those environments, they were held unusually accountable for issues on their team.

But mostly, we're all professionals. If someone sees something in the code review that they think is a problem, then the team should be able to come to some consensus about what should be done. If the team can't come to some consensus, then I wouldn't consider the problem so severe as to block the check-in. Stop bikeshedding and focus on what matters.

  • 1
    I think when there are issues, the issues are generally "reviewer confusing perfection with acceptability" not the review process itself. I think a lot of the negative behavior comes from people failing to see the code as a team creation and see it as a personal creation. – Gort the Robot Mar 29 '16 at 23:58
  • @StevenBurnap - sure, I've seen a lot of that, but I've also seen a lot of disagreements simply because reasonable people put different levels of priority on different trade offs. As much as I hate process, it can help here derail some of those disagreements. – Telastyn Mar 30 '16 at 0:15
6

It doesn't really matter. If a colleague were "blackmailing" you out of the proper scope of their reviewing job, you'd take it up with your manager or team lead and get it sorted out that way. No process is going to prevent someone with that sort of attitude to life from trying to get one over on you.

Not the answer you're looking for? Browse other questions tagged or ask your own question.