Re: [PATCH v6 2/3] bisect: rewrite `check_term_format` shell function in C

Pranit Bauva [off-list ref] writes:

+	/*
+	 * In theory, nothing prevents swapping completely good and bad,
+	 * but this situation could be confusing and hasn't been tested
+	 * enough. Forbid it for now.
+	 */
+
+	if ((strcmp(orig_term, "bad") && one_of(term, "bad", "new", NULL)) ||
+		 (strcmp(orig_term, "good") && one_of(term, "good", "old", NULL)))
+		return error(_("can't change the meaning of the term '%s'"), term);

The above comes from below

-	bad|new)
-		if test "$2" != bad
-		then
-			die "$(eval_gettext "can't change the meaning ...

So it is not your fault, but it is quite hard to understand.

The original says "You are trying to use 'bad' (or 'new') for
something that is not 'bad'--which is confusing; do not do it".

I _think_ the reason I find C version harder to read is the use of
strcmp(orig_term, "bad") to say "orig_term is not BAD".  The shell
version visually tells with "!=" that the meaning of the new term is
*NOT* "bad", and does not ahve such a problem.

Perhaps if we rewrote it to

	if ((!strcmp(orig_term, "good") &&
             one_of(term, "bad", "new", NULL)) ||
             (!strcmp(orig_term, "bad") &&
             one_of(term, "good", "old", NULL)))

i.e. "If you are using "bad" or "new" to mean "good", that is not a
good idea", as a follow-up change after the dust settles, the result
might be easier to read.

But this is just commenting for future reference, not suggesting to
update this patch at all.  If we were to do the change, that must
come as a separate step.

Thanks.