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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2017, 09:46 UTC
Message-ID: <xmqqbmvgy396.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1701090850340.3469@virtualbox>

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.