From: Ben Peart Date: Mon, 23 Jul 2018 13:12:51 GMT Subject: Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout Message-ID: In-Reply-To: On 7/21/2018 3:21 AM, Eric Sunshine wrote: > On Sat, Jul 21, 2018 at 2:34 AM Elijah Newren wrote: >> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh >> @@ -392,4 +392,17 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' ' >> +test_expect_failure 'failed cherry-pick with sparse-checkout' ' >> + pristine_detach initial && >> + git config core.sparseCheckout true && > > Should this be test_config()? > I think using test_config() here is fine but... >> + 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 && >> + echo "/*" >.git/info/sparse-checkout && >> + git read-tree --reset -u HEAD && >> + git config core.sparseCheckout false && > > See question above. > >> + 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.