Re: [PATCH] t6500: don't run detached auto gc at the end of the test script

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Thu, Apr 13, 2017 at 9:12 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Apr 13, 2017 at 09:03:26PM +0200, SZEDER Gábor wrote:
>
>> > Yeah, I had a similar thought. I can't think of any reason why it would
>> > trigger a false positive, as long as we account for test-failure and the
>> > --debug flag properly. I don't think we can just drop the "-f" from the
>> > final "rm", because it may be overriding funny permissions left by
>> > tests.
>>
>> FWIW, I used the following rather simple change during stress-testing
>> these patches (barring white-space damage):
>>
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 13b569682..d7fa15a69 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -763,7 +763,7 @@ test_done () {
>>
>>                 test -d "$remove_trash" &&

I'm not sure under what circumstances the trash dir could be missing at
this point...

>>                 cd "$(dirname "$remove_trash")" &&
>> -               rm -rf "$(basename "$remove_trash")"
>> +               rm -rf "$(basename "$remove_trash")" || exit 1

... but when it is already removed, then I think we should not exit
with error here.
Nothing that a pair of {} wouldn't handle.

> Oh, right. I thought "-f" would cause it to exit 0 even if some items
> failed to be removed, but that only applies to ENOENT. So I think that
> is probably a good change. I agree it's not a true error, but just a
> sign of something fishy, but I don't think it hurts to have extra lint
> checks.
>
> Replacing it the "exit 1" with a "die" that has a message is probably a
> good idea, though.

If we can't 'cd' or 'rm -rf', then they will tell us why they failed
anyway, most likely including the name of the trash directory.
Do we really need more error messages than that?