Test trash dir sanitizing
cover Test trash dir sanitizing
I encountered a Heisenbug[*] where t5510 would leave its trash directory without cleanup, though not reproducibly so. This mini series cleans up two spots which may or may not be related, but should be goog cleanup anyways.
[*] Running tests with prove -j4.
Michael J Gruber (2):
test-lib: quote TRASH_DIRECTORY
t5510: do not leave changed cwd
t/t5510-fetch.sh | 10 ++++++---- t/test-lib.sh | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-)
Signature · 1 line
2.8.0.rc0.181.g163d81c
PATCH 1/2 test-lib: quote TRASH_DIRECTORY
We always quote $TRASH_DIRECTORY to guard against funky path names. Do so in one more spot
--- t/test-lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
t/test-lib.sh +1 -1
@@ -868,7 +868,7 @@ case "$TRASH_DIRECTORY" in /*) ;; # absolute path is good *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;; esac -test ! -z "$debug" || remove_trash=$TRASH_DIRECTORY +test ! -z "$debug" || remove_trash="$TRASH_DIRECTORY" rm -fr "$TRASH_DIRECTORY" || { GIT_EXIT_OK=t echo >&5 "FATAL: Cannot prepare test area"
Signature · 1 line
2.8.0.rc0.181.g163d81c
Re: [PATCH 1/2] test-lib: quote TRASH_DIRECTORY
Quoted 13 lines · Michael J Gruber
We always quote $TRASH_DIRECTORY to guard against funky path names. Do so in one more spot
Signed-off-by Michael J Gruber <[email protected]>--- t/test-lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)t/test-lib.sh +0
-0
t/test-lib.sh case "$TRASH_DIRECTORY" in · line 871
Quoted 5 lines
t/test-lib.sh +1
-1@@ -868,7 +868,7 @@ case "$TRASH_DIRECTORY" in /*) ;; # absolute path is good *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;; esac -test ! -z "$debug" || remove_trash=$TRASH_DIRECTORY +test ! -z "$debug" || remove_trash="$TRASH_DIRECTORY"
I don't think this does anything. The shell doesn't do whitespace splitting on the right-hand side of a variable assignment:
$ foo='lots of spaces and "!'\'' funky chars' $ bar=$foo $ echo "$bar" lots of spaces and "!' funky chars
Of course we _do_ need quotes when we refer to $remove_trash as an argument (as with "$bar" above), but it looks like we do so correctly everywhere.
-Peff
Re: [PATCH 1/2] test-lib: quote TRASH_DIRECTORY
Jeff King venit, vidit, dixit 04.03.2016 12:51:
Quoted 24 lines · Michael J Gruber
Quoted 14 lines
We always quote $TRASH_DIRECTORY to guard against funky path names. Do so in one more spot
Signed-off-by Michael J Gruber <[email protected]>--- t/test-lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)t/test-lib.sh +0
-0t/test-lib.sh case "$TRASH_DIRECTORY" in · line 871
Quoted 5 lines
t/test-lib.sh +1
-1@@ -868,7 +868,7 @@ case "$TRASH_DIRECTORY" in /*) ;; # absolute path is good *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;; esac -test ! -z "$debug" || remove_trash=$TRASH_DIRECTORY +test ! -z "$debug" || remove_trash="$TRASH_DIRECTORY"I don't think this does anything. The shell doesn't do whitespace splitting on the right-hand side of a variable assignment:
Quoted 8 lines
$ foo='lots of spaces and "!'\'' funky chars' $ bar=$foo $ echo "$bar" lots of spaces and "!' funky charsOf course we _do_ need quotes when we refer to $remove_trash as an argument (as with "$bar" above), but it looks like we do so correctly everywhere.
I'm used to that behavior, yes, but:
- Is this true for every shell that we support?
- Having quotes there, too, is a good reminder to have it also where necessary.
Michael
Re: [PATCH 1/2] test-lib: quote TRASH_DIRECTORY
Quoted 11 lines · Michael J Gruber
Quoted 11 lines
t/test-lib.sh case "$TRASH_DIRECTORY" in · line 871
Quoted 2 lines
t/test-lib.sh +1
-1-test ! -z "$debug" || remove_trash=$TRASH_DIRECTORY +test ! -z "$debug" || remove_trash="$TRASH_DIRECTORY"I don't think this does anything. The shell doesn't do whitespace splitting on the right-hand side of a variable assignment:
$ foo='lots of spaces and "!'\'' funky chars' $ bar=$foo $ echo "$bar" lots of spaces and "!' funky chars
Quoted 7 lines
Quoted 3 lines
Of course we _do_ need quotes when we refer to $remove_trash as an argument (as with "$bar" above), but it looks like we do so correctly everywhere.
I'm used to that behavior, yes, but:
- Is this true for every shell that we support?
It better be, because we rely on it all through our scripts. :)
But yes, it is in POSIX, and I think any shell which did not respect it would be broken enough to be unusable (I don't have a copy of Solaris /bin/sh handy, but that's usually our go-to for "unusably broken").
Quoted 2 lines
- Having quotes there, too, is a good reminder to have it also where necessary.
We do already quote variable assignments unnecessarily in lots of places, so I don't mind it too much (it's definitely not _wrong_ to do so).
-Peff
PATCH 2/2 t5510: do not leave changed cwd
In reply to Michael J Grubert5510 carefully keeps the cwd at the test root by using either subshells or explicit cd'ing back to the root. Use a subshell for the last subtest, too.
--- t/t5510-fetch.sh | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
t/t5510-fetch.sh +6 -4
@@ -679,10 +679,12 @@ test_expect_success 'fetching with auto-gc does not lock up' ' EOF git clone "file://$D" auto-gc && test_commit test2 && - cd auto-gc && - git config gc.autoPackLimit 1 && - GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 && - ! grep "Should I try again" fetch.out + ( + cd auto-gc && + git config gc.autoPackLimit 1 && + GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 && + ! grep "Should I try again" fetch.out + ) ' test_done
Signature · 1 line
2.8.0.rc0.181.g163d81c
Re: [PATCH 2/2] t5510: do not leave changed cwd
Quoted 3 lines · Michael J Gruber
t5510 carefully keeps the cwd at the test root by using either subshells or explicit cd'ing back to the root. Use a subshell for the last subtest, too.
I doubt this caused the heisenbug you saw, as we should have an absolute path for the trash-dir, and we "cd" to its containing directory before deleting it. But this is definitely a good thing to be doing anyway, to prevent surprises for new tests added to t5510.
-Peff
Re: [PATCH 2/2] t5510: do not leave changed cwd
Jeff King venit, vidit, dixit 04.03.2016 12:52:
Quoted 10 lines · Michael J Gruber
Quoted 4 lines
t5510 carefully keeps the cwd at the test root by using either subshells or explicit cd'ing back to the root. Use a subshell for the last subtest, too.
I doubt this caused the heisenbug you saw, as we should have an absolute path for the trash-dir, and we "cd" to its containing directory before deleting it. But this is definitely a good thing to be doing anyway, to prevent surprises for new tests added to t5510.
Absolutely ;)
Michael
Re: [PATCH 2/2] t5510: do not leave changed cwd
Quoted 7 lines · Michael J Gruber
Jeff King venit, vidit, dixit 04.03.2016 12:52:
Quoted 6 lines · Michael J Gruber
Quoted 4 lines
t5510 carefully keeps the cwd at the test root by using either subshells or explicit cd'ing back to the root. Use a subshell for the last subtest, too.
Quoted 6 lines
Quoted 4 lines
I doubt this caused the heisenbug you saw, as we should have an absolute path for the trash-dir, and we "cd" to its containing directory before deleting it. But this is definitely a good thing to be doing anyway, to prevent surprises for new tests added to t5510.
Absolutely ;)
I'll take this one; thanks.