Re: [PATCH] git bisect old/new

[Phil Hord <phil.hord@gmail.com>]

On Tue, Jun 12, 2012 at 1:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
>
> >> + 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