Re: [PATCH v6] Implement --first-parent for git rev-list --bisect
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 28, 2018, 21:24 UTC
- Message-ID
- <xmqqwosadw0t.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <xmqqbm9mfcf4.fsf@gitster-ct.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
> Something like the following, perhaps?
Having said all that.
Show 10 quoted lines
> +# See the drawing near the top --- e4 is in the middle of the first parent chain > +printf "%s\n" e4 | > +test_output_expect_success '--bisect --first-parent' ' > + git rev-list --bisect --first-parent E ^F > +' > + > +printf "%s\n" E e1 e2 e3 e4 e5 e6 e7 e8 | > +test_output_expect_success "--first-parent" ' > + git rev-list --first-parent E ^F > +'
The above two are probably not controversial.
Show 10 quoted lines
> +test_output_expect_success '--bisect-vars --first-parent' ' > + git rev-list --bisect-vars --first-parent E ^F > +' <<-EOF > + bisect_rev='e5' > + bisect_nr=4 > + bisect_good=4 > + bisect_bad=3 > + bisect_all=9 > + bisect_steps=2 > +EOF
Perhaps this one isn't either.
Show 18 quoted lines
> +test_expect_success '--bisect-all --first-parent' ' > + cat >expect <<-EOF && > + $(git rev-parse tags/e5) (dist=4) > + $(git rev-parse tags/e4) (dist=4) > + $(git rev-parse tags/e6) (dist=3) > + $(git rev-parse tags/e3) (dist=3) > + $(git rev-parse tags/e7) (dist=2) > + $(git rev-parse tags/e2) (dist=2) > + $(git rev-parse tags/e8) (dist=1) > + $(git rev-parse tags/e1) (dist=1) > + $(git rev-parse tags/E) (dist=0) > + EOF > + > + # Make sure we have the same entries, nothing more, nothing less > + git rev-list --bisect-all --first-parent E ^F >actual && > + sort actual >actual.sorted && > + sort expect >expect.sorted && > + test_cmp expect.sorted actual.sorted &&
By sorting both, we attempt allow them to come out in any random order when sorting done by --bisect-all gets tiebroken differently between commits with the same dist value. As Johannes said, this is a bit too strict to insist that E *must* get dist 0, when the only thing we may care about are e2 and e7 get the same dist value, which is larger than the dist value given to E, etc. If we wanted to ease the over-strictness, we could omit " (dist=N)" from the 'expect' file, and then
sed -e 's/ (dist=[0-9]*)$//' actual | sort >actual.sorted && sort expect >expect.sorted && test_cmp expect.sorted actual.sorted &&
to compare only the object names.
This over-strictness would have caught a bug in Git if we reverted this new feature (i.e. "rev-list --first-parent --bisect-all" does not refuse to work---it just does not do what we expect), which gives distance of -1 in the output (!). From that point of view, I think it is also OK for the test to be spelling the values out like the above.
> + # 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 &&
I forgot to mention but I added "-n" here to make sure we numerically sort the list of distance values. Also you had a bogus regexp to catch digits inherited from "something like this" I showed in an older discussion (I think it is sufficient to say [0-9]* here).
The above makes sure that commits that have larger distance number are listed earlier in the output, and we do not care if which one of e4 or e5, both of which have distant number 4, is shown first---we'd be happy as long as all the ones at distance 4 are shown before the others at smaller distance number. So with this in place, I think we can omit the exact distance number from the earlier part of this test, but as I said, it would have caught the bug that produces a negative distance value in the output, so I am still on the fence, leaning a bit toward being stricter than necessary.
> + test_cmp actual.dists.sorted actual.dists > +' > + > test_done