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

On Fri, 29 Jun 2018 at 12:21, Johannes Schindelin
[off-list ref] wrote:

Hi Junio,

On Thu, 28 Jun 2018, Junio C Hamano wrote:

quoted

What I meant by "many separte grep calls" was to contrast these two
approaches:

 * Have one typical output spelled out as "expect", take an output
   from an actual run into "actual", make them comparable and then
   do a compare (which does not use grep; it uses sort in the
   "making comparable" phase)

 * Not have any typical output, take an output from an actual run,
   and have _code_ that inspects the output encode the rule over the
   output (e.g. "these must exist" is inspected with many grep
   invocations)

Two things the "output must have 9 entries, and these 9 must be
mentioned" we see at the beginning of this message does not verify
are (1) exact dist value given to each of these entries and (2) the
order in which these entries appear in the output.  The latter is
something we document, and the test should cover.  The former does
not have to be cast in stone (i.e. I do not think it does not make a
difference to label the edge commits with dist=1 or dist=0 as long
as everything is consistent), but if there is no strong reason to
keep it possible for us to later change how the numbers are assigned,
I am OK if the test cast it in stone.

Another reason (other than "many separate invocation of grep") to
favor the former approach (i.e. use real-looking expected output,
munge it and the real output into comparable forms and then compare)
is that it is easier to see what aspect of the output we care (and
we do not care) about.

It is harder to see the omission of exact dist value and ordering of
entries in the outpu in the latter approach, and more importantly,
know if the omission was deliberate (e.g. it was merely an example)
or a mere mistake.

With "using a real-looking expected output, make it and the actual
output comparable and then compare" approach, the aspects in the
output we choose not to care about will show in the "make them
comparable" munging.  If we do not care the exact dist values, there
would be something like s/dist=[0-9]*/dist=X/ to mask the exact
value before making the two comparable to see that the expect and
the actual files have the same entries.  If we still do care about
the entries are ordered by the dist values, there would be something
that sorts the entries with the actual dist values before doing that
masking to ensure if the order is correct.

The problem here is of course that you *cannot* set the exact output
in stone, because of sorting instabilities.

So you have to play tricks to sort (twice, with different keys) the
expected output and the actual output, to verify that all the expected
commits are listed (and only those) and to verify that they are ordered by
the distance, in descending order.

And this trick, while it still makes the test correct and stable and yadda
yadda, *also* makes this trick a lot less readable than my version. And
therefore makes it more difficult to verify that your test actually does
what it is supposed to do.

Ciao,
Dscho

Hello,

first of all I would like to thank all the feedback provided in this patch it
has truly helped me progress on my first contribution to git.

After looking through Junio's and Johannes's suggestions I believe that
the *only* test we should add will be something like:

-- snip --
test_expect_success '--first-parent --bisect-all lists correct revs' '
git rev-list --first-parent --bisect-all F..E >revs &&
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 &&
sed -e "s/.*(dist=\([0-9]*\)).*/\1/" revs >actual.dists &&
sort -r actual.dists >actual.dists.sorted &&
test_cmp actual.dists.sorted actual.dists
'
-- snap --

The only change I had to make was use -r in sort to
revert the sorting in `sort` otherwise we get it in
ascending order but the revs are provided in descending order.

Kind regards,
Tiago