git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: John Keeping <john@keeping.me.uk>
Cc: git@vger.kernel.org, Thomas Rast <trast@inf.ethz.ch>
Subject: Re: [PATCH] t/test-lib.sh: fix TRASH_DIRECTORY handling
Date: Sun, 14 Apr 2013 15:38:21 -0400	[thread overview]
Message-ID: <20130414193821.GC4599@sigill.intra.peff.net> (raw)
In-Reply-To: <11cf45ad9779240b588da4b25c8ae5b6cc61b427.1365957201.git.john@keeping.me.uk>

On Sun, Apr 14, 2013 at 05:34:56PM +0100, John Keeping wrote:

> After the location of $TRASH_DIRECTORY is adjusted by
> $TEST_OUTPUT_DIRECTORY, we go on to use the $test variable to make the
> trash directory and cd into it.  This means that when
> $TEST_OUTPUT_DIRECTORY is not "." and an absolute --root has not been
> specified, we do not remove the trash directory once the tests are
> complete (remove_trash is set to $TRASH_DIRECTORY).
> 
> Fix this by always referring to the trash directory as $TRASH_DIRECTORY.

Thanks, this seems to date back all the way to my f423ef5 (tests: allow
user to specify trash directory location, 2009-08-09), although I think
at that time it was not even possible to run the tests from any other
directory. So I am happy to blame Thomas's later patches for violating
my assumptions. :)

Definitely:

  Acked-by: Jeff King <peff@peff.net>

It probably makes sense to do the patch below on top, so that the error
doesn't happen again (I'd just as soon have it squashed into your patch,
since then then the motivation is even more obvious).

-- >8 --
Subject: [PATCH] t/test-lib.sh: drop "$test" variable

The $test variable is used as an interim buffer for
constructing $TRASH_DIRECTORY, and is almost compatible with
it (the exception being that $test has not been converted to
an absolute path). Let's get rid of it entirely so that
later code does not accidentally use it, thinking the two
are interchangeable.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index a16bc73..b3f2488 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -600,11 +600,11 @@ fi
 fi
 
 # Test repository
-test="trash directory.$(basename "$0" .sh)"
-test -n "$root" && test="$root/$test"
-case "$test" in
-/*) TRASH_DIRECTORY="$test" ;;
- *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$test" ;;
+TRASH_DIRECTORY="trash directory.$(basename "$0" .sh)"
+test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
+case "$TRASH_DIRECTORY" in
+/*) ;; # absolute path is good
+ *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
 esac
 test ! -z "$debug" || remove_trash=$TRASH_DIRECTORY
 rm -fr "$TRASH_DIRECTORY" || {
-- 
1.8.2.rc0.33.gd915649

  reply	other threads:[~2013-04-14 19:38 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-04-14 16:34 [PATCH] t/test-lib.sh: fix TRASH_DIRECTORY handling John Keeping
2013-04-14 19:38 ` Jeff King [this message]
2013-04-14 19:44   ` Thomas Rast

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130414193821.GC4599@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=john@keeping.me.uk \
    --cc=trast@inf.ethz.ch \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).