Re: [PATCH RFC 2/3] rust: implement a test balloon via the "varint" subsystem

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 8, 2025, 04:39 UTC
Message-ID: <xmqq8qipzhg3.fsf@gitster.g>
In-Reply-To: <8A7DBC60-286A-48FE-A3D3-CAFC11FD3AEA@gmail.com>

Ben Knoble <ben.knoble@gmail.com> writes:

Show 15 quoted lines

>> +#[no_mangle]
>> +pub unsafe extern "C" fn decode_varint(bufp: *mut *const c_uchar) -> usize {
>> +    let mut buf = *bufp;
>> +    let mut c = *buf;
>> +    let mut val = usize::from(c & 127);
>> +
>> +    buf = buf.add(1);
>> +
>> +    while (c & 128) != 0 {
>> +        val += 1;
>> +        if val == 0 || val.leading_zeros() < 7 {
>> +            return 0; // overflow
>
> Hm. I thought overflows panic in debug builds, in which case
> checking afterwards is too late? Does unsafe change that?

This code is a very faithful conversion from C so if somebody does not read Rust well, they can safely refer to the original in C.

In either variant, the leading zero's check asks "can we shift val by 7 bits to the left?" _before_ it actually shifts val (and or'es in the lower bits of c), so the "overflow" check is "if we processed any more data we _would_ overflow, so we stop before overflowing".

IOW, the code _is_ avoiding the "too late" condition.

This is a tangent, but as many people pointed out, calling this a test balloon is misreading. This is quite different from what we traditionally called a test balloon, where

 - we were already fairly sure that the construct is safe, but
   wanted to be extra careful to smoke out anybody who has trouble
   with it;

 - hence we use the construct in question in a place where nobody
   can compile it out, hoping that anybody with a system incapable
   of handling the construct in question would be broken badly,
   reporting the breakage to us;

 - this is done with an understanding that even a single "the
   compiler on this this platform with more than dozen thousands
   users cannot groke it" would automatically stop us, causing us to
   revert that test balloon code for _everybody_, refraining from
   using that construct for _everybody_ until the situation changes.

This thing is different at all points. We are not "fairlu sure that Rust is safe to use for everybody" Far from it. We are confident that requiring Rust would break known people. We are doing this not because we intend to stop once we know of folks who would be broken. Far from it.

It would really be nice to find a niche that can be a new optional feature that is not essential to the functioning of the system implemented in an already modularized part of the system (e.g., an optional merge strategy, diff algorithm, built-in textconv filter, a new ref backend, etc.). Then we can introduce Rust, knowing that some Rust-challenged systems will not be able to use these optional features. What Brian mentioned about two-hash interop feature, being only available on Rust-capable systems, could be such an optional feature, and if it can be done that way, that would be very welcome. If we can have Rust goodness soon enough without making it mandatory in too short a timeframe, that would be ideal.

I already said that I find 6 months advance notice to folks on Rust-challenged systems is way too short to be any good. If the only reason we give advance notice is because we want to make an excuse of cutting them off sooner while being able to say that we gave them advance notice, that may be sufficient. But if we truly want to help them by giving enough time to them so that they can help their platform themselves, by lobbying, fundraising, or otherwise campaigning to have usable Rust on their system, I really do not think it is sufficient.

Thanks.