Johannes Schindelin <johannes.schinde...@gmx.de> writes: > And the most important point is that we do all of this only during a > hopefully brief period in time that is mostly spent on reviewing the code > and finding serious bugs and fixing them.
You seem to misunderstand the purpose of the code review. We indeed spot bugs while reviewing patches, especially ones from less experienced folks, but that is the least important part of the review. In general, we review for: - Design. Is the feature make sense? Is it too narrow? Are there better ways? Does it fit well with the rest of the system? - Explanation. Is the purpose of the change in the bigger picture explained well enough to allow future people to answer this question: "We now have an additional requirement to the feature. If the original author knew about that when this was first introduced, would s/he consider that our design for this additional thing consistent with the original design? Should we design our enhancement in a different way?" - Maintainability. Does the implementation avoid reinventing data structures and helper functions that already exist to interact with elements in the system? Would a future change to some elements in the system that are touched by the implementation require changes to both existing code _and_ reinvented ones the patch introduced? - Correctness. Does the implementation actually reflect the design and the way the design was explained? For the "difftool in C" topic, the first two are mostly irrelevant as the goal of the topic is to first replicate what already exists faithfully (even in a bug-to-bug compatible way). The issues in correctness is something your daily use before submission would have caught, use of 'next' users as testers will help, and also caught by running test suite (again, before submission). I honestly do not expect glaring errors in the code from experienced contributors remaining when their patches are polished enough to be submit to the list.
