Re: [PATCH v4 4/4] t7800: run both builtin and scripted difftool, for now

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <Johannes.Schindelin@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.