Re: [PATCH v6] Implement --first-parent for git rev-list --bisect

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Something like the following, perhaps?

Having said all that.

> +# 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.

> +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.


> +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