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

Pranit Bauva [off-list ref] writes:

On Tue, Jul 26, 2016 at 11:02 PM, Junio C Hamano [off-list ref] wrote:

quoted

Torsten Bögershausen [off-list ref] writes:

quoted

On 07/25/2016 06:53 PM, Junio C Hamano wrote:

quoted

Pranit Bauva [off-list ref] writes:

quoted

quoted

quoted

quoted

quoted

quoted

+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}).