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

[Junio C Hamano <gitster@pobox.com>]

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

>> +#[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.