Re: read test snippet from stdin [was: [PATCH] t3900: add some more quotes]
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
read test snippet from stdin [was: [PATCH] t3900: add some more quotes]
> 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 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...
Re: [PATCH] t3900: add some more quotes
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 <
Re: [PATCH] t3900: add some more quotes
Jeff King 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 "$@"
Re: [PATCH] t3900: add some more quotes
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 "$@"
Re: [PATCH] t3900: add some more quotes
Johannes Sixt 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 ;-)
Re: [PATCH] t3900: add some more quotes
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 --- 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
Re: [PATCH] t3900: add some more quotes
On Wed, Jan 10, 2018 at 4:58 AM, 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. s/hander/handler/ > Signed-off-by: Beat Bolli
Re: [PATCH] t3900: add some more quotes
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
Re: [PATCH] t3900: add some more quotes
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
[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 hander, two files are deleted which must be quoted individually. Signed-off-by: Beat Bolli --- 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