Re: [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 1, 2017, 20:07 UTC
Message-ID: <xmqqr33hoetx.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <cover.1485950225.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 7 quoted lines

> It is quite preposterous to call this an "iteration" of the patch
> series, because the code is so different now. I say this because I want
> to caution that this code has not been tested as thoroughly, by far, as
> the first iteration.
>
> The primary purpose of code review is correctness, everything else is
> either a consequence of it, or a means to make reviewing easier.

You are utterly wrong here.

The primary purpose of code review is to spot and correct the problems the submitter has missed. The problems can span from stupid bugs that affect correctness to maintainability, to design mistakes, to clarity of explanation for both end users and future developers.

Among them, correctness problems are, as long as the problem to be solved is specified clearly enough, the easiest to spot by the submitter before the patch is sent out. The submitter is focused on solving one problem, and if the updated code does not even work as the submitter advertises it would, that can be caught by the submitter before the patch is even sent out.

Of course, humans are not perfect, and catching correctness problems is important, but that is not the only (let alone primary) thing.

When a submitter has been focusing on solving one problem, it is easy to lose the larger picture and to end up adding something that may be "correct" (in the sense of "works as advertised by the submitter") but does not fit well with the rest of the system, or covers some use cases but misses other important and related use cases. If the "does not fit well" surfaces to the end user level, that would become a design problem. If it affects the future developers, that would become a maintainability problem.

Compared to the correctness issue, these are much harder to spot by the submitter alone, who focused so intensely to get his code "correct". The review process is of greater value to spot these issues.

I've already read and commented on the series; as I said, I think the rewrite in 3/4 makes the resulting code much easier to read, and with the fix-up I just sent out, I think the end result is correct, works as advertised, the way it is advertised is clear to end users, and is maintainable.

But I do share your uneasiness about the "new-ness" of the code.