[Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Randall S. Becker]

Hi All,

t5318 is rather problematic and I have no good way to fix this. There is no /dev/zero on the platform, and the corrupt_graph_and_verify hard-codes if=/dev/zero, which is a linux-specific pseudo device. Please provide a more platform independent way of testing this feature. Pretty much all subtests from 46 onward fail as a result.

expecting success:
        corrupt_graph_and_verify 0 "\0" \
                "graph signature"

1+0 records in
1+0 records out
1 byte copied, 0.002248 s, 0.4 kB/s
0+0 records in
0+0 records out
0 bytes copied, 0.001815 s, 0.0 kB/s
dd: failed to open '/dev/zero': No such file or directory
not ok 46 - detect bad signature
#
#               corrupt_graph_and_verify 0 "\0" \
#                       "graph signature"
#

Regards,
Randall

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Jeff King]

On Fri, Feb 08, 2019 at 06:08:33AM -0500, Randall S. Becker wrote:

> t5318 is rather problematic and I have no good way to fix this. There
> is no /dev/zero on the platform, and the corrupt_graph_and_verify
> hard-codes if=/dev/zero, which is a linux-specific pseudo device.
> Please provide a more platform independent way of testing this
> feature. Pretty much all subtests from 46 onward fail as a result.

We did discuss this at the time of the patch, but it seems we already
use /dev/zero in a bunch of places:

  https://public-inbox.org/git/xmqqbm57rkg5.fsf@gitster-ct.c.googlers.com/

Were you just skipping the other tests before?

-Peff

RE: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Randall S. Becker]

On February 8, 2019 11:51, Jeff King wrote:
> On Fri, Feb 08, 2019 at 06:08:33AM -0500, Randall S. Becker wrote:
> 
> > t5318 is rather problematic and I have no good way to fix this. There
> > is no /dev/zero on the platform, and the corrupt_graph_and_verify
> > hard-codes if=/dev/zero, which is a linux-specific pseudo device.
> > Please provide a more platform independent way of testing this
> > feature. Pretty much all subtests from 46 onward fail as a result.
> 
> We did discuss this at the time of the patch, but it seems we already use
> /dev/zero in a bunch of places:
> 
>   https://public-inbox.org/git/xmqqbm57rkg5.fsf@gitster-ct.c.googlers.com/
> 
> Were you just skipping the other tests before?

I did not catch the implications of the review at the time - my bad. We were not intentionally skipping the tests. It looks like some are automatically skipped. t4153 automatically skips (missing TTY), and t5562 fails also but for a different reason (hang - we don't have apache2 to serve up http content).

Would you object to something like this:

if [ ! -e /dev/zero ]; then
	# use shred or some other mechanism (still trying to figure out a solution)
else
	# existing dd
fi

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Jeff King]

On Fri, Feb 08, 2019 at 12:49:59PM -0500, Randall S. Becker wrote:

> > We did discuss this at the time of the patch, but it seems we already use
> > /dev/zero in a bunch of places:
> > 
> >   https://public-inbox.org/git/xmqqbm57rkg5.fsf@gitster-ct.c.googlers.com/
> > 
> > Were you just skipping the other tests before?
> 
> I did not catch the implications of the review at the time - my bad. We were not intentionally skipping the tests. It looks like some are automatically skipped. t4153 automatically skips (missing TTY), and t5562 fails also but for a different reason (hang - we don't have apache2 to serve up http content).
> 
> Would you object to something like this:
> 
> if [ ! -e /dev/zero ]; then
> 	# use shred or some other mechanism (still trying to figure out a solution)
> else
> 	# existing dd
> fi

That's fine, as long as it's wrapped up in a function in order to keep
the tests readable.

Though I suspect we may be able to just find a solution that works
everywhere, without having two different implementations. If we know we
need $count bytes for dd, we could probably just generate a file with
that many NULs in it.

Other cases don't seem to actually care that they're getting NULs, and
are just redirecting stdin from /dev/zero to get an infinite amount of
input. They could probably use "yes" for that.

-Peff

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Johannes Sixt]

Am 08.02.19 um 19:03 schrieb Jeff King:
> On Fri, Feb 08, 2019 at 12:49:59PM -0500, Randall S. Becker wrote:
>> Would you object to something like this:
>>
>> if [ ! -e /dev/zero ]; then
>> 	# use shred or some other mechanism (still trying to figure out a solution)
>> else
>> 	# existing dd
>> fi
> 
> That's fine, as long as it's wrapped up in a function in order to keep
> the tests readable.
> 
> Though I suspect we may be able to just find a solution that works
> everywhere, without having two different implementations. If we know we
> need $count bytes for dd, we could probably just generate a file with
> that many NULs in it.
> 
> Other cases don't seem to actually care that they're getting NULs, and
> are just redirecting stdin from /dev/zero to get an infinite amount of
> input. They could probably use "yes" for that.

If the data does not have to be a sequence of zero bytes, the
alternatives are:

* `test-genrandom seed-string $size` for a sequence of reproducible
"random" bytes

* `printf "%0*d" $size 0` for a sequence of '0' characters.

In t5318, the zero bytes do matter, though.

-- Hannes

RE: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Randall S. Becker]

On February 8, 2019 13:03, Jeff King wrote:
> To: Randall S. Becker <rsbecker@nexbridge.com>
> Cc: 'Junio C Hamano' <gitster@pobox.com>; git@vger.kernel.org; 'Linux
> Kernel' <linux-kernel@vger.kernel.org>; git-packagers@googlegroups.com
> Subject: Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)
> 
> On Fri, Feb 08, 2019 at 12:49:59PM -0500, Randall S. Becker wrote:
> 
> > > We did discuss this at the time of the patch, but it seems we
> > > already use /dev/zero in a bunch of places:
> > >
> > >
> > > https://public-inbox.org/git/xmqqbm57rkg5.fsf@gitster-ct.c.googlers.
> > > com/
> > >
> > > Were you just skipping the other tests before?
> >
> > I did not catch the implications of the review at the time - my bad. We
> were not intentionally skipping the tests. It looks like some are automatically
> skipped. t4153 automatically skips (missing TTY), and t5562 fails also but for
> a different reason (hang - we don't have apache2 to serve up http content).
> >
> > Would you object to something like this:
> >
> > if [ ! -e /dev/zero ]; then
> > 	# use shred or some other mechanism (still trying to figure out a
> > solution) else
> > 	# existing dd
> > fi
> 
> That's fine, as long as it's wrapped up in a function in order to keep the tests
> readable.
> 
> Though I suspect we may be able to just find a solution that works
> everywhere, without having two different implementations. If we know we
> need $count bytes for dd, we could probably just generate a file with that
> many NULs in it.

For this, we could use truncate -s count file instead of dd to get a fixed size file of nulls. This would remove the need for /dev/zero in t5318 (the patch below probably will wrap badly in my mailer so I can submit a real patch separately.

@@ -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)) &&
+       truncate -s $orig_size "$objdir/info/commit-graph" &&
        test_must_fail git commit-graph verify 2>test_err &&
        grep -v "^+" test_err >err &&
        test_i18ngrep "$grepstr" err

This passes on my platform.

> Other cases don't seem to actually care that they're getting NULs, and are
> just redirecting stdin from /dev/zero to get an infinite amount of input. They
> could probably use "yes" for that.

What about reading from /dev/null?

Regards,
Randall

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Jeff King]

On Fri, Feb 08, 2019 at 01:47:04PM -0500, Randall S. Becker wrote:

> > Though I suspect we may be able to just find a solution that works
> > everywhere, without having two different implementations. If we know we
> > need $count bytes for dd, we could probably just generate a file with that
> > many NULs in it.
> 
> For this, we could use truncate -s count file instead of dd to get a
> fixed size file of nulls. This would remove the need for /dev/zero in
> t5318 (the patch below probably will wrap badly in my mailer so I can
> submit a real patch separately.

I don't think "truncate" is portable, though.

> > Other cases don't seem to actually care that they're getting NULs, and are
> > just redirecting stdin from /dev/zero to get an infinite amount of input. They
> > could probably use "yes" for that.
> 
> What about reading from /dev/null?

That would yield zero bytes, not an infinite number of them.

-Peff

RE: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Randall S. Becker]

On February 8, 2019 14:15, Jeff King wrote:
> On Fri, Feb 08, 2019 at 01:47:04PM -0500, Randall S. Becker wrote:
> 
> > > Though I suspect we may be able to just find a solution that works
> > > everywhere, without having two different implementations. If we know
> > > we need $count bytes for dd, we could probably just generate a file
> > > with that many NULs in it.
> >
> > For this, we could use truncate -s count file instead of dd to get a
> > fixed size file of nulls. This would remove the need for /dev/zero in
> > t5318 (the patch below probably will wrap badly in my mailer so I can
> > submit a real patch separately.
> 
> I don't think "truncate" is portable, though.

It is available AFAIK on Linux, POSIX, and Windows under Cygwin. That's more than /dev/zero has anyway. I have the patch ready if you want it.

> > > Other cases don't seem to actually care that they're getting NULs,
> > > and are just redirecting stdin from /dev/zero to get an infinite
> > > amount of input. They could probably use "yes" for that.
> >
> > What about reading from /dev/null?
> 
> That would yield zero bytes, not an infinite number of them.

So something like: yes | tr 'y' '\0' | stuff?

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> If the data does not have to be a sequence of zero bytes, the
> alternatives are:
>
> * `test-genrandom seed-string $size` for a sequence of reproducible
> "random" bytes
>
> * `printf "%0*d" $size 0` for a sequence of '0' characters.
>
> In t5318, the zero bytes do matter, though.

Also some consumers of these bytes may not be easy to read from a
pipe.

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Jeff King]

On Fri, Feb 08, 2019 at 02:26:17PM -0500, Randall S. Becker wrote:

> > > For this, we could use truncate -s count file instead of dd to get a
> > > fixed size file of nulls. This would remove the need for /dev/zero in
> > > t5318 (the patch below probably will wrap badly in my mailer so I can
> > > submit a real patch separately.
> > 
> > I don't think "truncate" is portable, though.
> 
> It is available AFAIK on Linux, POSIX, and Windows under Cygwin.
> That's more than /dev/zero has anyway. I have the patch ready if you
> want it.

Is it POSIX? Certainly truncate() is, but I didn't think the
command-line tool was. If it really is available everywhere, then yeah,
I'd be fine with it.

> > > > Other cases don't seem to actually care that they're getting NULs,
> > > > and are just redirecting stdin from /dev/zero to get an infinite
> > > > amount of input. They could probably use "yes" for that.
> > >
> > > What about reading from /dev/null?
> > 
> > That would yield zero bytes, not an infinite number of them.
> 
> So something like: yes | tr 'y' '\0' | stuff?

Exactly (if we even care about them being NULs; otherwise, we can omit
the "tr" invocation).

-Peff

RE: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Randall S. Becker]

> -----Original Message-----
> From: Jeff King <peff@peff.net>
> Sent: February 8, 2019 14:32
> To: Randall S. Becker <rsbecker@nexbridge.com>
> Cc: 'Junio C Hamano' <gitster@pobox.com>; git@vger.kernel.org; 'Linux
> Kernel' <linux-kernel@vger.kernel.org>; git-packagers@googlegroups.com
> Subject: Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)
> 
> On Fri, Feb 08, 2019 at 02:26:17PM -0500, Randall S. Becker wrote:
> 
> > > > For this, we could use truncate -s count file instead of dd to get
> > > > a fixed size file of nulls. This would remove the need for
> > > > /dev/zero in
> > > > t5318 (the patch below probably will wrap badly in my mailer so I
> > > > can submit a real patch separately.
> > >
> > > I don't think "truncate" is portable, though.
> >
> > It is available AFAIK on Linux, POSIX, and Windows under Cygwin.
> > That's more than /dev/zero has anyway. I have the patch ready if you
> > want it.
> 
> Is it POSIX? Certainly truncate() is, but I didn't think the command-line tool
> was. If it really is available everywhere, then yeah, I'd be fine with it.
> 
> > > > > Other cases don't seem to actually care that they're getting
> > > > > NULs, and are just redirecting stdin from /dev/zero to get an
> > > > > infinite amount of input. They could probably use "yes" for that.
> > > >
> > > > What about reading from /dev/null?
> > >
> > > That would yield zero bytes, not an infinite number of them.
> >
> > So something like: yes | tr 'y' '\0' | stuff?
> 
> Exactly (if we even care about them being NULs; otherwise, we can omit the
> "tr" invocation).

I'm a bit perplexed about this... Obviously added some debugging info, but why we're getting No REQUEST_METHOD is perplexing. Is this a lack of an apache2 instance?

expecting success:
        NOT_FIT_IN_SSIZE=$(ssize_b100dots) &&
        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" \
                yes | tr "y" "\\0" | git http-backend 2>err &&
        echo "Err is" &&
        cat err &&
        grep "fatal:.*CONTENT_LENGTH" err

Status: 500 Internal Server Error
Expires: Fri, 01 Jan 1980 00:00:00 GMT
Pragma: no-cache
Cache-Control: no-cache, max-age=0, must-revalidate

Err is
fatal: No REQUEST_METHOD from server
not ok 15 - CONTENT_LENGTH overflow ssite_t

Cheers,
Randall

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Jeff King]

[-cc kernel and packagers; this is really just git development at this
point]

On Fri, Feb 08, 2019 at 03:38:05PM -0500, Randall S. Becker wrote:

> > Exactly (if we even care about them being NULs; otherwise, we can omit the
> > "tr" invocation).
> 
> I'm a bit perplexed about this... Obviously added some debugging info,
> but why we're getting No REQUEST_METHOD is perplexing. Is this a lack
> of an apache2 instance?

No, this shouldn't be using apache at all. But...

> expecting success:
>         NOT_FIT_IN_SSIZE=$(ssize_b100dots) &&
>         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" \
>                 yes | tr "y" "\\0" | git http-backend 2>err &&
>         echo "Err is" &&
>         cat err &&
>         grep "fatal:.*CONTENT_LENGTH" err
> 
> Status: 500 Internal Server Error
> Expires: Fri, 01 Jan 1980 00:00:00 GMT
> Pragma: no-cache
> Cache-Control: no-cache, max-age=0, must-revalidate
> 
> Err is
> fatal: No REQUEST_METHOD from server
> not ok 15 - CONTENT_LENGTH overflow ssite_t

The problem is that you're setting the environment now for "yes". You'd
need:

  yes | tr "y" "\\0" | env \
    CONTENT_TYPE=... \
    REQUEST_METHOD=POST \
    ...etc...
    git http-backend

-Peff

RE: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Randall S. Becker]

On February 8, 2019 16:01, Jeff King wrote:
> On Fri, Feb 08, 2019 at 03:38:05PM -0500, Randall S. Becker wrote:
> 
> > > Exactly (if we even care about them being NULs; otherwise, we can
> > > omit the "tr" invocation).
> >
> > I'm a bit perplexed about this... Obviously added some debugging info,
> > but why we're getting No REQUEST_METHOD is perplexing. Is this a lack
> > of an apache2 instance?
> 
> No, this shouldn't be using apache at all. But...
> 
> > expecting success:
> >         NOT_FIT_IN_SSIZE=$(ssize_b100dots) &&
> >         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" \
> >                 yes | tr "y" "\\0" | git http-backend 2>err &&
> >         echo "Err is" &&
> >         cat err &&
> >         grep "fatal:.*CONTENT_LENGTH" err
> >
> > Status: 500 Internal Server Error
> > Expires: Fri, 01 Jan 1980 00:00:00 GMT
> > Pragma: no-cache
> > Cache-Control: no-cache, max-age=0, must-revalidate
> >
> > Err is
> > fatal: No REQUEST_METHOD from server
> > not ok 15 - CONTENT_LENGTH overflow ssite_t
> 
> The problem is that you're setting the environment now for "yes". You'd
> need:
> 
>   yes | tr "y" "\\0" | env \
>     CONTENT_TYPE=... \
>     REQUEST_METHOD=POST \
>     ...etc...
>     git http-backend

Aw crap. I feel really silly now. Thanks. Will advise.

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

On Fri, Feb 08, 2019 at 02:31:57PM -0500, Jeff King wrote:
> > It is available AFAIK on Linux, POSIX, and Windows under Cygwin.
> > That's more than /dev/zero has anyway. I have the patch ready if you
> > want it.
> 
> Is it POSIX? Certainly truncate() is, but I didn't think the
> command-line tool was. If it really is available everywhere, then yeah,
> I'd be fine with it.

It's not. POSIX doesn't specify the command, and macOS lacks it, I
believe.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

RE: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Randall S. Becker]

On February 8, 2019 17:07, brian m. carlson wrote:
> On Fri, Feb 08, 2019 at 02:31:57PM -0500, Jeff King wrote:
> > > It is available AFAIK on Linux, POSIX, and Windows under Cygwin.
> > > That's more than /dev/zero has anyway. I have the patch ready if you
> > > want it.
> >
> > Is it POSIX? Certainly truncate() is, but I didn't think the
> > command-line tool was. If it really is available everywhere, then
> > yeah, I'd be fine with it.
> 
> It's not. POSIX doesn't specify the command, and macOS lacks it, I believe.

I'm happy to modify the test (it is in one spot), to make a decision based on:
a) whether /dev/zero exists
b) whether the system is a NonStop
c) something else

What would you all prefer? It doesn't matter to me one way or another, as long as I can get the dependency to /dev/zero removed so tests will run here.

Thanks,
Randall

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

On Fri, Feb 08, 2019 at 05:12:43PM -0500, Randall S. Becker wrote:
> I'm happy to modify the test (it is in one spot), to make a decision based on:
> a) whether /dev/zero exists
> b) whether the system is a NonStop
> c) something else
> 
> What would you all prefer? It doesn't matter to me one way or another, as long as I can get the dependency to /dev/zero removed so tests will run here.

My preference is that we wrap the yes/tr invocation into a function
(maybe "infinite_nul") and use that where we currently require
/dev/zero.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Jeff King]

On Fri, Feb 08, 2019 at 05:12:43PM -0500, Randall S. Becker wrote:

> On February 8, 2019 17:07, brian m. carlson wrote:
> > On Fri, Feb 08, 2019 at 02:31:57PM -0500, Jeff King wrote:
> > > > It is available AFAIK on Linux, POSIX, and Windows under Cygwin.
> > > > That's more than /dev/zero has anyway. I have the patch ready if you
> > > > want it.
> > >
> > > Is it POSIX? Certainly truncate() is, but I didn't think the
> > > command-line tool was. If it really is available everywhere, then
> > > yeah, I'd be fine with it.
> > 
> > It's not. POSIX doesn't specify the command, and macOS lacks it, I believe.
> 
> I'm happy to modify the test (it is in one spot), to make a decision based on:
> a) whether /dev/zero exists
> b) whether the system is a NonStop
> c) something else
> 
> What would you all prefer? It doesn't matter to me one way or another,
> as long as I can get the dependency to /dev/zero removed so tests will
> run here.

For the case in t5318, I think we can just put the NULs in a file. Does
this work on your platform?

---
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 16d10ebce8..6d0ccc7eba 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -383,7 +383,8 @@ 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)) &&
+	gen_zero_bytes $(($orig_size - $zero_pos)) >zeroes &&
+	dd if=zeroes of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" &&
 	test_must_fail git commit-graph verify 2>test_err &&
 	grep -v "^+" test_err >err &&
 	test_i18ngrep "$grepstr" err
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 92cf8f812c..4afab14431 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1302,3 +1302,8 @@ test_set_port () {
 	port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
 	eval $var=$port
 }
+
+# Generate an output of $1 bytes of all zeroes (NULs, not ASCII zeroes).
+gen_zero_bytes () {
+	perl -e 'print "\0" x $ARGV[0]' "$@"
+}

For the others that need infinite zeroes, I think using "yes" makes more
sense, though we could also teach this function to accept an "infinity"
parameter.

-Peff

RE: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Randall S. Becker]

On February 8, 2019 17:19, brian m. carlson wrote:
> On Fri, Feb 08, 2019 at 05:12:43PM -0500, Randall S. Becker wrote:
> > I'm happy to modify the test (it is in one spot), to make a decision based
> on:
> > a) whether /dev/zero exists
> > b) whether the system is a NonStop
> > c) something else
> >
> > What would you all prefer? It doesn't matter to me one way or another, as
> long as I can get the dependency to /dev/zero removed so tests will run here.
> 
> My preference is that we wrap the yes/tr invocation into a function (maybe
> "infinite_nul") and use that where we currently require /dev/zero.

That's simple enough to do in test-lib-functions.sh for the situation where yes|tr is being piped, but that's t5562. In t5318 we have dd if=/dev/zero, and that's where truncate would need to work. The requirements of that test seem very specific to me and not that generalizable. I'm just dealing with new breakages on the platform.

RE: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Randall S. Becker]

On February 8, 2019 17:35, Jeff King wrote:
> On Fri, Feb 08, 2019 at 05:12:43PM -0500, Randall S. Becker wrote:
> > On February 8, 2019 17:07, brian m. carlson wrote:
> > > On Fri, Feb 08, 2019 at 02:31:57PM -0500, Jeff King wrote:
> > > > > It is available AFAIK on Linux, POSIX, and Windows under Cygwin.
> > > > > That's more than /dev/zero has anyway. I have the patch ready if
> > > > > you want it.
> > > >
> > > > Is it POSIX? Certainly truncate() is, but I didn't think the
> > > > command-line tool was. If it really is available everywhere, then
> > > > yeah, I'd be fine with it.
> > >
> > > It's not. POSIX doesn't specify the command, and macOS lacks it, I
> believe.
> >
> > I'm happy to modify the test (it is in one spot), to make a decision based
> on:
> > a) whether /dev/zero exists
> > b) whether the system is a NonStop
> > c) something else
> >
> > What would you all prefer? It doesn't matter to me one way or another,
> > as long as I can get the dependency to /dev/zero removed so tests will
> > run here.
> 
> For the case in t5318, I think we can just put the NULs in a file. Does this
> work on your platform?

Yes, should work just fine.

> 
> ---
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index
> 16d10ebce8..6d0ccc7eba 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -383,7 +383,8 @@ 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)) &&
> +	gen_zero_bytes $(($orig_size - $zero_pos)) >zeroes &&
> +	dd if=zeroes of="$objdir/info/commit-graph" bs=1 seek="$zero_pos"
> &&
>  	test_must_fail git commit-graph verify 2>test_err &&
>  	grep -v "^+" test_err >err &&
>  	test_i18ngrep "$grepstr" err
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index
> 92cf8f812c..4afab14431 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1302,3 +1302,8 @@ test_set_port () {
>  	port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
>  	eval $var=$port
>  }
> +
> +# Generate an output of $1 bytes of all zeroes (NULs, not ASCII zeroes).
> +gen_zero_bytes () {
> +	perl -e 'print "\0" x $ARGV[0]' "$@"
> +}

This function does work on platform, so it's good.

> For the others that need infinite zeroes, I think using "yes" makes more
> sense, though we could also teach this function to accept an "infinity"
> parameter.

You could be sneaky about it, I suppose
+# Generate an output of $1 bytes of all zeroes (NULs, not ASCII zeroes).
+ gen_zero_bytes () {
+ 	if [ $1 -eq -1 ]; then
+ 	        yes | tr 'y' '\0'
+ 	else
+ 	        perl -e 'print "\0" x $ARGV[0]' "$@"
+ }
Or something alone those lines. It's not even slightly elegant, though. It would be better inside perl, so just

+# Generate an output of $1 bytes of all zeroes (NULs, not ASCII zeroes). If $1 is < 0, output forever.
+ gen_zero_bytes () {
+ 	perl -e ' if ($ARGV[0] < 0) { while (-1) { print "\0" } } else { print "\0" x $ARGV[0] }' "$@"
+ }

Cheers,
Randall

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Jeff King]

On Fri, Feb 08, 2019 at 05:53:53PM -0500, Randall S. Becker wrote:

> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index
> > 92cf8f812c..4afab14431 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -1302,3 +1302,8 @@ test_set_port () {
> >  	port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
> >  	eval $var=$port
> >  }
> > +
> > +# Generate an output of $1 bytes of all zeroes (NULs, not ASCII zeroes).
> > +gen_zero_bytes () {
> > +	perl -e 'print "\0" x $ARGV[0]' "$@"
> > +}
> 
> This function does work on platform, so it's good.

Great. Since it sounds like you're preparing some patches to deal with
/dev/zero elsewhere, do you want to wrap it up in a patch as part of
that?

-Peff

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Johannes Sixt]

Am 09.02.19 um 05:24 schrieb Jeff King:
> On Fri, Feb 08, 2019 at 05:53:53PM -0500, Randall S. Becker wrote:
> 
>>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index
>>> 92cf8f812c..4afab14431 100644
>>> --- a/t/test-lib-functions.sh
>>> +++ b/t/test-lib-functions.sh
>>> @@ -1302,3 +1302,8 @@ test_set_port () {
>>>  	port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
>>>  	eval $var=$port
>>>  }
>>> +
>>> +# Generate an output of $1 bytes of all zeroes (NULs, not ASCII zeroes).
>>> +gen_zero_bytes () {
>>> +	perl -e 'print "\0" x $ARGV[0]' "$@"
>>> +}
>>
>> This function does work on platform, so it's good.
> 
> Great. Since it sounds like you're preparing some patches to deal with
> /dev/zero elsewhere, do you want to wrap it up in a patch as part of
> that?

Please do not use yes to generate an infinite amount of bytes. Our
implementation of yes() in test-lib.sh generates only 99 lines.

Perhaps do this.

----- 8< -----
Subject: [PATCH] t5318: avoid /dev/zero

Some platforms do not offer /dev/zero. Use printf and tr to generate
a certain amount of NUL bytes.

Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t5318-commit-graph.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 16d10ebce8..04d394274f 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -383,7 +383,8 @@ 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)) &&
+	printf "%0*d" $(($orig_size - $zero_pos)) 0 | tr 0 '\0' |
+		dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" &&
 	test_must_fail git commit-graph verify 2>test_err &&
 	grep -v "^+" test_err >err &&
 	test_i18ngrep "$grepstr" err
-- 
2.20.1.86.gb0de946387

RE: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Randall S. Becker]

On February 8, 2019 23:25, Jeff King wrote:
> On Fri, Feb 08, 2019 at 05:53:53PM -0500, Randall S. Becker wrote:
> 
> > > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index
> > > 92cf8f812c..4afab14431 100644
> > > --- a/t/test-lib-functions.sh
> > > +++ b/t/test-lib-functions.sh
> > > @@ -1302,3 +1302,8 @@ test_set_port () {
> > >  	port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
> > >  	eval $var=$port
> > >  }
> > > +
> > > +# Generate an output of $1 bytes of all zeroes (NULs, not ASCII zeroes).
> > > +gen_zero_bytes () {
> > > +	perl -e 'print "\0" x $ARGV[0]' "$@"
> > > +}
> >
> > This function does work on platform, so it's good.
> 
> Great. Since it sounds like you're preparing some patches to deal with
> /dev/zero elsewhere, do you want to wrap it up in a patch as part of that?

That's the plan. 😊 We're currently running a full test, so that takes time (32 hours on this box).

Cheers,
Randall

RE: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Randall S. Becker]

On February 9, 2019 3:40, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 09.02.19 um 05:24 schrieb Jeff King:
> > On Fri, Feb 08, 2019 at 05:53:53PM -0500, Randall S. Becker wrote:
> >
> >>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index
> >>> 92cf8f812c..4afab14431 100644
> >>> --- a/t/test-lib-functions.sh
> >>> +++ b/t/test-lib-functions.sh
> >>> @@ -1302,3 +1302,8 @@ test_set_port () {
> >>>  	port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
> >>>  	eval $var=$port
> >>>  }
> >>> +
> >>> +# Generate an output of $1 bytes of all zeroes (NULs, not ASCII zeroes).
> >>> +gen_zero_bytes () {
> >>> +	perl -e 'print "\0" x $ARGV[0]' "$@"
> >>> +}
> >>
> >> This function does work on platform, so it's good.
> >
> > Great. Since it sounds like you're preparing some patches to deal with
> > /dev/zero elsewhere, do you want to wrap it up in a patch as part of
> > that?
> 
> Please do not use yes to generate an infinite amount of bytes. Our
> implementation of yes() in test-lib.sh generates only 99 lines.
> 
> Perhaps do this.
> 
> ----- 8< -----
> Subject: [PATCH] t5318: avoid /dev/zero
> 
> Some platforms do not offer /dev/zero. Use printf and tr to generate a
> certain amount of NUL bytes.
> 
> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  t/t5318-commit-graph.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index
> 16d10ebce8..04d394274f 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -383,7 +383,8 @@ 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)) &&
> +	printf "%0*d" $(($orig_size - $zero_pos)) 0 | tr 0 '\0' |
> +		dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos"
> &&
>  	test_must_fail git commit-graph verify 2>test_err &&
>  	grep -v "^+" test_err >err &&
>  	test_i18ngrep "$grepstr" err
> --
> 2.20.1.86.gb0de946387

This would be fine with me. I'm going to prepare an alternative and let the committers decide.

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Jeff King]

On Sat, Feb 09, 2019 at 09:39:43AM +0100, Johannes Sixt wrote:

> > Great. Since it sounds like you're preparing some patches to deal with
> > /dev/zero elsewhere, do you want to wrap it up in a patch as part of
> > that?
> 
> Please do not use yes to generate an infinite amount of bytes. Our
> implementation of yes() in test-lib.sh generates only 99 lines.

Ah, thanks. That doesn't matter here, but it would for the other patches
under discussion.

> Perhaps do this.
> [...]
>  	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)) &&
> +	printf "%0*d" $(($orig_size - $zero_pos)) 0 | tr 0 '\0' |
> +		dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" &&

Using stdin instead of the tmpfile is nice, and shouldn't have any
problems. I do think your printf suggestion looks nice, but I wondered
if it might run into portability issues (not because of anything in
particular, but I often find that the more clever a shell solution, the
more likely we run into obscure problems).

But if it works everywhere, that's fine by me.

-Peff

Re: [Breakage] Git v2.21.0-rc0 - t5318 (NonStop)

[Johannes Sixt]

Am 10.02.19 um 00:29 schrieb Jeff King:
> On Sat, Feb 09, 2019 at 09:39:43AM +0100, Johannes Sixt wrote:
>>  	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)) &&
>> +	printf "%0*d" $(($orig_size - $zero_pos)) 0 | tr 0 '\0' |
>> +		dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" &&
> 
> Using stdin instead of the tmpfile is nice, and shouldn't have any
> problems. I do think your printf suggestion looks nice, but I wondered
> if it might run into portability issues (not because of anything in
> particular, but I often find that the more clever a shell solution, the
> more likely we run into obscure problems).
> 
> But if it works everywhere, that's fine by me.

Unfortunately, it does not work as intended: the printf solution cannot
print nothing, but that should be the case in all but one of the tests.

-- Hannes