On Tue, Mar 22, 2016 at 9:33 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Johannes Schindelin <johannes.schinde...@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 ;-).
I will definitely keep this in mind henceforth.
>
> 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.
OPT_CMDMODE() is actually a better option. I also noticed that it
isn't mentioned in Documentation/technical/api-parse-options.txt .
Should I send a patch to include it there just to make it easier for
someone who is new and isn't aware of the changes ?
> I agree with all the points you raised in your review. Just wanted
> to add some clarification myself.
>
> Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
