> Subject: Re: [PATCH 3/4] bisect: simplify the add of new bisect terms
s/add/addition/
Antoine Delaite <antoine.delaite@xxxxxxxxxxxxxxxxxxxxxxx> writes:
> +static const char *name_bad;
> +static const char *name_good;
Same remark as PATCH 2.
> } else if (starts_with(refname, "good-")) {
Did you forget this one?
> -
> - 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).
> @@ -890,6 +894,31 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
> }
>
> /*
> + * The terms used for this bisect session are stocked in
> + * BISECT_TERMS: it can be bad/good or new/old.
> + * We read them and stock them to adapt the messages
s/stock/store/ (two instances)
> + * accordingly. Default is bad/good.
> + */
> +void read_bisect_terms(void)
> +{
> + struct strbuf str = STRBUF_INIT;
> + const char *filename = git_path("BISECT_TERMS");
> + FILE *fp = fopen(filename, "r");
> +
> + if (!fp) {
I think you would want to error out if errno is not ENOENT.
> + 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.
> --- 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.
> + if test $start_bad_good -eq 1 -a ! -s "$GIT_DIR/BISECT_TERMS"
Avoid test -a (not strictly POSIX, and sometimes ambiguous). Use
test ... && test ...
instead.
> + 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.
> - gettextln "You need to give me at least one good and one bad revision.
> -(You can use \"git bisect bad\" and \"git bisect good\" for that.)" >&2
> + gettextln "You need to give me at least one $(bisect_voc bad) and one $(bisect_voc good) revision.
> +(You can use \"git bisect $(bisect_voc bad)\" and \"git bisect $(bisect_voc good)\" for that.)" >&2
I suspect you broke the i18n here too.
> +get_terms () {
> + if test -s "$GIT_DIR/BISECT_TERMS"
> + then
> + NAME_BAD="$(sed -n 1p "$GIT_DIR/BISECT_TERMS")"
> + NAME_GOOD="$(sed -n 2p "$GIT_DIR/BISECT_TERMS")"
> + fi
> +}
> +
> +check_and_set_terms () {
> + cmd="$1"
> + case "$cmd" in
> + bad|good)
> + if test -s "$GIT_DIR/BISECT_TERMS" -a "$cmd" != "$NAME_BAD" -a "$cmd" != "$NAME_GOOD"
> + then
> + die "$(eval_gettext "Invalid command : you're currently in a \$NAME_BAD/\$NAME_GOOD bisect.")"
No space before :
> + 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.
--
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