Re: [PATCH] t6500: don't run detached auto gc at the end of the test script
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Apr 13, 2017, 19:35 UTC
- Message-ID
- <CAM0VKjkVUPPsvpAu1WXeNg7sv1_V=Uxhhevf9fVkZvk9WERwwQ@mail.gmail.com>
- In-Reply-To
- <20170413191236.3usecfgkjujhblo6@sigill.intra.peff.net>
On Thu, Apr 13, 2017 at 9:12 PM, Jeff King <peff@peff.net> wrote:
Show 18 quoted lines
> 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.
Show 8 quoted lines
> 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?