Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Pranit Bauva writes: > On Tue, Jul 26, 2016 at 11:02 PM, Junio C Hamano wrote: >> Torsten Bögershausen writes: >> >>> On 07/25/2016 06:53 PM, Junio C Hamano wrote: Pranit Bauva writes: >>> >>> +enum terms_defined { >>> >>> + TERM_BAD = 1, >>> >>> + TERM_GOOD = 2, >>> >>> + TERM_NEW = 4, >>> >>> + TERM_OLD = 8 >>> >>> +}; >>> >>> + >> >> ... >>> Is there any risk that a more generic term like "TERM_BAD" may collide >>> with some other definition some day ? >>> >>> Would it make sense to call it GIT_BISECT_TERM_BAD, GBS_TERM_BAD, >>> BIS_TERM_BAD or something more unique ? >> >> I am not sure if the scope of these symbols would ever escape >> outside bisect-helper.c (and builtin/bisect.c eventually when we >> retire git-bisect.sh), but BISECT_TERM_{GOOD,BAD,OLD,NEW} would not >> be too bad. > > I agree that it wouldn't be too bad. This can be considered as low > hanging fruit and picked up after the completion of the project as > after the whole conversion, some re-ordering of code would need to be > done. > For eg. there is read_bisect_terms() is in bisect.c while > get_terms() is in builtin/bisect--helper.c but they both do the same > stuff except the later one uses strbuf and a lot more important stuff. The criteria to decide if a known "room for improvement" can be left as a "can be later touched up" is "would it be a long-term maintenance burden if it is left unfixed?", and from that point of view, I actually view the latter as a part of the necessary "polishing in response to review comments" for the initial version of a new topic to land in the official project's tree. As to TERM vs BISECT_TERM, I do not think it matters that much as I said (IOW, I do not think it would make it a maintenance burden if we kept using TERM_{BAD,GOOD,NEW,OLD}). -- 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
Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Hey Junio, On Tue, Jul 26, 2016 at 11:02 PM, Junio C Hamano wrote: > Torsten Bögershausen writes: > >> On 07/25/2016 06:53 PM, Junio C Hamano wrote: >>> Pranit Bauva writes: >>> >> >>> +enum terms_defined { >> >>> + TERM_BAD = 1, >> >>> + TERM_GOOD = 2, >> >>> + TERM_NEW = 4, >> >>> + TERM_OLD = 8 >> >>> +}; >> >>> + > >> ... >> Is there any risk that a more generic term like "TERM_BAD" may collide >> with some other definition some day ? >> >> Would it make sense to call it GIT_BISECT_TERM_BAD, GBS_TERM_BAD, >> BIS_TERM_BAD or something more unique ? > > I am not sure if the scope of these symbols would ever escape > outside bisect-helper.c (and builtin/bisect.c eventually when we > retire git-bisect.sh), but BISECT_TERM_{GOOD,BAD,OLD,NEW} would not > be too bad. I agree that it wouldn't be too bad. This can be considered as low hanging fruit and picked up after the completion of the project as after the whole conversion, some re-ordering of code would need to be done. For eg. there is read_bisect_terms() is in bisect.c while get_terms() is in builtin/bisect--helper.c but they both do the same stuff except the later one uses strbuf and a lot more important stuff. Maybe after the whole conversion, the above enum (or #define bitmap) should also be moved to bisect.h and be used consistently in bisect.c too. Regards, Pranit Bauva -- 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
Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Torsten Bögershausen writes: > On 07/25/2016 06:53 PM, Junio C Hamano wrote: >> Pranit Bauva writes: >> > >>> +enum terms_defined { > >>> + TERM_BAD = 1, > >>> + TERM_GOOD = 2, > >>> + TERM_NEW = 4, > >>> + TERM_OLD = 8 > >>> +}; > >>> + >> ... > Is there any risk that a more generic term like "TERM_BAD" may collide > with some other definition some day ? > > Would it make sense to call it GIT_BISECT_TERM_BAD, GBS_TERM_BAD, > BIS_TERM_BAD or something more unique ? I am not sure if the scope of these symbols would ever escape outside bisect-helper.c (and builtin/bisect.c eventually when we retire git-bisect.sh), but BISECT_TERM_{GOOD,BAD,OLD,NEW} would not be too bad. -- 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
Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C
On 07/25/2016 06:53 PM, Junio C Hamano wrote: Pranit Bauva writes: >>> +enum terms_defined { >>> + TERM_BAD = 1, >>> + TERM_GOOD = 2, >>> + TERM_NEW = 4, >>> + TERM_OLD = 8 >>> +}; >>> + >> >> What does TERM stand for ? The terms (words) used to denote the newer and the older parts of the history. Traditionally, as a regression-hunting tool (i.e. it used to work, where did I break it?), we called older parts of the history "good" and newer one "bad", but as people gained experience with the tool, it was found that the pair of words was error-prone to use for an opposite use case "I do not recall fixing it, but it seems to have been fixed magically, when did that happen?", and a more explicit "new" and "old" were introduced. Thanks for the explanation. Is there any risk that a more generic term like "TERM_BAD" may collide with some other definition some day ? Would it make sense to call it GIT_BISECT_TERM_BAD, GBS_TERM_BAD, BIS_TERM_BAD or something more unique ? -- 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
Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C
On Mon, Jul 25, 2016 at 6:53 PM, Junio C Hamano wrote: > Pranit Bauva writes: > >>> 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 ? > > We might want to ask if these should even be bitwise option. A word > with individually controllable bits (i.e. "flag word") makes sense > only when the bits within it are largely independent. But the code > does this pretty much upfront: > + 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.")); > > which is a very strong indication that these bits are not. > > I suspect that OPTION_CMDMODE would be a better choice to group > these four options and mark them mutually incompatible automatically > than OPT_BIT? I must say that Pranit used that at one point, but it felt weird to me to use that for things that is not really a command. -- 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
Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Pranit Bauva writes: >>> +enum terms_defined { >>> + TERM_BAD = 1, >>> + TERM_GOOD = 2, >>> + TERM_NEW = 4, >>> + TERM_OLD = 8 >>> +}; >>> + >> >> What does TERM stand for ? The terms (words) used to denote the newer and the older parts of the history. Traditionally, as a regression-hunting tool (i.e. it used to work, where did I break it?), we called older parts of the history "good" and newer one "bad", but as people gained experience with the tool, it was found that the pair of words was error-prone to use for an opposite use case "I do not recall fixing it, but it seems to have been fixed magically, when did that happen?", and a more explicit "new" and "old" were introduced. >> 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 ? We might want to ask if these should even be bitwise option. A word with individually controllable bits (i.e. "flag word") makes sense only when the bits within it are largely independent. But the code does this pretty much upfront: >>> + 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.")); which is a very strong indication that these bits are not. I suspect that OPTION_CMDMODE would be a better choice to group these four options and mark them mutually incompatible automatically than OPT_BIT? -- 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
Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C
Hey Torsten, On Fri, Jul 22, 2016 at 7:59 AM, Torsten Bögershausen wrote: > > > 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 ? I am not sure as why bitmaps would be a better choice except for git's source code. I saw the source code (especially config.c) and it uses "#defines" bitmap style. I haven't been able to find this method before. Also it uses "(1<<2)" instead of "4". >> 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" ? Yes. I will use a "else if". Thanks! >> + >> + 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, "bisec
Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C
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 [] 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, --ter
[PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C
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 +}; + 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 [] 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); + + 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.")); + if (!cmdmode)