Re: [PATCH 01/27] bisect: Fixup test rev-list-bisect/02

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 19, 2021, 16:31 UTC
Message-ID: <nycvar.QRO.7.76.6.2111191653390.63@tvgsbejvaqbjf.bet>
In-Reply-To: <CAPx1GvdKuBmxN0XM3PKJO+0V=P3OoyB4VDzmqshv7N+3vFF8gw@mail.gmail.com>

Hi,

On Thu, 18 Nov 2021, Chris Torek wrote:

Show 16 quoted lines

> On Thu, Nov 18, 2021 at 10:38 AM Jan Kara <jack@suse.cz> wrote:
> > diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
> > index b95a0212adff..48db52447fd3 100755
> > --- a/t/t6002-rev-list-bisect.sh
> > +++ b/t/t6002-rev-list-bisect.sh
> > @@ -247,8 +247,9 @@ test_expect_success 'set up fake --bisect refs' '
> >  test_expect_success 'rev-list --bisect can default to good/bad refs' '
> >         # the only thing between c3 and c1 is c2
> >         git rev-parse c2 >expect &&
> > -       git rev-list --bisect >actual &&
> > -       test_cmp expect actual
> > +       git rev-parse b2 >>expect &&
> > +       actual=$(git rev-list --bisect) &&
> > +       grep &>/dev/null $actual expect
>
> `&>` is a bashism; you need `>/dev/null 2>&1` here for general portability.

More importantly, why do you suppress the output in the first place? This will make debugging breakages harder.

Let's just not redirect the output?

I do see a more structural problem here, though. Throughout the test suite, it is our custom to generate files called `expect` with what we consider the expected output, and then generate `actual` with the actual output. We then compare the results and complain if they are not identical.

With this patch, we break that paradigm. All of a sudden, `expect` is not at all the expected output anymore, but a haystack in which we want to find one thing.

And even after reading the commit message twice, I am unconvinced that b2 (whatever that is) might be an equally good choice. I become even more doubtful about that statement when I look at the code comment at the beginning of the test case:

	# the only thing between c3 and c1 is c2

So either this code comment is wrong, or the patch. And if the code comment is wrong, I would like to know when it became wrong, and how, and why it slipped through our review.

Ciao, Dscho