Re: [PATCH 3/4] bisect: simplify the add of new bisect terms
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Jun 9, 2015, 07:01 UTC
- Message-ID
- <vpqbngpl5zu.fsf@anie.imag.fr>
- In-Reply-To
- <1433794930-5158-3-git-send-email-antoine.delaite@ensimag.grenoble-inp.fr>
> Subject: Re: [PATCH 3/4] bisect: simplify the add of new bisect terms
s/add/addition/
Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr> 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?
Show 12 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).
Show 7 quoted lines
> @@ -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)
Show 9 quoted lines
> + * 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.
Show 8 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.
Show 17 quoted lines
> --- 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.
Show 15 quoted lines
> +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 :
Show 20 quoted lines
> + 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/