[PATCH/RFC 0/2] Test trash dir sanitizing

[PATCH/RFC 0/2] Test trash dir sanitizing

[Michael J Gruber]

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(-)

-- 
2.8.0.rc0.181.g163d81c

[PATCH 1/2] test-lib: quote TRASH_DIRECTORY

[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 <git@drmicha.warpmail.net>
---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0b47eb6..8957916 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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"
-- 
2.8.0.rc0.181.g163d81c

Re: [PATCH 1/2] test-lib: quote TRASH_DIRECTORY

[Jeff King]

On Fri, Mar 04, 2016 at 11:53:49AM +0100, Michael J Gruber wrote:

> We always quote $TRASH_DIRECTORY to guard against funky path names. Do
> so in one more spot
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  t/test-lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0b47eb6..8957916 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -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

[Michael J Gruber]

Jeff King venit, vidit, dixit 04.03.2016 12:51:
> On Fri, Mar 04, 2016 at 11:53:49AM +0100, Michael J Gruber wrote:
> 
>> We always quote $TRASH_DIRECTORY to guard against funky path names. Do
>> so in one more spot
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>>  t/test-lib.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 0b47eb6..8957916 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -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.

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

[Jeff King]

On Fri, Mar 04, 2016 at 02:03:15PM +0100, Michael J Gruber wrote:

> >> -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.
> 
> 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").

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

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

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t5510-fetch.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 0c10c85..38321d1 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -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
-- 
2.8.0.rc0.181.g163d81c

Re: [PATCH 2/2] t5510: do not leave changed cwd

[Jeff King]

On Fri, Mar 04, 2016 at 11:53:50AM +0100, Michael J Gruber wrote:

> 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

[Michael J Gruber]

Jeff King venit, vidit, dixit 04.03.2016 12:52:
> On Fri, Mar 04, 2016 at 11:53:50AM +0100, Michael J Gruber wrote:
> 
>> 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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Jeff King venit, vidit, dixit 04.03.2016 12:52:
>> On Fri, Mar 04, 2016 at 11:53:50AM +0100, Michael J Gruber wrote:
>> 
>>> 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 ;)

I'll take this one; thanks.