From: Torsten Bögershausen Date: Fri, 22 Jul 2016 02:29:17 GMT Subject: Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C Message-ID: <7a408f75-74d9-7e59-35d6-3a931414a7f9@web.de> In-Reply-To: <010201560a4782ce-8d6545f1-2454-492b-a2c6-6e0f2ac824d4-000000@eu-west-1.amazonses.com> On 07/20/2016 11:47 PM, Pranit Bauva wrote: > Reimplement the `get_terms` and `bisect_terms` shell function in C and > add `bisect-terms` subcommand to `git bisect--helper` to call it from > git-bisect.sh . > > In the shell version, the terms were identified by strings but in C > version its done by bit manipulation and passing the integer value to > the function. > > Using `--bisect-terms` subcommand is a temporary measure to port shell > function in C so as to use the existing test suite. As more functions > are ported, this subcommand will be retired and will be called by some > other methods. > > Mentored-by: Lars Schneider > Mentored-by: Christian Couder > Signed-off-by: Pranit Bauva > --- > builtin/bisect--helper.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++- > git-bisect.sh | 35 ++--------------------- > 2 files changed, 75 insertions(+), 34 deletions(-) > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 001096a..185a8ad 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -8,6 +8,13 @@ > #include "run-command.h" > #include "prompt.h" > > +enum terms_defined { > + TERM_BAD = 1, > + TERM_GOOD = 2, > + TERM_NEW = 4, > + TERM_OLD = 8 > +}; > + What does TERM stand for ? It could be TERMinal, TERMinator or just TERM. Something like BIS_TERM_DEF_BAD .. may be more intuitive, and may avoid name clashes in the long run. And why are the defines 1,2,4,8 ? It looks as if a #define bitmap may be a better choice here ? How do we do these kind of bit-wise opions otherwise ? > static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS") > static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV") > static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK") > @@ -26,6 +33,7 @@ static const char * const git_bisect_helper_usage[] = { > N_("git bisect--helper --bisect-write []"), > N_("git bisect--helper --bisect-check-and-set-terms "), > N_("git bisect--helper --bisect-next-check [] + N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"), > NULL > }; > > @@ -359,6 +367,43 @@ static int bisect_next_check(const struct bisect_terms *terms, > return 0; > } > > +static int get_terms(struct bisect_terms *terms) > +{ > + FILE *fp; > + int res; > + fp = fopen(git_path_bisect_terms(), "r"); > + if (!fp) > + return -1; > + > + bisect_terms_reset(terms); > + res = strbuf_getline(&terms->term_bad, fp) == EOF || > + strbuf_getline(&terms->term_good, fp) == EOF; > + > + fclose(fp); > + return res ? -1 : 0; > +} > + > +static int bisect_terms(struct bisect_terms *terms, int term_defined) > +{ > + if (get_terms(terms)) { > + fprintf(stderr, "no terms defined\n"); > + return -1; > + } > + if (!term_defined) { > + printf("Your current terms are %s for the old state\nand " > + "%s for the new state.\n", terms->term_good.buf, > + terms->term_bad.buf); > + return 0; > + } > + > + if (term_defined == TERM_GOOD || term_defined == TERM_OLD) > + printf("%s\n", terms->term_good.buf); > + if (term_defined == TERM_BAD || term_defined == TERM_NEW) > + printf("%s\n", terms->term_bad.buf); May be a switch-case ? Or at least "else if" ? > + > + return 0; > +} > + > int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > { > enum { > @@ -369,9 +414,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > CHECK_EXPECTED_REVS, > BISECT_WRITE, > CHECK_AND_SET_TERMS, > - BISECT_NEXT_CHECK > + BISECT_NEXT_CHECK, > + BISECT_TERMS > } cmdmode = 0; > int no_checkout = 0, res = 0; > + enum terms_defined term_defined = 0; > struct option options[] = { > OPT_CMDMODE(0, "next-all", &cmdmode, > N_("perform 'git bisect next'"), NEXT_ALL), > @@ -389,6 +436,16 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > N_("check and set terms in a bisection state"), CHECK_AND_SET_TERMS), > OPT_CMDMODE(0, "bisect-next-check", &cmdmode, > N_("check whether bad or good terms exist"), BISECT_NEXT_CHECK), > + OPT_CMDMODE(0, "bisect-terms", &cmdmode, > + N_("print out the bisect terms"), BISECT_TERMS), > + OPT_BIT(0, "term-bad", &term_defined, > + N_("show the bad term"), TERM_BAD), > + OPT_BIT(0, "term-good", &term_defined, > + N_("show the good term"), TERM_GOOD), > + OPT_BIT(0, "term-new", &term_defined, > + N_("show the new term"), TERM_NEW), > + OPT_BIT(0, "term-old", &term_defined, > + N_("show the old term"), TERM_OLD), > OPT_BOOL(0, "no-checkout", &no_checkout, > N_("update BISECT_HEAD instead of checking out the current commit")), > OPT_END() > @@ -399,6 +456,16 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > argc = parse_options(argc, argv, prefix, options, > git_bisect_helper_usage, 0); > > + if (cmdmode != BISECT_TERMS && term_defined) > + die(_("--term-bad, --term-good, --term-new and --term-old " > + "can be used only with --bisect-terms")); > + > + if (term_defined != 0 && term_defined != TERM_BAD && > + term_defined != TERM_GOOD && term_defined != TERM_NEW && > + term_defined != TERM_OLD) > + die(_("only one option among --term-bad, --term-good, " > + "--term-new and --term-old can be used.")); > + []