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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 26, 2018, 22:20 UTC
Message-ID: <xmqqa7rhi40f.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAP8UFD1TeC4czp_8HCRw5CtjGO78A8gRezw_xspnm4MXuhQswg@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

> Obviousness is often not the same for everybody.

... which you just learned---what you thought obvious turns out to be not so obvious after all, so you adjust to help your readers.

Show 11 quoted lines

>> In this particular case it even feels as if this test is not even testing
>> what it should test at all:
>>
>> - it should verify that all of the commits in the first parent lineage are
>>   part of the list
>
> It does that.
>
>> - it should verify that none of the other commits are in the list
>
> It does that too.

But the point is it does a lot more by insisting exact output. For example, the version I reviewed had a two "expected output", and said that the actual output must match either one of them. I guess it was because there were two entries with the same distance and we cannot rely on which order they appear in the result? If a test history gained another entry with the same distance, then would we need 6 possible expected output because we cannot rely on the order in which these three come out?

That was the only thing I was complaining about. Dscho gave me too much credit and read a lot more good things than what I actually meant to say ;-).

>> And that is really all there is to test.

Another is that "rev-list --bisect-all" promises that the entries are ordered by the distance value. So taking the above three points, perhaps

	cat >expect <<EOF &&
	... as written in one of the expect list in Tiago's patch
	EOF

	# Make sure we have the same entries, nothing more, nothing less
	git rev-list --bisect-all $other_args >actual &&
	sort actual >actual.sorted &&
	sort expect >expect.sorted &&
	test_cmp expect.sorted actual.sorted

	# Make sure the entries are sorted in the dist order
	sed -e 's/.*(dist=\([1-9]*[0-9]\)).*/\1/' actual >actual.dists &&
	sort actual.dists >actual.dists.sorted &&
	test_cmp actual.dists.sorted actual.dists

is what I would have expected.