4 ms·

I was interested in the comment that they do "post-push" reviews. It seems to me that you have only two options with code reviews: a) Pre-push: now all your c

I was interested in the comment that they do "post-push" reviews. It seems to me that you have only two options with code reviews: a) Pre-push: now all your commits are delayed by the review process. Even if it's just by a few minutes, it breaks your flow. b) Post-push: now if the reviewer things you did it totally wrong, it's too late to fix it. You have to make an entirely new patch, and meanwhile people have been exposed to your already-pushed crap. On the other hand, this is no worse than not reviewing. In a post-push setup, what do you do with the reviewer's comments?

kamens 15y ago
Yet another reason why distributed source control plays so nice w/ code reviews. You can push anywhere you want -- doesn't have to be to your stable repository -- in the process of getting reviews. Or, you can push to stable and deal w/ the exact situation that you describe. We do a lot of the former and a little of the latter.
jonstjohn 15y ago
We commit everything to a feature branch where it gets reviewed. Nothing gets to our main branch until the feature is completed, reviews are done and builds pass. It is a really smooth work-flow and doesn't slow us down significantly. The reviews help us to work asynchronously and both improve and familiarize ourselves with other developers' code.
- apenwarr 15y ago
  Wow, nice. I've used the "feature branch until the continuous build passes" method before with great results, but also requiring code reviews before merging could be a great addition.
  - aaronblohowiak 15y ago
    this also allows you to squash and merge commits to make a tidy patch for mainline
    - jonstjohn 15y ago
      To elaborate a bit, we do not allow merging to the mainline until all reviews are completed, manual testing has been performed, and a continuous integration build passes on the feature branch. At that point, we merge to the mainline, then after we get a successful build there (to ensure there were no merge issues), we tag the mainline for release. This is all very well documented within our company and everybody understands the process. The result is that we have cleaner code and fewer bugs in production (although they still get there). It's really just second nature to all developers. We've been able to develop a very solid set of unit tests and browser-based tests (selenium) that give us a lot of confidence in our releases. Manual testing catches a few things that didn't make it into automated tested, including (but not limited) user interface issues.
azov 15y ago
Post-push reviews don't make sense to me - it's like checking movie tickets after you already watched a movie. Pre-push is much better because: 1. There's a clear outcome - the change is either approved and carries the name of the reviewer, or it is not. This creates focus and sense of responsibility. Post-push reviews tend to turn into pointless chats followed by no action: "- I think it is better to do it differently. - Yeah, probably, whatever". 2. It's easy to automate them - a simple hook can reject a change that was not reviewed. With post-push you have to nag developers to review every commit, then you have to nag them again to fix whatever the reviewers found, then you have to nag them more to review that fix itself - and so on and on ad nauseum. 3. The quality bar is higher. It is psychologically easier for reviewer to NOT approve something that doesn't look right then to insist that the author fixes it after the fact (asking permission vs forgiveness) 4. Other people don't base their work on code that haven't been reviewed. "yes, you're right, it would be better to do it differently, but now it is used in 150 places, too much trouble to fix" (and next time reviewer won't even bother to mention small problems, the whole process slowly degrades). 5. Most importantly, all code in your shared repo is always reviewed. With post-push, given sufficient volume of commits, there's always some potentially buggy stuff that nobody looked at - and you probably don't even know what it is. I would even argue that in some teams not reviewing at all would be better then post-push because if you spend time doing reviews, and then nothing happens - you're just wasting time and effort.

kamens 15y ago

Yet another reason why distributed source control plays so nice w/ code reviews. You can push anywhere you want -- doesn't have to be to your stable repository -- in the process of getting reviews. Or, you can push to stable and deal w/ the exact situation that you describe. We do a lot of the former and a little of the latter.

jonstjohn 15y ago

We commit everything to a feature branch where it gets reviewed. Nothing gets to our main branch until the feature is completed, reviews are done and builds pass. It is a really smooth work-flow and doesn't slow us down significantly. The reviews help us to work asynchronously and both improve and familiarize ourselves with other developers' code.

apenwarr 15y ago
Wow, nice. I've used the "feature branch until the continuous build passes" method before with great results, but also requiring code reviews before merging could be a great addition.
- aaronblohowiak 15y ago
  this also allows you to squash and merge commits to make a tidy patch for mainline
  - jonstjohn 15y ago
    To elaborate a bit, we do not allow merging to the mainline until all reviews are completed, manual testing has been performed, and a continuous integration build passes on the feature branch. At that point, we merge to the mainline, then after we get a successful build there (to ensure there were no merge issues), we tag the mainline for release. This is all very well documented within our company and everybody understands the process. The result is that we have cleaner code and fewer bugs in production (although they still get there). It's really just second nature to all developers. We've been able to develop a very solid set of unit tests and browser-based tests (selenium) that give us a lot of confidence in our releases. Manual testing catches a few things that didn't make it into automated tested, including (but not limited) user interface issues.

azov 15y ago

Post-push reviews don't make sense to me - it's like checking movie tickets after you already watched a movie. Pre-push is much better because: 1. There's a clear outcome - the change is either approved and carries the name of the reviewer, or it is not. This creates focus and sense of responsibility. Post-push reviews tend to turn into pointless chats followed by no action: "- I think it is better to do it differently. - Yeah, probably, whatever". 2. It's easy to automate them - a simple hook can reject a change that was not reviewed. With post-push you have to nag developers to review every commit, then you have to nag them again to fix whatever the reviewers found, then you have to nag them more to review that fix itself - and so on and on ad nauseum. 3. The quality bar is higher. It is psychologically easier for reviewer to NOT approve something that doesn't look right then to insist that the author fixes it after the fact (asking permission vs forgiveness) 4. Other people don't base their work on code that haven't been reviewed. "yes, you're right, it would be better to do it differently, but now it is used in 150 places, too much trouble to fix" (and next time reviewer won't even bother to mention small problems, the whole process slowly degrades). 5. Most importantly, all code in your shared repo is always reviewed. With post-push, given sufficient volume of commits, there's always some potentially buggy stuff that nobody looked at - and you probably don't even know what it is. I would even argue that in some teams not reviewing at all would be better then post-push because if you spend time doing reviews, and then nothing happens - you're just wasting time and effort.