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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, 

Thanks for the review, 
(sorry if you received this twice)

Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx> 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. 
--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]