Pranit Bauva <pranit.bauva@xxxxxxxxx> writes:
> On Tue, Jul 26, 2016 at 11:02 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Torsten Bögershausen <tboegi@xxxxxx> writes:
>>
>>> On 07/25/2016 06:53 PM, Junio C Hamano wrote:
>>>> Pranit Bauva <pranit.bauva@xxxxxxxxx> 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}).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html