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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 10, 2015, 07:55 UTC
Message-ID: <vpqr3pkatfy.fsf@anie.imag.fr>
In-Reply-To: <1809967391.331411.1433920237669.JavaMail.zimbra@ensimag.grenoble-inp.fr>

Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr> writes:

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

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, "<new-term>")) {
+	// special case for <new-term>
 } 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.

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

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

Show 28 quoted lines

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

Show 12 quoted lines

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