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