git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: workingjubilee@gmail.com
Cc: jonathantanmy@google.com, git@vger.kernel.org,
	christian.couder@gmail.com, johannes.schindelin@gmx.de,
	gitster@pobox.com
Subject: Re: [PATCH v7 1/1] Implement rev-list --bisect* --first-parent
Date: Thu,  7 Nov 2019 10:56:30 -0800	[thread overview]
Message-ID: <20191107185630.253388-1-jonathantanmy@google.com> (raw)
In-Reply-To: <CAPNHn3qj7v=xu1ogG4q9NrHvp1zfZFvUWQKJqf0DJcavxgsz6Q@mail.gmail.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.

      reply	other threads:[~2019-11-07 18:56 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-05  5:21 [Outreachy] [PATCH v7 0/1] Revenge of --bisect --first-parent workingjubilee
2019-11-05  5:21 ` [PATCH v7 1/1] Implement rev-list --bisect* --first-parent workingjubilee
2019-11-05 23:04   ` Jonathan Tan
2019-11-06  2:42     ` Junio C Hamano
2019-11-06 11:30       ` Johannes Schindelin
2019-11-06 21:36       ` Jonathan Tan
2019-11-07  3:35         ` Junio C Hamano
2019-11-07  5:07         ` Jubilee Young
2019-11-07 16:00     ` Jubilee Young
2019-11-07 18:56       ` Jonathan Tan [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191107185630.253388-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=workingjubilee@gmail.com \
    --subject='Re: [PATCH v7 1/1] Implement rev-list --bisect* --first-parent' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).