{"thread":{"id":"41608","subject":"[PATCH 1/2] test-lib: quote TRASH_DIRECTORY","startedAt":"2016-03-04T10:53:48Z","lastAt":"2016-03-04T18:38:31Z","messageCount":9,"participants":["Michael J Gruber","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"280180","messageId":"cover.1457088499.git.git@drmicha.warpmail.net","threadId":"41608","inReplyTo":null,"subject":"[PATCH/RFC 0/2] Test trash dir sanitizing","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-03-04T10:53:48Z","receivedAt":"2016-03-04T10:53:48Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"I encountered a Heisenbug[*] where t5510 would leave its trash directory without\ncleanup, though not reproducibly so. This mini series cleans up two spots which\nmay or may not be related, but should be goog cleanup anyways.\n\n[*] Running tests with prove -j4.\n\nMichael J Gruber (2):\n  test-lib: quote TRASH_DIRECTORY\n  t5510: do not leave changed cwd\n\n t/t5510-fetch.sh | 10 ++++++----\n t/test-lib.sh    |  2 +-\n 2 files changed, 7 insertions(+), 5 deletions(-)\n\n-- \n2.8.0.rc0.181.g163d81c\n"},{"id":"280179","messageId":"a07529d116612bcd1c411a697f8f8e37d7c18d3f.1457088499.git.git@drmicha.warpmail.net","threadId":"41608","inReplyTo":"cover.1457088499.git.git@drmicha.warpmail.net","subject":"[PATCH 1/2] test-lib: quote TRASH_DIRECTORY","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-03-04T10:53:49Z","receivedAt":"2016-03-04T10:53:49Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"We always quote $TRASH_DIRECTORY to guard against funky path names. Do\nso in one more spot\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\n t/test-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 0b47eb6..8957916 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -868,7 +868,7 @@ case \"$TRASH_DIRECTORY\" in\n /*) ;; # absolute path is good\n  *) TRASH_DIRECTORY=\"$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY\" ;;\n esac\n-test ! -z \"$debug\" || remove_trash=$TRASH_DIRECTORY\n+test ! -z \"$debug\" || remove_trash=\"$TRASH_DIRECTORY\"\n rm -fr \"$TRASH_DIRECTORY\" || {\n \tGIT_EXIT_OK=t\n \techo >&5 \"FATAL: Cannot prepare test area\"\n-- \n2.8.0.rc0.181.g163d81c\n"},{"id":"280181","messageId":"f6499302a5dfb52bf6c9430581103da9569434a3.1457088499.git.git@drmicha.warpmail.net","threadId":"41608","inReplyTo":"cover.1457088499.git.git@drmicha.warpmail.net","subject":"[PATCH 2/2] t5510: do not leave changed cwd","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-03-04T10:53:50Z","receivedAt":"2016-03-04T10:53:50Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"t5510 carefully keeps the cwd at the test root by using either subshells\nor explicit cd'ing back to the root. Use a subshell for the last\nsubtest, too.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\n t/t5510-fetch.sh | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 0c10c85..38321d1 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -679,10 +679,12 @@ test_expect_success 'fetching with auto-gc does not lock up' '\n \tEOF\n \tgit clone \"file://$D\" auto-gc &&\n \ttest_commit test2 &&\n-\tcd auto-gc &&\n-\tgit config gc.autoPackLimit 1 &&\n-\tGIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n-\t! grep \"Should I try again\" fetch.out\n+\t(\n+\t\tcd auto-gc &&\n+\t\tgit config gc.autoPackLimit 1 &&\n+\t\tGIT_ASK_YESNO=\"$D/askyesno\" git fetch >fetch.out 2>&1 &&\n+\t\t! grep \"Should I try again\" fetch.out\n+\t)\n '\n \n test_done\n-- \n2.8.0.rc0.181.g163d81c\n"},{"id":"280185","messageId":"20160304115127.GA8355@sigill.intra.peff.net","threadId":"41608","inReplyTo":"a07529d116612bcd1c411a697f8f8e37d7c18d3f.1457088499.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 1/2] test-lib: quote TRASH_DIRECTORY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-04T11:51:28Z","receivedAt":"2016-03-04T11:51:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 04, 2016 at 11:53:49AM +0100, Michael J Gruber wrote:\n\n> We always quote $TRASH_DIRECTORY to guard against funky path names. Do\n> so in one more spot\n> \n> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n> ---\n>  t/test-lib.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 0b47eb6..8957916 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -868,7 +868,7 @@ case \"$TRASH_DIRECTORY\" in\n>  /*) ;; # absolute path is good\n>   *) TRASH_DIRECTORY=\"$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY\" ;;\n>  esac\n> -test ! -z \"$debug\" || remove_trash=$TRASH_DIRECTORY\n> +test ! -z \"$debug\" || remove_trash=\"$TRASH_DIRECTORY\"\n\nI don't think this does anything. The shell doesn't do whitespace\nsplitting on the right-hand side of a variable assignment:\n\n  $ foo='lots of spaces and \"!'\\'' funky chars'\n  $ bar=$foo\n  $ echo \"$bar\"\n  lots of spaces and \"!' funky chars\n\nOf course we _do_ need quotes when we refer to $remove_trash as an\nargument (as with \"$bar\" above), but it looks like we do so correctly\neverywhere.\n\n-Peff\n"},{"id":"280186","messageId":"20160304115255.GB8355@sigill.intra.peff.net","threadId":"41608","inReplyTo":"f6499302a5dfb52bf6c9430581103da9569434a3.1457088499.git.git@drmicha.warpmail.net","subject":"Re: [PATCH 2/2] t5510: do not leave changed cwd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-04T11:52:55Z","receivedAt":"2016-03-04T11:52:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 04, 2016 at 11:53:50AM +0100, Michael J Gruber wrote:\n\n> t5510 carefully keeps the cwd at the test root by using either subshells\n> or explicit cd'ing back to the root. Use a subshell for the last\n> subtest, too.\n\nI doubt this caused the heisenbug you saw, as we should have an absolute\npath for the trash-dir, and we \"cd\" to its containing directory before\ndeleting it. But this is definitely a good thing to be doing anyway, to\nprevent surprises for new tests added to t5510.\n\n-Peff\n"},{"id":"280191","messageId":"56D98793.9000708@drmicha.warpmail.net","threadId":"41608","inReplyTo":"20160304115127.GA8355@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] test-lib: quote TRASH_DIRECTORY","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-03-04T13:03:15Z","receivedAt":"2016-03-04T13:03:15Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 04.03.2016 12:51:\n> On Fri, Mar 04, 2016 at 11:53:49AM +0100, Michael J Gruber wrote:\n> \n>> We always quote $TRASH_DIRECTORY to guard against funky path names. Do\n>> so in one more spot\n>>\n>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n>> ---\n>>  t/test-lib.sh | 2 +-\n>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> index 0b47eb6..8957916 100644\n>> --- a/t/test-lib.sh\n>> +++ b/t/test-lib.sh\n>> @@ -868,7 +868,7 @@ case \"$TRASH_DIRECTORY\" in\n>>  /*) ;; # absolute path is good\n>>   *) TRASH_DIRECTORY=\"$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY\" ;;\n>>  esac\n>> -test ! -z \"$debug\" || remove_trash=$TRASH_DIRECTORY\n>> +test ! -z \"$debug\" || remove_trash=\"$TRASH_DIRECTORY\"\n> \n> I don't think this does anything. The shell doesn't do whitespace\n> splitting on the right-hand side of a variable assignment:\n> \n>   $ foo='lots of spaces and \"!'\\'' funky chars'\n>   $ bar=$foo\n>   $ echo \"$bar\"\n>   lots of spaces and \"!' funky chars\n> \n> Of course we _do_ need quotes when we refer to $remove_trash as an\n> argument (as with \"$bar\" above), but it looks like we do so correctly\n> everywhere.\n\nI'm used to that behavior, yes, but:\n\n- Is this true for every shell that we support?\n- Having quotes there, too, is a good reminder to have it also where\nnecessary.\n\nMichael\n"},{"id":"280192","messageId":"56D987D0.5030208@drmicha.warpmail.net","threadId":"41608","inReplyTo":"20160304115255.GB8355@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] t5510: do not leave changed cwd","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-03-04T13:04:16Z","receivedAt":"2016-03-04T13:04:16Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 04.03.2016 12:52:\n> On Fri, Mar 04, 2016 at 11:53:50AM +0100, Michael J Gruber wrote:\n> \n>> t5510 carefully keeps the cwd at the test root by using either subshells\n>> or explicit cd'ing back to the root. Use a subshell for the last\n>> subtest, too.\n> \n> I doubt this caused the heisenbug you saw, as we should have an absolute\n> path for the trash-dir, and we \"cd\" to its containing directory before\n> deleting it. But this is definitely a good thing to be doing anyway, to\n> prevent surprises for new tests added to t5510.\n\nAbsolutely ;)\n\nMichael\n"},{"id":"280193","messageId":"20160304130939.GA15375@sigill.intra.peff.net","threadId":"41608","inReplyTo":"56D98793.9000708@drmicha.warpmail.net","subject":"Re: [PATCH 1/2] test-lib: quote TRASH_DIRECTORY","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-04T13:09:39Z","receivedAt":"2016-03-04T13:09:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 04, 2016 at 02:03:15PM +0100, Michael J Gruber wrote:\n\n> >> -test ! -z \"$debug\" || remove_trash=$TRASH_DIRECTORY\n> >> +test ! -z \"$debug\" || remove_trash=\"$TRASH_DIRECTORY\"\n> > \n> > I don't think this does anything. The shell doesn't do whitespace\n> > splitting on the right-hand side of a variable assignment:\n> > \n> >   $ foo='lots of spaces and \"!'\\'' funky chars'\n> >   $ bar=$foo\n> >   $ echo \"$bar\"\n> >   lots of spaces and \"!' funky chars\n> > \n> > Of course we _do_ need quotes when we refer to $remove_trash as an\n> > argument (as with \"$bar\" above), but it looks like we do so correctly\n> > everywhere.\n> \n> I'm used to that behavior, yes, but:\n> \n> - Is this true for every shell that we support?\n\nIt better be, because we rely on it all through our scripts. :)\n\nBut yes, it is in POSIX, and I think any shell which did not respect it\nwould be broken enough to be unusable (I don't have a copy of Solaris\n/bin/sh handy, but that's usually our go-to for \"unusably broken\").\n\n> - Having quotes there, too, is a good reminder to have it also where\n> necessary.\n\nWe do already quote variable assignments unnecessarily in lots of\nplaces, so I don't mind it too much (it's definitely not _wrong_ to do\nso).\n\n-Peff\n"},{"id":"280203","messageId":"xmqqoaauytp4.fsf@gitster.mtv.corp.google.com","threadId":"41608","inReplyTo":"56D987D0.5030208@drmicha.warpmail.net","subject":"Re: [PATCH 2/2] t5510: do not leave changed cwd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-04T18:38:31Z","receivedAt":"2016-03-04T18:38:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> Jeff King venit, vidit, dixit 04.03.2016 12:52:\n>> On Fri, Mar 04, 2016 at 11:53:50AM +0100, Michael J Gruber wrote:\n>> \n>>> t5510 carefully keeps the cwd at the test root by using either subshells\n>>> or explicit cd'ing back to the root. Use a subshell for the last\n>>> subtest, too.\n>> \n>> I doubt this caused the heisenbug you saw, as we should have an absolute\n>> path for the trash-dir, and we \"cd\" to its containing directory before\n>> deleting it. But this is definitely a good thing to be doing anyway, to\n>> prevent surprises for new tests added to t5510.\n>\n> Absolutely ;)\n\nI'll take this one; thanks.\n"}]}