On Sun, Apr 27, 2014 at 02:42:40PM +0200, Pawel Dziepak wrote: > Such approach, in my opinion, is much > better than reviewing patches on trac, which I find very inconvenient > for such purpose as it is not possible to add comments to an > individual line or a part of the code. Also, I am not a big fan of > github pull requests and I prefer the simplicity of mailing list and > basic git tools, but I will be happy if any solution that improves > code quality by mandatory code review will be adopted. Last, but not > least, there is actually a reason why I didn't proposed such changes > to our workflow earlier. While this may work very well with project > when the vast majority of developers is employed full-time I am not > sure it will work for us with our limited resources. May I, once again, suggest we setup Gerrit and use the git workflow that goes with it? Patches are submitted using a simple git push operation. They enter the Gerrit code review system where line-by-line reviews and votes (+1/-1) are possible. Gerrit can even be configured to enforce patches to get a score of at least "+2" and no "-" from reviewers before it can be applied. It seems the patches are properly rebased when merged to the main tree. The original patch author can make modifications to the patch, and re-submit it, if done properly, this is appended to the existing Gerrit entry so it's easy to review the new patch in context. Gerrit also handles dependencies between patches ("this patch needs xxx to be merged first"). I think this would lead to a much more solid state for the trunk, making it much easier to get a version of it tagged "release", with a MUCH shorter release cycle than we currently have, and more stable nightlies. With such a great tool in place, reviewing patches will become a much easier and rewarding job, and we can finally reply to people fast enough so we have a chance of them fixing the issues. Getting a review after two weeks that says "sorry, this patch you probably already gave up on getting applied has some coding style violations..." isn't a good way to get more contributors. With a tool like that, I think it's possible to make code reviews systematic, at least for "potentially breaking" changes. I'll add that Gerrit also supports checking that the patch doesn't break the build, and could easily be combined with an automated coding style checker. -- Adrien / PulkoMandy.