[Patch v1 0/3] 2.21.0-rc0 test fixes resulting from use of /dev/zero

[Patch v1 0/3] 2.21.0-rc0 test fixes resulting from use of /dev/zero

[randall.s.becker]

From: "Randall S. Becker" <randall.becker@nexbridge.ca>

This is a candidate packages of fixes to remove dependence on /dev/zero
and replaces it with the generate_zero_bytes function in test-lib-functions.sh

Randall S. Becker (3):
  test-lib-functions.sh: add generate_zero_bytes function
  t5318: replace use of /dev/zero with generate_zero_bytes
  t5562: replace /dev/zero with a pipe from generate_zero_bytes

 t/t5318-commit-graph.sh                |  2 +-
 t/t5562-http-backend-content-length.sh |  4 ++--
 t/test-lib-functions.sh                | 13 +++++++++++++
 3 files changed, 16 insertions(+), 3 deletions(-)

-- 
2.12.3

[Patch v1 1/3] test-lib-functions.sh: add generate_zero_bytes function

[randall.s.becker]

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

t5318 and t5562 used /dev/zero, which is not portable. This function
provides both a fixed block of NUL bytes and an infinite stream of NULs.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 t/test-lib-functions.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 92cf8f812..bbf68712c 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -116,6 +116,19 @@ remove_cr () {
 	tr '\015' Q | sed -e 's/Q$//'
 }
 
+# Generate an output of $1 bytes of all zeroes (NULs, not ASCII zeroes).
+# If $1 is 'infinity', output forever or until the receiving pipe stops reading,
+# whichever comes first.
+generate_zero_bytes () {
+	perl -e 'if ($ARGV[0] == "infinity") {
+		while (-1) {
+			print "\0"
+		}
+	} else {
+		print "\0" x $ARGV[0]
+	}' "$@"
+}
+
 # In some bourne shell implementations, the "unset" builtin returns
 # nonzero status when a variable to be unset was not set in the first
 # place.
-- 
2.12.3

Re: [Patch v1 1/3] test-lib-functions.sh: add generate_zero_bytes function

[Eric Sunshine]

On Sat, Feb 9, 2019 at 1:59 PM <randall.s.becker@rogers.com> wrote:
> t5318 and t5562 used /dev/zero, which is not portable. This function
> provides both a fixed block of NUL bytes and an infinite stream of NULs.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> @@ -116,6 +116,19 @@ remove_cr () {
> +# Generate an output of $1 bytes of all zeroes (NULs, not ASCII zeroes).
> +# If $1 is 'infinity', output forever or until the receiving pipe stops reading,
> +# whichever comes first.

This is a somewhat unusual API. A (perhaps) more intuitive behavior
would be for it to emit an infinite stream of NULs when given no
argument, and a limited number of NULs when given an argument.
Redefining the behavior like that also fixes the "problem" with the
current implementation erroring-out if no argument is provided.

> +generate_zero_bytes () {
> +       perl -e 'if ($ARGV[0] == "infinity") {

s/perl/"$PERL_PATH"/

> +               while (-1) {
> +                       print "\0"
> +               }

Or, more compactly:

    print "\0" while 1;

> +       } else {
> +               print "\0" x $ARGV[0]
> +       }' "$@"
> +}

RE: [Patch v1 1/3] test-lib-functions.sh: add generate_zero_bytes function

[Randall S. Becker]

On February 9, 2019 21:05, Eric Sunshine wrote:
> On Sat, Feb 9, 2019 at 1:59 PM <randall.s.becker@rogers.com> wrote:
> > t5318 and t5562 used /dev/zero, which is not portable. This function
> > provides both a fixed block of NUL bytes and an infinite stream of NULs.
> >
> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > ---
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh @@
> > -116,6 +116,19 @@ remove_cr () {
> > +# Generate an output of $1 bytes of all zeroes (NULs, not ASCII zeroes).
> > +# If $1 is 'infinity', output forever or until the receiving pipe
> > +stops reading, # whichever comes first.
> 
> This is a somewhat unusual API. A (perhaps) more intuitive behavior would be
> for it to emit an infinite stream of NULs when given no argument, and a
> limited number of NULs when given an argument.
> Redefining the behavior like that also fixes the "problem" with the current
> implementation erroring-out if no argument is provided.

At this point, I've supplied three different ways to solve this incompatibility for platforms not Linux and others have also provided fixes and discussed this at length. It is not a specific fix that matters to me, but that there is a fix at all. So thanks for all your comments and I will wait on direction on what the team wants me to do about it, if anything.

Regards,
Randall

Re: [Patch v1 1/3] test-lib-functions.sh: add generate_zero_bytes function

[Jeff King]

On Sat, Feb 09, 2019 at 09:05:04PM -0500, Eric Sunshine wrote:

> On Sat, Feb 9, 2019 at 1:59 PM <randall.s.becker@rogers.com> wrote:
> > t5318 and t5562 used /dev/zero, which is not portable. This function
> > provides both a fixed block of NUL bytes and an infinite stream of NULs.
> >
> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > ---
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > @@ -116,6 +116,19 @@ remove_cr () {
> > +# Generate an output of $1 bytes of all zeroes (NULs, not ASCII zeroes).
> > +# If $1 is 'infinity', output forever or until the receiving pipe stops reading,
> > +# whichever comes first.
> 
> This is a somewhat unusual API. A (perhaps) more intuitive behavior
> would be for it to emit an infinite stream of NULs when given no
> argument, and a limited number of NULs when given an argument.
> Redefining the behavior like that also fixes the "problem" with the
> current implementation erroring-out if no argument is provided.

Yeah, I agree that'd be a little more idiomatic for our code base, but
I'm fine with it either way.

> > +generate_zero_bytes () {
> > +       perl -e 'if ($ARGV[0] == "infinity") {
> 
> s/perl/"$PERL_PATH"/

This shouldn't be necessary. perl() is a function that uses $PERL_PATH
(so you only need $PERL_PATH when you're writing out another script that
doesn't run in the same process space as the rest of the test code).

-Peff

Re: [Patch v1 1/3] test-lib-functions.sh: add generate_zero_bytes function

[Eric Sunshine]

On Mon, Feb 11, 2019 at 7:37 PM Jeff King <peff@peff.net> wrote:
> On Sat, Feb 09, 2019 at 09:05:04PM -0500, Eric Sunshine wrote:
> > On Sat, Feb 9, 2019 at 1:59 PM <randall.s.becker@rogers.com> wrote:
> > > +generate_zero_bytes () {
> > > +       perl -e 'if ($ARGV[0] == "infinity") {
> >
> > s/perl/"$PERL_PATH"/
>
> This shouldn't be necessary. perl() is a function that uses $PERL_PATH
> (so you only need $PERL_PATH when you're writing out another script that
> doesn't run in the same process space as the rest of the test code).

Thanks for clarifying. I either didn't know or forgot about that.

RE: [Patch v1 1/3] test-lib-functions.sh: add generate_zero_bytes function

[randall.s.becker]

On February 11, 2019 20:18, Eric Sunshine wrote:
> On Mon, Feb 11, 2019 at 7:37 PM Jeff King <peff@peff.net> wrote:
> > On Sat, Feb 09, 2019 at 09:05:04PM -0500, Eric Sunshine wrote:
> > > On Sat, Feb 9, 2019 at 1:59 PM <randall.s.becker@rogers.com> wrote:
> > > > +generate_zero_bytes () {
> > > > +       perl -e 'if ($ARGV[0] == "infinity") {
> > >
> > > s/perl/"$PERL_PATH"/
> >
> > This shouldn't be necessary. perl() is a function that uses $PERL_PATH
> > (so you only need $PERL_PATH when you're writing out another script
> > that doesn't run in the same process space as the rest of the test code).
> 
> Thanks for clarifying. I either didn't know or forgot about that.

As did I, thank you.

[Patch v1 2/3] t5318: replace use of /dev/zero with generate_zero_bytes

[randall.s.becker]

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

This change removes the dependency on /dev/zero with generate_zero_bytes
appending NUL values to blocks generating wrong signatures for test cases.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 t/t5318-commit-graph.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 16d10ebce..65c1f45b0 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -383,7 +383,7 @@ corrupt_graph_and_verify() {
 	cp $objdir/info/commit-graph commit-graph-backup &&
 	printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
 	dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 &&
-	dd if=/dev/zero of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=$(($orig_size - $zero_pos)) &&
+	generate_zero_bytes $(($orig_size - $zero_pos)) >> "$objdir/info/commit-graph" &&
 	test_must_fail git commit-graph verify 2>test_err &&
 	grep -v "^+" test_err >err &&
 	test_i18ngrep "$grepstr" err
-- 
2.12.3

Re: [Patch v1 2/3] t5318: replace use of /dev/zero with generate_zero_bytes

[Eric Sunshine]

On Sat, Feb 9, 2019 at 2:00 PM <randall.s.becker@rogers.com> wrote:
> This change removes the dependency on /dev/zero with generate_zero_bytes
> appending NUL values to blocks generating wrong signatures for test cases.

This commit message says what the patch does but not _why_. At
minimum, it should explain that /dev/zero is not available on all
platforms, therefore, not portable, and (perhaps) cite NonStop as an
example.

> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> @@ -383,7 +383,7 @@ corrupt_graph_and_verify() {
> -       dd if=/dev/zero of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=$(($orig_size - $zero_pos)) &&
> +       generate_zero_bytes $(($orig_size - $zero_pos)) >> "$objdir/info/commit-graph" &&

Re: [Patch v1 2/3] t5318: replace use of /dev/zero with generate_zero_bytes

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sat, Feb 9, 2019 at 2:00 PM <randall.s.becker@rogers.com> wrote:
>> This change removes the dependency on /dev/zero with generate_zero_bytes
>> appending NUL values to blocks generating wrong signatures for test cases.
>
> This commit message says what the patch does but not _why_. At
> minimum, it should explain that /dev/zero is not available on all
> platforms, therefore, not portable, and (perhaps) cite NonStop as an
> example.

Does sombody want to do the honors?  [PATCH 1/3] would become wasted
effort until that happens.  On the other hand, if this is not urgent
(it is only urgent for those without /dev/zero, and to others it may
be distraction/disruption this close to the final release to add
increased risk of fat finger mistakes), obviously I can wait.

>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
>> @@ -383,7 +383,7 @@ corrupt_graph_and_verify() {
>> -       dd if=/dev/zero of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=$(($orig_size - $zero_pos)) &&
>> +       generate_zero_bytes $(($orig_size - $zero_pos)) >> "$objdir/info/commit-graph" &&

The original says "skip to the $zero_pos location in an existing
info/commit-graph file, and overwrite its existing contents up to
the $orig_size location with NULs".

If I recall the implementation of generate_zero_bytes in [PATCH 1/3]
correctly, it just generated stream of NULs of given number of bytes.

The only reason why this rewrite is not wrong is because the
previous line (omitted in your quote) truncates the file to
$zero_pos.  That is a bit tricky ;-).

Re: [Patch v1 2/3] t5318: replace use of /dev/zero with generate_zero_bytes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Sat, Feb 9, 2019 at 2:00 PM <randall.s.becker@rogers.com> wrote:
>>> This change removes the dependency on /dev/zero with generate_zero_bytes
>>> appending NUL values to blocks generating wrong signatures for test cases.
>>
>> This commit message says what the patch does but not _why_. At
>> minimum, it should explain that /dev/zero is not available on all
>> platforms, therefore, not portable, and (perhaps) cite NonStop as an
>> example.
>
> Does sombody want to do the honors?  [PATCH 1/3] would become wasted
> effort until that happens.  On the other hand, if this is not urgent
> (it is only urgent for those without /dev/zero, and to others it may
> be distraction/disruption this close to the final release to add
> increased risk of fat finger mistakes), obviously I can wait.

So, before I lose the access to my primary screen (I was told that
somehow I need to reimage the workstation today X-<), here is what I
have now.

-- >8 --
From: "Randall S. Becker" <rsbecker@nexbridge.com>
Date: Sat, 9 Feb 2019 13:59:29 -0500
Subject: [PATCH] t5318: replace use of /dev/zero with generate_zero_bytes

There are platforms (e.g. NonStop) that lack /dev/zero; use the
generate_zero_bytes helper we just introduced to append stream
of NULs at the end of the file.

The original, even though it uses "dd seek=... count=..." to make it
look like it is overwriting the middle part of an existing file, has
truncated the file before this step with another use of "dd", which
may make it tricky to see why this rewrite is a correct one.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5318-commit-graph.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 16d10ebce8..d4bd1522fe 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -383,7 +383,7 @@ corrupt_graph_and_verify() {
 	cp $objdir/info/commit-graph commit-graph-backup &&
 	printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
 	dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 &&
-	dd if=/dev/zero of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=$(($orig_size - $zero_pos)) &&
+	generate_zero_bytes $(($orig_size - $zero_pos)) >>"$objdir/info/commit-graph" &&
 	test_must_fail git commit-graph verify 2>test_err &&
 	grep -v "^+" test_err >err &&
 	test_i18ngrep "$grepstr" err
-- 
2.21.0-rc0-36-ge9bd4aa026

RE: [Patch v1 2/3] t5318: replace use of /dev/zero with generate_zero_bytes

[Randall S. Becker]

> -----Original Message-----
> From: Junio C Hamano <jch2355@gmail.com> On Behalf Of Junio C Hamano
> Sent: February 13, 2019 12:25
> To: Eric Sunshine <sunshine@sunshineco.us>
> Cc: randall.s.becker@rogers.com; Git List <git@vger.kernel.org>; Randall
S.
> Becker <rsbecker@nexbridge.com>
> Subject: Re: [Patch v1 2/3] t5318: replace use of /dev/zero with
> generate_zero_bytes
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> >
> >> On Sat, Feb 9, 2019 at 2:00 PM <randall.s.becker@rogers.com> wrote:
> >>> This change removes the dependency on /dev/zero with
> >>> generate_zero_bytes appending NUL values to blocks generating wrong
> signatures for test cases.
> >>
> >> This commit message says what the patch does but not _why_. At
> >> minimum, it should explain that /dev/zero is not available on all
> >> platforms, therefore, not portable, and (perhaps) cite NonStop as an
> >> example.
> >
> > Does sombody want to do the honors?  [PATCH 1/3] would become wasted
> > effort until that happens.  On the other hand, if this is not urgent
> > (it is only urgent for those without /dev/zero, and to others it may
> > be distraction/disruption this close to the final release to add
> > increased risk of fat finger mistakes), obviously I can wait.
> 
> So, before I lose the access to my primary screen (I was told that somehow
I
> need to reimage the workstation today X-<), here is what I have now.
> 
> -- >8 --
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> Date: Sat, 9 Feb 2019 13:59:29 -0500
> Subject: [PATCH] t5318: replace use of /dev/zero with generate_zero_bytes
> 
> There are platforms (e.g. NonStop) that lack /dev/zero; use the
> generate_zero_bytes helper we just introduced to append stream of NULs at
> the end of the file.
> 
> The original, even though it uses "dd seek=... count=..." to make it look
like it
> is overwriting the middle part of an existing file, has truncated the file
before
> this step with another use of "dd", which may make it tricky to see why
this
> rewrite is a correct one.

Here is how I interpret the test - might be wrong, but yanno...
The first dd copies something looking like reasonable data from the test
case.
The second dd copies zeros from seek to the end of a fixed size block.

My first attempt at a fix used truncate that extended the first to the
correct size (filling with zeros). My worry there is that I'm not sure there
is a guarantee of zeros, but that shouldn't matter for the test which just
wants a signature mismatch.

Others suggested using yes to fill in junk.

My second attempt was to create the generate_zero_bytes function to replace
exactly what the second dd was doing but not user /dev/zero. The fix was not
to change the conditions of the test - not debating the correctness of that
here - but to simply replicate the use of /dev/zero in context. So the
resulting file contains [reasonable-stuff]{seek}[0]{orig_size-seek}, which
is sufficient to satisfy the conditions of the test.

> 
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t5318-commit-graph.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index
> 16d10ebce8..d4bd1522fe 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -383,7 +383,7 @@ corrupt_graph_and_verify() {
>  	cp $objdir/info/commit-graph commit-graph-backup &&
>  	printf "$data" | dd of="$objdir/info/commit-graph" bs=1
> seek="$pos" conv=notrunc &&
>  	dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0
> &&
> -	dd if=/dev/zero of="$objdir/info/commit-graph" bs=1
> seek="$zero_pos" count=$(($orig_size - $zero_pos)) &&
> +	generate_zero_bytes $(($orig_size - $zero_pos))
> +>>"$objdir/info/commit-graph" &&
>  	test_must_fail git commit-graph verify 2>test_err &&
>  	grep -v "^+" test_err >err &&
>  	test_i18ngrep "$grepstr" err

Re: [Patch v1 2/3] t5318: replace use of /dev/zero with generate_zero_bytes

[Junio C Hamano]

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

> My second attempt was to create the generate_zero_bytes function to replace
> exactly what the second dd was doing but not user /dev/zero.

Yes, and I think the patch does that ;-)  It was just the original

    dd if=/dev/zero of=... bs=1 seek=$there count=$this_many

would have been impossible to rewrite with the new generate_zero_bytes
helper unless $there weren't seeking to the end of the file.

But the other dd before the one the patch rewrites truncates the
file to make that seek=$there seeking to the end of the file, so
simply appending output from genereate_zero_bytes is sufficient and
correct conversion.  I wanted to explain that for future readers who
may wonder if the patch is doing the exact conversion.

RE: [Patch v1 2/3] t5318: replace use of /dev/zero with generate_zero_bytes

[Randall S. Becker]

On February 13, 2019 16:01, Junio C Hamano wrote:
> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
> 
> > My second attempt was to create the generate_zero_bytes function to
> > replace exactly what the second dd was doing but not user /dev/zero.
> 
> Yes, and I think the patch does that ;-)  It was just the original
> 
>     dd if=/dev/zero of=... bs=1 seek=$there count=$this_many
> 
> would have been impossible to rewrite with the new generate_zero_bytes
> helper unless $there weren't seeking to the end of the file.
> 
> But the other dd before the one the patch rewrites truncates the file to
make
> that seek=$there seeking to the end of the file, so simply appending
output
> from genereate_zero_bytes is sufficient and correct conversion.  I wanted
to
> explain that for future readers who may wonder if the patch is doing the
> exact conversion.

Sounds like we need a documentation patch in the actual test suite rather
than in the commit <ducking>.

Cheers,
Randall

[Patch v1 3/3] t5562: replace /dev/zero with a pipe from generate_zero_bytes

[randall.s.becker]

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

This change removes the dependency on /dev/zero with an equivalent  pipe of
deliberately NUL bytes. This allows tests to proceed where /dev/zero
does not exist.

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..bbadde2c6 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 \
+	generate_zero_bytes infinity  | 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

Re: [Patch v1 3/3] t5562: replace /dev/zero with a pipe from generate_zero_bytes

[Eric Sunshine]

On Sat, Feb 9, 2019 at 1:59 PM <randall.s.becker@rogers.com> wrote:
> This change removes the dependency on /dev/zero with an equivalent  pipe of

Too many spaces between "equivalent" and "pipe".

> deliberately NUL bytes. This allows tests to proceed where /dev/zero
> does not exist.

It wouldn't hurt to cite "NonStop" as an example of a platform lacking
/dev/zero.

The first sentence is a bit off grammatically. Perhaps the entire
commit message can be collapsed simply to:

    Stop depending on /dev/zero which may not be available on all
    platforms (for instance, HP NonStop).

> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> diff --git 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 \
> +       generate_zero_bytes infinity  | 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
>  '

Re: [Patch v1 3/3] t5562: replace /dev/zero with a pipe from generate_zero_bytes

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

[jc: Summoning Dscho, J6t for their Windows expertise at the end]

> On Sat, Feb 9, 2019 at 1:59 PM <randall.s.becker@rogers.com> wrote:
>> This change removes the dependency on /dev/zero with an equivalent  pipe of
>
> Too many spaces between "equivalent" and "pipe".
>
>> deliberately NUL bytes. This allows tests to proceed where /dev/zero
>> does not exist.
>
> It wouldn't hurt to cite "NonStop" as an example of a platform lacking
> /dev/zero.
>
> The first sentence is a bit off grammatically. Perhaps the entire
> commit message can be collapsed simply to:
>
>     Stop depending on /dev/zero which may not be available on all
>     platforms (for instance, HP NonStop).
>
>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>> diff --git 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 \
>> +       generate_zero_bytes infinity  | 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 &&

Doesn't this "inifinity" mode have the same issue that was worked
around by 6129c930 ("test-lib: limit the output of the yes utility",
2016-02-02) on Windows?  If I read correctly, the process upstream
of the pipe (in this case, perl producing a stream of infinite NULs)
would not die when the downstream stops reading with SIGPIPE.

Or is it safe to assume that nobody expects to use http-backend on
Windows based servers so this test is a non-issue?

>>         grep "fatal:.*CONTENT_LENGTH" err
>>  '

Thanks.

Re: [Patch v1 3/3] t5562: replace /dev/zero with a pipe from generate_zero_bytes

[Johannes Sixt]

Am 12.02.19 um 18:24 schrieb Junio C Hamano:
>>> diff --git 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 \
>>> +       generate_zero_bytes infinity  | 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 &&
> 
> Doesn't this "inifinity" mode have the same issue that was worked
> around by 6129c930 ("test-lib: limit the output of the yes utility",
> 2016-02-02) on Windows?  If I read correctly, the process upstream
> of the pipe (in this case, perl producing a stream of infinite NULs)
> would not die when the downstream stops reading with SIGPIPE.

I think we do not have to worry, and the reason is that the
justification for 6129c930 is simply wrong.

As I did not find the patch series discussed here to pull and test, I
repeated the timing tests with t7610-mergetool.sh with and without
6129c930 reverted, and the difference is only in the noise. The reason
t7610 takes so long on Windows looks more like a consequence of the
10,000 processes that it spawns. It is a mystery to me how I came to the
conclusion that the change in 6129c930 would make a difference. :-(

-- Hannes

Re: [Patch v1 3/3] t5562: replace /dev/zero with a pipe from generate_zero_bytes

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 12.02.19 um 18:24 schrieb Junio C Hamano:
>>>> diff --git 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 \
>>>> +       generate_zero_bytes infinity  | 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 &&
>> 
>> Doesn't this "inifinity" mode have the same issue that was worked
>> around by 6129c930 ("test-lib: limit the output of the yes utility",
>> 2016-02-02) on Windows?  If I read correctly, the process upstream
>> of the pipe (in this case, perl producing a stream of infinite NULs)
>> would not die when the downstream stops reading with SIGPIPE.
>
> I think we do not have to worry, and the reason is that the
> justification for 6129c930 is simply wrong.

That's kinda surprising but in a pleasant way---it's good that we
have one less thing we need to worry about.

Thanks.

>
> As I did not find the patch series discussed here to pull and test, I
> repeated the timing tests with t7610-mergetool.sh with and without
> 6129c930 reverted, and the difference is only in the noise. The reason
> t7610 takes so long on Windows looks more like a consequence of the
> 10,000 processes that it spawns. It is a mystery to me how I came to the
> conclusion that the change in 6129c930 would make a difference. :-(
>
> -- Hannes

[PATCH] t5562: do not depend on /dev/zero

[Max Kirillov]

It was reported [1] that NonStop platform does not have /dev/zero.

The test uses /dev/zero as a dummy input. Passing case (http-backed
failed because of too big input size) should not be reading anything
from it. If http-backend would erroneously try to read any data
returning EOF probably would be even safer than providing some
meaningless data.

Replace /dev/zero with /dev/null to avoid issues with platforms which do
not have /dev/zero.

[1] https://public-inbox.org/git/20190209185930.5256-4-randall.s.becker@rogers.com/

Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
Signed-off-by: Max Kirillov <max@max630.net>
---
By the way, I don't think this requires such sofisticated
fix. In the success case the input would not be read at all.
You could replace it with /dev/null, the in failure (not 
immediate fail) git would fail due to truncated input or
something.

Also, as you experience hang issue [2] in earlier tests, this 
one should not have contributed to it.

[2] https://public-inbox.org/git/001901d4c22b$194bfe60$4be3fb20$@nexbridge.com/
 t/t5562-http-backend-content-length.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
index 90d890d02f..436c261c86 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -150,7 +150,7 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '
 		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 >/dev/null 2>err &&
 	grep "fatal:.*CONTENT_LENGTH" err
 '
 
-- 
2.19.0.1202.g68e1e8f04e

RE: [PATCH] t5562: do not depend on /dev/zero

[Randall S. Becker]

On February 15, 2019 11:43, Max Kirillov wrote:
> It was reported [1] that NonStop platform does not have /dev/zero.
> 
> The test uses /dev/zero as a dummy input. Passing case (http-backed failed
> because of too big input size) should not be reading anything from it. If
http-
> backend would erroneously try to read any data returning EOF probably
> would be even safer than providing some meaningless data.
> 
> Replace /dev/zero with /dev/null to avoid issues with platforms which do
not
> have /dev/zero.
> 
> [1] https://public-inbox.org/git/20190209185930.5256-4-
> randall.s.becker@rogers.com/
> 
> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
> By the way, I don't think this requires such sofisticated fix. In the
success
> case the input would not be read at all.
> You could replace it with /dev/null, the in failure (not immediate fail)
git
> would fail due to truncated input or something.
> 
> Also, as you experience hang issue [2] in earlier tests, this one should
not
> have contributed to it.
> 
> [2] https://public-
> inbox.org/git/001901d4c22b$194bfe60$4be3fb20$@nexbridge.com/
>  t/t5562-http-backend-content-length.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t5562-http-backend-content-length.sh
b/t/t5562-http-backend-
> content-length.sh
> index 90d890d02f..436c261c86 100755
> --- a/t/t5562-http-backend-content-length.sh
> +++ b/t/t5562-http-backend-content-length.sh
> @@ -150,7 +150,7 @@ test_expect_success 'CONTENT_LENGTH overflow
> ssite_t' '
>  		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 >/dev/null 2>err &&
>  	grep "fatal:.*CONTENT_LENGTH" err

FTR, this particular subtest is not the one that is hanging. This subtest
passes on NonStop with any and all (now) 4 solutions that have been floating
around.

Cheers,
Randall

Re: [PATCH] t5562: do not depend on /dev/zero

[Junio C Hamano]

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

> FTR, this particular subtest is not the one that is hanging. This subtest
> passes on NonStop with any and all (now) 4 solutions that have been floating
> around.

One thing I'd like to know more is if this test passes on NonStop
with this patch, i.e. /dev/zero replaced with /dev/null.

Thanks.

RE: [PATCH] t5562: do not depend on /dev/zero

[Randall S. Becker]

On February 15, 2019 13:01, Junio C Hamano wrote:
> To: Randall S. Becker <rsbecker@nexbridge.com>
> Cc: 'Max Kirillov' <max@max630.net>; git@vger.kernel.org; 'Johannes
> Schindelin' <Johannes.Schindelin@gmx.de>
> Subject: Re: [PATCH] t5562: do not depend on /dev/zero
> 
> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
> 
> > FTR, this particular subtest is not the one that is hanging. This
> > subtest passes on NonStop with any and all (now) 4 solutions that have
> > been floating around.
> 
> One thing I'd like to know more is if this test passes on NonStop with
this
> patch, i.e. /dev/zero replaced with /dev/null.

Yes, this particular subtest passes replacing /dev/null. The other three
subtests still hang. This subtest never did.

Re: [PATCH] t5562: do not depend on /dev/zero

[Junio C Hamano]

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

> On February 15, 2019 13:01, Junio C Hamano wrote:
>> To: Randall S. Becker <rsbecker@nexbridge.com>
>> Cc: 'Max Kirillov' <max@max630.net>; git@vger.kernel.org; 'Johannes
>> Schindelin' <Johannes.Schindelin@gmx.de>
>> Subject: Re: [PATCH] t5562: do not depend on /dev/zero
>> 
>> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
>> 
>> > FTR, this particular subtest is not the one that is hanging. This
>> > subtest passes on NonStop with any and all (now) 4 solutions that have
>> > been floating around.
>> 
>> One thing I'd like to know more is if this test passes on NonStop with
> this
>> patch, i.e. /dev/zero replaced with /dev/null.
>
> Yes, this particular subtest passes replacing /dev/null. The other three
> subtests still hang. This subtest never did.

Thanks.