Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 27, 2016, 16:13 UTC
Message-ID: <xmqqeg6fxdnx.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAFZEwPP=AL6V+AitmVD6qnt7FvUJiX1wqrn5C2UjEKxAnnumew@mail.gmail.com>

Pranit Bauva <pranit.bauva@gmail.com> writes:

Show 32 quoted lines

> On Tue, Jul 26, 2016 at 11:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Torsten Bögershausen <tboegi@web.de> writes:
>>
>>> On 07/25/2016 06:53 PM, Junio C Hamano wrote:
>>>> Pranit Bauva <pranit.bauva@gmail.com> writes:
>>>>
>>>>>>> >>> +enum terms_defined {
>>>>>>> >>> +       TERM_BAD = 1,
>>>>>>> >>> +       TERM_GOOD = 2,
>>>>>>> >>> +       TERM_NEW = 4,
>>>>>>> >>> +       TERM_OLD = 8
>>>>>>> >>> +};
>>>>>>> >>> +
>>>>>> >> ...
>>> Is there any risk that a more generic term like "TERM_BAD" may collide
>>> with some other definition some day ?
>>>
>>> Would it make sense to call it GIT_BISECT_TERM_BAD, GBS_TERM_BAD,
>>> BIS_TERM_BAD or something more unique ?
>>
>> I am not sure if the scope of these symbols would ever escape
>> outside bisect-helper.c (and builtin/bisect.c eventually when we
>> retire git-bisect.sh), but BISECT_TERM_{GOOD,BAD,OLD,NEW} would not
>> be too bad.
>
> I agree that it wouldn't be too bad. This can be considered as low
> hanging fruit and picked up after the completion of the project as
> after the whole conversion, some re-ordering of code would need to be
> done.
> For eg. there is read_bisect_terms() is in bisect.c while
> get_terms() is in builtin/bisect--helper.c but they both do the same
> stuff except the later one uses strbuf and a lot more important stuff.

The criteria to decide if a known "room for improvement" can be left as a "can be later touched up" is "would it be a long-term maintenance burden if it is left unfixed?", and from that point of view, I actually view the latter as a part of the necessary "polishing in response to review comments" for the initial version of a new topic to land in the official project's tree.

As to TERM vs BISECT_TERM, I do not think it matters that much as I said (IOW, I do not think it would make it a maintenance burden if we kept using TERM_{BAD,GOOD,NEW,OLD}).