Re: [PATCH 2/2] commit-graph tests: fix cryptic unportable "dd" invocation

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 21, 2019, 22:26 UTC
Message-ID: <878sy86anh.fsf@evledraar.gmail.com>
In-Reply-To: <20190221204310.GS1622@szeder.dev>

On Thu, Feb 21 2019, SZEDER Gábor wrote:

Show 42 quoted lines

> On Thu, Feb 21, 2019 at 08:28:49PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> Change an unportable invocation of "dd" that truncated the
>> commit-graph to call Perl's truncate() function instead.
>>
>> In POSIX it is unspecified what happens when count=0 is
>> provided[1]. The NetBSD "dd" behavior differs from GNU (and seemingly
>> other BSDs), which as left this test broken since
>> d2b86fbaa1 ("commit-graph: fix buffer read-overflow", 2019-01-15).
>>
>> In POSIX the truncate(2) and ftruncate(2) functions are
>> portable. We've used the latter since 271421cd34 ("Update partial HTTP
>> transfers.", 2005-09-30), but the truncate(1) command-line tool is
>> GNU-specific. Thus let's use Perl's version of it. We could also just
>> introduce a "test-tool truncate" in the future if we wanted to avoid
>> shelling out to perl.
>>
>> On Linux and NetBSD we don't need the "if -s $ARGV[0] > $ARGV[1]"
>> condition I'm adding. We never have a $zero_pos longer than the file
>> being truncated. But let's have that condition to future-proof the
>> code, and because "the behavior is undefined if LENGTH is greater than
>> the length of the file" (perldoc -f truncate).
>>
>> 1. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/dd.html
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  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 d4bd1522fe..d99bea6cce 100755
>> --- a/t/t5318-commit-graph.sh
>> +++ b/t/t5318-commit-graph.sh
>> @@ -382,7 +382,8 @@ corrupt_graph_and_verify() {
>>  	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
>>  	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 &&
>> +	perl -we 'truncate $ARGV[0], $ARGV[1] if -s $ARGV[0] > $ARGV[1]' \
>> +		$objdir/info/commit-graph $zero_pos &&
>
> This will make Dscho unhappy :)

Sorry Dscho :)

Although this is a one-off in one test, as opposed to a new "perl -e" in test-lib-functions.sh

Show 9 quoted lines

> Is there a problem with:
>
>   dd if=/dev/null of="$objdir/info/commit-graph" bs=1 seek="$zero_pos"
>
> ?
>
> To my understanding of the specs it's well-defined what it should do,
> even when $zero_pos is larget than the file size,  it's shorter,
> simpler, and doesn't introduce yet another Perl dependency.

I tried that as a one-off and it indeed works as a "truncate" on NetBSD & GNU.

My reading of POSIX "dd" and "lseek" docs is that we'd need some similar guard if we're going to be paranoid about a $zero_pos value past the end of the file. It doesn't look like that's portable, my assumption from reading the docs is that the seek=* will devolve without a stat() check on some "dd" implementations to an "lseek".

I'm not going to submit a re-roll of this because it works, and I'd still trust Perl's truncate(...) portability over dd.

But more importantly because it takes me *ages* to fully re-test anything on the slow BSD VMs I have access to, and I already tore town my one-off hacking env there after testing these patches...

Show 6 quoted lines

>>  	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 &&
>> --
>> 2.21.0.rc0.258.g878e2cd30e
>>