diff mbox series

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

Message ID 20190209185930.5256-4-randall.s.becker@rogers.com (mailing list archive)
State New, archived
Headers show
Series 2.21.0-rc0 test fixes resulting from use of /dev/zero | expand

Commit Message

Randall S. Becker Feb. 9, 2019, 6:59 p.m. UTC 
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(-)

Comments

Eric Sunshine Feb. 10, 2019, 2:12 a.m. UTC | #1 
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
>  '
Junio C Hamano Feb. 12, 2019, 5:24 p.m. UTC | #2 
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.
Johannes Sixt Feb. 12, 2019, 8:50 p.m. UTC | #3 
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
Junio C Hamano Feb. 13, 2019, 5:26 p.m. UTC | #4 
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
diff mbox series

Patch

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
 '