git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [Fix v2] t5562: remove dependency on /dev/zero
@ 2019-02-08 22:07 randall.s.becker
  2019-02-08 22:37 ` Jeff King
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: randall.s.becker @ 2019-02-08 22:07 UTC (permalink / raw)
  To: git; +Cc: Randall S. Becker

From: "Randall S. Becker" <rsbecker@nexbridge.com>

Replaced subtest 15 (CONTENT_LENGTH overflow ssite_t) use of /dev/zero
with yes and a translation of its result to a stream of NULL. This is
a more portable solution.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 t/t5562-http-backend-content-length.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
index 90d890d02..b8d1913e5 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -143,14 +143,14 @@ test_expect_success GZIP 'push gzipped empty' '
 
 test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
 	NOT_FIT_IN_SSIZE=$(ssize_b100dots) &&
-	env \
+	yes | tr "y" "\\0" | env \
 		CONTENT_TYPE=application/x-git-upload-pack-request \
 		QUERY_STRING=/repo.git/git-upload-pack \
 		PATH_TRANSLATED="$PWD"/.git/git-upload-pack \
 		GIT_HTTP_EXPORT_ALL=TRUE \
 		REQUEST_METHOD=POST \
 		CONTENT_LENGTH="$NOT_FIT_IN_SSIZE" \
-		git http-backend </dev/zero >/dev/null 2>err &&
+		git http-backend >/dev/null 2>err &&
 	grep "fatal:.*CONTENT_LENGTH" err
 '
 
-- 
2.12.3


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

* Re: [Fix v2] t5562: remove dependency on /dev/zero
  2019-02-08 22:07 [Fix v2] t5562: remove dependency on /dev/zero randall.s.becker
@ 2019-02-08 22:37 ` Jeff King
  2019-02-08 23:38 ` Junio C Hamano
  2019-02-09  8:46 ` Johannes Sixt
  2 siblings, 0 replies; 8+ messages in thread
From: Jeff King @ 2019-02-08 22:37 UTC (permalink / raw)
  To: randall.s.becker; +Cc: git, Randall S. Becker

On Fri, Feb 08, 2019 at 05:07:51PM -0500, randall.s.becker@rogers.com wrote:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> 
> Replaced subtest 15 (CONTENT_LENGTH overflow ssite_t) use of /dev/zero
> with yes and a translation of its result to a stream of NULL. This is
> a more portable solution.

They're NULs, not NULLs. :)

I think in this case, though, we don't actually care about the bytes,
and you can drop the "tr" invocation entirely.

-Peff

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

* Re: [Fix v2] t5562: remove dependency on /dev/zero
  2019-02-08 22:07 [Fix v2] t5562: remove dependency on /dev/zero randall.s.becker
  2019-02-08 22:37 ` Jeff King
@ 2019-02-08 23:38 ` Junio C Hamano
  2019-02-09 17:00   ` Randall S. Becker
  2019-02-09  8:46 ` Johannes Sixt
  2 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2019-02-08 23:38 UTC (permalink / raw)
  To: randall.s.becker; +Cc: git, Randall S. Becker

randall.s.becker@rogers.com writes:

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> Replaced subtest 15 (CONTENT_LENGTH overflow ssite_t) use of /dev/zero
> with yes and a translation of its result to a stream of NULL. This is
> a more portable solution.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  t/t5562-http-backend-content-length.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
> index 90d890d02..b8d1913e5 100755
> --- a/t/t5562-http-backend-content-length.sh
> +++ b/t/t5562-http-backend-content-length.sh
> @@ -143,14 +143,14 @@ test_expect_success GZIP 'push gzipped empty' '
>  
>  test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
>  	NOT_FIT_IN_SSIZE=$(ssize_b100dots) &&
> -	env \
> +	yes | tr "y" "\\0" | env \

I do not quite get this use of tr.  The original feeds a stream of
NULs out of /dev/zero to the command; the yes-to-tr pipe instead
feeds a stream of alternating NUL and LF.

Does the actual bytes fed to the consumer make any difference?  If
not, perhaps we can use 'yes' as-is?

>  		CONTENT_TYPE=application/x-git-upload-pack-request \
>  		QUERY_STRING=/repo.git/git-upload-pack \
>  		PATH_TRANSLATED="$PWD"/.git/git-upload-pack \
>  		GIT_HTTP_EXPORT_ALL=TRUE \
>  		REQUEST_METHOD=POST \
>  		CONTENT_LENGTH="$NOT_FIT_IN_SSIZE" \
> -		git http-backend </dev/zero >/dev/null 2>err &&
> +		git http-backend >/dev/null 2>err &&
>  	grep "fatal:.*CONTENT_LENGTH" err
>  '

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

* Re: [Fix v2] t5562: remove dependency on /dev/zero
  2019-02-08 22:07 [Fix v2] t5562: remove dependency on /dev/zero randall.s.becker
  2019-02-08 22:37 ` Jeff King
  2019-02-08 23:38 ` Junio C Hamano
@ 2019-02-09  8:46 ` Johannes Sixt
  2019-02-09 18:25   ` Junio C Hamano
  2 siblings, 1 reply; 8+ messages in thread
From: Johannes Sixt @ 2019-02-09  8:46 UTC (permalink / raw)
  To: randall.s.becker; +Cc: git, Randall S. Becker

Am 08.02.19 um 23:07 schrieb randall.s.becker@rogers.com:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> 
> Replaced subtest 15 (CONTENT_LENGTH overflow ssite_t) use of /dev/zero
> with yes and a translation of its result to a stream of NULL. This is
> a more portable solution.
> 
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  t/t5562-http-backend-content-length.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
> index 90d890d02..b8d1913e5 100755
> --- a/t/t5562-http-backend-content-length.sh
> +++ b/t/t5562-http-backend-content-length.sh
> @@ -143,14 +143,14 @@ test_expect_success GZIP 'push gzipped empty' '
>  
>  test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
>  	NOT_FIT_IN_SSIZE=$(ssize_b100dots) &&
> -	env \
> +	yes | tr "y" "\\0" | env \
>  		CONTENT_TYPE=application/x-git-upload-pack-request \
>  		QUERY_STRING=/repo.git/git-upload-pack \
>  		PATH_TRANSLATED="$PWD"/.git/git-upload-pack \
>  		GIT_HTTP_EXPORT_ALL=TRUE \
>  		REQUEST_METHOD=POST \
>  		CONTENT_LENGTH="$NOT_FIT_IN_SSIZE" \
> -		git http-backend </dev/zero >/dev/null 2>err &&
> +		git http-backend >/dev/null 2>err &&
>  	grep "fatal:.*CONTENT_LENGTH" err
>  '

How many bytes are needed here? yes() in test-lib.sh generates only 99
'y', if I am not mistaken.

-- Hannes

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

* RE: [Fix v2] t5562: remove dependency on /dev/zero
  2019-02-08 23:38 ` Junio C Hamano
@ 2019-02-09 17:00   ` Randall S. Becker
  0 siblings, 0 replies; 8+ messages in thread
From: Randall S. Becker @ 2019-02-09 17:00 UTC (permalink / raw)
  To: 'Junio C Hamano'; +Cc: git

On February 8, 2019 18:39, Junio C Hamano wrote:
> randall.s.becker@rogers.com writes:
> > Replaced subtest 15 (CONTENT_LENGTH overflow ssite_t) use of /dev/zero
> > with yes and a translation of its result to a stream of NULL. This is
> > a more portable solution.
> >
> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > ---
> >  t/t5562-http-backend-content-length.sh | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/t/t5562-http-backend-content-length.sh
> > b/t/t5562-http-backend-content-length.sh
> > index 90d890d02..b8d1913e5 100755
> > --- a/t/t5562-http-backend-content-length.sh
> > +++ b/t/t5562-http-backend-content-length.sh
> > @@ -143,14 +143,14 @@ test_expect_success GZIP 'push gzipped empty' '
> >
> >  test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
> >  	NOT_FIT_IN_SSIZE=$(ssize_b100dots) &&
> > -	env \
> > +	yes | tr "y" "\\0" | env \
> 
> I do not quite get this use of tr.  The original feeds a stream of NULs
out of
> /dev/zero to the command; the yes-to-tr pipe instead feeds a stream of
> alternating NUL and LF.

That's why we're going to go with a generate_zero_bytes function per Peff.
I'm working on a more comprehensive patch covering t5562, t5318, and
test-lib-functions.sh that will (hopefully) be satisfactory and remove the
dependency on /dev/zero and fixes the related new breakages in 2.21.0-rc0.

The test case in t5318 is specific about wanting zero bytes although the
test is just intending to generate a corrupt block that generates a
different hash, so yes 'yes' might be sufficient, but I don't like
randomness myself if we're taking two different tests being involved.

My current intent is to add to test-lib-functions.sh, a method of
generalizing blocks of zeros to a pipe:

+# Generate an output of $1 bytes of all zeroes (NULs, not ASCII zeroes).
+# If $1 is < 0, output forever or until the receiving pipe stops reading,
whichever comes first.
+ generate_zero_bytes () {
+ 	perl -e ' if ($ARGV[0] < 0) { while (-1) { print "\0" } } else {
print "\0" x $ARGV[0] }' "$@"
+ }

And then fit that into the two tests, then submit as a patch.

> Does the actual bytes fed to the consumer make any difference?  If not,
> perhaps we can use 'yes' as-is?
> 
> >  		CONTENT_TYPE=application/x-git-upload-pack-request \
> >  		QUERY_STRING=/repo.git/git-upload-pack \
> >  		PATH_TRANSLATED="$PWD"/.git/git-upload-pack \
> >  		GIT_HTTP_EXPORT_ALL=TRUE \
> >  		REQUEST_METHOD=POST \
> >  		CONTENT_LENGTH="$NOT_FIT_IN_SSIZE" \
> > -		git http-backend </dev/zero >/dev/null 2>err &&
> > +		git http-backend >/dev/null 2>err &&
> >  	grep "fatal:.*CONTENT_LENGTH" err
> >  '

Regards,
Randall


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

* Re: [Fix v2] t5562: remove dependency on /dev/zero
  2019-02-09  8:46 ` Johannes Sixt
@ 2019-02-09 18:25   ` Junio C Hamano
  2019-02-09 19:07     ` Randall S. Becker
  2019-02-09 21:54     ` Johannes Sixt
  0 siblings, 2 replies; 8+ messages in thread
From: Junio C Hamano @ 2019-02-09 18:25 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: randall.s.becker, git, Randall S. Becker

Johannes Sixt <j6t@kdbg.org> writes:

> How many bytes are needed here? yes() in test-lib.sh generates only 99
> 'y', if I am not mistaken.

I think we will not use "yes" in the end for this topic, which makes
this comment totally irrelevant to the thread, but I wonder why we
have the limit of 99 there?  It cannot be "we do not want to worry
about sigpipe" affecting the end result of the test (after all the
reader may stop reading from after reading just one, and the status
of the upstream process that would die with sigpipe is lost anyway).

It turns out it is about sigpipe ;-) but in somewhat a different
way.  To prevent others from wasting their time wondering about
this, probably we want to have something like the attached?

 t/README      | 9 +++++++++
 t/test-lib.sh | 6 +++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 1326fd7505..f4e1a82657 100644
--- a/t/README
+++ b/t/README
@@ -927,6 +927,15 @@ library for your script to use.
    test_oid_init or test_oid_cache.  Providing an unknown key is an
    error.
 
+ - yes [<string>]
+
+   This is often seen in modern UNIX but some platforms lack it, so
+   the test harness overrides the platform implementation with a
+   more limited one.  Use this only when feeding a handful lines of
+   output to the downstream---unlike the real version, it generates
+   only up to 99 lines.
+
+
 Prerequisites
 -------------
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 42b1a0aa7f..541a37f4c0 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1313,7 +1313,11 @@ then
 	fi
 fi
 
-# Provide an implementation of the 'yes' utility
+# Provide an implementation of the 'yes' utility; the upper bound
+# limit is there to help Windows that cannot stop this loop from
+# wasting cycles when the downstream stops reading, so do not be
+# tempted to turn it into an infinite loop. cf. 6129c930 ("test-lib:
+# limit the output of the yes utility", 2016-02-02)
 yes () {
 	if test $# = 0
 	then

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

* RE: [Fix v2] t5562: remove dependency on /dev/zero
  2019-02-09 18:25   ` Junio C Hamano
@ 2019-02-09 19:07     ` Randall S. Becker
  2019-02-09 21:54     ` Johannes Sixt
  1 sibling, 0 replies; 8+ messages in thread
From: Randall S. Becker @ 2019-02-09 19:07 UTC (permalink / raw)
  To: 'Junio C Hamano', 'Johannes Sixt'
  Cc: git, 'Randall S. Becker'

On February 9, 2019 13:25, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > How many bytes are needed here? yes() in test-lib.sh generates only 99
> > 'y', if I am not mistaken.
> 
> I think we will not use "yes" in the end for this topic, which makes this
> comment totally irrelevant to the thread, but I wonder why we have the
limit
> of 99 there?  It cannot be "we do not want to worry about sigpipe"
affecting
> the end result of the test (after all the reader may stop reading from
after
> reading just one, and the status of the upstream process that would die
with
> sigpipe is lost anyway).
> 
> It turns out it is about sigpipe ;-) but in somewhat a different way.  To
> prevent others from wasting their time wondering about this, probably we
> want to have something like the attached?
> 
>  t/README      | 9 +++++++++
>  t/test-lib.sh | 6 +++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/t/README b/t/README
> index 1326fd7505..f4e1a82657 100644
> --- a/t/README
> +++ b/t/README
> @@ -927,6 +927,15 @@ library for your script to use.
>     test_oid_init or test_oid_cache.  Providing an unknown key is an
>     error.
> 
> + - yes [<string>]
> +
> +   This is often seen in modern UNIX but some platforms lack it, so
> +   the test harness overrides the platform implementation with a
> +   more limited one.  Use this only when feeding a handful lines of
> +   output to the downstream---unlike the real version, it generates
> +   only up to 99 lines.
> +
> +
>  Prerequisites
>  -------------
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh index 42b1a0aa7f..541a37f4c0
100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1313,7 +1313,11 @@ then
>  	fi
>  fi
> 
> -# Provide an implementation of the 'yes' utility
> +# Provide an implementation of the 'yes' utility; the upper bound #
> +limit is there to help Windows that cannot stop this loop from #
> +wasting cycles when the downstream stops reading, so do not be #
> +tempted to turn it into an infinite loop. cf. 6129c930 ("test-lib:
> +# limit the output of the yes utility", 2016-02-02)
>  yes () {
>  	if test $# = 0
>  	then

Sadly, I already the other path ready, but did not have a chance to send it.
I'm ok either way as long as I can get the tests running.

Regards,
Randall


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

* Re: [Fix v2] t5562: remove dependency on /dev/zero
  2019-02-09 18:25   ` Junio C Hamano
  2019-02-09 19:07     ` Randall S. Becker
@ 2019-02-09 21:54     ` Johannes Sixt
  1 sibling, 0 replies; 8+ messages in thread
From: Johannes Sixt @ 2019-02-09 21:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: randall.s.becker, git, Randall S. Becker

Am 09.02.19 um 19:25 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> How many bytes are needed here? yes() in test-lib.sh generates only 99
>> 'y', if I am not mistaken.
> 
> I think we will not use "yes" in the end for this topic, which makes
> this comment totally irrelevant to the thread, but I wonder why we
> have the limit of 99 there?  It cannot be "we do not want to worry
> about sigpipe" affecting the end result of the test (after all the
> reader may stop reading from after reading just one, and the status
> of the upstream process that would die with sigpipe is lost anyway).
> 
> It turns out it is about sigpipe ;-) but in somewhat a different
> way.  To prevent others from wasting their time wondering about
> this, probably we want to have something like the attached?

That would certainly be helpful!

-- Hannes

> 
>  t/README      | 9 +++++++++
>  t/test-lib.sh | 6 +++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/t/README b/t/README
> index 1326fd7505..f4e1a82657 100644
> --- a/t/README
> +++ b/t/README
> @@ -927,6 +927,15 @@ library for your script to use.
>     test_oid_init or test_oid_cache.  Providing an unknown key is an
>     error.
>  
> + - yes [<string>]
> +
> +   This is often seen in modern UNIX but some platforms lack it, so
> +   the test harness overrides the platform implementation with a
> +   more limited one.  Use this only when feeding a handful lines of
> +   output to the downstream---unlike the real version, it generates
> +   only up to 99 lines.
> +
> +
>  Prerequisites
>  -------------
>  
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 42b1a0aa7f..541a37f4c0 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1313,7 +1313,11 @@ then
>  	fi
>  fi
>  
> -# Provide an implementation of the 'yes' utility
> +# Provide an implementation of the 'yes' utility; the upper bound
> +# limit is there to help Windows that cannot stop this loop from
> +# wasting cycles when the downstream stops reading, so do not be
> +# tempted to turn it into an infinite loop. cf. 6129c930 ("test-lib:
> +# limit the output of the yes utility", 2016-02-02)
>  yes () {
>  	if test $# = 0
>  	then
> 


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

end of thread, other threads:[~2019-02-09 21:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-08 22:07 [Fix v2] t5562: remove dependency on /dev/zero randall.s.becker
2019-02-08 22:37 ` Jeff King
2019-02-08 23:38 ` Junio C Hamano
2019-02-09 17:00   ` Randall S. Becker
2019-02-09  8:46 ` Johannes Sixt
2019-02-09 18:25   ` Junio C Hamano
2019-02-09 19:07     ` Randall S. Becker
2019-02-09 21:54     ` Johannes Sixt

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).