Re: [PATCH v7 00/13] nd/pack-objects-pack-struct updates

> On Sat, Mar 24, 2018 at 07:33:40AM +0100, Nguyễn Thái Ngọc Duy wrote:
>
>> +unsigned long oe_get_size_slow(struct packing_data *pack,
>> +                            const struct object_entry *e)
>> +{
>> +     struct packed_git *p;
>> +     struct pack_window *w_curs;
>> +     unsigned char *buf;
>> +     enum object_type type;
>> +     unsigned long used, avail, size;
>> +
>> +     if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
>> +             read_lock();
>> +             if (sha1_object_info(e->idx.oid.hash, &size) < 0)
>> +                     die(_("unable to get size of %s"),
>> +                         oid_to_hex(&e->idx.oid));
>> +             read_unlock();
>> +             return size;
>> +     }
>> +
>> +     p = oe_in_pack(pack, e);
>> +     if (!p)
>> +             die("BUG: when e->type is a delta, it must belong to a pack");
>> +
>> +     read_lock();
>> +     w_curs = NULL;
>> +     buf = use_pack(p, &w_curs, e->in_pack_offset, &avail);
>> +     used = unpack_object_header_buffer(buf, avail, &type, &size);
>> +     if (used == 0)
>> +             die(_("unable to parse object header of %s"),
>> +                 oid_to_hex(&e->idx.oid));
>> +
>> +     unuse_pack(&w_curs);
>> +     read_unlock();
>> +     return size;
>> +}
>
> It took me a while to figure out why this treated deltas and non-deltas
> differently. At first I thought it was an optimization (since we can
> find non-delta sizes quickly by looking at the headers).  But I think
> it's just that you want to know the size of the actual _delta_, not the
> reconstructed object. And there's no way to ask sha1_object_info() for
> that.
>
> Perhaps the _extended version of that function should learn an
> OBJECT_INFO_NO_DEREF flag or something to tell it return the true delta
> type and size. Then this whole function could just become a single call.
>
> But short of that, it's probably worth a comment explaining what's going
> on.

>> +Running tests with special setups
>> +---------------------------------
>> +
>> +The whole test suite could be run to test some special features
>> +that cannot be easily covered by a few specific test cases. These
>> +could be enabled by running the test suite with correct GIT_TEST_
>> +environment set.
>> +
>> +GIT_TEST_SPLIT_INDEX forces split-index mode on the whole test suite.
>> +
>> +GIT_TEST_FULL_IN_PACK_ARRAY exercises the uncommon pack-objects code
>> +path where there are more than 1024 packs even if the actual number of
>> +packs in repository is below this limit.
>> +
>> +GIT_TEST_OE_SIZE_BITS=<bits> exercises the uncommon pack-objects
>> +code path where we do not cache objecct size in memory and read it
>> +from existing packs on demand. This normally only happens when the
>> +object size is over 2GB. This variable forces the code path on any
>> +object larger than 2^<bits> bytes.
>
> It's nice to have these available to test the uncommon cases. But I have
> a feeling nobody will ever run them, since it requires extra effort (and
> takes a full test run).