Re: [PATCH v6] Implement --first-parent for git rev-list --bisect
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 28, 2018, 20:45 UTC
- Message-ID
- <xmqqbm9mfcf4.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <xmqqy3cqfi8c.fsf@gitster-ct.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >> I would have preferred to reuse the already existing commits generated in >> the `setup` phase rather than generating yet another batch, and to not >> introduce an inconsistent way to present a commit graph (compare your >> diagram with the one in >> https://github.com/git/git/blob/v2.18.0/t/t6002-rev-list-bisect.sh#L64-L90 >> i.e. *in the same file*) > > As I already said in the previous round, I do agree with these. > That is, ... > >>> >>> +# We generate the following commit graph: >>> +# >>> +# B ------ C > ... the above graph construction should not be necessary. An > earlier part of t6002 would have already created a history of > suitable shape to use for writing the following tests.
Something like the following, perhaps? I have a feeling that use of test-output-expect-success is trying to be too cute without making the result easier to read, and also makes it harder to debug the test script when it does not work as expected (e.g. you need to see where the output from the actual command is stored by going to the definition of the shell function), and would have preferred to see these three tests written without it (and instead using explicit 'expect' vs 'actual' comparison), but at least the patch below shows what I meant when I suggested updating the tests to reuse the existing history (I do not speak for Johannes, but I am guessing we are on the same page on this one).
Thanks.
t/t6002-rev-list-bisect.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh index a661408038..d27d0087d6 100755 --- a/t/t6002-rev-list-bisect.sh +++ b/t/t6002-rev-list-bisect.sh @@ -263,4 +263,50 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' ' test_cmp expect.sorted actual.sorted ' +# 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 +' + +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 + +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 && + # Make sure the entries are sorted in the dist order + sed -e "s/.*(dist=\([1-9]*[0-9]\)).*/\1/" actual >actual.dists && + sort -r -n actual.dists >actual.dists.sorted && + test_cmp actual.dists.sorted actual.dists +' + test_done