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

[Pranit Bauva <pranit.bauva@gmail.com>]

On Fri, May 13, 2016 at 4:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> 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.

Not just that, it would also be fundamentally more correct as there is
a difference between " !strcmp("good") " and " strcmp("bad") ". Yours
approach suggests that it should be "good" specifically which is
technically more correct as then only it would be sensible to check
whether it is trying to change the meaning of the term.

> 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.

Will do this change in a separate patch after the dust settles.

Regards,
Pranit Bauva