I also had this idea when I played with Git, but I decided against it. I experienced that I do micro-commits with Git, where only a few lines where changed.

Now going through these commits as a code review has the result that you follow the original programmers path way to close. I did this when I needed to get back into my own source code after a vacation.

All was fine following through the commits, the code was clearly clean and functional.

But when I inspected the source code as a whole I found a major problem which lead to an hours-long refactoring session.

As a consequence I now use branch-per-feature more often and review the changes of a whle branch before merging back into master.

So while I don't recommend git commits as a base for code reviews, they are a great way to find out the orginal programmers intent.

My system is to make changes and additions and then before I check in I code review every file that has changed. As I review the files I note all the changes in the commit notes.

Example from my open source project source control:

json.codeplex.com/.../ListDownloadableCommits.aspx

I'm with Markus, a new feature requires a new branch and then do micro commits and also for the review and finally merge is back.

Ayende,

I think the drawback of that approach is that the original developer who wrote that line is not involved in the process. So he won't know about it unless he looks at the history.

I think I like the Crucible approach better ( http://www.atlassian.com/software/crucible/). The developer is told to fix something, it's traceable and hopefully, he'll learn from it and won't do it again.

Haven't used it yet though.. or don't know if there are alternatives.

As I understand Ayende he is doing this commit as a code review on new branch, so the original author can easily go over it. If the original author agrees with some review mark, he can just easily pull this change to his branch.

I guess my only concern would be some sort of cascading effect. Does 'accepting" one of the later commits mean that the developer will need one or more of the previous ones? If that's the case then I'd prefer something more along the lines of James' approach, at least for coupled commits.

This is Mr_Simple's method. Keeps things brief and clear - oh, and simple.

I agree with Steven: one of the most important parts of code review, in my experience, is receiving the feedback and putting in the extra work to resolve the issues personally. I found this work to be the most effective way of learning to avoid common mistakes or improve initial code quality but I feel it also encourages the development of a healthy personal interest in the quality of the application as a whole.

__This is me going over another guy's commit, every time that I found something I didn't like, I fixed it and committed with an explanation.

I don't really understand how it should be done cleanly otherwise...

A big commit of unrelated fixes? BAD!

Note that commit logs like "Changing the way we handle returning category id" is also not perfect : you should focus on the "why", impact, etc..., not so much on the "what".

Of course sometimes you have other constraints and can't do perfect stuffs.

Did you make the commits in a different branch or the same branch?

Parag,

Same branch