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

From: Christian Couder <christian.couder@gmail.com>
Date: Jun 10, 2015, 15:47 UTC
Message-ID: <CAP8UFD0-R1UqSDbEeTKq_gX+uFw2egZetgziqaUiRqdivHvhPA@mail.gmail.com>
In-Reply-To: <xmqqwpzbmvr3.fsf@gitster.dls.corp.google.com>

On Wed, Jun 10, 2015 at 5:24 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 28 quoted lines

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> "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.

I don't think this would improve maintainability, at least not for me. The doc currently uses "good/bad" everywhere. For example the description is:

   This command uses git rev-list --bisect to help drive the binary search
   process to find which change introduced a bug, given an old "good" commit
   object name and a later "bad" commit object name.

And this is logical if the default is "good/bad". If we use name_new and name_old in the code, we make it harder for us to see if the doc matches the code.

And unless we rewrite a lot of them, the tests too will mostly be still using "good/bad" so it will make it harder to maintain them too.