[PATCH 2/4] bisect: replace hardcoded "bad|good" by variables

From: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
Date: Jun 10, 2015, 07:10 UTC
Message-ID: <1809967391.331411.1433920237669.JavaMail.zimbra@ensimag.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 <Matthieu.Moy@grenoble-inp.fr> 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.

Show 16 quoted lines

>> - 
>> - 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?

Show 15 quoted lines

>> + 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.

Show 21 quoted lines

>> --- 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'.

Show 7 quoted lines

>> + 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'.

Show 23 quoted lines

>> + 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.