# [PATCH 1/2] test-lib: quote TRASH_DIRECTORY 9 messages from 2016-03-04 to 2016-03-04. Participants: Michael J Gruber, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/41608 ## Michael J Gruber, 2016-03-04 10:53 Subject: [PATCH/RFC 0/2] Test trash dir sanitizing Message-ID: URL: https://gitlist.dev/e/cover.1457088499.git.git%40drmicha.warpmail.net ``` 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, 2016-03-04 10:53 Subject: [PATCH 1/2] test-lib: quote TRASH_DIRECTORY Message-ID: URL: https://gitlist.dev/e/a07529d116612bcd1c411a697f8f8e37d7c18d3f.1457088499.git.git%40drmicha.warpmail.net In-Reply-To: ``` We always quote $TRASH_DIRECTORY to guard against funky path names. Do so in one more spot Signed-off-by: Michael J Gruber --- 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 ``` ## Michael J Gruber, 2016-03-04 10:53 Subject: [PATCH 2/2] t5510: do not leave changed cwd Message-ID: URL: https://gitlist.dev/e/f6499302a5dfb52bf6c9430581103da9569434a3.1457088499.git.git%40drmicha.warpmail.net In-Reply-To: ``` 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 --- 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 ``` ## Jeff King, 2016-03-04 11:51 Subject: Re: [PATCH 1/2] test-lib: quote TRASH_DIRECTORY Message-ID: <20160304115127.GA8355@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160304115127.GA8355%40sigill.intra.peff.net In-Reply-To: ``` 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 > --- > 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 ``` ## Jeff King, 2016-03-04 11:52 Subject: Re: [PATCH 2/2] t5510: do not leave changed cwd Message-ID: <20160304115255.GB8355@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160304115255.GB8355%40sigill.intra.peff.net In-Reply-To: ``` 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, 2016-03-04 13:03 Subject: Re: [PATCH 1/2] test-lib: quote TRASH_DIRECTORY Message-ID: <56D98793.9000708@drmicha.warpmail.net> URL: https://gitlist.dev/e/56D98793.9000708%40drmicha.warpmail.net In-Reply-To: <20160304115127.GA8355@sigill.intra.peff.net> ``` 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 >> --- >> 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 ``` ## Michael J Gruber, 2016-03-04 13:04 Subject: Re: [PATCH 2/2] t5510: do not leave changed cwd Message-ID: <56D987D0.5030208@drmicha.warpmail.net> URL: https://gitlist.dev/e/56D987D0.5030208%40drmicha.warpmail.net In-Reply-To: <20160304115255.GB8355@sigill.intra.peff.net> ``` 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 ``` ## Jeff King, 2016-03-04 13:09 Subject: Re: [PATCH 1/2] test-lib: quote TRASH_DIRECTORY Message-ID: <20160304130939.GA15375@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160304130939.GA15375%40sigill.intra.peff.net In-Reply-To: <56D98793.9000708@drmicha.warpmail.net> ``` 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 ``` ## Junio C Hamano, 2016-03-04 18:38 Subject: Re: [PATCH 2/2] t5510: do not leave changed cwd Message-ID: URL: https://gitlist.dev/e/xmqqoaauytp4.fsf%40gitster.mtv.corp.google.com In-Reply-To: <56D987D0.5030208@drmicha.warpmail.net> ``` Michael J Gruber 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. ```