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

Commit Message

Randall S. Becker Feb. 9, 2019, 6:59 p.m. UTC

From: "Randall S. Becker" <[e-mail]>

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 <[e-mail]>
---
 t/t5318-commit-graph.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eric Sunshine Feb. 10, 2019, 2:07 a.m. UTC | #1

On Sat, Feb 9, 2019 at 2:00 PM <[e-mail]> 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 <[e-mail]>
> ---
> 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" &&

Junio C Hamano Feb. 12, 2019, 5:18 p.m. UTC | #2

Eric Sunshine <[e-mail]> writes:

> On Sat, Feb 9, 2019 at 2:00 PM <[e-mail]> 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 <[e-mail]>
>> ---
>> 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 ;-).

Junio C Hamano Feb. 13, 2019, 5:25 p.m. UTC | #3

Junio C Hamano <[e-mail]> writes:

> Eric Sunshine <[e-mail]> writes:
>
>> On Sat, Feb 9, 2019 at 2:00 PM <[e-mail]> 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" <[e-mail]>
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 <[e-mail]>
Signed-off-by: Junio C Hamano <[e-mail]>
---
 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

Randall S. Becker Feb. 13, 2019, 6:18 p.m. UTC | #4

> -----Original Message-----
> From: Junio C Hamano <[e-mail]> On Behalf Of Junio C Hamano
> Sent: February 13, 2019 12:25
> To: Eric Sunshine <[e-mail]>
> Cc: [e-mail]; Git List <[e-mail]>; Randall
S.
> Becker <[e-mail]>
> Subject: Re: [Patch v1 2/3] t5318: replace use of /dev/zero with
> generate_zero_bytes
> 
> Junio C Hamano <[e-mail]> writes:
> 
> > Eric Sunshine <[e-mail]> writes:
> >
> >> On Sat, Feb 9, 2019 at 2:00 PM <[e-mail]> 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" <[e-mail]>
> 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 <[e-mail]>
> Signed-off-by: Junio C Hamano <[e-mail]>
> ---
>  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

Junio C Hamano Feb. 13, 2019, 9 p.m. UTC | #5

"Randall S. Becker" <[e-mail]> 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.

Randall S. Becker Feb. 13, 2019, 9:03 p.m. UTC | #6

On February 13, 2019 16:01, Junio C Hamano wrote:
> "Randall S. Becker" <[e-mail]> 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

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