From: Jeff King <peff@peff.net>
Date: Mon, 24 Apr 2017 09:46:54 GMT
Subject: Re: [PATCH] test-lib: abort when can't remove trash directory
Message-ID: <20170424094654.eucylyv7k2l332jw@sigill.intra.peff.net>
In-Reply-To: <18ee407c-db3b-16a2-4822-0ff302045b27@web.de>

On Mon, Apr 24, 2017 at 11:39:26AM +0200, Torsten Bögershausen wrote:

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

ENOENT is treated specially by "rm -f". We cover that case explicitly
with the "test -d" above (in the bit you didn't quote), and then rely on
"rm" to report other errors.  Like:

  $ rm -rf /etc/passwd ; echo $?
  rm: cannot remove '/etc/passwd': Permission denied
  1

> I think it should be
> 
> > > +			cd "$(dirname "$TRASH_DIRECTORY")" &&
> > > +			rm -r "$TRASH_DIRECTORY" ||
> > > +			error "Tests passed but test cleanup failed; aborting"

I think we need the "-f" to overcome rm's tendency to prompt in some
cases:

  $ echo content >foo
  $ chmod 0 foo
  $ rm foo
  rm: remove write-protected regular file 'foo'? ^C
  $ rm -f foo
  [no output; it just works]

-Peff