5 ms·

I think post-commit reviews are a bad practice. The point of code review is to prevent issues in the first place. If you have a privacy issue and discover that

by groby_b 11y ago

I think post-commit reviews are a bad practice. The point of code review is to prevent issues in the first place. If you have a privacy issue and discover that two days after you pushed the code to prod, that's not much fun. And if each part of the review takes 2 days in the first place, there might be an issue with your code reviews (or your checkins are gargantuan).

relaunched 11y ago
They specifically address areas where the default to pre-commit review, for the reason you just called out: We switch to pre-commit reviews instead of the usual post-commit review flow if the potential errors/mistakes are costly and can cause significant problems. Some examples of such cases are: Touching code which deals with privacy and anonymity of users. Touching a core abstraction since a lot of other code might depend on it. Touching some parts of the infrastructure where mistakes can cause a downtime.
- mdlowman 11y ago
  You're right, but where I work almost anything can cause a bug that affects users. Pre-review eliminates some of those bugs. I would not move to this workflow, although it's interesting to hear from somewhere that it works.
- chipgap98 11y ago
  While the examples given do highlight areas where a bug could do critical damage to a company, there are plenty of ways to break things that cause problems for users. If you put code through at least a lightweight review process you can filter out many of these bugs. While post commit review might work for smaller changes I would rather err on the side of caution and use precommit review. We deploy about the same number of times/day/engineer at my company, so I don't think that the review process is what drives that metric.
- groby_b 11y ago
  That makes the assumption you can correctly all identify all those places. And maybe I'm too chicken, and you actually can - but I'd personally still prefer pre-commit. I'm a belt-and-suspenders gal :) But I'm really curious - if you can talk about it, that is - why devs push on average almost 4 changes per day & dev, but a review takes 2 days roundtrip.
nashadelic 11y ago
There really can't be a one-size-fits-all. New devs must earn the right to commit without review (post-commit-review), devs with proven quality records get post-commit-review, sensitive code areas get pre-commit-reviews.
- rue 11y ago
  Thinking that ‘proven devs’ don’t make mistakes, or more importantly wouldn’t benefit from ideas in review is a danger in itself.
  - nashadelic 11y ago
    I didn't say they bypass reviews, I said they get post-reviews, they can push without being blocked by a review.
  - nostrademons 11y ago
    At Google the worst offenders for broken builds & minor bugs were often the most experienced & productive devs. They developed reputations around their particular teams, which meant their teammates would think "Oh, X wrote this, he usually knows what he's doing, so I don't have to be quite as careful with the review." As a result, a bunch of silly mistakes would make it through. I had one (very productive) coworker who would slip intentional bugs or style violations into his changes to see if the reviewer was paying attention. Strangely, this didn't hold for major bugs, which were often done by newbies to the codebase. Major bugs are often created by not understanding the implications of a change or the other systems that might be impacted; experienced devs usually know this by heart, so they don't screw it up in their designs.
    - cpeterso 11y ago
      > I had one (very productive) coworker who would slip intentional bugs or style violations into his changes to see if the reviewer was paying attention. I've heard of this process ("bebugging", the opposite of "debugging") being used for statistical QA analysis. If you purposely inject 10 bugs (don't forget to remove them! :) and QA or fuzzing finds 7 of them plus (say) 42 other bugs, then you can extrapolate that you have about 18 undiscovered bugs (10/7*42-42 = 18). This assumes your 10 injected bugs are representative of the actual bugs you find, which is harder than it sounds.
RogerL 11y ago
There is a continuum between pre-commit and push to production. You can push to integration, push to QA, push to nightly build, and so on. I'm highly skeptical of code reviews as the way to find bugs. I think code reviews have other values, just not that one so much. Getting multiple eyes on code and the results of that code, getting it into the hands of QA, dog fooding it, all play a role. That all sounds way more ad-hoc than I mean it. I used to write life critical software, and you have well defined ways to handling source control and reviews. I advocate figuring out what your project needs, adopting that, and then continually adapting as the project changes, as any project will.
- Kaedon 11y ago
  Absolutely. I think code reviews make sense for knowledge sharing and improving the quality of the code. Most people during code reviews don't actually find bugs in their comments, according to some research done by Microsoft. http://research.microsoft.com/pubs/180283/ICSE%202013-codereview.pdf http://research.microsoft.com/pubs/180283/ICSE%202013-codere...
- cpeterso 11y ago
  The Quora post specifically says they review code after it is live in production: At Quora, we generally do post-commit code reviews. That is, the code goes out live in production first and someone comes and reviews the code later.
ceronman 11y ago
There is more to code reviews than just preventing issues. Code review is a valuable tool for maintaining code quality, and share code knowledge across the team. Those two things can be achieved with post-commit reviews.

by groby_b 11y ago

relaunched 11y ago

They specifically address areas where the default to pre-commit review, for the reason you just called out: We switch to pre-commit reviews instead of the usual post-commit review flow if the potential errors/mistakes are costly and can cause significant problems. Some examples of such cases are: Touching code which deals with privacy and anonymity of users. Touching a core abstraction since a lot of other code might depend on it. Touching some parts of the infrastructure where mistakes can cause a downtime.

mdlowman 11y ago
You're right, but where I work almost anything can cause a bug that affects users. Pre-review eliminates some of those bugs. I would not move to this workflow, although it's interesting to hear from somewhere that it works.
chipgap98 11y ago
While the examples given do highlight areas where a bug could do critical damage to a company, there are plenty of ways to break things that cause problems for users. If you put code through at least a lightweight review process you can filter out many of these bugs. While post commit review might work for smaller changes I would rather err on the side of caution and use precommit review. We deploy about the same number of times/day/engineer at my company, so I don't think that the review process is what drives that metric.
groby_b 11y ago
That makes the assumption you can correctly all identify all those places. And maybe I'm too chicken, and you actually can - but I'd personally still prefer pre-commit. I'm a belt-and-suspenders gal :) But I'm really curious - if you can talk about it, that is - why devs push on average almost 4 changes per day & dev, but a review takes 2 days roundtrip.

nashadelic 11y ago

There really can't be a one-size-fits-all. New devs must earn the right to commit without review (post-commit-review), devs with proven quality records get post-commit-review, sensitive code areas get pre-commit-reviews.

rue 11y ago
Thinking that ‘proven devs’ don’t make mistakes, or more importantly wouldn’t benefit from ideas in review is a danger in itself.
- nashadelic 11y ago
  I didn't say they bypass reviews, I said they get post-reviews, they can push without being blocked by a review.
- nostrademons 11y ago
  At Google the worst offenders for broken builds & minor bugs were often the most experienced & productive devs. They developed reputations around their particular teams, which meant their teammates would think "Oh, X wrote this, he usually knows what he's doing, so I don't have to be quite as careful with the review." As a result, a bunch of silly mistakes would make it through. I had one (very productive) coworker who would slip intentional bugs or style violations into his changes to see if the reviewer was paying attention. Strangely, this didn't hold for major bugs, which were often done by newbies to the codebase. Major bugs are often created by not understanding the implications of a change or the other systems that might be impacted; experienced devs usually know this by heart, so they don't screw it up in their designs.
  - cpeterso 11y ago
    > I had one (very productive) coworker who would slip intentional bugs or style violations into his changes to see if the reviewer was paying attention. I've heard of this process ("bebugging", the opposite of "debugging") being used for statistical QA analysis. If you purposely inject 10 bugs (don't forget to remove them! :) and QA or fuzzing finds 7 of them plus (say) 42 other bugs, then you can extrapolate that you have about 18 undiscovered bugs (10/7*42-42 = 18). This assumes your 10 injected bugs are representative of the actual bugs you find, which is harder than it sounds.

RogerL 11y ago

There is a continuum between pre-commit and push to production. You can push to integration, push to QA, push to nightly build, and so on. I'm highly skeptical of code reviews as the way to find bugs. I think code reviews have other values, just not that one so much. Getting multiple eyes on code and the results of that code, getting it into the hands of QA, dog fooding it, all play a role. That all sounds way more ad-hoc than I mean it. I used to write life critical software, and you have well defined ways to handling source control and reviews. I advocate figuring out what your project needs, adopting that, and then continually adapting as the project changes, as any project will.

Kaedon 11y ago
Absolutely. I think code reviews make sense for knowledge sharing and improving the quality of the code. Most people during code reviews don't actually find bugs in their comments, according to some research done by Microsoft. http://research.microsoft.com/pubs/180283/ICSE%202013-codereview.pdf http://research.microsoft.com/pubs/180283/ICSE%202013-codere...
cpeterso 11y ago
The Quora post specifically says they review code after it is live in production: At Quora, we generally do post-commit code reviews. That is, the code goes out live in production first and someone comes and reviews the code later.

ceronman 11y ago

There is more to code reviews than just preventing issues. Code review is a valuable tool for maintaining code quality, and share code knowledge across the team. Those two things can be achieved with post-commit reviews.