Re: [PATCH] git bisect old/new
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 12, 2012, 05:43 UTC
- Message-ID
- <7v1ull5bld.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <CAP8UFD1mgKNQRZpuqYgyL__DGpoZ3cOGJC0amoqWT+1Bn_0iTg@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> 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.
Show 7 quoted lines
>> + git bisect new [<rev>] >> + git bisect old [<rev>...] > > maybe: > > git bisect (bad|new) [<rev>] > git bisect (good|old) [<rev>...]
Definitely.
Show 12 quoted lines
>> @@ -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.
Show 12 quoted lines
>> @@ -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?
Show 17 quoted lines
>> @@ -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...
Show 15 quoted lines
>> >> /* >> - * "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.
Show 8 quoted lines
>> @@ -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.
Show 12 quoted lines
>> +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.
Show 11 quoted lines
>> @@ -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.
Show 8 quoted lines
>> @@ -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.
Show 9 quoted lines
>> @@ -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).
Show 11 quoted lines
>> @@ -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