threads / patch / 41608

patch, 2 partstest-lib: quote TRASH_DIRECTORY

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

## tl;dr

9 messages between Mar 4, 2016 and Mar 4, 2016. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Michael J Gruber· Mar 4, 2016, 10:53 UTC · lore

[PATCH/RFC 0/2] 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(-)

-- 
2.8.0.rc0.181.g163d81c

Michael J Gruber· Mar 4, 2016, 10:53 UTC · re: Michael J Gruber · lore

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

Show changes to t/test-lib.sh +1 −1

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

Jeff King· Mar 4, 2016, 11:51 UTC · re: Michael J Gruber · lore

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

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

Show 18 quoted lines

> 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

Michael J Gruber· Mar 4, 2016, 13:03 UTC · re: Jeff King · lore

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

Jeff King venit, vidit, dixit 04.03.2016 12:51:

Show 32 quoted lines

> 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

Jeff King· Mar 4, 2016, 13:09 UTC · re: Michael J Gruber · lore

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

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

Show 18 quoted lines

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

Michael J Gruber· Mar 4, 2016, 10:53 UTC · re: Michael J Gruber · lore

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

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

Show changes to t/t5510-fetch.sh +6 −4

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

Jeff King· Mar 4, 2016, 11:52 UTC · re: Michael J Gruber · lore

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

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

Michael J Gruber· Mar 4, 2016, 13:04 UTC · re: Jeff King · lore

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

Jeff King venit, vidit, dixit 04.03.2016 12:52:

Show 10 quoted lines

> 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

Junio C Hamano· Mar 4, 2016, 18:38 UTC · re: Michael J Gruber · lore

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

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

Show 13 quoted lines

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