[BUG] Git 2.43.0-rc0 - t4216 unpack(Q) invalid type

>cause. Unpack("Q>".... What version does git now require for perl?
>I have v5.30.3 available, nothing more recent.
>
>expecting success of 4216.141 'Bloom reader notices too-small data chunk':
>check_corrupt_graph BDAT clear 00000000 && echo "warning: ignoring

>changed-path chunk" \
>"(4 < 12) in commit-graph file" >expect.err && test_cmp expect.err err
>
>Invalid type 'Q' in unpack at
>/home/jenkinsbuild/.jenkins/workspace/Git_Pipeline/t/lib-chunk/corrupt-chun

>\
>#"(4 < 12) in commit-graph file" >expect.err && #test_cmp expect.err err #
>
>expecting success of 4216.142 'Bloom reader notices out-of-bounds filter
>offsets':
>check_corrupt_graph BIDX 12 FFFFFFFF &&
># use grep to avoid depending on exact chunk size grep "warning: ignoring

>range offset (4294967295) for changed-path filter at pos 3 of
>.git/objects/info/commit-graph" err
>
>Invalid type 'Q' in unpack at
>/home/jenkinsbuild/.jenkins/workspace/Git_Pipeline/t/lib-chunk/corrupt-chun

>clear 00000000 && #echo "warning: commit-graph changed-path index chunk" \
>#"is too small" >expect.err && #test_cmp expect.err err #
>
>expecting success of 4216.144 'Bloom reader notices out-of-order index
>offsets':
># we do not know any real offsets, but we can pick # something plausible;

>corrupt_graph BIDX 4 0000000c00000005 && echo "warning: ignoring decreasing
>changed-path index offsets" \
>"(12 > 5) for positions 1 and 2 of .git/objects/info/commit-graph"
>>expect.err &&
>git -c core.commitGraph=false log -- A/B/file2 >expect.out && git -c
>core.commitGraph=true log -- A/B/file2 >out 2>err && test_cmp expect.out

>point of ## actually reading from the bogus offsets anyway.
>#corrupt_graph BIDX 4 0000000c00000005 && #echo "warning: ignoring
>decreasing changed-path index offsets" \
>#"(12 > 5) for positions 1 and 2 of .git/objects/info/commit-graph"
>>expect.err &&
>#git -c core.commitGraph=false log -- A/B/file2 >expect.out && #git -c
>core.commitGraph=true log -- A/B/file2 >out 2>err && #test_cmp expect.out

>On Fri, Nov 03, 2023 at 10:50:19AM -0400, rsbecker@nexbridge.com wrote:
>> In RC0, the following tests are failing (with verbose). They look like
>> the same root cause. Unpack("Q>".... What version does git now require for perl?
>> I have v5.30.3 available, nothing more recent.
>
>The perl used in the test suite is supposed to be vanilla enough to support any
>ancient version. The perl5 Git import doesn't have version tags that go back that far,
>but the quadwords in pack/unpack go back at least to a commit from 1998.
>
>So I suspect this is not a version issue, but rather a build-time config one. The docs
>say:
>
>  Q  An unsigned quad value.
>      (Quads are available only if your system supports 64-bit integer
>      values _and_ if Perl has been compiled to support those.  Raises
>      an exception otherwise.)
>
>It would probably be possible to rewrite the use of "Q" here to grab two 32-bit
>values instead. But I'd guess that on your system it is not as simple as a shift-and-
>add to then treat them as a 64-bit value, since presumably the problem is that perl's
>ints are all strictly 32-bit.
>
>What does this script produce for you:
>
>  perl -e '
>    my $bytes = "\1\2\3\4\5\6\7\8";
>    my $q = eval { unpack("Q>", $bytes) };
>    print "Q = ", defined($q) ? $q : "($@)", "\n";
>    my ($n1, $n2) = unpack("NN", $bytes);
>    print "n1 = $n1\n";
>    print "n2 = $n2\n";
>    print "computed quad = ", ($n1 << 32) | $n2, "\n";
>  '
>
>I get:
>
>  Q = 72623859790382904
>  n1 = 16909060
>  n2 = 84281144
>  computed quad = 72623859790382904
>
>but I'm guessing you get an exception report for Q, and that the computed quad is
>probably equal to n2 (the shift of n1 goes totally off the end).
>
>We may not be without hope, though. These 64-bit values are file offsets we're
>reading from the chunk files. The format naturally uses 64-bit values here to
>accommodate arbitrarily large files. But in our tests, the offsets are all going to be
>relatively small. So our "$n1" in practice will always be 0.
>
>> This same problem also happens in t5318, t5319, t5324
>
>Yep. The offending code is in lib-chunk.sh, so the new tests added in all of those
>scripts which use it will run into the same problem.

> +# Some platforms' perl builds don't support 64-bit integers, and hence do not
> +# allow packing/unpacking quadwords with "Q". The chunk format uses 64-bit file
> +# offsets to support files of any size, but in practice our test suite will
> +# only use small files. So we can fake it by asking for two 32-bit values and
> +# discarding the first (most significant) one, which is equivalent as long as
> +# it's just zero.
> +sub unpack_quad {
> +	my $bytes = shift;
> +	my ($n1, $n2) = unpack("NN", $bytes);
> +	die "quad value exceeds 32 bits" if $n1;
> +	return $n2;
> +};

> +sub pack_quad {
> +	my $n = shift;
> +	my $ret = pack("NN", 0, $n);
> +	# double check that our original $n did not exceed the 32-bit limit.
> +	# This is presumably impossible on a 32-bit system (which would have
> +	# truncated much earlier), but would still alert us on a 64-bit build
> +	# of a new test that would fail on a 32-bit build (though we'd
> +	# presumably see the die() from unpack_quad() in such a case).
> +	die "quad round-trip failed" if unpack_quad($ret) != $n;
> +	return $ret;
> +}

>  # read until we find table-of-contents entry for chunk;
>  # note that we cheat a bit by assuming 4-byte alignment and
>  # that no ToC entry will accidentally look like a header.
>  #
>  # If we don't find the entry, copy() will hit EOF and exit
>  # (which should cause the caller to fail the test).
>  while (copy(4) ne $chunk) { }
> -my $offset = unpack("Q>", copy(8));
> +my $offset = unpack_quad(copy(8));
>  
>  # In clear mode, our length will change. So figure out
>  # the length by comparing to the offset of the next chunk, and
> @@ -38,11 +62,11 @@ sub copy {
>  	my $id;
>  	do {
>  		$id = copy(4);
> -		my $next = unpack("Q>", get(8));
> +		my $next = unpack_quad(get(8));
>  		if (!defined $len) {
>  			$len = $next - $offset;
>  		}
> -		print pack("Q>", $next - $len + length($bytes));
> +		print pack_quad($next - $len + length($bytes));
>  	} while (unpack("N", $id));
>  }

> > +sub unpack_quad {
> > +	my $bytes = shift;
> > +	my ($n1, $n2) = unpack("NN", $bytes);
> > +	die "quad value exceeds 32 bits" if $n1;
> > +	return $n2;
> > +};
> 
> Is this an unnecessary ';' at the end?

>On Fri, Nov 03, 2023 at 10:50:19AM -0400, rsbecker@nexbridge.com wrote:
>
>> In RC0, the following tests are failing (with verbose). They look like
>> the same root cause. Unpack("Q>".... What version does git now require for perl?
>> I have v5.30.3 available, nothing more recent.
>
>The perl used in the test suite is supposed to be vanilla enough to support any
>ancient version. The perl5 Git import doesn't have version tags that go back that far,
>but the quadwords in pack/unpack go back at least to a commit from 1998.
>
>So I suspect this is not a version issue, but rather a build-time config one. The docs
>say:
>
>  Q  An unsigned quad value.
>      (Quads are available only if your system supports 64-bit integer
>      values _and_ if Perl has been compiled to support those.  Raises
>      an exception otherwise.)
>
>It would probably be possible to rewrite the use of "Q" here to grab two 32-bit
>values instead. But I'd guess that on your system it is not as simple as a shift-and-
>add to then treat them as a 64-bit value, since presumably the problem is that perl's
>ints are all strictly 32-bit.
>
>What does this script produce for you:
>
>  perl -e '
>    my $bytes = "\1\2\3\4\5\6\7\8";
>    my $q = eval { unpack("Q>", $bytes) };
>    print "Q = ", defined($q) ? $q : "($@)", "\n";
>    my ($n1, $n2) = unpack("NN", $bytes);
>    print "n1 = $n1\n";
>    print "n2 = $n2\n";
>    print "computed quad = ", ($n1 << 32) | $n2, "\n";
>  '
>
>I get:
>
>  Q = 72623859790382904
>  n1 = 16909060
>  n2 = 84281144
>  computed quad = 72623859790382904
>
>but I'm guessing you get an exception report for Q, and that the computed quad is
>probably equal to n2 (the shift of n1 goes totally off the end).
>
>We may not be without hope, though. These 64-bit values are file offsets we're
>reading from the chunk files. The format naturally uses 64-bit values here to
>accommodate arbitrarily large files. But in our tests, the offsets are all going to be
>relatively small. So our "$n1" in practice will always be 0.
>
>> This same problem also happens in t5318, t5319, t5324
>
>Yep. The offending code is in lib-chunk.sh, so the new tests added in all of those
>scripts which use it will run into the same problem.

>     my $bytes = "\1\2\3\4\5\6\7\8";
>     my $q = eval { unpack("Q>", $bytes) };
>     print "Q = ", defined($q) ? $q : "($@)", "\n";
>     my ($n1, $n2) = unpack("NN", $bytes);
>     print "n1 = $n1\n";
>     print "n2 = $n2\n";
>     print "computed quad = ", ($n1 << 32) | $n2, "\n";  '

> What I get from Perl is 
> $ perl -e '
> >     my $bytes = "\1\2\3\4\5\6\7\8";
> >     my $q = eval { unpack("Q>", $bytes) };
> >     print "Q = ", defined($q) ? $q : "($@)", "\n";
> >     my ($n1, $n2) = unpack("NN", $bytes);
> >     print "n1 = $n1\n";
> >     print "n2 = $n2\n";
> >     print "computed quad = ", ($n1 << 32) | $n2, "\n";  '
> Q = (Invalid type 'Q' in unpack at -e line 3.
> )
> n1 = 16909060
> n2 = 84281144
> computed quad = 84281144

>On Fri, Nov 03, 2023 at 12:07:17PM -0400, rsbecker@nexbridge.com wrote:
>
>> What I get from Perl is
>> $ perl -e '
>> >     my $bytes = "\1\2\3\4\5\6\7\8";
>> >     my $q = eval { unpack("Q>", $bytes) };
>> >     print "Q = ", defined($q) ? $q : "($@)", "\n";
>> >     my ($n1, $n2) = unpack("NN", $bytes);
>> >     print "n1 = $n1\n";
>> >     print "n2 = $n2\n";
>> >     print "computed quad = ", ($n1 << 32) | $n2, "\n";  '
>> Q = (Invalid type 'Q' in unpack at -e line 3.
>> )
>> n1 = 16909060
>> n2 = 84281144
>> computed quad = 84281144
>
>OK, that matches what I expected. Hopefully the patch I just sent (our mails just
>crossed) will fix it for you.
>
>> Because perl itself is 32-bit, not 64-bit on this platform. So even
>> moving git to 64-bit will not correct the issue.
>
>Yep, exactly.