Hi, On Thu, 18 Nov 2021, Chris Torek wrote: > On Thu, Nov 18, 2021 at 10:38 AM Jan Kara <jack@xxxxxxx> 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
