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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Chris,

On Tue, 26 Jun 2018, Christian Couder wrote:

> On Mon, Jun 25, 2018 at 7:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Tiago Botelho <tiagonbotelho@gmail.com> writes:
> >
> >> +test_expect_success "--bisect-all --first-parent" '
> >> +cat >expect1 <<EOF &&
> >> +$(git rev-parse CC) (dist=2)
> >> +$(git rev-parse EX) (dist=1)
> >> +$(git rev-parse D) (dist=1)
> >> +$(git rev-parse FX) (dist=0)
> >> +EOF
> >> +
> >> +cat >expect2 <<EOF &&
> >> +$(git rev-parse CC) (dist=2)
> >> +$(git rev-parse D) (dist=1)
> >> +$(git rev-parse EX) (dist=1)
> >> +$(git rev-parse FX) (dist=0)
> >> +EOF
> >> +
> >> +git rev-list --bisect-all --first-parent FX ^A >actual &&
> >> +  ( test_cmp expect1 actual || test_cmp expect2 actual )
> >> +'
> >
> > I hate to say this, but the above looks like a typical
> > unmaintainable mess.
> >
> > What happens when you or somebody else later needs to update the
> > graph to be tested to add one more commit (or even more)?  Would it
> > be enough to add another "rev-parse" plus "dist=X" line in both
> > expects?  Or do we see a trap for combinatorial explosion that
> > requires us to add new expect$N?
> 
> What about the following then:
> 
> test_dist_order () {
>     file="$1"
>     n="$2"
>     while read -r hash dist
>     do
>         d=$(echo "$dist" | sed -e "s/(dist=\(.*\))/\1/")
>         case "$d" in
>             ''|*[!0-9]*) return 1 ;;
>             *) ;;
>         esac
>         test "$d" -le "$n" || return 1
>         n="$d"
>     done <"$file"
> }
> 
> test_expect_success "--bisect-all --first-parent" '
>     cat >expect <<EOF &&
> $(git rev-parse CC) (dist=2)
> $(git rev-parse EX) (dist=1)
> $(git rev-parse D) (dist=1)
> $(git rev-parse FX) (dist=0)
> EOF
>     sort expect >expect_sorted &&
>     git rev-list --bisect-all --first-parent FX ^A >actual &&
>     sort actual >actual_sorted &&
>     test_cmp expect_sorted actual_sorted &&
>     test_dist_order actual 2
> '
> 
> This feels overkill to me, but it should scale if we ever make more
> complex tests.

I *think* that you misunderstand Junio. At least when I read this:

> $(git rev-parse CC) (dist=2)
> $(git rev-parse EX) (dist=1)
> $(git rev-parse D) (dist=1)
> $(git rev-parse FX) (dist=0)

I go: Huh?!?!?!?!?!

What's CC? Is it Christian Couder? Creative Commons? Crudely Complex?

The point, for me, is: if this test fails, at some stage in the future,
for any reason, it will be a major pain to even dissect what the test is
supposed to do. This is no good. And you can do better. A lot better. You
can write the test in a way that the head of a reader does not hurt, and
at the same time it is still obvious what it does, and obvious that it
does the right thing.

One thing that makes the brain stumble for certain is when you deviate
from the conventions, especially when it is for no good reason at all. In
this case (and I am not sure why you, as a long-time contributor, did not
spot this before public review):

- the titles of the test cases leave a lot of room for improvement,

- the lines are too long,

- the convention to end the `test_expect_success` line with an opening
  single quote is not heeded,

- the graph is definitely written in an unnecessarily different format
  than in the same test script!!!

- speaking of the graph: there is a perfectly fine commit graph already.
  Why on earth is it not reused?

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 should verify that none of the other commits are in the list

And that is really all there is to test. You can even write that in a much
easier way:

-- snip --
test_expect_success '--first-parent --bisect-all lists correct revs' '
	git rev-list --first-parent --bisect-all F..E >revs &&
        # E and e1..e8 need to be shown, F and f1..f4 not
	test_line_count = 9 revs &&
	for rev in E e1 e2 e3 e4 e5 e6 e7 e8
	do
		grep "^$(git rev-parse $rev) " revs || {
			echo "$rev not shown" >&2 &&
			return 1
		}
	done
'
-- snap --

To test more precisely for the order or the distance would be both
overkill and likely to be unreadable.

To test `--bisect-vars` here would be excessive, as the part that handles
that particular option is not even touched. All that is touched is the
logic in the bisect algorithm in conjunction with --first-parent. And that
is all that should be tested here.

With a test like the one I outlined above, I only have one more gripe
about the patch: the commit message does nothing to explain this part of
the diff:

+                               if ((bisect_flags & BISECT_FIRST_PARENT)) {
+                                       if (weight(q) < 0)
+                                               q = NULL;
+                                       break;
+                               }

And I would really, really like that to be explained in the commit
message. Because to me, it is completely opaque why this needs to be here.
The rest of the diff is pretty obvious, though.

Ciao,
Johannes