Torsten Bögershausen <[email protected]> writes:
> []
>>>
>>> - cd "$(dirname "$remove_trash")" &&
>>> - rm -rf "$(basename "$remove_trash")" ||
>>> - error "Tests passed but test cleanup failed; aborting"
>>> + cd "$(dirname "$TRASH_DIRECTORY")" &&
>>> + rm -fr "$TRASH_DIRECTORY" ||
>>> + error "Tests passed but test cleanup failed; aborting"
>>> + fi
>>
>> Yeah, that looks good to me.
>
> Does it ?
> Checking the error code of "rm -f" doesn't work as expected from the script:
> rm -rf DoesNotExist ; echo $?
> 0
>
> I think it should be
>
>>> + cd "$(dirname "$TRASH_DIRECTORY")" &&
>>> + rm -r "$TRASH_DIRECTORY" ||
>>> + error "Tests passed but test cleanup failed; aborting"
What Peff told you in his response is all correct, but besides, you
can see the patch and realize that the original has been using "rm
-rf" for ages.
This change is about not using $remove_trash variable, as it felt
brittle and error prone than detecting $debug case and removing
$TRASH_DIRECTORY. So in that sense, I shouldn't have even rolled
the removal of basename into it.
Having said that, because people care about the number of subprocess
invocations, I am tempted to suggest doing
cd "$TRASH_DIRECTORY/.." &&
rm -fr "$TRASH_DIRECTORY" ||
error ...
which should reduce another process ;-)
