Re: [PATCH v7 1/1] Implement rev-list --bisect* --first-parent

[Jonathan Tan <jonathantanmy@google.com>]

> > > @@ -964,7 +981,12 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
> > >
> > >       bisect_common(&revs);
> > >
> > > -     find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr);
> > > +     if (skipped_revs.nr)
> > > +             bisect_flags |= BISECT_FIND_ALL;
> > > +     if (revs.first_parent_only)
> > > +             bisect_flags |= BISECT_FIRST_PARENT;
> > > +
> > > +     find_bisection(&revs.commits, &reaches, &all, bisect_flags);
> > >       revs.commits = managed_skipped(revs.commits, &tried);
> > >
> > >       if (!revs.commits) {
> >
> > I don't see how revs.first_parent_only is ever set in this function. If
> > it's never set, undo this change, since this code is never executed.
> 
> In this function, 

(1)
> we call bisect_rev_setup() using the revs struct we
> made,

(2)
> which then calls setup_revisions() on the revs,

(3)
> which appears to
> call handle_revision_opt() with that struct again,

(4)
> which finally is
> allowed to set revs->first_parent_only = 1; in revision.c.
> 
> So unless I am horribly misreading something, we do set it.

Thanks for performing this analysis. Working backwards:

(4) handle_revision_opt() in revision.c sets revs->first_parent_only
only if argv[0] is "--first-parent".

(3) setup_revisions() calls handle_revision_opt() in a loop over its
argv parameter.

(2) bisect_rev_setup() initializes rev_argv (with ARGV_ARRAY_INIT, a
blank array). Then, it pushes "bisect_rev_setup", an OID in
"bad_format", some OIDs in "good_format", and possibly some paths. Then
it calls setup_revisions() with rev_argv as the argv parameter. Notice
that there is no "--first-parent" sent at all.

> > > +test_expect_success '--bisect-all --first-parent returns correct order' '
> > > +     git rev-list --bisect-all --first-parent E ^F >actual &&
> > > +
> > > +     # Make sure the entries are sorted in the dist order
> > > +     sed -e "s/.*dist=\([0-9]\).*/\1/" actual >actual.dists &&
> > > +     sort -r -n actual.dists >actual.dists.sorted &&
> > > +     test_cmp actual.dists.sorted actual.dists
> > > +'
> > > +
> > > +# NEEDSWORK: this test could afford being hardened against other
> > > +# changes in the same file.
> > > +test_expect_success '--bisect-all --first-parent compares correctly' '
> > > +     cat >expect <<-EOF &&
> > > +     $(git rev-parse tags/e5) (tag: e5, dist=4)
> > > +     $(git rev-parse tags/e4) (tag: e4, dist=4)
> > > +     $(git rev-parse tags/e6) (tag: e6, dist=3)
> > > +     $(git rev-parse tags/e3) (tag: e3, dist=3)
> > > +     $(git rev-parse tags/e7) (tag: e7, dist=2)
> > > +     $(git rev-parse tags/e2) (tag: e2, dist=2)
> > > +     $(git rev-parse tags/e8) (tag: e8, dist=1)
> > > +     $(git rev-parse tags/e1) (tag: e1, dist=1)
> > > +     $(git rev-parse tags/E) (tag: E, dist=0)
> > > +EOF
> > > +
> > > +git rev-list --bisect-all --first-parent E ^F >actual &&
> > > +     sort actual >actual.sorted &&
> > > +     sort expect >expect.sorted &&
> > > +     test_cmp expect.sorted actual.sorted
> > > +'
> >
> > I think these 2 tests can be combined, since the latter also checks the
> > dists. Also, correct the indentation of the latter test.
> 
> I understand they are similar tests, but... Is there a tangible
> reason for combining them? Especially when their logic can
> live and breathe completely separately, compacting tests
> reduces the resolution of the information we can extract from failure.
> 
> I would rather simply drop one and preserve 1 test = 1 data point.

In general, I agree that 1 test should represent 1 data point, and the
reason for that being (as you said) the resolution of the information we
can extract from failure. But here, the resolution is diminished if we
don't combine the tests. Here, if either test fails, because of the
postprocessing we've had to do on the output, we lose signal. But if we
had the combined test, we wouldn't.