Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> +static inline void oe_set_size(struct object_entry *e,
> + unsigned long size)
> +{
> + e->size_ = size;
> + e->size_valid = e->size_ == size;
A quite similar comment as my earlier one applies here. I wonder if
this is easier to read?
e->size_valid = fits_in_32bits(size);
if (e->size_valid)
e->size_ = size;
Stepping back a bit in a different tangent,
- fits_in_32bits() is a good public name if the helper is about
seeing if the given quantity fits in 32bit uint,
- but that carves it in stone that our e->size_ *will* be 32bit
forever, which is not good.
So, it may be a good idea to call it size_cacheable_in_oe(size) or
something to ask "I have this 'size'; is it small enough to fit in
the field in the oe, i.e. allow us to cache it, as opposed to having
to go back to the object every time?" Of course, this would declare
that the helper can only be used for that particular field, but that
is sort of the point of such a change, to allow us to later define
the e->size_ field to different sizes without affecting other stuff.
> + 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?
> +}
> +
> #endif
