Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout

[Eric Sunshine <sunshine@sunshineco.com>]

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 &&
    '

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).