Hi, On Tue, 22 Mar 2016, Pranit Bauva wrote: > Convert the code literally without changing its design even though it > seems that its obscure as to the use of comparing revision to different s/its/it is/ > bisect arguments which seems like a problem in shell because of the way > function arguments are handled. I agree that it is obscure. That is why I would suggest to fix it during the conversion. Using 'new_term' and 'orig_term' (or something similar) would make much more sense. Another good idea would be to include the shell code, or at least to provide a link such as: https://github.com/git/git/blob/v2.8.0-rc4/git-bisect.sh#L572-L597 > The argument handling is kind of hard coded right now because it is not > really be meant to be used like this and this is just for testing > purposes whether this new method is as functional as its counter part. > The shell counter part of the method has been retained for historical > purposes. Still, it would make more sense (both in terms of readability and in terms of code safety) to introduce and use a function like 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; } > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 3324229..6cdae82 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -2,27 +2,64 @@ > #include "cache.h" > #include "parse-options.h" > #include "bisect.h" > +#include "refs.h" > > static const char * const git_bisect_helper_usage[] = { > N_("git bisect--helper --next-all [--no-checkout]"), > + N_("git bisect--helper --check-term-format <term> <revision>"), Good. This imitates the existing code. > NULL > }; > > +static int check_term_format(const char *term, const char *revision, int flags); > + > +static int check_term_format(const char *term, const char *revision, int flag) { Since you define the check_term_format() function here, the declaration above is unnecessary. Let's just delete it. > + if (check_refname_format(term, flag)) > + die("'%s' is not a valid term", term); > + > + 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. > + if (!strcmp(term, "bad") || !strcmp(term, "new")) > + if(strcmp(revision, "bad")) > + die("can't change the meaning of term '%s'", term); > + > + if (!strcmp(term, "good") || !strcmp(term, "old")) > + if (strcmp(revision, "good")) > + die("can't change the meaning of term '%s'", term); These two can be combined. Actually, these *four* can easily be combined: if ((one_of(term, "bad", "new", NULL) && strcmp(orig, "bad")) || (one_of(term, "good", "old", NULL) && strcmp(orig, "good"))) die("can't change the meaning of term '%s'", term); > int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > { > int next_all = 0; > int no_checkout = 0; > + const char *term; Better use the existing convention: int check_term_format = 0; > 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. > OPT_END() > }; > > argc = parse_options(argc, argv, prefix, options, > git_bisect_helper_usage, 0); > > + > + if (term != NULL) { > + if (argc > 0) Here you need to test for a precise argc, not for a range. > + return check_term_format(term, argv[0], 0); > + else > + die("no revision provided with check_for_term"); > + } > + > if (!next_all) > usage_with_options(git_bisect_helper_usage, options); > > diff --git a/git-bisect.sh b/git-bisect.sh > index 5d1cb00..ea237be 100755 > --- a/git-bisect.sh > +++ b/git-bisect.sh > @@ -564,8 +564,8 @@ write_terms () { > then > die "$(gettext "please use two different terms")" > fi > - check_term_format "$TERM_BAD" bad > - check_term_format "$TERM_GOOD" good > + git bisect--helper --check-term-format="$TERM_BAD" bad > + git bisect--helper --check-term-format="$TERM_GOOD" good 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. Did you also run the test suite after compiling, to verify that the documented expectations are still met after the conversion? Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html