# [PATCH 2/4] bisect: replace hardcoded "bad|good" by variables 2 messages from 2015-06-10 to 2015-06-10. Participants: Antoine Delaite, Matthieu Moy. Thread: https://gitlist.dev/t/39580 ## Antoine Delaite, 2015-06-10 07:10 Subject: [PATCH 2/4] bisect: replace hardcoded "bad|good" by variables Message-ID: <1809967391.331411.1433920237669.JavaMail.zimbra@ensimag.grenoble-inp.fr> URL: https://gitlist.dev/e/1809967391.331411.1433920237669.JavaMail.zimbra%40ensimag.grenoble-inp.fr In-Reply-To: <308677275.323594.1433875347392.JavaMail.zimbra@ensimag.grenoble-inp.fr> ``` Hi, Thanks for the review, (sorry if you received this twice) Matthieu Moy wrote: >> +static const char *name_bad; >> +static const char *name_good; > >Same remark as PATCH 2. After the discussion you had with Christian I think we will keep name_bad/good for now. >> - >> - fprintf(stderr, "The merge base %s is bad.\n" >> - "This means the bug has been fixed " >> - "between %s and [%s].\n", >> - bad_hex, bad_hex, good_hex); >> - >> + if (!strcmp(name_bad, "bad")) { >> + fprintf(stderr, "The merge base %s is bad.\n" >> + "This means the bug has been fixed " >> + "between %s and [%s].\n", >> + bad_hex, bad_hex, good_hex); >> + } > >You need an "else" here. Maybe it comes later, but as a reviewer, I want >to check that you did not forget it now (because I don't trust myself to >remember that it must be added later). Should I put an else {} with nothing in beetween? >> + name_bad = "bad"; >> + name_good = "good"; >> + } else { >> + strbuf_getline(&str, fp, '\n'); >> + name_bad = strbuf_detach(&str, NULL); >> + strbuf_getline(&str, fp, '\n'); >> + name_good = strbuf_detach(&str, NULL); >> + } > >I would have kept just > > name_bad = "bad"; > name_good = "good"; > >in this patch, and introduce BISECT_TERMS in a separate one. Should not I introduce BISECT_TERMS in bisect.c and git-bisect.sh with the same commit? I did some rebase though and now name_bad and name_good appears in the first commit, and read_bisect_terms in the second. >> --- a/git-bisect.sh >> +++ b/git-bisect.sh >> @@ -77,6 +77,7 @@ bisect_start() { >> orig_args=$(git rev-parse --sq-quote "$@") >> bad_seen=0 >> eval='' >> + start_bad_good=0 >> if test "z$(git rev-parse --is-bare-repository)" != zfalse >> then >> mode=--no-checkout >> @@ -101,6 +102,9 @@ bisect_start() { >> die "$(eval_gettext "'\$arg' does not appear to be a valid revision")" >> break >> } >> + >> + start_bad_good=1 >> + > >Why do you need this variable? It seems to me that you are hardcoding >once more that terms can be either "good/bad" or "old/new", which you >tried to eliminate from the previous round. I answered to Junio on this too, it is because our function which create the bisect_terms file is not called after 'git bisect start bad_rev good_rev'. >> + then >> + echo "$NAME_BAD" >"$GIT_DIR/BISECT_TERMS" && >> + echo "$NAME_GOOD" >>"$GIT_DIR/BISECT_TERMS" >> + fi && > >Why not do this unconditionnally? Whether terms are good/bad or old/new, >you can write them to BISECT_TERMS. Because after a git bisect start we don't yet what terms are used. This line is only for the case 'git bisect start bad_rev good_rev'. >> + fi >> + case "$cmd" in >> + bad|good) >> + if test ! -s "$GIT_DIR/BISECT_TERMS" >> + then >> + echo "bad" >"$GIT_DIR/BISECT_TERMS" && >> + echo "good" >>"$GIT_DIR/BISECT_TERMS" >> + fi >> + NAME_BAD="bad" >> + NAME_GOOD="good" ;; >> + esac ;; >> + esac >> +} >> + >> +bisect_voc () { >> + case "$1" in >> + bad) echo "bad" ;; >> + good) echo "good" ;; >> + esac >> +} > >It's weird to have these hardcoded "bad"/"good" when you already have >BISECT_TERMS in the same patch. bisect_voc is used to display what commands the user can do, thus the builtins tags. I did not find a way to not hardcode them. The other points have been taken into account. ``` ## Matthieu Moy, 2015-06-10 07:55 Subject: Re: [PATCH 2/4] bisect: replace hardcoded "bad|good" by variables Message-ID: URL: https://gitlist.dev/e/vpqr3pkatfy.fsf%40anie.imag.fr In-Reply-To: <1809967391.331411.1433920237669.JavaMail.zimbra@ensimag.grenoble-inp.fr> ``` Antoine Delaite writes: >>> - >>> - fprintf(stderr, "The merge base %s is bad.\n" >>> - "This means the bug has been fixed " >>> - "between %s and [%s].\n", >>> - bad_hex, bad_hex, good_hex); >>> - >>> + if (!strcmp(name_bad, "bad")) { >>> + fprintf(stderr, "The merge base %s is bad.\n" >>> + "This means the bug has been fixed " >>> + "between %s and [%s].\n", >>> + bad_hex, bad_hex, good_hex); >>> + } >> >>You need an "else" here. Maybe it comes later, but as a reviewer, I want >>to check that you did not forget it now (because I don't trust myself to >>remember that it must be added later). > > Should I put an else {} with nothing in beetween? What you want to avoid is a senario where the if branch is not taken silently in the future. Two ways to avoid that: if (!strcmp(name_bad, "bad")) { // special case for good/bad } else { die("BUG: terms %s/%s not managed", name_bad, name_good); } Think of someone trying to debug the code later: if you encounter a die("BUG: ..."), gdb will immediately tell you what's going one. Debugging the absence of something is much more painful. Alternatively: if (!strcmp(name_bad, "bad")) { // special case for good/bad } else { fprintf("very generic message not saying \"which means that ...\""); } In both cases, adding a new pair of terms should look like if (!strcmp(name_bad, "bad")) { // special case for good/bad +} else if(!strcmp(name_bad, "")) { + // special case for } else { die("BUG: terms %s/%s not managed", name_bad, name_good); } I have a slight preference for the "else" with a generic message. It will be dead code for now, but may turn useful if we ever allow arbitrary pairs of terms. > >>> + name_bad = "bad"; >>> + name_good = "good"; >>> + } else { >>> + strbuf_getline(&str, fp, '\n'); >>> + name_bad = strbuf_detach(&str, NULL); >>> + strbuf_getline(&str, fp, '\n'); >>> + name_good = strbuf_detach(&str, NULL); >>> + } >> >>I would have kept just >> >> name_bad = "bad"; >> name_good = "good"; >> >>in this patch, and introduce BISECT_TERMS in a separate one. > > Should not I introduce BISECT_TERMS in bisect.c and git-bisect.sh > with the same commit? Make sure you have a bisectable history. If you use two commits, you should make sure that the intermediate step is consistant. If the change is small enough, it's probably better to have a single commit. No strong opinion on that (at least not before I see the code). > I did some rebase though and now name_bad and name_good appears in the > first commit, and read_bisect_terms in the second. > >>> --- a/git-bisect.sh >>> +++ b/git-bisect.sh >>> @@ -77,6 +77,7 @@ bisect_start() { >>> orig_args=$(git rev-parse --sq-quote "$@") >>> bad_seen=0 >>> eval='' >>> + start_bad_good=0 >>> if test "z$(git rev-parse --is-bare-repository)" != zfalse >>> then >>> mode=--no-checkout >>> @@ -101,6 +102,9 @@ bisect_start() { >>> die "$(eval_gettext "'\$arg' does not appear to be a valid revision")" >>> break >>> } >>> + >>> + start_bad_good=1 >>> + >> >>Why do you need this variable? It seems to me that you are hardcoding >>once more that terms can be either "good/bad" or "old/new", which you >>tried to eliminate from the previous round. > > I answered to Junio on this too, it is because our function which create > the bisect_terms file is not called after > 'git bisect start bad_rev good_rev'. Then the variable name is not good. If the variable is there to mean "we did not define the terms yet", then bisect_terms_defined={0,1} would be much better (probably not the best possible name, though). >>> +bisect_voc () { >>> + case "$1" in >>> + bad) echo "bad" ;; >>> + good) echo "good" ;; >>> + esac >>> +} >> >>It's weird to have these hardcoded "bad"/"good" when you already have >>BISECT_TERMS in the same patch. > > bisect_voc is used to display what commands the user can do, thus the > builtins tags. I did not find a way to not hardcode them. Well, if you really want to say good/bad, then using just good/bad would be simpler than $(bisect_voc good)/$(bisect_voc bad), no? bisect_voc is just the identitity function (or returns the empty string for input other than good/bad). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ```