Re: [PATCH 2/4] t6030: explicitly test for bisection cleanup

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 8, 2016, 08:17 UTC
Message-ID: <CAPig+cSVmA0xbs8AFzeOno15=-rsjd_bGGBt_h9ujRp2g+-d0A@mail.gmail.com>
In-Reply-To: <CAFZEwPMpa1BHTP5BEqBHpYiab9G32tVXvLTZ95xhVeOcObCCBg@mail.gmail.com>

On Wed, Jun 8, 2016 at 4:07 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:

Show 20 quoted lines

> On Wed, Jun 8, 2016 at 4:51 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
>>> @@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs in any order' '
>>> +test_expect_success 'git bisect reset cleans bisection state properly' '
>>> +       git bisect reset &&
>>> +       git bisect start &&
>>> +       git bisect good $HASH1 &&
>>> +       git bisect bad $HASH4 &&
>>> +       git bisect reset &&
>>> +       test -z "$(git for-each-ref "refs/bisect/*")" &&
>>
>> I wonder if this would be more easily read as:
>>
>>     git for-each-ref "refs/bisect/*" >actual &&
>>     test_must_be_empty actual &&
>
> I just tried to imitate what the test t6030 previously had (a lot of
> occurrences). Should I still change it to your specified format?
> Should I also change the others as a side cleanup "while I am at it"?

No, if the 'test -z "$(...)"' is already used heavily in that script, just stick with it. As for a side cleanup, perhaps if you have time later on, but don't let it derail your project timeline. It's not that important.