git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] t3900: add some more quotes
@ 2018-01-10  9:58 Beat Bolli
  2018-01-10 10:33 ` Jeff King
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Beat Bolli @ 2018-01-10  9:58 UTC (permalink / raw)
  To: git; +Cc: Beat Bolli, Johannes Schindelin, Jeff King

In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
were added to protect against spaces in $HOME. In the test_when_finished
hander, two files are deleted which must be quoted individually.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 t/t3900-i18n-commit.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index 9e4e694d9..dc00db87b 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -40,7 +40,7 @@ test_expect_success 'UTF-16 refused because of NULs' '
 '
 
 test_expect_success 'UTF-8 invalid characters refused' '
-	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+	test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
 	echo "UTF-8 characters" >F &&
 	printf "Commit message\n\nInvalid surrogate:\355\240\200\n" \
 		>"$HOME/invalid" &&
@@ -49,7 +49,7 @@ test_expect_success 'UTF-8 invalid characters refused' '
 '
 
 test_expect_success 'UTF-8 overlong sequences rejected' '
-	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+	test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
 	rm -f "$HOME/stderr" "$HOME/invalid" &&
 	echo "UTF-8 overlong" >F &&
 	printf "\340\202\251ommit message\n\nThis is not a space:\300\240\n" \
@@ -59,7 +59,7 @@ test_expect_success 'UTF-8 overlong sequences rejected' '
 '
 
 test_expect_success 'UTF-8 non-characters refused' '
-	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+	test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
 	echo "UTF-8 non-character 1" >F &&
 	printf "Commit message\n\nNon-character:\364\217\277\276\n" \
 		>"$HOME/invalid" &&
@@ -68,7 +68,7 @@ test_expect_success 'UTF-8 non-characters refused' '
 '
 
 test_expect_success 'UTF-8 non-characters refused' '
-	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+	test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
 	echo "UTF-8 non-character 2." >F &&
 	printf "Commit message\n\nNon-character:\357\267\220\n" \
 		>"$HOME/invalid" &&
-- 
2.15.0.rc1.299.gda03b47c3


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] t3900: add some more quotes
  2018-01-10  9:58 [PATCH] t3900: add some more quotes Beat Bolli
@ 2018-01-10 10:33 ` Jeff King
  2018-01-10 17:21   ` Johannes Schindelin
  2018-01-10 17:44 ` Eric Sunshine
  2018-01-10 19:02 ` [PATCH] " Johannes Sixt
  2 siblings, 1 reply; 14+ messages in thread
From: Jeff King @ 2018-01-10 10:33 UTC (permalink / raw)
  To: Beat Bolli; +Cc: git, Johannes Schindelin

On Wed, Jan 10, 2018 at 10:58:32AM +0100, Beat Bolli wrote:

> In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
> were added to protect against spaces in $HOME. In the test_when_finished
> hander, two files are deleted which must be quoted individually.

Doh. I can't believe I missed that when reading the patch.

Thanks.

-Peff

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] t3900: add some more quotes
  2018-01-10 10:33 ` Jeff King
@ 2018-01-10 17:21   ` Johannes Schindelin
  0 siblings, 0 replies; 14+ messages in thread
From: Johannes Schindelin @ 2018-01-10 17:21 UTC (permalink / raw)
  To: Jeff King; +Cc: Beat Bolli, git

Hi,

On Wed, 10 Jan 2018, Jeff King wrote:

> On Wed, Jan 10, 2018 at 10:58:32AM +0100, Beat Bolli wrote:
> 
> > In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
> > were added to protect against spaces in $HOME. In the test_when_finished
> > hander, two files are deleted which must be quoted individually.
> 
> Doh. I can't believe I missed that when reading the patch.

D'oh, and I can't believe that I missed this when writing the patch.

Thanks, Beat!
Dscho

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] t3900: add some more quotes
  2018-01-10  9:58 [PATCH] t3900: add some more quotes Beat Bolli
  2018-01-10 10:33 ` Jeff King
@ 2018-01-10 17:44 ` Eric Sunshine
  2018-01-10 22:38   ` [PATCH v2] " Beat Bolli
  2018-01-10 19:02 ` [PATCH] " Johannes Sixt
  2 siblings, 1 reply; 14+ messages in thread
From: Eric Sunshine @ 2018-01-10 17:44 UTC (permalink / raw)
  To: Beat Bolli; +Cc: Git List, Johannes Schindelin, Jeff King

On Wed, Jan 10, 2018 at 4:58 AM, Beat Bolli <dev+git@drbeat.li> wrote:
> In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
> were added to protect against spaces in $HOME. In the test_when_finished
> hander, two files are deleted which must be quoted individually.

s/hander/handler/

> Signed-off-by: Beat Bolli <dev+git@drbeat.li>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] t3900: add some more quotes
  2018-01-10  9:58 [PATCH] t3900: add some more quotes Beat Bolli
  2018-01-10 10:33 ` Jeff King
  2018-01-10 17:44 ` Eric Sunshine
@ 2018-01-10 19:02 ` " Johannes Sixt
  2018-01-10 19:43   ` Junio C Hamano
  2018-01-10 19:53   ` Jeff King
  2 siblings, 2 replies; 14+ messages in thread
From: Johannes Sixt @ 2018-01-10 19:02 UTC (permalink / raw)
  To: Beat Bolli; +Cc: git, Johannes Schindelin, Jeff King

Am 10.01.2018 um 10:58 schrieb Beat Bolli:
> In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
> were added to protect against spaces in $HOME. In the test_when_finished
> hander, two files are deleted which must be quoted individually.
> 
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
>   t/t3900-i18n-commit.sh | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
> index 9e4e694d9..dc00db87b 100755
> --- a/t/t3900-i18n-commit.sh
> +++ b/t/t3900-i18n-commit.sh
> @@ -40,7 +40,7 @@ test_expect_success 'UTF-16 refused because of NULs' '
>   '
>   
>   test_expect_success 'UTF-8 invalid characters refused' '
> -	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
> +	test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&

Should that not better be

	test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\""

i.e., delay the expansion of $HOME to protect against double-quotes in 
the path?

-- Hannes

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] t3900: add some more quotes
  2018-01-10 19:02 ` [PATCH] " Johannes Sixt
@ 2018-01-10 19:43   ` Junio C Hamano
  2018-01-10 19:53   ` Jeff King
  1 sibling, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2018-01-10 19:43 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Beat Bolli, git, Johannes Schindelin, Jeff King

Johannes Sixt <j6t@kdbg.org> writes:

>>     test_expect_success 'UTF-8 invalid characters refused' '
>> -	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
>> +	test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
>
> Should that not better be
>
> 	test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\""
>
> i.e., delay the expansion of $HOME to protect against double-quotes in
> the path?

Yes ;-)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] t3900: add some more quotes
  2018-01-10 19:02 ` [PATCH] " Johannes Sixt
  2018-01-10 19:43   ` Junio C Hamano
@ 2018-01-10 19:53   ` Jeff King
  2018-01-10 21:31     ` Junio C Hamano
  2018-01-11 11:39     ` read test snippet from stdin [was: [PATCH] t3900: add some more quotes] SZEDER Gábor
  1 sibling, 2 replies; 14+ messages in thread
From: Jeff King @ 2018-01-10 19:53 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Beat Bolli, git, Johannes Schindelin

On Wed, Jan 10, 2018 at 08:02:09PM +0100, Johannes Sixt wrote:

> > diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
> > index 9e4e694d9..dc00db87b 100755
> > --- a/t/t3900-i18n-commit.sh
> > +++ b/t/t3900-i18n-commit.sh
> > @@ -40,7 +40,7 @@ test_expect_success 'UTF-16 refused because of NULs' '
> >   '
> >   test_expect_success 'UTF-8 invalid characters refused' '
> > -	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
> > +	test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
> 
> Should that not better be
> 
> 	test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\""
> 
> i.e., delay the expansion of $HOME to protect against double-quotes in the
> path?

Yeah. One of the reasons for both of the errors in this thread is the
nested double-quoting. Using single quotes is awkward because we're
already using them to delimit the whole snippet.  I've often wondered if
our tests would be more readable taking the snippet over stdin.
Something like:

diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index 9e4e694d93..09ad4d8878 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -39,14 +39,14 @@ test_expect_success 'UTF-16 refused because of NULs' '
 	test_must_fail git commit -a -F "$TEST_DIRECTORY"/t3900/UTF-16.txt
 '
 
-test_expect_success 'UTF-8 invalid characters refused' '
-	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+test_expect_success 'UTF-8 invalid characters refused' - <<\EOT
+	test_when_finished 'rm -f "$HOME/stderr $HOME/invalid"' &&
 	echo "UTF-8 characters" >F &&
 	printf "Commit message\n\nInvalid surrogate:\355\240\200\n" \
 		>"$HOME/invalid" &&
 	git commit -a -F "$HOME/invalid" 2>"$HOME"/stderr &&
 	test_i18ngrep "did not conform" "$HOME"/stderr
-'
+EOT
 
 test_expect_success 'UTF-8 overlong sequences rejected' '
 	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 1701fe2a06..be8a47d304 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -391,11 +391,32 @@ test_verify_prereq () {
 	error "bug in the test script: '$test_prereq' does not look like a prereq"
 }
 
+# Read from stdin into the variable given in $1.
+test_read_to_eof () {
+	# Bash's "read" is sufficiently flexible that we can skip the extra
+	# process.
+	if test -n "$BASH_VERSION"
+	then
+		# 64k should be enough for anyone...
+		read -N 65536 -r "$1"
+	else
+		# command substitution eats trailing whitespace, so we add
+		# and then remove a non-whitespace character.
+		eval "$1=\$(cat; printf x)"
+		eval "$1=\${$1%x}"
+	fi
+}
+
 test_expect_failure () {
 	test_start_
 	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test-expect-failure"
+	if test "$2" = "-"
+	then
+		test_read_to_eof test_block
+		set -- "$1" "$test_block"
+	fi
 	test_verify_prereq
 	export test_prereq
 	if ! test_skip "$@"
@@ -416,6 +437,11 @@ test_expect_success () {
 	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test-expect-success"
+	if test "$2" = "-"
+	then
+		test_read_to_eof test_block
+		set -- "$1" "$test_block"
+	fi
 	test_verify_prereq
 	export test_prereq
 	if ! test_skip "$@"

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] t3900: add some more quotes
  2018-01-10 19:53   ` Jeff King
@ 2018-01-10 21:31     ` Junio C Hamano
  2018-01-11  9:38       ` Jeff King
  2018-01-11 11:39     ` read test snippet from stdin [was: [PATCH] t3900: add some more quotes] SZEDER Gábor
  1 sibling, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2018-01-10 21:31 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Sixt, Beat Bolli, git, Johannes Schindelin

Jeff King <peff@peff.net> writes:

> Yeah. One of the reasons for both of the errors in this thread is the
> nested double-quoting. Using single quotes is awkward because we're
> already using them to delimit the whole snippet.  I've often wondered if
> our tests would be more readable taking the snippet over stdin.
> Something like:
> +test_expect_success 'UTF-8 invalid characters refused' - <<\EOT
> +	test_when_finished 'rm -f "$HOME/stderr $HOME/invalid"' &&
> ...
> -'
> +EOT
> 
> +# Read from stdin into the variable given in $1.
> +test_read_to_eof () {
> +	# Bash's "read" is sufficiently flexible that we can skip the extra
> +	# process.
> +	if test -n "$BASH_VERSION"
> +	then
> +		# 64k should be enough for anyone...
> +		read -N 65536 -r "$1"
> +	else
> +		# command substitution eats trailing whitespace, so we add
> +		# and then remove a non-whitespace character.
> +		eval "$1=\$(cat; printf x)"
> +		eval "$1=\${$1%x}"
> +	fi
> +}

Yuck.  Hacky but nice.

I think this will make it easier to read but I suspect here text
would use temporary files and may slow things down a bit?  I do not
know if it is even measurable, though.

> +
>  test_expect_failure () {
>  	test_start_
>  	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
>  	test "$#" = 2 ||
>  	error "bug in the test script: not 2 or 3 parameters to test-expect-failure"
> +	if test "$2" = "-"
> +	then
> +		test_read_to_eof test_block
> +		set -- "$1" "$test_block"
> +	fi
>  	test_verify_prereq
>  	export test_prereq
>  	if ! test_skip "$@"
> @@ -416,6 +437,11 @@ test_expect_success () {
>  	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
>  	test "$#" = 2 ||
>  	error "bug in the test script: not 2 or 3 parameters to test-expect-success"
> +	if test "$2" = "-"
> +	then
> +		test_read_to_eof test_block
> +		set -- "$1" "$test_block"
> +	fi
>  	test_verify_prereq
>  	export test_prereq
>  	if ! test_skip "$@"

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2] t3900: add some more quotes
  2018-01-10 17:44 ` Eric Sunshine
@ 2018-01-10 22:38   ` " Beat Bolli
  2018-01-10 23:09     ` Johannes Schindelin
  2018-01-10 23:09     ` Junio C Hamano
  0 siblings, 2 replies; 14+ messages in thread
From: Beat Bolli @ 2018-01-10 22:38 UTC (permalink / raw)
  To: sunshine; +Cc: dev+git, git, johannes.schindelin, peff

In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
were added to protect against spaces in $HOME. In the test_when_finished
handler, two files are deleted which must be quoted individually.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---

Diff to v1:

s/hander/handler/ in the message.

 t/t3900-i18n-commit.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index 9e4e694d9..dc00db87b 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -40,7 +40,7 @@ test_expect_success 'UTF-16 refused because of NULs' '
 '
 
 test_expect_success 'UTF-8 invalid characters refused' '
-	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+	test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
 	echo "UTF-8 characters" >F &&
 	printf "Commit message\n\nInvalid surrogate:\355\240\200\n" \
 		>"$HOME/invalid" &&
@@ -49,7 +49,7 @@ test_expect_success 'UTF-8 invalid characters refused' '
 '
 
 test_expect_success 'UTF-8 overlong sequences rejected' '
-	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+	test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
 	rm -f "$HOME/stderr" "$HOME/invalid" &&
 	echo "UTF-8 overlong" >F &&
 	printf "\340\202\251ommit message\n\nThis is not a space:\300\240\n" \
@@ -59,7 +59,7 @@ test_expect_success 'UTF-8 overlong sequences rejected' '
 '
 
 test_expect_success 'UTF-8 non-characters refused' '
-	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+	test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
 	echo "UTF-8 non-character 1" >F &&
 	printf "Commit message\n\nNon-character:\364\217\277\276\n" \
 		>"$HOME/invalid" &&
@@ -68,7 +68,7 @@ test_expect_success 'UTF-8 non-characters refused' '
 '
 
 test_expect_success 'UTF-8 non-characters refused' '
-	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+	test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&
 	echo "UTF-8 non-character 2." >F &&
 	printf "Commit message\n\nNon-character:\357\267\220\n" \
 		>"$HOME/invalid" &&
-- 
2.15.0.rc1.299.gda03b47c3


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] t3900: add some more quotes
  2018-01-10 22:38   ` [PATCH v2] " Beat Bolli
@ 2018-01-10 23:09     ` Johannes Schindelin
  2018-01-10 23:09     ` Junio C Hamano
  1 sibling, 0 replies; 14+ messages in thread
From: Johannes Schindelin @ 2018-01-10 23:09 UTC (permalink / raw)
  To: Beat Bolli; +Cc: sunshine, git, peff

Hi Beat,

On Wed, 10 Jan 2018, Beat Bolli wrote:

> diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
> index 9e4e694d9..dc00db87b 100755
> --- a/t/t3900-i18n-commit.sh
> +++ b/t/t3900-i18n-commit.sh
> @@ -40,7 +40,7 @@ test_expect_success 'UTF-16 refused because of NULs' '
>  '
>  
>  test_expect_success 'UTF-8 invalid characters refused' '
> -	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
> +	test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&

It is probably worth quoting the dollar characters, too, as pointed out by
Hannes Sixt: we want to interpolate the value only when needed, just in
case that the HOME variable contains double quotes.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] t3900: add some more quotes
  2018-01-10 22:38   ` [PATCH v2] " Beat Bolli
  2018-01-10 23:09     ` Johannes Schindelin
@ 2018-01-10 23:09     ` Junio C Hamano
  1 sibling, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2018-01-10 23:09 UTC (permalink / raw)
  To: Beat Bolli; +Cc: sunshine, git, johannes.schindelin, peff

Beat Bolli <dev+git@drbeat.li> writes:

> In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
> were added to protect against spaces in $HOME. In the test_when_finished
> handler, two files are deleted which must be quoted individually.
>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
>
> Diff to v1:
>
> s/hander/handler/ in the message.
> ...

OK, but that forgets to fix a more important issue raised in the
discussion, no?

Here is what I ended up queuing in the meantime.  Thanks.

-- >8 --
From: Beat Bolli <dev+git@drbeat.li>
Date: Wed, 10 Jan 2018 10:58:32 +0100
Subject: [PATCH] t3900: add some more quotes

In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
were added to protect against spaces in $HOME. In the test_when_finished
command, two files are deleted which must be quoted individually.

[jc: with \$HOME in the test_when_finished command quoted, as
pointed out by j6t].

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3900-i18n-commit.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index 9e4e694d93..b92ff95977 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -40,7 +40,7 @@ test_expect_success 'UTF-16 refused because of NULs' '
 '
 
 test_expect_success 'UTF-8 invalid characters refused' '
-	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+	test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\"" &&
 	echo "UTF-8 characters" >F &&
 	printf "Commit message\n\nInvalid surrogate:\355\240\200\n" \
 		>"$HOME/invalid" &&
@@ -49,7 +49,7 @@ test_expect_success 'UTF-8 invalid characters refused' '
 '
 
 test_expect_success 'UTF-8 overlong sequences rejected' '
-	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+	test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\"" &&
 	rm -f "$HOME/stderr" "$HOME/invalid" &&
 	echo "UTF-8 overlong" >F &&
 	printf "\340\202\251ommit message\n\nThis is not a space:\300\240\n" \
@@ -59,7 +59,7 @@ test_expect_success 'UTF-8 overlong sequences rejected' '
 '
 
 test_expect_success 'UTF-8 non-characters refused' '
-	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+	test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\"" &&
 	echo "UTF-8 non-character 1" >F &&
 	printf "Commit message\n\nNon-character:\364\217\277\276\n" \
 		>"$HOME/invalid" &&
@@ -68,7 +68,7 @@ test_expect_success 'UTF-8 non-characters refused' '
 '
 
 test_expect_success 'UTF-8 non-characters refused' '
-	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+	test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\"" &&
 	echo "UTF-8 non-character 2." >F &&
 	printf "Commit message\n\nNon-character:\357\267\220\n" \
 		>"$HOME/invalid" &&
-- 
2.16.0-rc1-187-g8dee184084


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] t3900: add some more quotes
  2018-01-10 21:31     ` Junio C Hamano
@ 2018-01-11  9:38       ` Jeff King
  0 siblings, 0 replies; 14+ messages in thread
From: Jeff King @ 2018-01-11  9:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Sixt, Beat Bolli, git, Johannes Schindelin

On Wed, Jan 10, 2018 at 01:31:22PM -0800, Junio C Hamano wrote:

> > +# Read from stdin into the variable given in $1.
> > +test_read_to_eof () {
> > +	# Bash's "read" is sufficiently flexible that we can skip the extra
> > +	# process.
> > +	if test -n "$BASH_VERSION"
> > +	then
> > +		# 64k should be enough for anyone...
> > +		read -N 65536 -r "$1"
> > +	else
> > +		# command substitution eats trailing whitespace, so we add
> > +		# and then remove a non-whitespace character.
> > +		eval "$1=\$(cat; printf x)"
> > +		eval "$1=\${$1%x}"
> > +	fi
> > +}
> 
> Yuck.  Hacky but nice.
> 
> I think this will make it easier to read but I suspect here text
> would use temporary files and may slow things down a bit?  I do not
> know if it is even measurable, though.

Yeah, since I was able to contain the horrible-ness in this function, I
think the overall readability/portability is probably OK. My main
concern was that it would be slower (hence the bash hackery). I applied
the patch below on top to see what kind of impact we could measure
across the whole suite:

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index be8a47d304..6f2e6e7560 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -441,6 +441,15 @@ test_expect_success () {
 	then
 		test_read_to_eof test_block
 		set -- "$1" "$test_block"
+	else
+		# this is obviously a dumb thing to do, but it's just
+		# a performance-testing hack.
+		test_read_to_eof test_block <<EOF
+$2
+EOF
+		# our here-doc hackery added an extra newline
+		set -- "$1" "${test_block%
+}"
 	fi
 	test_verify_prereq
 	export test_prereq

After doing five trial timings each of "make test" on:

  - stock git with dash
  - this patch with dash (so using "cat" to read the here-doc)
  - stock git with bash
  - this patch with bash (so using an internal "read")

I couldn't come up with any definitive slowdown.

In the first two, there's a fair bit of run-to-run noise (easily 10%),
and my best run was actually _with_ the patch (by only 3%, but still the
opposite of what you'd expect).

Running the (stock) suite with bash is definitively slower than with
dash (by about 20%). Adding in this patch didn't seem to make it any
slower.

So I dunno. Maybe it's not so crazy an idea.

-Peff

^ permalink raw reply	[flat|nested] 14+ messages in thread

* read test snippet from stdin [was: [PATCH] t3900: add some more quotes]
  2018-01-10 19:53   ` Jeff King
  2018-01-10 21:31     ` Junio C Hamano
@ 2018-01-11 11:39     ` SZEDER Gábor
  2018-01-11 12:11       ` Jeff King
  1 sibling, 1 reply; 14+ messages in thread
From: SZEDER Gábor @ 2018-01-11 11:39 UTC (permalink / raw)
  To: Jeff King; +Cc: SZEDER Gábor, Johannes Sixt, Beat Bolli, git, Johannes Schindelin


> I've often wondered if
> our tests would be more readable taking the snippet over stdin.
> Something like:
> 
> diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
> index 9e4e694d93..09ad4d8878 100755
> --- a/t/t3900-i18n-commit.sh
> +++ b/t/t3900-i18n-commit.sh
> @@ -39,14 +39,14 @@ test_expect_success 'UTF-16 refused because of NULs' '
>  	test_must_fail git commit -a -F "$TEST_DIRECTORY"/t3900/UTF-16.txt
>  '
>  
> -test_expect_success 'UTF-8 invalid characters refused' '

Note that the test snippet started right after that last single quote,
i.e. it started with a newline.

> -	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
> +test_expect_success 'UTF-8 invalid characters refused' - <<\EOT
> +	test_when_finished 'rm -f "$HOME/stderr $HOME/invalid"' &&

And now it starts at the beginning of this line, i.e. without that
leading neline.  This change leads to the following when run with '-v':

  expecting success: 	test_when_finished 'rm -f "$HOME/stderr $HOME/invalid"' &&
	echo "UTF-8 characters" >F &&
	printf "Commit message\n\nInvalid surrogate:\355\240\200\n" \
		>"$HOME/invalid" &&
	git commit -a -F "$HOME/invalid" 2>"$HOME"/stderr &&
	test_i18ngrep "did not conform" "$HOME"/stderr

Notice how the "expecting success" and the first line of the test ended
up in the same line.  I find this more annoying than the lack of empty
line between the colored and indented test code and the uncolored and
unindented test output.

>  	echo "UTF-8 characters" >F &&
>  	printf "Commit message\n\nInvalid surrogate:\355\240\200\n" \
>  		>"$HOME/invalid" &&
>  	git commit -a -F "$HOME/invalid" 2>"$HOME"/stderr &&
>  	test_i18ngrep "did not conform" "$HOME"/stderr
> -'
> +EOT
>  
>  test_expect_success 'UTF-8 overlong sequences rejected' '
>  	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 1701fe2a06..be8a47d304 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -391,11 +391,32 @@ test_verify_prereq () {
>  	error "bug in the test script: '$test_prereq' does not look like a prereq"
>  }
>  
> +# Read from stdin into the variable given in $1.
> +test_read_to_eof () {
> +	# Bash's "read" is sufficiently flexible that we can skip the extra
> +	# process.
> +	if test -n "$BASH_VERSION"
> +	then
> +		# 64k should be enough for anyone...
> +		read -N 65536 -r "$1"
> +	else
> +		# command substitution eats trailing whitespace, so we add
> +		# and then remove a non-whitespace character.
> +		eval "$1=\$(cat; printf x)"
> +		eval "$1=\${$1%x}"
> +	fi
> +}

Command substitutions don't eat trailing whitespaces in general, only
trailing newlines.  POSIX:

  The shell shall expand the command substitution by executing command
  in a subshell environment (see Shell Execution Environment) and
  replacing the command substitution (the text of command plus the
  enclosing "$()" or backquotes) with the standard output of the
  command, removing sequences of one or more <newline>s at the end of
  the substitution.

Bash and dash conform to this.

How about this alternative (also adding the missing leading newline
mentioned above):

+		eval "$1='
+'\$(cat)'
+'"

The indentation is yuck, but overall perhaps a bit less hacky...


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: read test snippet from stdin [was: [PATCH] t3900: add some more quotes]
  2018-01-11 11:39     ` read test snippet from stdin [was: [PATCH] t3900: add some more quotes] SZEDER Gábor
@ 2018-01-11 12:11       ` Jeff King
  0 siblings, 0 replies; 14+ messages in thread
From: Jeff King @ 2018-01-11 12:11 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Johannes Sixt, Beat Bolli, git, Johannes Schindelin

On Thu, Jan 11, 2018 at 12:39:28PM +0100, SZEDER Gábor wrote:

> > diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
> > index 9e4e694d93..09ad4d8878 100755
> > --- a/t/t3900-i18n-commit.sh
> > +++ b/t/t3900-i18n-commit.sh
> > @@ -39,14 +39,14 @@ test_expect_success 'UTF-16 refused because of NULs' '
> >  	test_must_fail git commit -a -F "$TEST_DIRECTORY"/t3900/UTF-16.txt
> >  '
> >  
> > -test_expect_success 'UTF-8 invalid characters refused' '
> 
> Note that the test snippet started right after that last single quote,
> i.e. it started with a newline.
> 
> > -	test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
> > +test_expect_success 'UTF-8 invalid characters refused' - <<\EOT
> > +	test_when_finished 'rm -f "$HOME/stderr $HOME/invalid"' &&
> 
> And now it starts at the beginning of this line, i.e. without that
> leading neline.  This change leads to the following when run with '-v':

Yeah, I noticed that, too. It would be easy enough to add the extra
newline ourselves when printing the verbose output (quite a few of our
older tests don't start with a newline already, so it would be improving
them, too).

Alternatively, I considered adding a whole new helper function that
would skip the need to say "-" as the test body. And then it would
always know we were reading from the here-doc.

> > +		# command substitution eats trailing whitespace, so we add
> > +		# and then remove a non-whitespace character.
> > +		eval "$1=\$(cat; printf x)"
> > +		eval "$1=\${$1%x}"
> > +	fi
> > +}
> 
> Command substitutions don't eat trailing whitespaces in general, only
> trailing newlines.  POSIX:

Yeah, I wondered about that, but didn't bother digging since the
solution is the same either way.

> How about this alternative (also adding the missing leading newline
> mentioned above):
> 
> +		eval "$1='
> +'\$(cat)'
> +'"
> 
> The indentation is yuck, but overall perhaps a bit less hacky...

I wrote something very similar at first, before finding the printf trick
on Stack Overflow. I thought the indentation on what I wrote was too
ugly. :)

I don't have a strong preference, and certainly if one is more portable
than the other, we should choose that.

The main point of my email was just to say "do people even like the
concept/direction?"

-Peff

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-10  9:58 [PATCH] t3900: add some more quotes Beat Bolli
2018-01-10 10:33 ` Jeff King
2018-01-10 17:21   ` Johannes Schindelin
2018-01-10 17:44 ` Eric Sunshine
2018-01-10 22:38   ` [PATCH v2] " Beat Bolli
2018-01-10 23:09     ` Johannes Schindelin
2018-01-10 23:09     ` Junio C Hamano
2018-01-10 19:02 ` [PATCH] " Johannes Sixt
2018-01-10 19:43   ` Junio C Hamano
2018-01-10 19:53   ` Jeff King
2018-01-10 21:31     ` Junio C Hamano
2018-01-11  9:38       ` Jeff King
2018-01-11 11:39     ` read test snippet from stdin [was: [PATCH] t3900: add some more quotes] SZEDER Gábor
2018-01-11 12:11       ` Jeff King

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox