4 ms·

Git commits as code review?

7 points by rayvega 17y ago

smcq 17y ago
We use git commit reviews as our primary review method, it's much lower overhead than sit down code review meetings. On the other hand, it's not quite as thorough, which works fine with a small team that's already high bandwidth.
- awa 17y ago
  Try http://www.reviewboard.org/ http://www.reviewboard.org/ or something like it
gruseom 17y ago
The idea here is that when reviewing someone's code, instead of writing up the problems that you found and handing the write-up to the author (which they may or may not understand, and may or may not act on), you actually just fix the code as you go, committing each little change you make with an explanatory comment as the commit message. The author can then look over the history to see not only what the comments were, but exactly what each one means in the code. You're guaranteed that the problems are fixed, and (assuming the original programmer actually does look over the history) the learning value is much higher. I've done exactly this on a couple of occasions and it worked very well. Among other things, it's easier on the reviewer. (Who likes writing up a big document or email? Much better to actually fix code.) Personally, though, I find it more productive to work more collaboratively to begin with.
- icefox 17y ago
  I do this for Arora when I get a commit that just needs a minor tweak. It is easier to just merge in their patch and then make a new commit fixing it up then to tell them to fix one thing, wait for them to actually do it, fetch, and then merge. But for the patches where there are more fundamental design changes it comes down to time. I could email them back with a list of things to fix or spend the next few hours fixing it myself. If I am interested in getting the patch in then I might.
stcredzero 17y ago
We are going to be using a code coverage tool in conjunction with our source code management tool to do code reviews. Every branch of every method changed for a particular Change Request will be instrumented with something subclassed from a breakpoint, and the developers will get a randomly assigned "review partner" and they will ensure all of the checked in code is covered by unit tests. The "coverage map" of affected methods is generated automatically, and the insertion of the code probes is also done automatically.

7 points by rayvega 17y ago

smcq 17y ago

We use git commit reviews as our primary review method, it's much lower overhead than sit down code review meetings. On the other hand, it's not quite as thorough, which works fine with a small team that's already high bandwidth.

awa 17y ago
Try http://www.reviewboard.org/ http://www.reviewboard.org/ or something like it

gruseom 17y ago

The idea here is that when reviewing someone's code, instead of writing up the problems that you found and handing the write-up to the author (which they may or may not understand, and may or may not act on), you actually just fix the code as you go, committing each little change you make with an explanatory comment as the commit message. The author can then look over the history to see not only what the comments were, but exactly what each one means in the code. You're guaranteed that the problems are fixed, and (assuming the original programmer actually does look over the history) the learning value is much higher. I've done exactly this on a couple of occasions and it worked very well. Among other things, it's easier on the reviewer. (Who likes writing up a big document or email? Much better to actually fix code.) Personally, though, I find it more productive to work more collaboratively to begin with.

icefox 17y ago
I do this for Arora when I get a commit that just needs a minor tweak. It is easier to just merge in their patch and then make a new commit fixing it up then to tell them to fix one thing, wait for them to actually do it, fetch, and then merge. But for the patches where there are more fundamental design changes it comes down to time. I could email them back with a list of things to fix or spend the next few hours fixing it myself. If I am interested in getting the patch in then I might.

stcredzero 17y ago

We are going to be using a code coverage tool in conjunction with our source code management tool to do code reviews. Every branch of every method changed for a particular Change Request will be instrumented with something subclassed from a breakpoint, and the developers will get a randomly assigned "review partner" and they will ensure all of the checked in code is covered by unit tests. The "coverage map" of affected methods is generated automatically, and the insertion of the code probes is also done automatically.