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

[Junio C Hamano <gitster@pobox.com>]

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.

I agree with all the points you raised in your review.  Just wanted
to add some clarification myself.

Thanks.