Re: [PATCH v6 09/11] pack-objects: shrink size field in struct object_entry

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 19, 2018, 18:45 UTC
Message-ID: <CACsJy8AbkaSLBYqtFiCwFoqRXguy1hTs=XriefZ3WQJjo_sy8Q@mail.gmail.com>
In-Reply-To: <xmqqlgenvs07.fsf@gitster-ct.c.googlers.com>

On Mon, Mar 19, 2018 at 7:29 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

>>>> +     if (!e->size_valid) {
>>>> +             unsigned long real_size;
>>>> +
>>>> +             if (sha1_object_info(e->idx.oid.hash, &real_size) < 0 ||
>>>> +                 size != real_size)
>>>> +                     die("BUG: 'size' is supposed to be the object size!");
>>>> +     }
>>>
>>> If an object that is smaller than 4GB is fed to this function with
>>> an incorrect size, we happily record it in e->size_ and declare it
>>> is valid.  Wouldn't that be equally grave error as we are catching
>>> in this block?
>>
>> That adds an extra sha1_object_info() to all objects and it's
>> expensive (I think it's one of the reasons we cache values in
>> object_entry in the first place). I think there are also a few
>> occasions we reuse even bad in-pack objects (there are even tests for
>> that) so it's not always safe to die() here.
>
> So what?  My point is that I do not see the point in checking if the
> size is correct on only one side (i.e. size is too big to fit in
> e->size_) and not the other.  If it is worth checking (perhaps under
> "#ifndef NDEBUG" or some other debug option?) then I'd think we
> should spend cycles for all objects and check.

There is a difference. For sizes smaller than 2^32, whatever you pass to oe_set_size() will be returned by oe_size(), consistently. It does not matter if this size is "good" or not. With sizes > 2^32, we make the assumption that this size must be the same as one found in the object database. If it's different, oe_size() will return something else other than oe_set_size() is given. This check here is to make sure we do not accidentally let the caller fall into this trap.

Yes, it may be a good thing to check anyway even for sizes < 2^32. I'm a bit uncomfortable doing that though. I was trying to exercise this code the other day by reducing size_ field down to 4 bits, and a couple tests broke but I still don't understand how. It's probably just me pushing the limits too hard, not a bug in these changes. But it does tell me that I don't understand pack-objects enough to assert that "all calls to oe_set_size() give good size".

-- 
Duy