On Mon, Mar 19, 2018 at 7:29 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>>> + 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
