Re: [PATCH v2] bisect--helper: convert a function in shell to C

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> static int one_of(const char *term, ...)
>> {
>>       va_list matches;
>>       const char *match;
>>
>>       va_start(matches, term);
>>       while ((match = va_arg(matches, const char *)))
>>               if (!strcmp(term, match))
>>                       return 1;
>>       va_end(matches);
>>
>>       return 0;
>> }
>
> This is a very good suggestion.  We tend to avoid explicit
> comparison to NULL and zero, but we avoid assignment in condition
> part of if/while statements even more, so
>
>         while ((match = va_arg(matches, const char *)) != NULL)
>
> would be the best compromise to make it clear and readable.
>
>>> +    if (!strcmp(term, "help") || !strcmp(term, "start") ||
>>> +            !strcmp(term, "skip") || !strcmp(term, "next") ||
>>> +            !strcmp(term, "reset") || !strcmp(term, "visualize") ||
>>> +            !strcmp(term, "replay") || !strcmp(term, "log") ||
>>> +            !strcmp(term, "run"))
>>> +            die("can't use the builtin command '%s' as a term", term);
>>
>> This would look so much nicer using the one_of() function above.
>>
>> Please also note that our coding convention (as can be seen from the
>> existing code in builtin/*.c) is to indent the condition differently than
>> the block, either using an extra tab, or by using 4 spaces instead of the
>> tab.
>
> In general, "or by using 4 spaces" is better spelled as "or by
> indenting so that the line aligns with the beginning of the inside
> of the opening parenthesis on the above line"; "if (" happens to
> take 4 display spaces and that is where "4" comes from and it would
> be different for "while (" condition ;-).

>
> But with one_of(...) this part would change a lot, I imagine.
>>>      struct option options[] = {
>>>              OPT_BOOL(0, "next-all", &next_all,
>>>                       N_("perform 'git bisect next'")),
>>>              OPT_BOOL(0, "no-checkout", &no_checkout,
>>>                       N_("update BISECT_HEAD instead of checking out the current commit")),
>>> +            OPT_STRING(0, "check-term-format", &term, N_("term"),
>>> +                     N_("check the format of the ref")),
>>
>> Hmm. The existing code suggests to use OPT_BOOL instead.
>> ...
>> The existing convention is to make the first argument *not* a value of the
>> "option", i.e. `--check-term-format "$TERM_BAD"` without an equal sign.
>
> I think it is preferrable to keep using OPT_BOOL() for this new one
> if we are incrementally building on top of existing code.
>
> But if the convention is that the option is to specify what opration
> is invoked, using OPT_CMDMODE() to implement all of them would be a
> worthwhile cleanup to consider at some point.