Re: [PATCH] test-lib: abort when can't remove trash directory

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 25, 2017, 02:31 UTC
Message-ID: <xmqq4lxdxlp0.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <18ee407c-db3b-16a2-4822-0ff302045b27@web.de>

Torsten Bögershausen <tboegi@web.de> writes:

Show 22 quoted lines

> []
>>>
>>> -		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 ;-)