From: Ben Peart <peartben@gmail.com>
Date: Mon, 23 Jul 2018 18:22:07 GMT
Subject: Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout
Message-ID: <795b0bbc-c9d3-f627-4798-80d0e25bfade@gmail.com>
In-Reply-To: <CAPig+cR3ic5OAAhNNaSu3YXMnpm=zw6XjspYSU4jouH6po05gg@mail.gmail.com>



On 7/23/2018 2:09 PM, Eric Sunshine wrote:
> On Mon, Jul 23, 2018 at 9:12 AM Ben Peart <peartben@gmail.com> wrote:
>> On 7/21/2018 3:21 AM, Eric Sunshine wrote:
>>> On Sat, Jul 21, 2018 at 2:34 AM Elijah Newren <newren@gmail.com> wrote:
>>>> +       rm .git/info/sparse-checkout
>>>
>>> Should this cleanup be done by test_when_finished()?
>>
>> I think trying to use test_when_finished() for this really degrades the
>> readability of the test.  See below:
>>
>> test_expect_success 'failed cherry-pick with sparse-checkout' '
>>          pristine_detach initial &&
>>          test_config core.sparsecheckout true &&
>>          echo /unrelated >.git/info/sparse-checkout &&
>>          git read-tree --reset -u HEAD &&
>>          test_when_finished "echo \"/*\" >.git/info/sparse-checkout && git
>> read-tree --reset -u HEAD && rm .git/info/sparse-checkout" &&
>>          test_must_fail git cherry-pick -Xours picked>actual &&
>>          test_i18ngrep ! "Changes not staged for commit:" actual
>> '
>>
>> Given it takes multiple commands, I'd prefer to keep the setup and
>> cleanup of the sparse checkout settings symmetrical.
> 
> Some observations:
> 
> The test_when_finished() ought to be called before the initial
> git-read-tree, otherwise you risk leaving a .git/info/sparse-checkout
> sitting around if git-read-tree fails.
> 
> The tear-down code could be moved to a function, in which case,
> test_when_finished() would simply call that function.
> 
> Multi-line quoted strings are valid, so you don't need to string out
> all the tear-down steps on a single line like that, and instead spread
> them across multiple lines to improve readability.
> 
> test_when_finished() doesn't expect just a single quoted string as
> argument. In fact, it can take many (unquoted) arguments, which also
> allows you to spread the tear-down steps over multiple lines to
> improve readability.
> 
> Multiple test_when_finished() invocations are allowed, so you could
> spread out the tear-down commands that way (though they'd have to be
> in reverse order, which would be bad for readability in this case,
> thus not recommended).
> 
> Correctness ought to trump readability, not the other way around.
> 
> So, one possibility, which seems pretty readable to me:
> 
>      test_expect_failure 'failed cherry-pick with sparse-checkout' '
>         pristine_detach initial &&
>         test_config core.sparseCheckout true &&
>         test_when_finished "
>             echo \"/*\" >.git/info/sparse-checkout
>             git read-tree --reset -u HEAD
>             rm .git/info/sparse-checkout" &&
>         echo /unrelated >.git/info/sparse-checkout &&
>         git read-tree --reset -u HEAD &&
>         test_must_fail git cherry-pick -Xours picked>actual &&
>         test_i18ngrep ! "Changes not staged for commit:" actual &&
>      '
> 

Minus the trailing && on the last line, that works for me.  Thank you - 
readability and correctness.

> Notice that I dropped the internal &&-chain in test_when_finish() to
> ensure that the final 'rm' is invoked even if the cleanup
> git-read-tree fails (though all bets are probably off, anyhow, if it
> does fail).
>