Re: [PATCH 4/4] bisect: add the terms old/new

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2015, 15:24 UTC
Message-ID: <xmqqwpzbmvr3.fsf@gitster.dls.corp.google.com>
In-Reply-To: <vpq7frcast5.fsf@anie.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 21 quoted lines

> "Somebody else did it like that" is not a good justification. Especially
> when the previous code was not merged: the code wasn't finished.
>
> But I actually disagree with the fact that it was not the idea. The
> point of having the terms in BISECT_TERMS was precisely to be generic
> enough. Had the goal been just to distinguish good/bad and old/new, we
> would have needed only one bit of information, and encoding it with the
> existance/non-existance of a file would have been sufficient (as you
> tried to do in addition to BISECT_TERMS).
>
>> For now we just rebased, corrected and finishing to implement
>> functionalities.
>
> functionalities is one thing, but the code should be maintainable to be
> merged in git.git. Git would not be where it is if Junio was merging
> patches based on "it works, we'll see if the code is good enough later"
> kinds of judgments ;-).
>
> Moving from "one hardcoded pair of terms" to "two hardcoded pairs of
> terms" is a nice feature, but hardly a step in the right direction wrt
> maintainability.

Nicely put. From that point of view, the variable names and the underlying machinery in general should call these two "new" vs "old". I.e. name_new=bad name_old=good would be the default, not name_bad=bad name_good=good.