On Tue, Jun 12, 2012 at 1:43 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> 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?
I tend to agree. But I like more generic code and less hard-coded in
almost all cases.
> >> @@ -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...
Previously we discussed using yes/no, among others.
http://permalink.gmane.org/gmane.comp.version-control.git/182496
>
> >>
> >> /*
> >> - * "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?
I think you read this backwards. This change removes the full-stop.
Phil
--
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