Christian Couder <christian.couder@xxxxxxxxx> writes:
Thanks for comments.
> If you used some design that was discussed on the mailing list or if
> there have been relevant discussions on the mailing list, it would be
> nice to have links to the email thread in the commit message.
Perhaps.
>> + git bisect new [<rev>]
>> + git bisect old [<rev>...]
>
> maybe:
>
> git bisect (bad|new) [<rev>]
> git bisect (good|old) [<rev>...]
Definitely.
>> @@ -104,6 +106,44 @@ For example, `git bisect reset HEAD` will leave you on the current
>> bisection commit and avoid switching commits at all, while `git bisect
>> reset bisect/bad` will check out the first bad revision.
>>
>> +Alternative research: bisect new and bisect old
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +If you are not looking for a regression but for a change of a given
>> +property, you can use:
>
> I would rather say:
> ...
Good.
>> @@ -403,9 +406,10 @@ struct commit_list *find_bisection(struct commit_list *list,
>> static int register_ref(const char *refname, const unsigned char *sha1,
>> int flags, void *cb_data)
>> {
>> - if (!strcmp(refname, "bad")) {
>> + if (!strcmp(refname, bisect_term_bad)) {
>> current_bad_sha1 = sha1;
>> - } else if (!prefixcmp(refname, "good-")) {
>> + } else if (!prefixcmp(refname, "good-") ||
>> + !prefixcmp(refname, "old-")) {
>
> I don't like very much "good" and "old" to be hardcoded here.
Really?
>> @@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)
>> if (is_expected_rev(current_bad_sha1)) {
>> char *bad_hex = sha1_to_hex(current_bad_sha1);
>> char *good_hex = join_sha1_array_hex(&good_revs, ' ');
>> + if (!strcmp(bisect_term_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);
>> + } else {
>> + fprintf(stderr, "The merge base %s is new.\n"
>> + "The property has changed "
>> + "between %s and [%s].\n",
>> + bad_hex, bad_hex, good_hex);
>> + }
>
> I don't like very much "new" to be harcoded here too.
Why not? It is not like we will be adding any more synonym pair
beyond good/bad, so...
>>
>> /*
>> - * "check_merge_bases" checks that merge bases are not "bad".
>> + * "check_merge_bases" checks that merge bases are not "bad" (resp. "new").
>> *
>> - * - If one is "bad", it means the user assumed something wrong
>> + * - If one is "bad" (resp. "new"), it means the user assumed something wrong
>> * and we must exit with a non 0 error code.
>> - * - If one is "good", that's good, we have nothing to do.
>> + * - If one is "good" (resp. "old"), that's good, we have nothing to do.
>> * - If one is "skipped", we can't know but we should warn.
>> * - If we don't know, we should check it out and ask the user to test.
>> */
>
> I am not sure changing the comments is worth it...
I think it is probably a good idea to cast in stone that we support
two pairs, i.e. good/bad or old/new. I would have said "or" instead
of "resp." above, though.
>> @@ -889,6 +901,30 @@ 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.
>
> I am not sure saying "it can be bad/good or new/old" adds anything.
It makes it clear that we are not allowing arbitrary pair of words
to substitute the good/bad pair, which is a plus.
>> +void read_bisect_terms(void)
>> +{
>> + struct strbuf str = STRBUF_INIT;
>> + const char *filename = git_path("BISECT_TERMS");
>> + FILE *fp = fopen(filename, "r");
>> +
>> + if (!fp)
>> + die_errno("Could not open file '%s'", filename);
>
> This is not very compatible with older git versions.
> I know that it's kind of strange to upgrade git in the middle of a
> bisection but why not just use "bad"/"good" if there is no file?
Good thinking.
>> @@ -898,6 +934,8 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
>> */
>> int bisect_next_all(const char *prefix, int no_checkout)
>> {
>> + read_bisect_terms();
>> +
>> struct rev_info revs;
>> struct commit_list *tried;
>> int reaches = 0, all = 0, nr, steps;
>
> We put all declarations at the beginning of functions.
Good eyes.
>> @@ -22,7 +22,15 @@ git bisect replay <logfile>
>> git bisect log
>> show bisect log.
>> git bisect run <cmd>...
>> - use <cmd>... to automatically bisect.
>> + use <cmd>... to automatically bisect
>
> Why this change?
To end a sentence with a full-stop?
> But anyway if possible I'd rather have:
>
> git bisect (bad|new) [<rev>]
> git bisect (good|old) [<rev>...]
Yes.
>> @@ -32,6 +40,8 @@ OPTIONS_SPEC=
>>
>> _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
>> _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
>> +NEW="bad"
>> +OLD="good"
>
> Why not BISECT_BAD_TERM/BISECT_GOOD_TERM instead of NEW/OLD?
> It should be consistent with bisect.c
It's kind of too long. Isn't BISECT_GOOD vs BISECT_BAD good enough
(and if so make bisect.c consistent with it).
>> @@ -184,8 +210,8 @@ bisect_write() {
>> rev="$2"
>> nolog="$3"
>> case "$state" in
>> - bad) tag="$state" ;;
>> - good|skip) tag="$state"-"$rev" ;;
>> + bad|new) tag="$state" ;;
>> + good|skip|old) tag="$state"-"$rev" ;;
>
> Why not "$BISECT_TERM_BAD" instead of "bad|new" and
> "$BISECT_TERM_GOOD|skip" instead of "good|skip|old"?
If the point is to make sure "git bisect good" will error out when
we are in new/old mode, I agree (and also the other case/esac in the
remainder of the patch that allows you feed bad and new mixed).
These case arms look indented in a funny way, but is it only because
of e-mail quoting, by the way?
>> *) die "$(eval_gettext "Bad bisect_write argument: \$state")" ;;
>> esac
--
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