Antoine Delaite <antoine.delaite@xxxxxxxxxxxxxxxxxxxxxxx> 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, "<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.
>
>>> + 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/
--
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