Johannes Schindelin [off-list ref] 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.
quoted
+ 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.quoted
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.
Hi Junio,
On Tue, 22 Mar 2016, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
quoted
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.
Good point, I keep forgetting that OPT_CMDMODE() was introduced
specifically for subcommands.
Ciao,
Dscho
On Tue, Mar 22, 2016 at 9:33 PM, Junio C Hamano [off-list ref] wrote:
Johannes Schindelin [off-list ref] writes:
quoted
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.
quoted
quoted
+ 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.
quoted
quoted
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.
On Tue, Mar 22, 2016 at 10:52 AM, Pranit Bauva [off-list ref] wrote:
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 ?
Patches to outdated documentation are most awesome. ;)
Hi,
On Tue, 22 Mar 2016, Stefan Beller wrote:
On Tue, Mar 22, 2016 at 10:52 AM, Pranit Bauva [off-list ref] wrote:
quoted
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 ?
Patches to outdated documentation are most awesome. ;)
Yes!
Thanks,
Johannes